feat(pipes-alpha): support for customer-managed KMS keys to encrypt pipe data #33546
Conversation
badmintoncryer
changed the title
github-actions
bot
added
p2
distinguished-contributor
| @@ -23,7 +23,7 @@ can be filtered, transformed and enriched. | |||
|
|
|||
|  | |||
|
|
|||
| For more details see the [service documentation](https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-pipes.html). | |||
There was a problem hiding this comment.
Removed several unnecessary spaces.
badmintoncryer
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33546 +/- ##
=======================================
Coverage 82.17% 82.17%
=======================================
Files 119 119
Lines 6862 6862
Branches 1158 1158
=======================================
Hits 5639 5639
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
badmintoncryer
changed the title
aws-cdk-automation
added
the
pr/needs-community-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -295,6 +303,7 @@ export class Pipe extends PipeBase { | |||
| targetParameters: target.targetParameters, | |||
| desiredState: props.desiredState, | |||
| logConfiguration: logConfiguration, | |||
| kmsKeyIdentifier: props.kmsKey?.keyArn, | |||
There was a problem hiding this comment.
Just a question—did it work correctly even without setting a key policy?
When I previously attempted to contribute to this, I encountered an error without a key policy, but I couldn't come up with a proper way to configure it and had to give up at the time.
It might have been my misunderstanding back then, or something might have changed with an update. If it worked fine, please just ignore this question.
There was a problem hiding this comment.
You are absolutely right.. I misunderstood that this implementation would be work fine. Thank you for your great suggestion.
How about adding the restriction that makes pipeName mandatory when kmsKey is provided?
This idea is not ideal, but it would provide the minimum feature that enables us to configure CMK for Pipes.
There was a problem hiding this comment.
Thank you for confirming. So that's how it is...
I think your suggested solution is good.
Personally, I've been considering the following three options:
- Make name mandatory when kmsKey is specified. This is the same as the suggestion you provided.
- Instead of setting name to undefined when it's not specified, automatically generate it using Names.uniquid. However, I understand this would be a breaking change.
- Configure kmsKey using a custom resource. I think this option is not ideal.
Based on the above, I believe option 1 or option 2 would be good, and I think option 1 would be better to avoid a breaking change.
Issue # (if applicable)
None
Reason for this change
AWS Pipes supports for encrypting data by customer managed KMS key instead of Amazon managed key.
https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-encryption-pipes-cmkey.html
The L2 Pipe construct does not support this feature now.
Description of changes
kmsKeyprop toPipePropsDescribe any new or updated permissions being added
None
Description of how you validated changes
Add both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license