feat(s3-deployment): control object access #15730
Conversation
|
|
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
sormy
changed the title
sormy
changed the title
sormy
force-pushed
the
master
branch
2 times, most recently
from
5b6f201 to
132a0c7
Compare
sormy
changed the title
sormy
force-pushed
the
master
branch
3 times, most recently
from
1c56d62 to
9f8a610
Compare
|
Is there anything else I can do for this CR to proceed? |
There was a problem hiding this comment.
Please see my comments below.
@otaviomacedo - can you also take a look?
| @@ -325,6 +325,7 @@ test('system metadata is correctly transformed', () => { | |||
| websiteRedirectLocation: 'example', | |||
| cacheControl: [s3deploy.CacheControl.setPublic(), s3deploy.CacheControl.maxAge(cdk.Duration.hours(1))], | |||
| expires: expiration, | |||
| accessControl: s3.BucketAccessControl.BUCKET_OWNER_FULL_CONTROL, | |||
There was a problem hiding this comment.
can you add a separate test case for each type of BucketAccessControl, so we can ensure that all types are correctly transformed by toKebabCase()?
There was a problem hiding this comment.
I have added the test.
| @@ -165,6 +166,14 @@ export interface BucketDeploymentProps { | |||
| */ | |||
| readonly serverSideEncryptionCustomerAlgorithm?: string; | |||
|
|
|||
| /** | |||
| * Sets the ACL for the object when the command is performed. | |||
| * If you use this parameter you must have the "s3:PutObjectAcl" permission included in the list of actions for your IAM policy. | |||
There was a problem hiding this comment.
Which IAM policy should have this permission configured?
I'm not very familiar with this module but usually the CDK ensures that the permissions are correctly configured.
There was a problem hiding this comment.
You are right. This permission is granted by construct. I have removed this sentence.
| @@ -177,6 +178,7 @@ new s3deploy.BucketDeployment(this, 'DeployWebsite', { | |||
| storageClass: StorageClass.INTELLIGENT_TIERING, | |||
| serverSideEncryption: ServerSideEncryption.AES_256, | |||
| cacheControl: [CacheControl.setPublic(), CacheControl.maxAge(cdk.Duration.hours(1))], | |||
| accessControl: s3.BucketAccessControl.BUCKET_OWNER_FULL_CONTROL, | |||
There was a problem hiding this comment.
We usually like to add a couple of sentences on what this feature, when it should be used and any caveats.
Some require a separate section but most just a couple of sentences.
There was a problem hiding this comment.
So accessControl is one of system metadata properties like serverSideEncryption and etc. It seems like there is no additional information provided for other system metadata properties here. I have added a note about how system metadata maps to aws s3 sync arguments and where additional information can be obtained.
| @@ -165,6 +166,14 @@ export interface BucketDeploymentProps { | |||
| */ | |||
| readonly serverSideEncryptionCustomerAlgorithm?: string; | |||
|
|
|||
| /** | |||
| * Sets the ACL for the object when the command is performed. | |||
There was a problem hiding this comment.
Should this be "objects in this bucket"?
There was a problem hiding this comment.
I have updated docblock to match other properties related to system metadata.
nija-at
changed the title
|
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 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Add support for BucketDeployment accessControl property to aws-s3-deployment package. This is needed to run, for example, `aws s3 sync` with `--acl bucket-owner-full-control`. Without this feature there is no easy way (without hacking cdk nodes) to sync assets to bucket located in another account. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for BucketDeployment accessControl property to aws-s3-deployment package. This is needed to run, for example, `aws s3 sync` with `--acl bucket-owner-full-control`. Without this feature there is no easy way (without hacking cdk nodes) to sync assets to bucket located in another account. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for BucketDeployment accessControl property to aws-s3-deployment package. This is needed to run, for example, `aws s3 sync` with `--acl bucket-owner-full-control`. Without this feature there is no easy way (without hacking cdk nodes) to sync assets to bucket located in another account. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for BucketDeployment accessControl property to aws-s3-deployment package.
This is needed to run, for example,
aws s3 syncwith--acl bucket-owner-full-control.Without this feature there is no easy way (without hacking cdk nodes) to sync assets to bucket located in another account.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license