Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-redshift): KmsKeyId uses KeyArn instead of KeyId #17032

Closed
gkubes opened this issue Oct 18, 2021 · 4 comments · Fixed by #17108
Closed

(aws-redshift): KmsKeyId uses KeyArn instead of KeyId #17032

gkubes opened this issue Oct 18, 2021 · 4 comments · Fixed by #17108
Labels
@aws-cdk/aws-redshift Related to Amazon Redshift bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@gkubes
Copy link

gkubes commented Oct 18, 2021

What is the problem?

When a Cluster is created the KmsKeyId property is built using the KeyArn instead of the KeyId.

Reproduction Steps

const encryptionKey = new kms.Key(this, "KMSKey", {
    enabled: true,
    enableKeyRotation: true
});

const wbrCluster = new redshift.Cluster(this, "WBRRedshiftCluster", {
    ...
    encrypted: true,
    encryptionKey: encryptionKey,
    
});

What did you expect to happen?

"KmsKeyId": {
  "Ref": "KMSKey"
},

or

"KmsKeyId": {
  "Fn::GetAtt": [
    "WBRDataKMSKey",
    "KeyId"
  ]
},

What actually happened?

"KmsKeyId": {
  "Fn::GetAtt": [
    "WBRDataKMSKey",
    "Arn"
  ]
},

CDK CLI Version

1.126.0

Framework Version

No response

Node.js Version

12.0

OS

Amazon Linux 2012

Language

Typescript

Language Version

No response

Other information

No response

@gkubes gkubes added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2021
@github-actions github-actions bot added the @aws-cdk/aws-redshift Related to Amazon Redshift label Oct 18, 2021
@njlynch
Copy link
Contributor

njlynch commented Oct 18, 2021

Thanks for the bug report.

I suspect this is because the Redshift Cluster construct was largely built off of the RDS Cluster; RDS' KmsKeyId does actually expect the ARN, not the Key ID.

Does this prevent successful deployment? Sometimes the documented standard is the ID, but the ARN is accepted as well.

Either way, this is a simple enough fix. Contributions welcome! Ideally, we'd update the integ test as well to deploy with a Key, to verify the behavior.

@njlynch njlynch added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2021
@njlynch njlynch removed their assignment Oct 18, 2021
@njlynch njlynch added the good first issue Related to contributions. See CONTRIBUTING.md label Oct 18, 2021
@gkubes
Copy link
Author

gkubes commented Oct 18, 2021

I wasn't able to confirm if this prevents successful deployment but it can impact the ability to migrate from vanilla CFN to CDK because the change from KeyId to KeyArn triggers an encryption change in the cluster.

@gkubes
Copy link
Author

gkubes commented Oct 18, 2021

As a workaround, I was able to comment out the encryptionKey property from the cluster definition and provide an override.

(cluster.node.defaultChild as redshift.CfnCluster).addPropertyOverride("KmsKeyId", encryptionKey.keyId);

@mergify mergify bot closed this as completed in #17108 Oct 25, 2021
mergify bot pushed a commit that referenced this issue Oct 25, 2021
Field was incorrectly using key arn instead of id.

Fixes #17032


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
Field was incorrectly using key arn instead of id.

Fixes aws#17032


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-redshift Related to Amazon Redshift bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants