-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): Initial CloudFront redesign #8982
Conversation
Enables creating a basic, non-customizable distribution from an S3 bucket. Example: new Distribution(this, 'dist', { origin: Origin.fromBucket(websiteBucket), });
… Resources (instead of Constructs)
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
@njlynch @rix0rrr @skinny85 Following our discussion today, wanted to drop my thoughts on the whole behavior/origin relationship. If you still feel uncomfortable with this and want to proceed with the current modeling, i am also perfectly cool with that. This is what I had in mind: // 90th percentile: only default behavior
new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
origin: cloudfront.Origin.fromBucket(myBucket, {}),
allowedMethods: ALL,
compress: true
}
})
// 10th percentile: 1 default behavior, 3 custom behaviors
// at construction
new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
origin: cloudfront.Origin.fromBucket(myBucket),
allowedMethods: ALL,
compress: true
},
additionalBehaviors: {
'/api/*': {
origin: cloudfront.Origin.fromHttpServer({
domainName: 'www.example.com',
protocolPolicy: OriginProtocolPolicy.HTTPS_ONLY,
}),
allowedMethods: ALL,
compress: true
},
'/api/_admin/*': {
origin: cloudfront.Origin.fromHttpServer({
domainName: 'www.example.com',
protocolPolicy: OriginProtocolPolicy.HTTPS_ONLY,
}),
allowedMethods: [GET],
compress: false
},
'/assets/*': {
origin: cloudfront.Origin.fromBucket(myBucket),
allowedMethods: ALL,
compress: true
}
}
})
// post construction
const distribution = new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
origin: cloudfront.Origin.fromBucket(myBucket),
allowedMethods: ALL,
compress: true
}
})
distribution.addBehavior('/api/*', {
origin: cloudfront.Origin.fromHttpServer({
domainName: 'www.example.com',
protocolPolicy: OriginProtocolPolicy.HTTPS_ONLY,
}),
allowedMethods: ALL,
compress: true
});
distribution.addBehavior('/api/_admin/*', {
origin: cloudfront.Origin.fromHttpServer({
domainName: 'www.example.com',
protocolPolicy: OriginProtocolPolicy.HTTPS_ONLY,
}),
allowedMethods: [GET],
compress: false
});
distribution.addBehavior('/assets/*', {
origin: cloudfront.Origin.fromHttpServer(myBucket),
allowedMethods: ALL,
compress: true
}); Basically flipping the model and only thinking about behaviors rather than origins. An origin is just a property of a specific behavior (which is actually how CFN models it as well) Notice that in the example above we have multiple behaviors that map to the same origin, creating a somewhat awkward N:1 mapping. Is it though?
Thoughts? |
So the minimalist example would be: new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: { origin: cloudfront.Origin.fromBucket(myBucket) },
}); It's a little more verbose than the RFC-proposed minimum, but it's still certainly sleeker than what we have today. It also definitely codifies the defaultBehavior up-front, which solves the main gap. With the multi-behavior example, we're ending up with 4 origins where we only need 2. Could we solve this before synthesis? Is there a pattern within the CDK to effectively hash a Construct to establish uniqueness so we don't end up creating multiple duplicate origins? Overall thoughts? I absolutely hate @rix0rrr / @skinny85 - Whaddya think? Should we adopt this model? |
This change also extends the set of properties available on Distribution, Behavior, and Origin, and adds support for multi-origin and multi-behavior setups.
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.
As this is my first significant implementation, and first of a new module, I added some review comments to areas where I was unsure of my approach and wanted feedback.
/** | ||
* The price class determines how many edge locations CloudFront will use for your distribution. | ||
*/ | ||
export enum PriceClass { |
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.
Note: These were moved from the web_distribution
file. Could cause breakage for customers who have imported directly by filename.
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.
Guess we can live with that.
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.
Readme looks great. See some comments.
/** | ||
* The price class determines how many edge locations CloudFront will use for your distribution. | ||
*/ | ||
export enum PriceClass { |
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.
Guess we can live with that.
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.
Very nice!
Suggestions from review. Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
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.
Looks great!
Next time please "resolve" any comments you addressed so it's easier to follow up
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). |
See aws/aws-cdk-rfcs#174 for the design that inspired this implementation. The goal of this initial PR is to get the new Distribution construct to the point where it supports the most common use cases, in terms of single- and multi-origin and behaviors with the most common properties exposed and working. This is not intended to be a complete/exhaustive implementation, but does allow for users with S3 buckets to create CloudFront distributions more easily than the existing construct today. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
See aws/aws-cdk-rfcs#174 for the design that inspired this implementation.
The goal of this initial PR is to get the new Distribution construct to the point where it supports the most common use cases, in terms of single- and multi-origin and behaviors with the most common properties exposed and working. This is not intended to be a complete/exhaustive implementation, but does allow for users with S3 buckets to create CloudFront distributions more easily than the existing construct today.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license