-
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
feat(cloudfront): vpc origins #33318
Conversation
httpsPort: props.httpsPort, | ||
name: props.vpcOriginName ?? Names.uniqueResourceName(this, {}), | ||
originProtocolPolicy: props.protocolPolicy, | ||
originSslProtocols: props.originSslProtocols ?? [OriginSslPolicy.TLS_V1_2], |
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.
The default value of originSslProtocols
is ['SSLv3', 'TLSv1']
.
This explicit default ['TLSv1.2']
is same as the AWS management console.
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.
For details, see #33318 (comment)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33318 +/- ##
=======================================
Coverage 82.16% 82.16%
=======================================
Files 119 119
Lines 6857 6857
Branches 1157 1157
=======================================
Hits 5634 5634
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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)
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)
Dismissing outdated PRLinter review.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
I apologize in advance for not being too familiar with CloudFront and CloudFront-Origin modules.
Can you explain to me the difference between the two VpcOrigins
(one of cloudfront module and one in cloudfront-origin module). It's very confusing for me and I see that in the added unit test of cloudfront-origins
,
const instance = new ec2.Instance(stack, 'Instance', {
vpc,
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE4_GRAVITON, ec2.InstanceSize.MICRO),
machineImage: ec2.MachineImage.latestAmazonLinux2023(),
});
// WHEN
new cloudfront.Distribution(stack, 'Distribution', {
defaultBehavior: {
origin: VpcOrigin.withEc2Instance(instance),
},
});
This will create a AWS resource called AWS::CloudFront::VpcOrigin
but i don't see how it's being created as VpcOrigin
from cloudfront-origin
module doesn't create the L1 resource.
httpsPort: props.httpsPort, | ||
name: props.vpcOriginName ?? Names.uniqueResourceName(this, { maxLength: 64 }), | ||
originProtocolPolicy: props.protocolPolicy, | ||
originSslProtocols: props.originSslProtocols ?? [OriginSslPolicy.TLS_V1_2], |
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.
Which source did you see the default value? I don't see it in the CloudFormation documentation.
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.
I created a simple CfnVpcOrigin
resource:
export class VpcOrigin extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);
const vpc = ec2.Vpc.fromLookup(this, 'Vpc', { isDefault: true });
const alb = new elbv2.ApplicationLoadBalancer(this, 'ALB', { vpc });
new cloudfront.CfnVpcOrigin(this, 'VpcOrigin', {
vpcOriginEndpointConfig: { name: 'ALBVpcOrigin', arn: alb.loadBalancerArn },
});
}
}
Created VPC origin in AWS management console shows the default values:
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.vpcOriginArn = this.getResourceArnAttribute(resource.attrArn, { | ||
service: 'cloudfront', | ||
region: '', |
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.
is vpc origin arn region agnostic?
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.
Resource types defined by Amazon CloudFront
Resource types ARN vpcorigin
arn:${Partition}:cloudfront::${Account}:vpcorigin/${Id}
} | ||
} | ||
|
||
class VpcOriginWithVpcOrigin extends VpcOrigin { |
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 name is very confusing
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.
Please see my comment at #33318 (comment)
machineImage: ec2.MachineImage.latestAmazonLinux2023(), | ||
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED }, | ||
}); | ||
new cloudfront.VpcOrigin(stack, 'VpcOriginFromInstance', { |
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.
What's the purpose of this call?
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 integ test verifies AWS::CloudFront::VpcOrigin
resources are deployed with no errors.
The VpcOrigin resources are not attached to any CloudFront distributions in this test.
} | ||
} | ||
|
||
class VpcOriginWithEndpoint extends VpcOrigin { |
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.
what's the use of this class if it's not exported?
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.
Similar to S3BucketOriginWithOAC
and S3BucketOriginWithOAI
.
VpcOriginWith****
is used internally by VpcOrigin.with***
static methods.
S3BucketOrigin.withOriginAccessControl
static method creates S3BucketOriginWithOAC
.
aws-cdk/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts
Lines 70 to 72 in 4b0094b
public static withOriginAccessControl(bucket: IBucket, props?: S3BucketOriginWithOACProps): cloudfront.IOrigin { | |
return new S3BucketOriginWithOAC(bucket, props); | |
} |
S3BucketOriginWithOAC
is not exported.class S3BucketOriginWithOAC extends S3BucketOrigin { |
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. |
Pull request has been modified.
@GavinZZ Thank you for your review.
cloudfront.VpcOriginThis is the L2 construct represents This resource is very simliar to
cloudfront_origins.VpcOriginThis is the helper class represents VPC origin similar to
Instead, we cloud create separated origin classes:
Example1 - simple usage: declare const alb: elbv2.ApplicationLoadBalancer;
new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
origin: new origins.VpcApplicationLoadBalancerOrigin(alb),
}
}); Example2 - create explicit declare const alb: elbv2.ApplicationLoadBalancer;
const vpcOrigin = new cloudfront.VpcOrigin(this, 'VpcOrigin', {
endpoint: cloudfront.VpcOrigin.applicationLoadBalancer(alb),
});
new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
origin: new origins.XxxxxxxxxOrigin(vpcOrigin), // How to name the class?
}
}); |
L1 resource is created here: class VpcOriginWithEndpoint extends VpcOrigin {
// ...
public bind(_scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
this.vpcOrigin ??= new cloudfront.VpcOrigin(_scope, 'VpcOrigin', {
endpoint: this.vpcOriginEndpoint,
vpcOriginName: this.props.vpcOriginName,
httpPort: this.props.httpPort,
httpsPort: this.props.httpsPort,
protocolPolicy: this.props.protocolPolicy,
originSslProtocols: this.props.originSslProtocols,
});
return super.bind(_scope, options);
}
} Similar to how aws-cdk/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts Lines 120 to 123 in 4b0094b
|
|
||
// Call ec2:DescribeSecurityGroups API to retrieve the VPC origins security group. | ||
const getSg = new cr.AwsCustomResource(this, 'GetSecurityGroup', { | ||
onCreate: { |
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.
Should the docs also provide an onUpdate
and onDelete
?
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.
onUpdate is needed when the vpc id is changed but unusual.
onDelete is not needed because this custom resource creates no resources.
The security group will be deleted by CloudFront after all VPC origins (in the VPC) are deleted.
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.
Ah okay, my bad. I thought the onDelete
would be triggered when the origin would be deleted, therefore it would have to detach itself from the security group and therefore would need the security group ids and invoke this custom resource again.
Co-authored-by: Gijs Martens <gijs.martens@coolblue.nl>
Co-authored-by: Gijs Martens <gijs.martens@coolblue.nl>
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.
@Tietew thanks for the detailed answer to my questions. That's really helpful to me to understand the changes. LGTM.
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). |
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 |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #32396.
Reason for this change
VPC origins has been added to CloudFront and now CloudFormation supports it.
For details, see https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-vpc-origins.html
Description of changes
Added an L2 construct
cloudfront.VpcOrigin
forAWS::CloudFront::VpcOrigin
.It will be created implicitly by origin class described below.
You can create it explicitly to share VPC origins between distributions.
Added an origin class
cloudfront_origins.VpcOrigin
for distribution configuration.It can be configured with an Application Load Balancer, a Network Load Balancer, an EC2 instance, or a
cloudfront.VpcOrigin
construct.Describe any new or updated permissions being added
No permissions are added automatically.
See README how to allow connections from VPC origins.
Description of how you validated changes
Unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license