fix(s3): deprecate unencrypted encryption configuration for s3 bucket#24770
fix(s3): deprecate unencrypted encryption configuration for s3 bucket#24770mergify[bot] merged 17 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Exemption Request |
Unfortunately, unless the tests are updated, the build will not pass so we cannot grant this exemption. |
|
Actually, in this case it's failing because the tests aren't using |
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
|
@TheRealAmazonKendra I also deprecated |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
| */ | ||
| export enum TableEncryption { | ||
| /** | ||
| * Previous option. Tables can not be unencrypted now. |
There was a problem hiding this comment.
Given that aws-glue is still alpha, we should consider removing this property altogether, so we don't carry this dead weight up to graduation.
I'd like to hear what other contributors think.
There was a problem hiding this comment.
It sounds fair to me.
Waiting for some more comments and I'll proceed.
There was a problem hiding this comment.
Agreed on this.
There was a problem hiding this comment.
Removed UNENCRYPTED property.
Co-authored-by: Kendra Neil <[email protected]>
Pull request has been modified.
Co-authored-by: Kendra Neil <[email protected]>
There was a problem hiding this comment.
The behaviour of Bucket hasn't changed, so we shouldn't change the tests, either. If users pass UNENCRYPTED, we still want it to work the same way as before, which means we still need these tests in place.
Having said that, the test coverage for this feature has a lot of gaps, at the moment, such as the S3_MANAGED case. So, please either convert them to new test cases or don't modify the tests at all (apart from replacing test with testDeprecated where appropriate).
There was a problem hiding this comment.
Thanks. Modified S3_MANAGED tests to new test cases.
otaviomacedo
left a comment
There was a problem hiding this comment.
Thanks, @clueleaf. I think we're almost there. Just check out my comment about the unit tests, which I didn't catch in the first pass.
Pull request has been modified.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
S3 now applies SSE-S3 by default if server side encryption is not configured.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/serv-side-encryption.html
This PR simply deprecates
UNENCRYPTEDconfiguration ofBucketEncryption, and changes some docs because they do not seem precise anymore.Behavior of S3 bucket creation is not modified. Many other constructs create S3 buckets internally and updating all those related constructs will result in a huge impact.
This PR also deprecates
TableEncryption.UNENCRYPTEDof AWS Glue and creates an S3 bucket withS3_MANAGEDif table encryption is not specified.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license