fix(deployment): Fn::Join over list-returning intrinsics + AWS::NotificationARNs in Fn::Sub (#838) - #847
Merged
Merged
Conversation
…tion bugs
cdkd resolves every CloudFormation intrinsic itself in
src/deployment/intrinsic-function-resolver.ts (unlike the CDK CLI, which
defers them to CloudFormation), so the less-common intrinsics and deep
nesting are where cdkd is most likely to diverge. The existing
intrinsic-functions fixture only exercises Ref / Fn::GetAtt / Fn::Join /
Fn::Sub.
The new CdkdIntrinsicsTortureExample stack is cheap (SNS topic + SQS queue
+ ten AWS::SSM::Parameter; no VPC / NAT / Lambda) and computes each SSM
parameter Value via a harder intrinsic, built with the raw CFn escape hatch
(new ssm.CfnParameter + addPropertyOverride('Value', <intrinsic>)) so the
synth template carries the exact intrinsic shape under test.
Coverage beyond intrinsic-functions:
- Fn::Cidr ('10.0.0.0/16', 8, 8) -> eight /24 blocks (Fn::Select[3] and the
full Fn::Join'ed list asserted)
- Fn::FindInMap (Mappings section with a {Ref: AWS::Region} top-level key +
a region-independent DEFAULT row)
- Fn::GetAZs + Fn::Select[0] (first AZ, computed the same way cdkd sorts it)
- Fn::Base64
- nested Fn::Split + Fn::Select + Fn::Join (expect a|c|e)
- deeply-nested two-arg Fn::Sub (literal-map var via a nested Fn::Join +
${AWS::Region} + a ${<Queue>.Arn} GetAtt)
- ALL pseudo-parameters (AWS::AccountId / AWS::Region / AWS::Partition /
AWS::StackName / AWS::URLSuffix / AWS::NotificationARNs)
verify.sh (BSD/macOS-portable, real deploy-rc capture, explicit
[verify] PASS) deploys, reads each parameter back via aws ssm get-parameter,
and asserts it equals an expected value computed independently from the
account / region, so a wrong resolution pinpoints the offending intrinsic;
a failed deploy prints the failing resource + error for triage. It then
destroys and asserts clean (state.json gone + zero orphan SSM parameters).
The pseudo assertion deliberately pins cdkd's documented behavior that
AWS::NotificationARNs resolves to the literal "undefined" inside Fn::Sub.
New scenario tag intrinsics-torture in the canonical taxonomy
(scripts/build-scenario-coverage-matrix.ts); coverage matrices regenerated;
README + testing-guide + changelog entries added.
NOTE: not yet run against real AWS -- needs /run-integ intrinsics-torture
before merge.
…ue list (#838) IntrinsicFunctionResolver.resolveJoin assumed Fn::Join's second argument was always a literal array and called values.map(...) directly. But CloudFormation also allows the second argument to be a SINGLE intrinsic that returns a list (Fn::Cidr, Fn::GetAZs, Fn::Split, or a Ref to a CommaDelimitedList parameter), which crashed deploy with "values.map is not a function". resolveJoin now resolves the second argument first when it is not already an array (it may be a list-returning intrinsic), then maps over the resulting list. If the resolved value is still not an array it throws a clear error naming the accepted shapes, for CloudFormation parity. Adds unit tests covering Fn::Join over Fn::Cidr, Fn::Split, Fn::GetAZs, a Ref to a CommaDelimitedList parameter, the literal-array regression case, and the non-list error path. Closes #838
…pty in Fn::Sub/Ref
cdkd's intrinsic resolver returned `undefined` for the AWS::NotificationARNs
pseudo-parameter. In an Fn::Sub body that left the literal placeholder
`${AWS::NotificationARNs}` in the output (the pseudo branch is skipped on
`undefined`, and the subsequent Ref attempt throws + keeps the placeholder),
diverging from CloudFormation.
cdkd has no stack-notification-ARN concept (a cdkd deploy never sets SNS
notification ARNs on a stack), so the list is always empty. CloudFormation
resolves an empty AWS::NotificationARNs list to an empty string in an
Fn::Sub / Ref string context, so the resolver now returns '' (empty string)
for AWS::NotificationARNs in both Fn::Sub and bare Ref contexts.
- src/deployment/intrinsic-function-resolver.ts: return '' instead of undefined
- tests/unit/deployment/intrinsic-functions.test.ts: add a describe block
asserting Fn::Sub('${AWS::NotificationARNs}') and Ref: AWS::NotificationARNs
both resolve to ''
- tests/integration/intrinsics-torture: update the verify.sh expected value
from `notif=undefined` to `notif=` (the prior expectation pinned a
non-existent "undefined" behavior); update the stack/README/changelog notes
to describe the CFn-parity empty-string resolution
Owner
Author
|
Independent 3-axis review complete (code re-review + test review; spec N/A for a bug fix). No blockers. Code review confirmed the Fn::Join 2nd-arg list-intrinsic resolution (no literal-array regression, throws on non-list) and AWS::NotificationARNs -> '' parity (strictly better: the bare-Ref path previously threw). Test review confirmed exact-value unit + integ assertions for both fixes incl. real-AWS readback. Setting pr-review bound to c4fdf1a. |
|
🎉 This PR is included in version 0.221.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two real fixes to cdkd's hand-rolled intrinsic-function resolver
(
src/deployment/intrinsic-function-resolver.ts), both surfaced by a newfailure-seeking
intrinsics-tortureintegration test:(a) issue #838 --
Fn::Joinover a list-returning intrinsicFn::Joinwith a list-returning intrinsic (Fn::Cidr/Fn::GetAZs/Fn::Split) as its second argument crashed withvalues.map is not a function. The resolver assumed the second argument was already an array andcalled
.map()on it, but a list-returning intrinsic is an object(
{ "Fn::Cidr": [...] }) until it is resolved. The fix resolves the secondargument FIRST, then maps over the resulting list. Closes #838.
(b)
AWS::NotificationARNsinFn::Sub/RefAWS::NotificationARNsresolved toundefined, which left the literal${AWS::NotificationARNs}placeholder in anFn::Suboutput (the pseudobranch is skipped on
undefined, and the subsequentRefattempt throws andkeeps the placeholder). cdkd has no stack-notification-ARN concept, so the
list is always empty -- and CloudFormation resolves an empty
AWS::NotificationARNslist to an empty string in a string context. Theresolver now returns
''(empty string) forAWS::NotificationARNsin bothFn::Suband bareRefcontexts, matching CloudFormation's own behaviorinstead of leaking an unresolved placeholder downstream.
Tests
tests/unit/deployment/intrinsic-functions.test.ts):Fn::Joinover
Fn::Cidr/Fn::GetAZs/Fn::Split, andAWS::NotificationARNsresolving to
''in bothFn::SubandRef.tests/integration/intrinsics-torture/**): a newfailure-seeking fixture (an SNS topic + SQS queue + ten
AWS::SSM::Parameters, no VPC / NAT / Lambda) that stress-tests theless-common + deeply-nested intrinsics the existing
intrinsic-functionsfixture never exercised --
Fn::Cidr,Fn::FindInMap,Fn::GetAZs+Fn::Select,Fn::Base64, nestedFn::Split+Fn::Select+Fn::Join,a deeply-nested two-arg
Fn::Sub, and ALL pseudo-parameters. Each SSMparameter is built with the raw CFn escape hatch
(
new ssm.CfnParameter+addPropertyOverride('Value', <intrinsic>)) so thesynth template carries the EXACT intrinsic shape under test.
verify.shreads each parameter back and asserts it equals a value computed
independently from the account / region, so a wrong resolution pinpoints
which intrinsic cdkd got wrong; it then destroys and asserts clean (state
gone + zero orphan SSM parameters).
intrinsics-torture; coverage matrices regenerated;testing-guide entry added to
docs/testing.md.Validation
intrinsics-tortureinteg passed GREEN end-to-end(
/run-integ intrinsics-torture) -- deploy clean, every intrinsic resolvedto the expected value, destroy clean with 0 orphans.
bench-cdk-samplebroad integ passed clean in the sameworktree (
integ-broad+integ-destroymarkers fresh).green.
Closes #838