fix(core): filter invalid references in nested stacks #35479
Conversation
github-actions
bot
added
the
beginning-contributor
github-actions
bot
added
bug
dgandhi62
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
- Added checks in ref.ts and cfn-utils-provider.ts to filter out invalid nested stack references
dgandhi62
force-pushed
the
fix-nestedstack-bug
branch
from
1d8ad7d to
568a091
Compare
There was a problem hiding this comment.
i would prefer you write your own PR description to describe what you've done, and not rely on AI. A human is reading it (me), and I find it more trustworthy if you've spent the time describing what you've done. If you've written it and not AI, then it should be more succinct -- after all, its the entry point for the reviewer to get an understanding of what this PR entails.
| // Filter out invalid nested stack references | ||
| if (isInvalidNestedStackReference(consumer, token)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
instead of filtering out invalid nested stack references, did you consider / find out where we are building invalid nested stack references?
this looks like a reactive solution, which is sometimes okay, but i would like to parse whether we can find a proactive solution where we do not create bad nested stack references in the first place.
There was a problem hiding this comment.
Thank you for the feedback! I have implemented two filters, the primary one identifying and removing the source of the invalid reference creation and the secondary one being more reactionary in nature. I will update the pr description to add more details on the first
| return shouldFilter; | ||
| } | ||
|
|
||
| // Skip a reference that uses the raw parameter name instead of a nested stack parameter |
There was a problem hiding this comment.
do we know that a raw parameter name is always invalid?
There was a problem hiding this comment.
The raw parameter name would be invalid in the context of a nested stack because the child stack would not have access to the parent's parameters
|
|
||
| describe('refs', () => { | ||
| describe('isInvalidNestedStackReference', () => { | ||
| test('returns true for invalid nested stack parameter references', () => { |
There was a problem hiding this comment.
i need a definition for what you're considering as valid or invalid nested stack parameters. it seems like the current criteria is that it uses a raw parameter name. thats fine if its true, but a citation is needed. i would like to know a) why that is the criteria and b) why we are creating invalid references and have to filter for them instead of not creating them in the first place.
There was a problem hiding this comment.
The filter is primarily because of the inherent cdk functionality I see coded in the createNestedStackParameter() function, where it codes out 'reference-to-' when creating a parameter id. It gets stripped of its alphanumerics later on in the id generation process. I've mentioned this briefly in the pr but I can add more detail about this. Thank you for the feedback
dgandhi62
removed
the
beginning-contributor
Issue # (if applicable)
Closes #35402
Reason for this change
When nested stacks reference parent stack parameters, CDK incorrectly generates two different CloudFormation references in the nested stack template - a correct one and an incorrect one. The valid reference looks like
{"Ref": "referencetoParentStackPrivateSubnetIdsF7323B10Ref"}whereas the invalid reference looks like
{"Ref": "PrivateSubnetIds"}In particular, the issue reported identified this occuring when cloudformation resources required the values to be stringified as JSON. The core issue was in the CfnUtils.stringify() function, which returns references with the original parameter name from the parent stack. However, nested stacks can only access the parameters which have been explicitly passed down as nested stack parameters (i.e, those with 'referenceto' prefixes).
The function calls can be seen as follows- CloudFormationLang.toJSON() -> tokenAwareStringify() -> renderIntrinsic() -> CfnUtils.stringify()
Description of changes
Checks are added to bypass invalid nested parameter references before they can be created. These checks include type checks, as well as ensuring that a nested stack reference include the term 'referenceto'. This is due to CDK's inherent functionality and naming convention (as seen in createNestedStackParameter(), which gets stripped of its alphanumerics later on).
Furthermore, another safety check to filter invalid references is added during reference resolution phase. This is a much more broader check and not specific to the CfnUtils.stringify() function. Therefore, if there exist any invalid references outside of stringify, this prevents them from throwing an error in the deployment (although they would still exist in the cloudformation template).
Describe any new or updated permissions being added
No new permissions added
Description of how you validated changes
Unit Tests: Added unit tests covering parameter reference scenarios in nested stacks
Integration Tests: Implemented a synthesis-level validation that scans generated CloudFormation templates for invalid parameter references. Also ran a full deployment test that verifies stacks deploy successfully with the fix
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license