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(cfn-include): allow dynamic mappings to be used in Fn::FindInMap #13428

Merged
merged 2 commits into from
Mar 6, 2021
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"Parameters": {
"Stage": {
"Type": "String",
"AllowedValues": ["beta"],
"Default": "beta"
}
},
"Mappings": {
"beta": {
"region": {
"key1": "name"
}
}
},
"Resources": {
"Bucket": {
"Type": "AWS::S3::Bucket",
"Properties": {
"BucketName": {
"Fn::FindInMap": [
{ "Ref": "Stage" },
"region",
"key1"
]
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,14 @@ describe('CDK Include', () => {
}).toThrow(/Mapping with name 'NonExistentMapping' was not found in the template/);
});

test('can ingest a template that uses Fn::FindInMap with the first argument being a dynamic reference', () => {
includeTestTemplate(stack, 'find-in-map-with-dynamic-mapping.json');

expect(stack).toMatchTemplate(
loadTestFileToJsObject('find-in-map-with-dynamic-mapping.json'),
);
});

test('handles renaming Mapping references', () => {
const cfnTemplate = includeTestTemplate(stack, 'only-mapping-and-bucket.json');
const someMapping = cfnTemplate.getMapping('SomeMapping');
Expand Down
16 changes: 12 additions & 4 deletions packages/@aws-cdk/core/lib/cfn-parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,11 +566,19 @@ export class CfnParser {
case 'Fn::FindInMap': {
const value = this.parseValue(object[key]);
// the first argument to FindInMap is the mapping name
const mapping = this.finder.findMapping(value[0]);
if (!mapping) {
throw new Error(`Mapping used in FindInMap expression with name '${value[0]}' was not found in the template`);
let mappingName: string;
if (Token.isUnresolved(value[0])) {
// the first argument can be a dynamic expression like Ref: Param;
// if it is, we can't find the mapping in advance
mappingName = value[0];
} else {
const mapping = this.finder.findMapping(value[0]);
if (!mapping) {
Copy link
Contributor

@NetaNir NetaNir Mar 6, 2021

Choose a reason for hiding this comment

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

This error seems at odds with allowing unresolved values. Is this error preventing something from breaking down the road? If so, will it break if this value is unresolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this error is to prevent situations where the argument to Fn::FindInMap is a static string (so something like { 'Fn::FinInMap': ['MappingName', 'Lvl1', 'Lvl2'] }), but a Mapping with that name could not be found in the template (so, continuing the above example, MappingName would not be in the template).

In case the value is dynamic, like in the test for this PR, we can't prevent this mistake from happening. We also can't handle things like re-naming the Mapping from CDK code correctly with dynamic mappings. But I think that's fine - we do the validation when we can, and when we can't, we just trust the template author they modeled things correctly.

throw new Error(`Mapping used in FindInMap expression with name '${value[0]}' was not found in the template`);
}
mappingName = mapping.logicalId;
}
return Fn._findInMap(mapping.logicalId, value[1], value[2]);
return Fn._findInMap(mappingName, value[1], value[2]);
}
case 'Fn::Select': {
const value = this.parseValue(object[key]);
Expand Down