(core): Templating error on custom-resource introduced with version 1.92 #13465
Comments
mzakarian
added
bug
|
I think this is actually an issue with how The change in #13074 encodes the API requests as JSON strings so it will be possible to use tokens in e.g. map keys. The However, in this use case |
Our previous implementation of `toJSON()` was quite hacky. It replaced values inside the structure with objects that had a custom `toJSON()` serializer, and then called `JSON.stringify()` on the result. The resulting JSON would have special markers in it where the Token values would be string-substituted back in. It's actually easier and gives us more control to just implement JSONification ourselves in a Token-aware recursive function. This change has been split off from a larger, upcoming PR in order to make the individual reviews smaller. Incidentally also fixes #13465, as the type of encoded tokens is assumed to match the type of the encoded value (e.g., a `string[]`-encoded token is assumed to produce a list at deploy-time and so will not be quoted). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
|
🤦♂️ @jogold you are correct. I feel silly now, I was solving the wrong problem :( |
|
@rix0rrr the only way to solve this right now for the user is this: {
NetworkInterfaceIds: [cdk.Token.asList(cdk.Fn.join(',', endpoint.vpcEndpointNetworkInterfaceIds))]
}which will give the following correct CF: {
"Fn::Join": [
"",
[
"{\"action\":\"describeNetworkInterfaces\",\"service\":\"EC2\",\"parameters\":{\"NetworkInterfaceIds\":[",
{
"Fn::Join": [
",",
{
"Fn::GetAtt": [
"EndpointEEF1FD8F",
"NetworkInterfaceIds"
]
}
]
},
"]},\"physicalResourceId\":{\"id\":\"physical-od\"},\"outputPath\":\"NetworkInterfaces.0.PrivateIpAddress\"}"
]
]
} |
|
@rix0rrr actually the code above doesn't work because the items in the This is very ugly but it works: {
NetworkInterfaceIds: [cdk.Token.asList(cdk.Stack.of(stack).toJsonString(cdk.Fn.join('","', endpoint.vpcEndpointNetworkInterfaceIds)))]
}{
"Fn::Join": [
"",
[
"{\"action\":\"describeNetworkInterfaces\",\"service\":\"EC2\",\"parameters\":{\"NetworkInterfaceIds\":[\"",
{
"Fn::Join": [
"\\\",\\\"",
{
"Fn::GetAtt": [
"EndpointEEF1FD8F",
"NetworkInterfaceIds"
]
}
]
},
"\"]},\"physicalResourceId\":{\"id\":\"physical-od\"},\"outputPath\":\"NetworkInterfaces.0.PrivateIpAddress\"}"
]
]
} |
|
Oh wow. I hadn't even thought that far ahead. I was thinking we'll resort to Not sure it's worth it though, as it's probably going to break in further edge cases. |
|
Oh no it's actually fine. This will only trigger for CFN intrinsics which cannot be anything other than lists of strings anyway. |
|
Doing this will turn |
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.
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 aws#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 aws#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 aws#13465.
----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We are doing this, because we need to put an api gateway behind a load balancer (company policy). If there is a better solution to do this, feedback is welcome.
Reproduction Steps
this is the output with aws-cdk/core 1.91
this is the output with aws-cdk/core 1.92
What did you expect to happen?
After using
cdk deploywe expected the same behavior as in version 1.91 with successful deployment.What actually happened?
The error only occurs during a
cdk deploywith version 1.92.Environment
Other
We assume that this issue is related to this change:
This is 🐛 Bug Report
The text was updated successfully, but these errors were encountered: