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

Nested Fn::Join doesn't work with Fn::Split #5655

Closed
guseggert opened this issue Jan 6, 2020 · 2 comments · Fixed by #5679
Closed

Nested Fn::Join doesn't work with Fn::Split #5655

guseggert opened this issue Jan 6, 2020 · 2 comments · Fixed by #5679
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@guseggert
Copy link

guseggert commented Jan 6, 2020

If Fn::Split is used with nested Fn::Joins, then an optimization attempting to flatten the joins incorrectly assumes the value of the join is an array at synth time, when it fact it's a late-binding CloudFormation list.

I'm trying to append a new security group to a list of existing security groups that are imported via a CFN value.

Reproduction Steps

const securityGroupIds = Fn.split(",", Fn.importValue("SecurityGroupIDs"));
const securityGroup = new SecurityGroup(this, "SecurityGroup", {
    description: `security group`,
    vpc: vpc
});

// mimic synth, fails with error
stack.resolve(Fn.join(",", [
                Fn.join(",", securityGroupIds),
                securityGroup.securityGroupId,
            ]))

Error Log

      values.splice(i, 1, ...el['Fn::Join'][1]);
             ^
source-map-support.js:465
TypeError: Resolution error: Found non-callable @@iterator.
Object creation stack:
  at new FnJoin (node_modules/@aws-cdk/core/lib/cfn-fn.ts:655:26)
  at Function.join (node_modules/@aws-cdk/core/lib/cfn-fn.ts:45:12)
...

Environment

  • **CLI Version :1.19.0 (build 5597bbe)
  • Framework Version: "@aws-cdk/core": "1.19.0"
  • **OS :MacOS
  • **Language :Typescript

Other

The bug occurs here:

https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/core/lib/private/cloudformation-lang.ts#L158

The value that it's trying to spread into the splice is not an array, but an object that CFN will resolve into a list:

> JSON.stringify(el['Fn::Join'][1])
"{"Fn::Split":[",",{"Fn::ImportValue":"SecurityGroupIDs"}]}"

I think that if the isSplicableFnJoinIntrinsic function were updated to also check Array.isArray(obj['Fn::Join'][1]), then it would skip the optimization in this case.


This is 🐛 Bug Report

@guseggert guseggert added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 6, 2020
@SomayaB SomayaB added the @aws-cdk/core Related to core CDK functionality label Jan 6, 2020
@eladb
Copy link
Contributor

eladb commented Jan 6, 2020

Thanks. Ack

eladb pushed a commit that referenced this issue Jan 7, 2020
Fn.join has an optimization to flatten nested joins with the same delimiter:

    Fn.join(",", [ Fn.join(",", [ "a", "b" ]), "c" ]) == Fn.join(",", [ "a", "b", "c" ])

The logic in `isSplicableFnJoinIntrinsic` checks if the object is an Fn::Join which uses the same delimiter, and then splices (`...`) the inner value onto the outer Fn::Join instead of nesting the inner Fn::Join. This can only work if the inner value is a real array (otherwise, we get `Found non-callable @@iterator`�).

The fix is to add an additional check to `isSplicableFnJoinIntrinsic` which verifies the the inner value is indeed an array.

Fixes #5655
@mergify mergify bot closed this as completed in #5679 Jan 7, 2020
mergify bot pushed a commit that referenced this issue Jan 7, 2020
Fn.join has an optimization to flatten nested joins with the same delimiter:

    Fn.join(",", [ Fn.join(",", [ "a", "b" ]), "c" ]) == Fn.join(",", [ "a", "b", "c" ])

The logic in `isSplicableFnJoinIntrinsic` checks if the object is an Fn::Join which uses the same delimiter, and then splices (`...`) the inner value onto the outer Fn::Join instead of nesting the inner Fn::Join. This can only work if the inner value is a real array (otherwise, we get `Found non-callable @@iterator`�).

The fix is to add an additional check to `isSplicableFnJoinIntrinsic` which verifies the the inner value is indeed an array.

Fixes #5655
@marcheil
Copy link

marcheil commented Apr 20, 2020

This also breaks with split -> importvalue -> sub nesting. Like so:

Fn::Split: [
            ",",
            Fn::ImportValue: !Sub: "${parameter}-literal-string"
          ] 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants