fix(core): cross-stack references to NestedStack list values produces invalid outputs#32575
Conversation
nmussy
left a comment
There was a problem hiding this comment.
Mostly nits about coverage, LGTM overall though 👍
packages/@aws-cdk-testing/framework-integ/test/core/test/integ.nested-stack-references.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk-testing/framework-integ/test/core/test/integ.nested-stack-references.ts
Outdated
Show resolved
Hide resolved
… invalid outputs Referencing a list attribute of a resource defined within a NestedStack synthesizes successfully but the nested stack will fail deployment with the error: ``` Template format error: Every Value member must be a string. ``` This prevents deploying resources such into NestedStack instances if a reference to that resource's list attribute needs to be referenced in a cross-stack context. For example, deploying a `InterfaceVpcEndpoint` instance in a nested stack and attempting to reference its `vpcEndpointDnsEntries` property within a different stack will cause this error. To fix this issue, a similar strategy to `exportStringListValue` is used to join the reference's values into a string and expose that value as the output from the nested stack. The reference to the serialized value is then re-exported as normally needed to hoist it to the top-level parent stack. The final reference that imports the value is then re-written to also deserialize the imported string back to the original list. fixes aws#27233 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
8219bff to
28721e3
Compare
nmussy
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes
kaizencc
left a comment
There was a problem hiding this comment.
sorry for the super late review @brandondahler. my main concern is the change between Reference and Intrinsic. Let's figure that out, and then I'm happy to send this one into main!
| * Will create Outputs along the chain of Nested Stacks, and return the final `{ Fn::GetAtt }`. | ||
| */ | ||
| export function referenceNestedStackValueInParent(reference: Reference, targetStack: Stack) { | ||
| export function referenceNestedStackValueInParent(reference: Reference, targetStack: Stack): Intrinsic { |
There was a problem hiding this comment.
why are we returning an Intrinsic here? it should be a Reference right.
There was a problem hiding this comment.
We have to use Intrinsic here because we're reversing the tokenized list via Fn.split (line 354). We're having to use Fn.split because we're having to do a join when creating the output within the nested stack since the output cannot be a list.
Prior to this update, the values returned from here were only able to reference strings and therefore were just outer stack Reference's to the nested stack's resource attribute. After this update, the values returned from here may either reference a string or a list. In the list case, it is an implementation detail of this method that we're having to re-split the Reference on the nested stack's resource out to its list representation again (and therefore are returning an Intrinsic instead).
Ultimately the choice from a type perspective was either returning an Intrinsic or an IResolvable, either would be correct per the type hierarchies involved.
| } | ||
|
|
||
| export function getExportable(stack: Stack, reference: Reference): Reference { | ||
| export function getExportable(stack: Stack, reference: Reference): Intrinsic { |
There was a problem hiding this comment.
i understand that this is a private function but I am concerned about the downgrade from returning a Reference to an Intrinsic. i want to learn more about why thats something you've deemed necessary.
There was a problem hiding this comment.
This ultimately changed because referenceNestedStackValueInParent changed, see that comment for more details. TL;DR Fn.split returns an Intrinsic (not a Reference) when untokenized.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
@Mergifyio requeue |
❌ Command disallowed due to command restrictions in the Mergify configuration.Details
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue
Closes #27233.
Reason for this change
Referencing a list attribute of a resource defined within a
NestedStacksynthesizes successfully but the nested stack will fail deployment with the error:This prevents deploying resources into a
NestedStackinstance if a reference to one of that resource's list attribute exists within a cross-stack context. For example, deploying aInterfaceVpcEndpointinstance in a nested stack and attempting to reference itsvpcEndpointDnsEntriesproperty within a different stack will cause this error.See new integration test at
packages/@aws-cdk-testing/framework-integ/test/core/test/integ.nested-stack-references.tsfor minimal reproduction.Description of changes
A similar strategy to
exportStringListValueis used to serialized the reference's values into a string and expose that value as the output from the nested stack. The reference to the serialized value is then made exportable as normally needed to hoist it to the top-level parent stack. The final reference that imports the value is then re-written to also deserialize the imported string back to the original list.The return types of some internal methods were modified to handle the fact that core/lib/private/refs.ts's
getExportableno longer necessarily returns a Reference. This was needed because an exportable may now be a value derived from a reference instead of only a direct reference.Describe any new or updated permissions being added
N/A
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license