feat(cloudfront): s3 origin access control #31150
Conversation
| replace the `S3Origin` construct (now deprecated) with `S3BucketOrigin.withOriginAccessControl()` which automatically | ||
| creates and sets up a OAC for you. | ||
| The OAI will be deleted as part of the | ||
| stack update. The logical IDs of the resources managed by |
There was a problem hiding this comment.
I am concerned that removing OAI permission and adding OAC permission in a single CFN stack deployment will create a short period of time where both permissions are not present for the bucket in rare situation. I have experienced a case where when IAM policy changed via CFN, even though the CFN reports update complete, the IAM policy still isn't truly reflected when invoking the system.
We should update this part of the README to recommend a 2-step migration approach. I will create a todo task to track this.
| * Imports an S3 origin access control from its id. | ||
| */ | ||
| public static fromOriginAccessControlId(scope: Construct, id: string, originAccessControlId: string): IOriginAccessControl { | ||
| class Import extends Resource implements IOriginAccessControl { |
There was a problem hiding this comment.
I believe this should be class Import extends OriginAccessControlBase implements IOriginAccessControl
That is the case for Function:
https:/gracelu0/aws-cdk/blob/b4377a5a3ad6d30730a084de6bc29c909a5c0e9d/packages/aws-cdk-lib/aws-lambda/lib/function.ts#L708
There was a problem hiding this comment.
Yep makes sense, I'll update it
| /** | ||
| * The identifier of the Distribution this Origin is used for. | ||
| */ | ||
| readonly distributionId: string; |
There was a problem hiding this comment.
This new attribute in this interface is a required attribute. This is a breaking change I believe because classes that implements this interface will need to be updated to have distributionId. Maybe can we make this optional?
There was a problem hiding this comment.
Yes good catch, will update to be optional
| @@ -1118,6 +1134,9 @@ export class CloudFrontWebDistribution extends cdk.Resource implements IDistribu | |||
|
|
|||
| let s3OriginConfig: CfnDistribution.S3OriginConfigProperty | undefined; | |||
| if (originConfig.s3OriginSource) { | |||
| if (originConfig.s3OriginSource.originAccessIdentity && originConfig.s3OriginSource.originAccessControl) { | |||
| throw Error('Only one of origin access identity or origin access control can be defined.'); | |||
| } | |||
There was a problem hiding this comment.
Would this block users from doing 2-step migration approach where they would have both OAI and OAC?
There was a problem hiding this comment.
Removed this as we are deprecating CloudFrontWebDistribution - was an old change I accidentally pushed.
|
Comments on closed issues and PRs are hard for our team to see. |
| // Failed to update bucket policy, assume using imported bucket | ||
| if (!result.statementAdded) { | ||
| Annotations.of(scope).addWarningV2('@aws-cdk/aws-cloudfront-origins:updateBucketPolicy', | ||
| 'Cannot update bucket policy of an imported bucket. Set overrideImportedBucketPolicy to true or update the policy manually instead.'); |
There was a problem hiding this comment.
I searched and cannot find info on overrideImportedBucketPolicy. Was the plan to have it as a feature flag?
Imo, we should just throw the warning as feature flag adds to the code complexity.
There was a problem hiding this comment.
Ah yes it was part of the previous design, I'll remove since we are not updating policies of imported buckets.
| // TODO: update this warning | ||
| Annotations.of(scope).addWarningV2('@aws-cdk/aws-cloudfront-origins:updateKeyPolicy', | ||
| 'If you run into a circular dependency issue during deployment, please refer to README for instructions.'); | ||
| } |
There was a problem hiding this comment.
This warning probably will leave users scratching their head because when they see this warning, they wouldn't have seen the circular dependency error.
We should update this to change the key policy. It will allow users to have other workarounds.
There was a problem hiding this comment.
Do you mean add to the key policy here and let them run into the circular dependency issue? Then at that time they would see the warning message and follow it to configure a workaround
| * An optional Origin Access Control | ||
| * @default - an Origin Access Control will be created. | ||
| */ | ||
| readonly originAccessControl?: cloudfront.IOriginAccessControl; |
There was a problem hiding this comment.
I found that this interface follows this "extends path":
S3BucketOriginWithOACProps -> S3BucketOriginBaseProps -> cloudfront.OriginProps -> OriginOptions
And in OriginOptions, there is a prop originAccessControlId. So the user can do:
origins.S3BucketOrigin.withOriginAccessControl(bucket, {
originAccessControlId: 'oacId123',
originAccessControl: new cloudfront.S3OriginAccessControl(stack, 'MyOac'),
})
Then in the CFN template, the originAccessControlId will be ignored.
There was a problem hiding this comment.
Good catch, we should remove originAccessControlId from OriginOptions and only allow user to configure OAC with object of type IOriginAccessControl. This follows the design guidelines to use strong typing when referencing resources: https:/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#construct-interface.
Issue # (if applicable)
n/a
Reason for this change
Note: This PR is a WIP, not merging to the main branch. Merging to a feature branch first to facilitate smaller PR reviews.
Support OAC for S3 origins, see RFC for more details
Description of changes
CloudFrontWebDistributionin favour of existing classDistributionOriginAccessControlBaseand subclassS3OriginAccessControlS3Originin favour of new classesS3BucketOriginandS3StaticWebsiteOriginS3BucketOriginS3StaticWebsiteOrigincloudfrontandcloudfront-originsDescription of how you validated changes
integ.s3-origin-oac: basic use case setting up a distribution with an S3 origin with OAC, using assertions to check the distribution has the oac configured and the oac has the expected default propertiesS3OriginAccessControl(more tests to follow in coming PRs)
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license