Skip to content

Commit

Permalink
fix(core): cannot use container assets with new-style synthesis (#8575)
Browse files Browse the repository at this point in the history
When referencing loations that contained unresolved Tokens (
account, region, URL suffix) the synthesizer generated:

    {Fn::Sub: { Fn::Join: ... }}

That is not allowed, the first argument to `{Fn::Sub}` must be a string
literal.

This was a problem for all asset types, but the most-used file asset
(`lambda.Code`) would only render `{ bucketName, objectKey }` which
typically don't contain tokens, so you'd only notice when using
Docker image assets.

Fixes #8540.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Jun 17, 2020
1 parent 82d8f11 commit 357d5f7
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ export class DefaultStackSynthesizer implements IStackSynthesizer {
},
};

const httpUrl = cfnify(`https://s3.${this.stack.region}.${this.stack.urlSuffix}/${this.bucketName}/${objectKey}`);
const { region, urlSuffix } = stackLocationOrInstrinsics(this.stack);
const httpUrl = cfnify(`https://s3.${region}.${urlSuffix}/${this.bucketName}/${objectKey}`);
const s3ObjectUrl = cfnify(`s3://${this.bucketName}/${objectKey}`);

// Return CFN expression
Expand Down Expand Up @@ -275,10 +276,12 @@ export class DefaultStackSynthesizer implements IStackSynthesizer {
},
};

const { account, region, urlSuffix } = stackLocationOrInstrinsics(this.stack);

// Return CFN expression
return {
repositoryName: cfnify(this.repositoryName),
imageUri: cfnify(`${this.stack.account}.dkr.ecr.${this.stack.region}.${this.stack.urlSuffix}/${this.repositoryName}:${imageTag}`),
imageUri: cfnify(`${account}.dkr.ecr.${region}.${urlSuffix}/${this.repositoryName}:${imageTag}`),
};
}

Expand Down Expand Up @@ -408,11 +411,31 @@ function replaceAll(s: string, search: string, replace: string) {
}

/**
* If the string still contains placeholders, wrap it in a Fn::Sub so they will be substituted at CFN deploymen time
* If the string still contains placeholders, wrap it in a Fn::Sub so they will be substituted at CFN deployment time
*
* (This happens to work because the placeholders we picked map directly onto CFN
* placeholders. If they didn't we'd have to do a transformation here).
*/
function cfnify(s: string): string {
return s.indexOf('${') > -1 ? Fn.sub(s) : s;
}

/**
* Return the stack locations if they're concrete, or the original CFN intrisics otherwise
*
* We need to return these instead of the tokenized versions of the strings,
* since we must accept those same ${AWS::AccountId}/${AWS::Region} placeholders
* in bucket names and role names (in order to allow environment-agnostic stacks).
*
* We'll wrap a single {Fn::Sub} around the final string in order to replace everything,
* but we can't have the token system render part of the string to {Fn::Join} because
* the CFN specification doesn't allow the {Fn::Sub} template string to be an arbitrary
* expression--it must be a string literal.
*/
function stackLocationOrInstrinsics(stack: Stack) {
return {
account: resolvedOr(stack.account, '${AWS::AccountId}'),
region: resolvedOr(stack.region, '${AWS::Region}'),
urlSuffix: resolvedOr(stack.urlSuffix, '${AWS::URLSuffix}'),
};
}
15 changes: 13 additions & 2 deletions packages/@aws-cdk/core/test/evaluate-cfn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,19 @@ export function evaluateCFN(object: any, context: {[key: string]: string} = {}):
},

'Fn::Sub'(argument: string | [string, Record<string, string>]) {
const template: string = evaluate(Array.isArray(argument) ? argument[0] : argument);
const placeholders: Record<string, string> = Array.isArray(argument) ? evaluate(argument[1]) : context;
let template;
let placeholders: Record<string, string>;
if (Array.isArray(argument)) {
template = argument[0];
placeholders = evaluate(argument[1]);
} else {
template = argument;
placeholders = context;
}

if (typeof template !== 'string') {
throw new Error('The first argument to {Fn::Sub} must be a string literal (cannot be the result of an expression)');
}

return template.replace(/\$\{([a-zA-Z0-9.:-]*)\}/g, (_: string, key: string) => {
if (key in placeholders) { return placeholders[key]; }
Expand Down

0 comments on commit 357d5f7

Please sign in to comment.