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(core): toJsonString() cannot handle list intrinsics #13544

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Mar 11, 2021

The previous attempt at a fix missed one important case: the types
of the values involved in the { Fn::Join } expression didn't actually
match up. They all needed to be strings, but the previous implentation
just dropped list-typed values in there.

Unfortunately, there is no way to do it correctly with just string
manipulation in CloudFormation ({ Fn::Join } etc), so we'll have
to resort to using a Custom Resource if we encounter list values.

Actually fixes #13465.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The previous attempt at a fix missed one important case: the types
of the values involved in the `{ Fn::Join }` expression didn't actually
match up. They all needed to be strings, but the previous implentation
just dropped list-typed values in there.

Unfortunately, there is no way to do it correctly with just string
manipulation in CloudFormation (`{ Fn::Join }` etc), so we'll have
to resort to using a Custom Resource if we encounter list values.

Actually fixes #13465.
@rix0rrr rix0rrr requested a review from eladb March 11, 2021 14:59
@rix0rrr rix0rrr self-assigned this Mar 11, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 11, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 11, 2021
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Mar 11, 2021
@rix0rrr rix0rrr changed the title fix(core): toJsonString() cannot handle lists fix(core): toJsonString() cannot handle list intrinsics Mar 11, 2021
@rix0rrr rix0rrr requested a review from a team March 11, 2021 15:12
Comment on lines +230 to +232
// But that has the unfortunate side effect that if `CFN_EVAL({ Ref: MyList }) == []`, then it would
// evaluate to `[""]`, which is a different value. Since CloudFormation does not have arbitrary
// conditionals there's no way to deal with this case properly.
Copy link
Contributor

@jogold jogold Mar 11, 2021

Choose a reason for hiding this comment

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

@rix0rrr I think I found a way with a Fn::Join on a Fn::Split acting like a replace:

cdk.Fn.join('', cdk.Fn.split('""', myString)) // acts like a replace

For #13465 and the issue with the array of ENIs, it looks like that

[cdk.Token.asList(cdk.Fn.join('', cdk.Fn.split('""', cdk.Stack.of(stack).toJsonString(cdk.Fn.join('","', endpoint.vpcEndpointNetworkInterfaceIds)))))]

Gives the following CF

{
  "Fn::Join": [
    "",
    [
      "{\"action\":\"describeNetworkInterfaces\",\"service\":\"EC2\",\"parameters\":{\"NetworkInterfaceIds\":[",
      {
        "Fn::Join": [
          "",
          {
            "Fn::Split": [
              "\"\"",
              {
                "Fn::Join": [
                  "",
                  [
                    "\"",
                    {
                      "Fn::Join": [
                        "\\\",\\\"",
                        {
                          "Fn::GetAtt": [
                            "OtherEndpoint88C37F72",
                            "NetworkInterfaceIds"
                          ]
                        }
                      ]
                    },
                    "\""
                  ]
                ]
              }
            ]
          }
        ]
      },
      "]},\"physicalResourceId\":{\"id\":\"physical-id\"},\"outputPath\":\"NetworkInterfaces.0.PrivateIpAddress\"}"
    ]
  ]
}

Successfully tested this with a GatewayVpcEndpoint which has no ENIs and hence returns an empty list for the vpcEndpointNetworkInterfaceIds attribute.

Even if it's really really ugly, it's better than a custom resource, right? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EHRMAGERD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are like a wizard.

A dirty wizard...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it though.

Copy link
Contributor Author

@rix0rrr rix0rrr Mar 12, 2021

Choose a reason for hiding this comment

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

Wait no. This solution will break when presented with [""].

That value would successfully make it through the transformation initially and then be substituted away to [].

Copy link
Contributor

Choose a reason for hiding this comment

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

Bu then maybe the CR is the right solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also thinking about it... afraid nothing's to be done, as:

JOIN(<whatever>, []) == JOIN(<whatever>, [""]) == ""

Once we've JOINed, we've lost the ability to make the distinction between [] and [""]... right?

(I'm starting to doubt myself now)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's a way to do it. You loose information after the first join:

[].join('","') === [''].join('","')

and the only "first operation" you can do on the array with CF is a Fn::Join.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK our comments crossed each other... I don't think it's possible in pure CF. This is sad 😢 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IF ONLY we had a functional { Fn::If }...

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 79711fe
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit a5be042 into master Mar 16, 2021
@mergify mergify bot deleted the huijbers/json-real-list branch March 16, 2021 16:13
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Mar 17, 2021
The previous attempt at a fix missed one important case: the types
of the values involved in the `{ Fn::Join }` expression didn't actually
match up. They all needed to be strings, but the previous implentation
just dropped list-typed values in there.

Unfortunately, there is no way to do it correctly with just string
manipulation in CloudFormation (`{ Fn::Join }` etc), so we'll have
to resort to using a Custom Resource if we encounter list values.

Actually fixes aws#13465.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Mar 18, 2021
The previous attempt at a fix missed one important case: the types
of the values involved in the `{ Fn::Join }` expression didn't actually
match up. They all needed to be strings, but the previous implentation
just dropped list-typed values in there.

Unfortunately, there is no way to do it correctly with just string
manipulation in CloudFormation (`{ Fn::Join }` etc), so we'll have
to resort to using a Custom Resource if we encounter list values.

Actually fixes aws#13465.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
The previous attempt at a fix missed one important case: the types
of the values involved in the `{ Fn::Join }` expression didn't actually
match up. They all needed to be strings, but the previous implentation
just dropped list-typed values in there.

Unfortunately, there is no way to do it correctly with just string
manipulation in CloudFormation (`{ Fn::Join }` etc), so we'll have
to resort to using a Custom Resource if we encounter list values.

Actually fixes aws#13465.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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 contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(core): Templating error on custom-resource introduced with version 1.92
4 participants