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.

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

fix(cloudfront): fromOriginAccessIdentityName is a misnomer #20772

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,6 @@ class S3BucketOrigin extends cloudfront.OriginBase {
}

protected renderS3OriginConfig(): cloudfront.CfnDistribution.S3OriginConfigProperty | undefined {
return { originAccessIdentity: `origin-access-identity/cloudfront/${this.originAccessIdentity.originAccessIdentityName}` };
return { originAccessIdentity: `origin-access-identity/cloudfront/${this.originAccessIdentity.originAccessIdentityId}` };
}
}
65 changes: 55 additions & 10 deletions packages/@aws-cdk/aws-cloudfront/lib/origin-access-identity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,35 @@ export interface OriginAccessIdentityProps {
*/
export interface IOriginAccessIdentity extends cdk.IResource, iam.IGrantable {
/**
* The Origin Access Identity Name
* The Origin Access Identity Id (physical id)
* It is misnamed and superseded by the correctly named originAccessIdentityId
*
* @deprecated use originAccessIdentityId instead
*/
readonly originAccessIdentityName: string;

/**
* The Origin Access Identity Id (physical id)
* This was called originAccessIdentityName before
*/
readonly originAccessIdentityId: string;
}

abstract class OriginAccessIdentityBase extends cdk.Resource {
/**
* The Origin Access Identity Name (physical id)
* The Origin Access Identity Id (physical id)
* It is misnamed and superseded by the correctly named originAccessIdentityId
*
* @deprecated use originAccessIdentityId instead
*/
public abstract readonly originAccessIdentityName: string;

/**
* The Origin Access Identity Id (physical id)
* This was called originAccessIdentityName before
*/
public abstract readonly originAccessIdentityId: string;

/**
* Derived principal value for bucket access
*/
Expand All @@ -45,7 +64,7 @@ abstract class OriginAccessIdentityBase extends cdk.Resource {
region: '', // global
account: 'cloudfront',
resource: 'user',
resourceName: `CloudFront Origin Access Identity ${this.originAccessIdentityName}`,
resourceName: `CloudFront Origin Access Identity ${this.originAccessIdentityId}`,
},
);
}
Expand All @@ -60,18 +79,32 @@ abstract class OriginAccessIdentityBase extends cdk.Resource {
*/
export class OriginAccessIdentity extends OriginAccessIdentityBase implements IOriginAccessIdentity {
/**
* Creates a OriginAccessIdentity by providing the OriginAccessIdentityName
* Creates a OriginAccessIdentity by providing the OriginAccessIdentityId.
* It is misnamed and superseded by the correctly named fromOriginAccessIdentityId.
*
* @deprecated use `fromOriginAccessIdentityId`
*/
public static fromOriginAccessIdentityName(
scope: Construct,
id: string,
originAccessIdentityName: string): IOriginAccessIdentity {
return OriginAccessIdentity.fromOriginAccessIdentityId(scope, id, originAccessIdentityName);
}

/**
* Creates a OriginAccessIdentity by providing the OriginAccessIdentityId.
*/
public static fromOriginAccessIdentityId(
scope: Construct,
id: string,
originAccessIdentityId: string): IOriginAccessIdentity {

class Import extends OriginAccessIdentityBase {
public readonly originAccessIdentityName = originAccessIdentityName;
public readonly originAccessIdentityId = originAccessIdentityId;
public readonly originAccessIdentityName = originAccessIdentityId;
public readonly grantPrincipal = new iam.ArnPrincipal(this.arn());
constructor(s: Construct, i: string) {
super(s, i, { physicalName: originAccessIdentityName });
super(s, i, { physicalName: originAccessIdentityId });
}
}

Expand All @@ -93,11 +126,23 @@ export class OriginAccessIdentity extends OriginAccessIdentityBase implements IO
public readonly grantPrincipal: iam.IPrincipal;

/**
* The Origin Access Identity Name (physical id)
* The Origin Access Identity Id (physical id)
* It is misnamed and superseded by the correctly named originAccessIdentityId
*
* @attribute
* @deprecated use originAccessIdentityId instead
*/
public get originAccessIdentityName() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do want to call out that Id as an @attribute return value is allegedly "not currently supported by CloudFormation".

If true, and Cloudformation does not actually return this value, I think we should @deprecate public get originAccessIdentiyName and not supply an alternative. It all depends on whether cloudformation does return the Id, which would require someone to test this out themselves and confirm.

Screen Shot 2022-07-01 at 10 49 07 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if they would support it in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested it and it's currently supported if I create an OAI ID and reference the attribute in a CfnOutput. Seems to just work

Copy link
Contributor

Choose a reason for hiding this comment

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

going to remove the label and let this merge

Copy link
Contributor

Choose a reason for hiding this comment

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

correction - we just use ref here which does return the OAI Id. But, using the ID attribute still works will return the exact same value as ref so...

return this.originAccessIdentityId;
}

/**
* The Origin Access Identity Id (physical id)
* This was called originAccessIdentityName before
*
* @attribute
*/
public readonly originAccessIdentityName: string;
public readonly originAccessIdentityId: string;

/**
* CDK L1 resource
Expand All @@ -112,8 +157,8 @@ export class OriginAccessIdentity extends OriginAccessIdentityBase implements IO
this.resource = new CfnCloudFrontOriginAccessIdentity(this, 'Resource', {
cloudFrontOriginAccessIdentityConfig: { comment },
});
// physical id - OAI name
this.originAccessIdentityName = this.getResourceNameAttribute(this.resource.ref);
// physical id - OAI Id
this.originAccessIdentityId = this.getResourceNameAttribute(this.resource.ref);

// Canonical user to grant access to in the S3 Bucket Policy
this.cloudFrontOriginAccessIdentityS3CanonicalUserId = this.resource.attrS3CanonicalUserId;
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1107,7 +1107,7 @@ export class CloudFrontWebDistribution extends cdk.Resource implements IDistribu
}));

s3OriginConfig = {
originAccessIdentity: `origin-access-identity/cloudfront/${originConfig.s3OriginSource.originAccessIdentity.originAccessIdentityName}`,
originAccessIdentity: `origin-access-identity/cloudfront/${originConfig.s3OriginSource.originAccessIdentity.originAccessIdentityId}`,
};
} else {
s3OriginConfig = {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const oai = new cloudfront.CfnCloudFrontOriginAccessIdentity(stack, 'OAI', {
},
});

const oaiImported = cloudfront.OriginAccessIdentity.fromOriginAccessIdentityName(
const oaiImported = cloudfront.OriginAccessIdentity.fromOriginAccessIdentityId(
stack,
'OAIImported',
oai.ref,
Expand Down
11 changes: 10 additions & 1 deletion packages/@aws-cdk/aws-cloudfront/test/oai.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Template } from '@aws-cdk/assertions';
import * as cdk from '@aws-cdk/core';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { OriginAccessIdentity } from '../lib';

describe('Origin Access Identity', () => {
Expand Down Expand Up @@ -61,11 +62,19 @@ describe('Origin Access Identity', () => {
});
});

test('Builds ARN of CloudFront user', () => {
testDeprecated('Builds ARN of CloudFront user for fromOriginAccessIdentityName', () => {
const stack = new cdk.Stack();

const oai = OriginAccessIdentity.fromOriginAccessIdentityName(stack, 'OAI', 'OAITest');

expect(oai.grantPrincipal.policyFragment.principalJson.AWS[0]).toMatch(/:iam::cloudfront:user\/CloudFront Origin Access Identity OAITest$/);
});

test('Builds ARN of CloudFront user for fromOriginAccessIdentityId', () => {
const stack = new cdk.Stack();

const oai = OriginAccessIdentity.fromOriginAccessIdentityId(stack, 'OAI', 'OAITest');

expect(oai.grantPrincipal.policyFragment.principalJson.AWS[0]).toMatch(/:iam::cloudfront:user\/CloudFront Origin Access Identity OAITest$/);
});
});