From e6bb60dc5cb186dca16b70daf4990e845b2825e1 Mon Sep 17 00:00:00 2001 From: Eugene Kozlov <68875428+kozlove-aws@users.noreply.github.com> Date: Wed, 17 Feb 2021 17:37:46 -0600 Subject: [PATCH] fix(deadline): VersionQuery cross-stack issue (#306) --- .../aws-rfdk/lib/deadline/lib/render-queue.ts | 26 ++++++ .../aws-rfdk/lib/deadline/lib/repository.ts | 17 ++++ .../deadline/lib/thinkbox-docker-images.ts | 11 +++ .../lib/deadline/test/render-queue.test.ts | 86 ++++++++++++------- .../lib/deadline/test/repository.test.ts | 22 ++++- .../test/thinkbox-docker-images.test.ts | 21 ++++- 6 files changed, 147 insertions(+), 36 deletions(-) diff --git a/packages/aws-rfdk/lib/deadline/lib/render-queue.ts b/packages/aws-rfdk/lib/deadline/lib/render-queue.ts index 7aeab38ab..276a41bcf 100644 --- a/packages/aws-rfdk/lib/deadline/lib/render-queue.ts +++ b/packages/aws-rfdk/lib/deadline/lib/render-queue.ts @@ -52,13 +52,16 @@ import { import { Construct, IConstruct, + Stack, } from '@aws-cdk/core'; import { ECSConnectOptions, InstanceConnectOptions, IRepository, + IVersion, RenderQueueProps, + VersionQuery, } from '.'; import { @@ -232,6 +235,11 @@ export class RenderQueue extends RenderQueueBase implements IGrantable { */ private readonly taskDefinition: Ec2TaskDefinition; + /** + * The version of Deadline installed in the container images + */ + private readonly version?: IVersion; + constructor(scope: Construct, id: string, props: RenderQueueProps) { super(scope, id); @@ -243,6 +251,8 @@ export class RenderQueue extends RenderQueueBase implements IGrantable { throw new Error(`renderQueueSize.desired cannot be greater than 1 - got ${props.renderQueueSize.desired}`); } + this.version = props?.version; + let externalProtocol: ApplicationProtocol; if ( props.trafficEncryption?.externalTLS ) { externalProtocol = ApplicationProtocol.HTTPS; @@ -444,6 +454,22 @@ export class RenderQueue extends RenderQueueBase implements IGrantable { tagConstruct(this); } + protected onValidate(): string[] { + const validationErrors = []; + + // Using the output of VersionQuery across stacks can cause issues. CloudFormation stack outputs cannot change if + // a resource in another stack is referencing it. + if (this.version instanceof VersionQuery) { + const versionStack = Stack.of(this.version); + const thisStack = Stack.of(this); + if (versionStack != thisStack) { + validationErrors.push('A VersionQuery can not be supplied from a different stack'); + } + } + + return validationErrors; + } + /** * @inheritdoc */ diff --git a/packages/aws-rfdk/lib/deadline/lib/repository.ts b/packages/aws-rfdk/lib/deadline/lib/repository.ts index 62303edab..9785c1a19 100644 --- a/packages/aws-rfdk/lib/deadline/lib/repository.ts +++ b/packages/aws-rfdk/lib/deadline/lib/repository.ts @@ -64,6 +64,7 @@ import { import { DatabaseConnection } from './database-connection'; import { IHost } from './host-ref'; +import { VersionQuery } from './version-query'; import { IVersion } from './version-ref'; /** @@ -585,6 +586,22 @@ export class Repository extends Construct implements IRepository { tagConstruct(this); } + protected onValidate(): string[] { + const validationErrors = []; + + // Using the output of VersionQuery across stacks can cause issues. CloudFormation stack outputs cannot change if + // a resource in another stack is referencing it. + if (this.version instanceof VersionQuery) { + const versionStack = Stack.of(this.version); + const thisStack = Stack.of(this); + if (versionStack != thisStack) { + validationErrors.push('A VersionQuery can not be supplied from a different stack'); + } + } + + return validationErrors; + } + /** * @inheritdoc */ diff --git a/packages/aws-rfdk/lib/deadline/lib/thinkbox-docker-images.ts b/packages/aws-rfdk/lib/deadline/lib/thinkbox-docker-images.ts index 568aa142f..8a517fe75 100644 --- a/packages/aws-rfdk/lib/deadline/lib/thinkbox-docker-images.ts +++ b/packages/aws-rfdk/lib/deadline/lib/thinkbox-docker-images.ts @@ -20,6 +20,7 @@ import { Construct, CustomResource, Duration, + Stack, Token, } from '@aws-cdk/core'; @@ -28,6 +29,7 @@ import { RenderQueueImages, ThinkboxManagedDeadlineDockerRecipes, UsageBasedLicensingImages, + VersionQuery, } from '.'; /** @@ -225,6 +227,15 @@ AWS Thinkbox EULA. errors.push(ThinkboxDockerImages.AWS_THINKBOX_EULA_MESSAGE); } + // Using the output of VersionQuery across stacks can cause issues. CloudFormation stack outputs cannot change if + // a resource in another stack is referencing it. + if (this.version instanceof VersionQuery) { + const versionStack = Stack.of(this.version); + const thisStack = Stack.of(this); + if (versionStack != thisStack) { + errors.push('A VersionQuery can not be supplied from a different stack'); + } + } return errors; } diff --git a/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts b/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts index 731e399ee..e7046138b 100644 --- a/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts +++ b/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts @@ -14,6 +14,7 @@ import { not, objectLike, ResourcePart, + SynthUtils, } from '@aws-cdk/assert'; import { Certificate, @@ -81,6 +82,7 @@ describe('RenderQueue', () => { let repository: Repository; let version: IVersion; + let renderQueueVersion: IVersion; let renderQueueCommon: RenderQueue; @@ -99,10 +101,11 @@ describe('RenderQueue', () => { images = { remoteConnectionServer: rcsImage, }; + renderQueueVersion = new VersionQuery(stack, 'Version'); renderQueueCommon = new RenderQueue(stack, 'RenderQueueCommon', { images, repository, - version, + version: renderQueueVersion, vpc, }); }); @@ -223,7 +226,7 @@ describe('RenderQueue', () => { new RenderQueue(isolatedStack, 'RenderQueue', { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, renderQueueSize: {}, }); @@ -243,7 +246,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: renderQueueVersion, vpc, renderQueueSize: { min, @@ -262,16 +265,16 @@ describe('RenderQueue', () => { test('configures minimum number of ASG instances', () => { // GIVEN const min = 1; + const isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, renderQueueSize: { min, }, }; - const isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -297,7 +300,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: renderQueueVersion, vpc, renderQueueSize: { desired: 2, @@ -318,16 +321,16 @@ describe('RenderQueue', () => { beforeEach(() => { // GIVEN + isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, renderQueueSize: { desired, }, }; - isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -354,14 +357,14 @@ describe('RenderQueue', () => { beforeEach(() => { // GIVEN + isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: {}, }; - isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -392,16 +395,16 @@ describe('RenderQueue', () => { beforeEach(() => { // GIVEN + isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { internalProtocol: ApplicationProtocol.HTTPS, }, }; - isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN renderQueue = new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -519,16 +522,16 @@ describe('RenderQueue', () => { beforeEach(() => { // GIVEN + isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { internalProtocol: ApplicationProtocol.HTTP, }, }; - isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -563,7 +566,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { externalTLS: { @@ -608,7 +611,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: renderQueueVersion, vpc, trafficEncryption: { externalTLS: { @@ -655,7 +658,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { externalTLS: { @@ -743,7 +746,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { externalTLS: { @@ -778,7 +781,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { externalTLS: { @@ -811,7 +814,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, trafficEncryption: { externalTLS: { @@ -849,7 +852,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, hostname: { zone, @@ -1278,7 +1281,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, hostname: { zone, @@ -1688,10 +1691,11 @@ describe('RenderQueue', () => { availabilityZone: 'us-west-2b', }), ]; + const isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, vpcSubnets: { subnets, @@ -1700,7 +1704,6 @@ describe('RenderQueue', () => { subnets, }, }; - const isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -1721,14 +1724,14 @@ describe('RenderQueue', () => { test('can specify instance type', () => { // GIVEN + const isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, instanceType: InstanceType.of(InstanceClass.C5, InstanceSize.LARGE), repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, }; - const isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -1741,14 +1744,14 @@ describe('RenderQueue', () => { test('no deletion protection', () => { // GIVEN + const isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, deletionProtection: false, }; - const isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -1768,13 +1771,13 @@ describe('RenderQueue', () => { test('drop invalid http header fields enabled', () => { // GIVEN + const isolatedStack = new Stack(app, 'IsolatedStack'); const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, }; - const isolatedStack = new Stack(app, 'IsolatedStack'); // WHEN new RenderQueue(isolatedStack, 'RenderQueue', props); @@ -1803,7 +1806,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, }; @@ -1832,7 +1835,7 @@ describe('RenderQueue', () => { const props: RenderQueueProps = { images, repository, - version, + version: new VersionQuery(isolatedStack, 'Version'), vpc, hostname: { zone, @@ -2268,4 +2271,23 @@ describe('RenderQueue', () => { }); }); + test('validates VersionQuery is not in a different stack', () => { + // GIVEN + const newStack = new Stack(app, 'NewStack'); + // WHEN + new RenderQueue(newStack, 'RenderQueueNew', { + images, + repository, + version, + vpc, + }); + + // WHEN + function synth() { + SynthUtils.synthesize(newStack); + } + + // THEN + expect(synth).toThrow('A VersionQuery can not be supplied from a different stack'); + }); }); diff --git a/packages/aws-rfdk/lib/deadline/test/repository.test.ts b/packages/aws-rfdk/lib/deadline/test/repository.test.ts index ef342768b..0b930a501 100644 --- a/packages/aws-rfdk/lib/deadline/test/repository.test.ts +++ b/packages/aws-rfdk/lib/deadline/test/repository.test.ts @@ -11,6 +11,7 @@ import { haveResourceLike, ResourcePart, stringLike, + SynthUtils, } from '@aws-cdk/assert'; import {AutoScalingGroup} from '@aws-cdk/aws-autoscaling'; import {DatabaseCluster} from '@aws-cdk/aws-docdb'; @@ -49,6 +50,7 @@ import { DatabaseConnection, IVersion, Repository, + VersionQuery, } from '../lib'; import { REPO_DC_ASSET, @@ -78,7 +80,7 @@ beforeEach(() => { patchVersion: 2, repository: { objectKey: 'testInstaller', - s3Bucket: new Bucket(stack, 'InstallerBucket'), + s3Bucket: new Bucket(stack, 'LinuxInstallerBucket'), }, }, linuxFullVersionString: () => '10.1.9.2', @@ -1058,3 +1060,21 @@ describe('tagging', () => { }, }); }); + +test('validates VersionQuery is not in a different stack', () => { + // GIVEN + const newStack = new Stack(app, 'NewStack'); + version = new VersionQuery(stack, 'Version'); + new Repository(newStack, 'Repository', { + vpc, + version, + }); + + // WHEN + function synth() { + SynthUtils.synthesize(newStack); + } + + // THEN + expect(synth).toThrow('A VersionQuery can not be supplied from a different stack'); +}); \ No newline at end of file diff --git a/packages/aws-rfdk/lib/deadline/test/thinkbox-docker-images.test.ts b/packages/aws-rfdk/lib/deadline/test/thinkbox-docker-images.test.ts index bec255ac9..27771b463 100644 --- a/packages/aws-rfdk/lib/deadline/test/thinkbox-docker-images.test.ts +++ b/packages/aws-rfdk/lib/deadline/test/thinkbox-docker-images.test.ts @@ -36,7 +36,6 @@ import { describe('ThinkboxDockerRecipes', () => { let app: App; - let depStack: Stack; let stack: Stack; let images: ThinkboxDockerImages; let userAwsThinkboxEulaAcceptance: AwsThinkboxEulaAcceptance; @@ -179,9 +178,8 @@ AWS Thinkbox EULA. beforeEach(() => { // GIVEN app = new App(); - depStack = new Stack(app, 'DepStack'); - version = new VersionQuery(depStack, 'Version'); stack = new Stack(app, 'Stack'); + version = new VersionQuery(stack, 'Version'); // WHEN images = new ThinkboxDockerImages(stack, 'Images', { @@ -231,5 +229,22 @@ AWS Thinkbox EULA. })); }); }); + + test('validates VersionQuery is not in a different stack', () => { + // GIVEN + const newStack = new Stack(app, 'NewStack'); + new ThinkboxDockerImages(newStack, 'Images', { + version, + userAwsThinkboxEulaAcceptance, + }); + + // WHEN + function synth() { + SynthUtils.synthesize(newStack); + } + + // THEN + expect(synth).toThrow('A VersionQuery can not be supplied from a different stack'); + }); }); });