-
Notifications
You must be signed in to change notification settings - Fork 4k
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): clarifying blockPublicAccess behavior with v2 #33668
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33668 +/- ##
=======================================
Coverage 82.21% 82.21%
=======================================
Files 119 119
Lines 6876 6876
Branches 1162 1162
=======================================
Hits 5653 5653
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue
Closes #32811
For clarity I did try a neater solution first #33613
but this was caught as a breaking change
Reason for this change
By default when you create an s3 bucket, all public access is already blocked. However if you then use CDK to specify 1 or more access point you want to unblock, all undefined block types will be auto set to false, and when it deploys you will see everything uncheck even if you only wanted to uncheck 1 thing.
So to fix this we should instead default all values to true when at least 1 option is specified, to mimic to experience when a user in the console unchecks the boxes.
Description of changes
blockPublicAccessV2
property ofBucketProps
and class
BlockPublicAccessV2
created.For the
blockPublicAccessV2
property I just did what probably should have been done to begin with, and assigned it to the typeblockPublicAccessOptions
.The class is only there to help with the preset options.
I added this function to set the automatic
true
defaults if at least 1 option is used,updated the readme context in a few places to indicate that the old method is deprecated
Description of how you validated changes
I added a duplicate test of the existing s3 tests under the same describe pattern.
For other tests not in s3 I just replaced with the new method.
Also added my own integ which tests both the new and old behavior. blockPublicAccess really isn't defined all that often, at least not in the CDK package itself, so most of the tests didn't need to be changes, or simply swapping to V2 didn't change the snapshots since it's so similar to previous.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license