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.

Sign up for GitHub

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

Jump to bottom

fix(sns-subscriptions): restrict encryption of queue to only the respective sns topic (under feature flag) #20521

Merged
merged 11 commits into from
Jul 8, 2022
Merged

fix(sns-subscriptions): restrict encryption of queue to only the respective sns topic (under feature flag) #20521

merged 11 commits into from
Jul 8, 2022

Conversation

daschaa
Copy link
Contributor

@daschaa daschaa commented May 27, 2022

Implements #20339

The change adds a conditional that only the corresponding SNS Topic can decrypt the SQS queue with the KMS-Key if one was given.

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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 May 27, 2022

@github-actions github-actions bot added the p2 label May 27, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team May 27, 2022 09:33
@TheRealAmazonKendra TheRealAmazonKendra changed the base branch from v1-main to main June 2, 2022 09:11
@corymhall corymhall changed the title chore(sns-subscriptions): restrict encryption of queue to only the respective sns topic fix(sns-subscriptions): restrict encryption of queue to only the respective sns topic Jun 3, 2022
corymhall
corymhall previously requested changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@daschaa, unfortunately since this does technically introduce a breaking change, this will need to live behind a feature flag.

@mergify mergify bot dismissed corymhall’s stale review July 7, 2022 16:28

Pull request has been modified.

@daschaa daschaa changed the title fix(sns-subscriptions): restrict encryption of queue to only the respective sns topic fix(sns-subscriptions): restrict encryption of queue to only the respective sns topic (under feature flag) Jul 8, 2022
@daschaa
Copy link
Contributor Author

daschaa commented Jul 8, 2022

@corymhall I have now added the feature flag. Since this is my first time implementing a feature flag, it would be cool if you could double check everything and give some feedback if it is ok like it is implemented.
For the unit tests, i could not get the testFutureBehavior and testLegacyBehavior to work. My solution was to just pass it in myself as context when instantiating a new App. I hope it is ok?
Thanks in advance!

corymhall
corymhall previously requested changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@daschaa thanks for implementing the feature flag! I just have a couple of cosmetic comments and then we are good to go!

@daschaa
Copy link
Contributor Author

daschaa commented Jul 8, 2022

@corymhall Thanks for the review! I have added everything (I think) ❤️

@mergify mergify bot dismissed corymhall’s stale review July 8, 2022 12:56

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jul 8, 2022

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 48fa296
  • 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 4e0c80f into aws:main Jul 8, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 8, 2022

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

daschaa added a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
@daschaa
fix(sns-subscriptions): restrict encryption of queue to only the resp…
f7f9ff2 
…ective sns topic (under feature flag) (aws#20521)

Implements aws#20339 

The change adds a conditional that only the corresponding SNS Topic can decrypt the SQS queue with the KMS-Key if one was given.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@daschaa daschaa mentioned this pull request Jul 9, 2022
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request Jul 14, 2022
@daschaa
docs(contributing): add additional step in the feature flags section (#…
ab7cff6 
…21074)

[Follow-Up to this Comment.](#20521 (comment))
> Can you also add this info [here](https://github.com/aws/aws-cdk/blob/68f09b7c6f8e6c1ddf13fdd4e116d12333d24c46/packages/%40aws-cdk/cx-api/README.md?plain=1#L14)? We haven't found the best way to document these yet so until then I figure that will have to do.

In the contributing documentation for the feature flags the part for adding an entry in the feature flags README was added in this PR.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@daschaa daschaa mentioned this pull request Jul 15, 2022
Closed
2 tasks
@biffgaut biffgaut mentioned this pull request May 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corymhall corymhall corymhall approved these changes

Assignees
No one assigned
Labels
p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daschaa @aws-cdk-automation @corymhall