Skip to content
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.

Sign up for GitHub

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-origins): allow setting a user defined origin id #22349

Merged
merged 3 commits into from
Oct 12, 2022
Merged

feat(cloudfront-origins): allow setting a user defined origin id #22349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a77eaaa
feat(cloudfront-origins): allow setting a user defined origin id
corymhall Oct 4, 2022
ed2e1e6
Merge branch 'main' into corymhall/cloudfront/originid
mergify[bot] Oct 4, 2022
8ca142d
Merge branch 'main' into corymhall/cloudfront/originid
mergify[bot] Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-cloudfront-origins/test/http-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ test('Renders minimal example with just a domain name', () => {

test('renders an example with all available props', () => {
const origin = new HttpOrigin('www.example.com', {
originId: 'MyCustomOrigin',
originPath: '/app',
connectionTimeout: Duration.seconds(5),
connectionAttempts: 2,
Expand All @@ -44,7 +45,7 @@ test('renders an example with all available props', () => {
const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });

expect(originBindConfig.originProperty).toEqual({
id: 'StackOrigin029E19582',
id: 'MyCustomOrigin',
domainName: 'www.example.com',
originPath: '/app',
connectionTimeout: 5,
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-cloudfront-origins/test/load-balancer-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,12 @@ test('Can customize properties of the origin', () => {
connectionAttempts: 3,
connectionTimeout: Duration.seconds(5),
protocolPolicy: cloudfront.OriginProtocolPolicy.MATCH_VIEWER,
originId: 'MyCustomOrigin',
});
const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });

expect(originBindConfig.originProperty).toEqual({
id: 'StackOrigin029E19582',
id: 'MyCustomOrigin',
domainName: loadBalancer.loadBalancerDnsName,
connectionAttempts: 3,
connectionTimeout: 5,
Expand Down
79 changes: 79 additions & 0 deletions packages/@aws-cdk/aws-cloudfront-origins/test/origin-group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,85 @@ describe('Origin Groups', () => {
});
});

test('correctly render the OriginGroups property of DistributionConfig with originId set', () => {
const failoverOrigin = new origins.S3Origin(s3.Bucket.fromBucketName(stack, 'ImportedBucket', 'imported-bucket'), { originId: 'MyCustomOrigin1' });
const originGroup = new origins.OriginGroup({
primaryOrigin,
fallbackOrigin: failoverOrigin,
fallbackStatusCodes: [500],
});

new cloudfront.Distribution(stack, 'Distribution', {
defaultBehavior: { origin: originGroup },
});

const primaryOriginId = 'DistributionOrigin13547B94F';
const originGroupId = 'DistributionOriginGroup1A1A31B49';
Template.fromStack(stack).hasResourceProperties('AWS::CloudFront::Distribution', {
DistributionConfig: {
DefaultCacheBehavior: {
TargetOriginId: originGroupId,
},
Origins: [
{
Id: primaryOriginId,
DomainName: {
'Fn::GetAtt': ['Bucket83908E77', 'RegionalDomainName'],
},
S3OriginConfig: {
OriginAccessIdentity: {
'Fn::Join': ['', [
'origin-access-identity/cloudfront/',
{ Ref: 'DistributionOrigin1S3Origin5F5C0696' },
]],
},
},
},
{
Id: 'MyCustomOrigin1',
DomainName: {
'Fn::Join': ['', [
'imported-bucket.s3.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
]],
},
S3OriginConfig: {
OriginAccessIdentity: {
'Fn::Join': ['', [
'origin-access-identity/cloudfront/',
{ Ref: 'DistributionOrigin2S3OriginE484D4BF' },
]],
},
},
},
],
OriginGroups: {
Items: [
{
FailoverCriteria: {
StatusCodes: {
Items: [500],
Quantity: 1,
},
},
Id: 'DistributionOriginGroup1A1A31B49',
Members: {
Items: [
{ OriginId: primaryOriginId },
{ OriginId: 'MyCustomOrigin1' },
],
Quantity: 2,
},
},
],
Quantity: 1,
},
},
});
});

test('cannot have an Origin with their own failover configuration as the primary Origin', () => {
const failoverOrigin = new origins.S3Origin(s3.Bucket.fromBucketName(stack, 'ImportedBucket', 'imported-bucket'));
const originGroup = new origins.OriginGroup({
Expand Down
53 changes: 53 additions & 0 deletions packages/@aws-cdk/aws-cloudfront-origins/test/rest-api-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,56 @@ test('Correctly renders the origin', () => {
},
});
});

test('Correctly renders the origin, with custom originId', () => {
const api = new apigateway.RestApi(stack, 'RestApi');
api.root.addMethod('GET');

const origin = new RestApiOrigin(api, { originId: 'MyCustomOrigin' });
const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });

expect(stack.resolve(originBindConfig.originProperty)).toEqual({
id: 'MyCustomOrigin',
domainName: {
'Fn::Select': [2, {
'Fn::Split': ['/', {
'Fn::Join': ['', [
'https://', { Ref: 'RestApi0C43BF4B' },
'.execute-api.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
'/',
{ Ref: 'RestApiDeploymentStageprod3855DE66' },
'/',
]],
}],
}],
},
originPath: {
'Fn::Join': ['', ['/', {
'Fn::Select': [3, {
'Fn::Split': ['/', {
'Fn::Join': ['', [
'https://',
{ Ref: 'RestApi0C43BF4B' },
'.execute-api.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
'/',
{ Ref: 'RestApiDeploymentStageprod3855DE66' },
'/',
]],
}],
}],
}]],
},
customOriginConfig: {
originProtocolPolicy: 'https-only',
originSslProtocols: [
'TLSv1.2',
],
},
});
});
70 changes: 70 additions & 0 deletions packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,61 @@ describe('With bucket', () => {
},
});
});

test('Can set a custom originId', () => {
const bucket = new s3.Bucket(stack, 'Bucket');
const bucket2 = new s3.Bucket(stack, 'Bucket2');
const origin2 = new S3Origin(bucket2, { originId: 'MyOtherCustomOrigin' });
const origin = new S3Origin(bucket, { originId: 'MyCustomOrigin' });
const distro = new cloudfront.Distribution(stack, 'Dist', {
defaultBehavior: { origin },
});
distro.addBehavior('/test', origin2);

Template.fromStack(stack).hasResourceProperties('AWS::CloudFront::Distribution', {
DistributionConfig: {
CacheBehaviors: [
{
CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6',
Compress: true,
PathPattern: '/test',
TargetOriginId: 'MyOtherCustomOrigin',
ViewerProtocolPolicy: 'allow-all',
},
],
DefaultCacheBehavior: {
CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6',
Compress: true,
TargetOriginId: 'MyCustomOrigin',
ViewerProtocolPolicy: 'allow-all',
},
Origins: [
{
Id: 'MyCustomOrigin',
},
{
Id: 'MyOtherCustomOrigin',
},
],
},
});
});
test('Cannot set an originId duplicates', () => {
const bucket = new s3.Bucket(stack, 'Bucket');
const bucket2 = new s3.Bucket(stack, 'Bucket2');
const origin = new S3Origin(bucket, { originId: 'MyCustomOrigin' });
const origin2 = new S3Origin(bucket2, { originId: 'MyCustomOrigin' });
expect(() => {
new cloudfront.Distribution(stack, 'Dist', {
defaultBehavior: { origin },
additionalBehaviors: {
Origin2: {
origin: origin2,
},
},
});
}).toThrow(/Origin with id MyCustomOrigin already exists/);
});
});

describe('With website-configured bucket', () => {
Expand Down Expand Up @@ -195,4 +250,19 @@ describe('With website-configured bucket', () => {
},
});
});

test('Can set an originId', () => {
const bucket = new s3.Bucket(stack, 'Bucket', {
websiteIndexDocument: 'index.html',
});
const origin = new S3Origin(bucket, { originId: 'MyCustomOrigin' });
new cloudfront.Distribution(stack, 'Dist', { defaultBehavior: { origin } });
Template.fromStack(stack).hasResourceProperties('AWS::CloudFront::Distribution', {
DistributionConfig: {
Origins: [{
Id: 'MyCustomOrigin',
}],
},
});
});
});
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-cloudfront/lib/distribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,10 @@ export class Distribution extends Resource implements IDistribution {
const scope = new Construct(this, `Origin${originIndex}`);
const originId = Names.uniqueId(scope).slice(-ORIGIN_ID_MAX_LENGTH);
const originBindConfig = origin.bind(scope, { originId });
const duplicateId = this.boundOrigins.find(boundOrigin => boundOrigin.originProperty?.id === originBindConfig.originProperty?.id);
if (duplicateId) {
throw new Error(`Origin with id ${duplicateId.originProperty?.id} already exists. OriginIds must be unique within a distribution`);
}
if (!originBindConfig.failoverConfig) {
this.boundOrigins.push({ origin, originId, ...originBindConfig });
} else {
Expand All @@ -370,7 +374,7 @@ export class Distribution extends Resource implements IDistribution {
this.addOriginGroup(originGroupId, originBindConfig.failoverConfig.statusCodes, originId, failoverOriginId);
return originGroupId;
}
return originId;
return originBindConfig.originProperty?.id ?? originId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? originBindConfig.originProperty.id === originId under our implementation of OriginBase.bind().

I suppose someone could write their own CustomOrigin extends IOrigin and implement bind where the given originId does not become the originBindConfig.originProperty.id, but is that what we are defending against here?

If so... fine. But these will 99% of the time be the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will be different if the user provides their own ID.

For example.

const origin = new S3Origin(bucket, { originId: 'MyCustomOrigin' });

When we then call bind on line 358 we pass in the generated originId (from line 357. The change I made in bind is to take the originId passed in S3Origin first. So originBindConfig.originProperty.id will be MyCustomOrigin.

}
}

Expand Down
11 changes: 10 additions & 1 deletion packages/@aws-cdk/aws-cloudfront/lib/origin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ export interface OriginOptions {
* @default - origin shield not enabled
*/
readonly originShieldRegion?: string;

/**
* A unique identifier for the origin. This value must be unique within the distribution.
*
* @default - an originid will be generated for you
*/
readonly originId?: string;
}

/**
Expand Down Expand Up @@ -118,6 +125,7 @@ export abstract class OriginBase implements IOrigin {
private readonly connectionAttempts?: number;
private readonly customHeaders?: Record<string, string>;
private readonly originShieldRegion?: string
private readonly originId?: string;

protected constructor(domainName: string, props: OriginProps = {}) {
validateIntInRangeOrUndefined('connectionTimeout', 1, 10, props.connectionTimeout?.toSeconds());
Expand All @@ -130,6 +138,7 @@ export abstract class OriginBase implements IOrigin {
this.connectionAttempts = props.connectionAttempts;
this.customHeaders = props.customHeaders;
this.originShieldRegion = props.originShieldRegion;
this.originId = props.originId;
}

/**
Expand All @@ -146,7 +155,7 @@ export abstract class OriginBase implements IOrigin {
return {
originProperty: {
domainName: this.domainName,
id: options.originId,
id: this.originId ?? options.originId,
originPath: this.originPath,
connectionAttempts: this.connectionAttempts,
connectionTimeout: this.connectionTimeout?.toSeconds(),
Expand Down