feat(cloudfront): s3 origin access control L2 construct #30663
Conversation
github-actions
bot
added
effort/medium
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| }); | ||
| ``` | ||
|
|
||
| If the feature flag is not enabled (i.e. set to `false`), an origin access identity will be created by default. |
There was a problem hiding this comment.
| If the feature flag is not enabled (i.e. set to `false`), an origin access identity will be created by default. | |
| If the feature flag is not enabled (`undefined` or set to `false`), an origin access identity will be created by default. |
Or something along the line.
| @@ -15,6 +28,44 @@ export interface S3OriginProps extends cloudfront.OriginProps { | |||
| * @default - An Origin Access Identity will be created. | |||
There was a problem hiding this comment.
Should we update the @default docstring?
There was a problem hiding this comment.
Yes, will specify the feature flag behaviour for creating OAI by default
|
|
||
| /** | ||
| * An optional Origin Access Control | ||
| * @default - An Origin Access Control will be created. |
There was a problem hiding this comment.
nit: maybe mention that OAC will be created when FF is enabeld.
| private originAccessIdentity!: cloudfront.IOriginAccessIdentity; | ||
| abstract class S3BucketOrigin extends cloudfront.OriginBase { | ||
| public static withAccessIdentity(bucket: s3.IBucket, props: S3OriginProps = {}): S3BucketOrigin { | ||
| return new (class OriginAccessIdentity extends S3BucketOrigin { |
There was a problem hiding this comment.
This nested class looks weird to me. Can you instead split them out?
abstract class S3BucketOrigin extends cloudfront.OriginBase {
...
}
class OriginAccessIdentity extends S3BucketOrigin {
...
}
class OriginAccessControl extends S3BucketOrigin {
...
}
And on line 90~100, you can do
else if (props.originAccessControl) {
this.origin = new OriginAccessControl(bucket, props);
} else if (props.originAccessIdentity) {
this.origin = new OriginAccessIdentity(bucket, props);
} else {
this.origin = FeatureFlags.of(bucket.stack)
.isEnabled(cxapi.CLOUDFRONT_USE_ORIGIN_ACCESS_CONTROL_BY_DEFAULT) ?
OriginAccessControl(bucket, props) :
new OriginAccessIdentity(bucket, props);
}
There was a problem hiding this comment.
This was one of the suggested changes from the RFC (see feedback). I agree the structure looks a bit unconventional, but what I like is that the API is very clear on lines 90-100:
else if (props.originAccessControl) {
this.origin = S3BucketOrigin.withAccessControl(bucket, props);
} else if (props.originAccessIdentity) {
this.origin = S3BucketOrigin.withAccessIdentity(bucket, props);
} else {
this.origin = FeatureFlags.of(bucket.stack)
.isEnabled(cxapi.CLOUDFRONT_USE_ORIGIN_ACCESS_CONTROL_BY_DEFAULT) ?
S3BucketOrigin.withAccessControl(bucket, props) :
S3BucketOrigin.withAccessIdentity(bucket, props);
}
since it is a S3 origin with either OAI or OAC configured
| } else if (props.originAccessIdentity) { | ||
| this.origin = S3BucketOrigin.withAccessIdentity(bucket, props); | ||
| } else { | ||
| this.origin = FeatureFlags.of(bucket.stack) |
There was a problem hiding this comment.
Should you check current stack or bucket stack for cross stack usage? IMO ideally it doesn't make a difference since the Feature flag is app-level not stack-level. Just want to confirm.
| If your bucket previously used OAI, there will be a best-effort attempt to remove both the policy statement | ||
| that allows access to the OAI and the origin access identity itself. |
There was a problem hiding this comment.
Curious if the same bucket can be used by two differnt CloudFront distributions?
There was a problem hiding this comment.
Yes, the same S3 origin can be used by multiple CloudFront distributions. Hmm might be tricky then to only remove the OAI associated with the distribution in the current stack if we don't have the OAI ID, is that your concern?
Hi @watany-dev , we are continuing to work on this feature! There was some additional feedback on the RFC, so we will open a new PR for this once the RFC is approved. |
Issue # (if applicable)
Closes #21771 .
Reason for this change
See RFC
Description of changes
See RFC
Description of how you validated changes
Unit tests and integration tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license