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

fix(s3-notifications): notifications allowed with imported kms keys #18989

Merged
merged 17 commits into from Feb 22, 2022

Conversation

markussiebert
Copy link
Contributor


fixes: #18988

If you add an sqs queue as notification target to an s3 bucket, and this sqs queue is encrypted with an imported kms IKey, the stack won't synthesize. Instead of failing, it should warn the user, that it can not ensure the correct kms key policy permissions.

This fix will solve this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Feb 16, 2022

@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Feb 16, 2022
Copy link
Contributor

@peterwoodworth peterwoodworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me @markussiebert, just a small typo!

packages/@aws-cdk/aws-s3-notifications/lib/sqs.ts Outdated Show resolved Hide resolved
@peterwoodworth peterwoodworth removed the @aws-cdk/aws-kms Related to AWS Key Management label Feb 16, 2022
@peterwoodworth peterwoodworth changed the title fix: notifications allowed with imported kms keys fix(s3-notifications): notifications allowed with imported kms keys Feb 16, 2022
@mergify mergify bot dismissed peterwoodworth’s stale review February 17, 2022 07:54

Pull request has been modified.

@peterwoodworth
Copy link
Contributor

Hey @markussiebert, you don't have to worry about merging master in. That will happen automatically once we approve it 😄

@peterwoodworth peterwoodworth added the pr/do-not-merge This PR should not be merged at this time. label Feb 17, 2022
peterwoodworth
peterwoodworth previously approved these changes Feb 17, 2022
Copy link
Contributor

@peterwoodworth peterwoodworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! @otaviomacedo is the owner here so I'll let him take a final look before merging 🙂

@mergify mergify bot dismissed peterwoodworth’s stale review February 18, 2022 11:39

Pull request has been modified.

peterwoodworth
peterwoodworth previously approved these changes Feb 18, 2022
Copy link
Contributor

@peterwoodworth peterwoodworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the optimization you've made here. Clever! Still looks great to me

@mergify mergify bot dismissed peterwoodworth’s stale review February 22, 2022 09:59

Pull request has been modified.

otaviomacedo
otaviomacedo previously approved these changes Feb 22, 2022
@otaviomacedo otaviomacedo removed the pr/do-not-merge This PR should not be merged at this time. label Feb 22, 2022
@mergify mergify bot dismissed otaviomacedo’s stale review February 22, 2022 17:12

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Feb 22, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 3d2adfb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 7441418 into aws:master Feb 22, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 22, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(s3-notifications): SqsDestination with imported key fails to synth
5 participants