From 8913b8326e315c8cea3b9e36d115ad7e187ed723 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 16 Feb 2021 16:39:43 +0100 Subject: [PATCH] fix(core): `exportValue()` does not work with resource names (#13052) Most L2 resources employ the "PhysicalName" protocol, which checks usage of resource names across environment borders, and can automatically turn auto-named resources into physically-named resources if the situation calls for it. Unfortunately, this wrapped token is a generic IResolvable, not a Reference, and so did not work with the `exportValue()` automatic reference detection. Make the token returned by `getResourceNameAttribute()` etc. a `Reference` that mimics the underlying `Reference` to make this work out. Fixes #13002, fixes #12918. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 2 - packages/@aws-cdk/core/README.md | 4 +- packages/@aws-cdk/core/lib/resource.ts | 36 ++++++++-- packages/@aws-cdk/core/lib/token.ts | 25 ++++++- .../core/test/cross-environment-token.test.ts | 70 ++++++++++++++++++- 5 files changed, 123 insertions(+), 14 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index 51b2874311af8..b54957f32d500 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -2405,7 +2405,5 @@ describe('bucket', () => { expect(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true, })).toThrow(/Cannot use \'autoDeleteObjects\' property on a bucket without setting removal policy to \'DESTROY\'/); - - }); }); diff --git a/packages/@aws-cdk/core/README.md b/packages/@aws-cdk/core/README.md index 880aea79a55fc..0e0b38943a803 100644 --- a/packages/@aws-cdk/core/README.md +++ b/packages/@aws-cdk/core/README.md @@ -141,7 +141,7 @@ DEPLOYMENT 1: break the relationship - Make sure `stack2` no longer references `bucket.bucketName` (maybe the consumer stack now uses its own bucket, or it writes to an AWS DynamoDB table, or maybe you just remove the Lambda Function altogether). -- In the `stack1` class, call `this.exportAttribute(this.bucket.bucketName)`. This +- In the `stack1` class, call `this.exportValue(this.bucket.bucketName)`. This will make sure the CloudFormation Export continues to exist while the relationship between the two stacks is being broken. - Deploy (this will effectively only change the `stack2`, but it's safe to deploy both). @@ -149,7 +149,7 @@ DEPLOYMENT 1: break the relationship DEPLOYMENT 2: remove the resource - You are now free to remove the `bucket` resource from `stack1`. -- Don't forget to remove the `exportAttribute()` call as well. +- Don't forget to remove the `exportValue()` call as well. - Deploy again (this time only the `stack1` will be changed -- the bucket will be deleted). ## Durations diff --git a/packages/@aws-cdk/core/lib/resource.ts b/packages/@aws-cdk/core/lib/resource.ts index 917a2c442dcf1..6445fe718c547 100644 --- a/packages/@aws-cdk/core/lib/resource.ts +++ b/packages/@aws-cdk/core/lib/resource.ts @@ -4,11 +4,12 @@ import { IConstruct, Construct as CoreConstruct } from './construct-compat'; import { Construct } from 'constructs'; import { ArnComponents } from './arn'; -import { Lazy } from './lazy'; +import { IStringProducer, Lazy } from './lazy'; import { generatePhysicalName, isGeneratedWhenNeededMarker } from './private/physical-name-generator'; import { IResolveContext } from './resolvable'; import { Stack } from './stack'; -import { Token } from './token'; +import { Token, Tokenization } from './token'; +import { Reference } from './reference'; /** * Represents the environment a given resource lives in. @@ -181,7 +182,7 @@ export abstract class Resource extends CoreConstruct implements IResource { * @experimental */ protected getResourceNameAttribute(nameAttr: string) { - return Lazy.uncachedString({ + return mimicReference(nameAttr, { produce: (context: IResolveContext) => { const consumingStack = Stack.of(context.scope); @@ -214,8 +215,8 @@ export abstract class Resource extends CoreConstruct implements IResource { * @experimental */ protected getResourceArnAttribute(arnAttr: string, arnComponents: ArnComponents) { - return Token.asString({ - resolve: (context: IResolveContext) => { + return mimicReference(arnAttr, { + produce: (context: IResolveContext) => { const consumingStack = Stack.of(context.scope); if (this.stack.environment !== consumingStack.environment) { this._enableCrossEnvironment(); @@ -227,3 +228,28 @@ export abstract class Resource extends CoreConstruct implements IResource { }); } } + +/** + * Produce a Lazy that is also a Reference (if the base value is a Reference). + * + * If the given value is a Reference (or resolves to a Reference), return a new + * Reference that mimics the same target and display name, but resolves using + * the logic of the passed lazy. + * + * If the given value is NOT a Reference, just return a simple Lazy. + */ +function mimicReference(refSource: any, producer: IStringProducer): string { + const reference = Tokenization.reverse(refSource, { + // If this is an ARN concatenation, just fail to extract a reference. + failConcat: false, + }); + if (!Reference.isReference(reference)) { + return Lazy.uncachedString(producer); + } + + return Token.asString(new class extends Reference { + resolve(context: IResolveContext) { + return producer.produce(context); + } + }(reference, reference.target, reference.displayName)); +} \ No newline at end of file diff --git a/packages/@aws-cdk/core/lib/token.ts b/packages/@aws-cdk/core/lib/token.ts index f92a2560cac7c..e17227d0f8ef5 100644 --- a/packages/@aws-cdk/core/lib/token.ts +++ b/packages/@aws-cdk/core/lib/token.ts @@ -164,9 +164,16 @@ export class Tokenization { * * In case of a string, the string must not be a concatenation. */ - public static reverse(x: any): IResolvable | undefined { + public static reverse(x: any, options: ReverseOptions = {}): IResolvable | undefined { if (Tokenization.isResolvable(x)) { return x; } - if (typeof x === 'string') { return Tokenization.reverseCompleteString(x); } + if (typeof x === 'string') { + if (options.failConcat === false) { + // Handle this specially because reverseCompleteString might fail + const fragments = Tokenization.reverseString(x); + return fragments.length === 1 ? fragments.firstToken : undefined; + } + return Tokenization.reverseCompleteString(x); + } if (Array.isArray(x)) { return Tokenization.reverseList(x); } if (typeof x === 'number') { return Tokenization.reverseNumber(x); } return undefined; @@ -220,6 +227,20 @@ export class Tokenization { } } +/** + * Options for the 'reverse()' operation + */ +export interface ReverseOptions { + /** + * Fail if the given string is a concatenation + * + * If `false`, just return `undefined`. + * + * @default true + */ + readonly failConcat?: boolean; +} + /** * Options to the resolve() operation * diff --git a/packages/@aws-cdk/core/test/cross-environment-token.test.ts b/packages/@aws-cdk/core/test/cross-environment-token.test.ts index 4ab55922369e4..a0d833996121c 100644 --- a/packages/@aws-cdk/core/test/cross-environment-token.test.ts +++ b/packages/@aws-cdk/core/test/cross-environment-token.test.ts @@ -244,6 +244,36 @@ nodeunitShim({ }, }); +test.each([undefined, 'SomeName'])('stack.exportValue() on name attributes with PhysicalName=%s', physicalName => { + // Check that automatic exports and manual exports look the same + // GIVEN - auto + const appA = new App(); + const producerA = new Stack(appA, 'Producer'); + const resourceA = new MyResource(producerA, 'Resource', physicalName); + + const consumerA = new Stack(appA, 'Consumer'); + new CfnOutput(consumerA, 'ConsumeName', { value: resourceA.name }); + new CfnOutput(consumerA, 'ConsumeArn', { value: resourceA.arn }); + + // WHEN - manual + const appM = new App(); + const producerM = new Stack(appM, 'Producer'); + const resourceM = new MyResource(producerM, 'Resource', physicalName); + producerM.exportValue(resourceM.name); + producerM.exportValue(resourceM.arn); + + // THEN - producers are the same + const templateA = appA.synth().getStackByName(producerA.stackName).template; + const templateM = appM.synth().getStackByName(producerM.stackName).template; + + expect(templateA).toEqual(templateM); +}); + +test('can instantiate resource with constructed physicalname ARN', () => { + const stack = new Stack(); + new MyResourceWithConstructedArnAttribute(stack, 'Resource'); +}); + class MyResource extends Resource { public readonly arn: string; public readonly name: string; @@ -251,20 +281,54 @@ class MyResource extends Resource { constructor(scope: Construct, id: string, physicalName?: string) { super(scope, id, { physicalName }); - this.arn = this.getResourceArnAttribute('simple-arn', { + const res = new CfnResource(this, 'Resource', { + type: 'My::Resource', + properties: { + resourceName: this.physicalName, + }, + }); + + this.name = this.getResourceNameAttribute(res.ref.toString()); + this.arn = this.getResourceArnAttribute(res.getAtt('Arn').toString(), { region: '', account: '', resource: 'my-resource', resourceName: this.physicalName, service: 'myservice', }); - this.name = this.getResourceNameAttribute('simple-name'); + } +} - new CfnResource(this, 'Resource', { +/** + * Some resources build their own Arn attribute by constructing strings + * + * This will be because the L1 doesn't expose a `{ Fn::GetAtt: ['Arn'] }`. + * + * They won't be able to `exportValue()` it, but it shouldn't crash. + */ +class MyResourceWithConstructedArnAttribute extends Resource { + public readonly arn: string; + public readonly name: string; + + constructor(scope: Construct, id: string, physicalName?: string) { + super(scope, id, { physicalName }); + + const res = new CfnResource(this, 'Resource', { type: 'My::Resource', properties: { resourceName: this.physicalName, }, }); + + this.name = this.getResourceNameAttribute(res.ref.toString()); + this.arn = this.getResourceArnAttribute(Stack.of(this).formatArn({ + resource: 'my-resource', + resourceName: res.ref.toString(), + service: 'myservice', + }), { + resource: 'my-resource', + resourceName: this.physicalName, + service: 'myservice', + }); } }