fix(deployment): resolve composite conditions + prune Condition-gated resources on update (#840) - #846
Merged
Merged
Conversation
Owner
Author
|
Independent 3-axis review complete (code + test; spec N/A for a bug fix). Test review: clean, with the prune-on-UPDATE path integ-asserted (deploy condition=true -> redeploy condition=false -> resource DELETED on AWS). Code review found one latent hazard: the {Condition: X} reference branch fired for every property, so a resource property literally {Condition: "string"} could be coerced to false. Fixed in d97cf5a by gating that branch on context.conditionResolver presence (set only during evaluateConditions, the sole valid context) + a regression unit test. Setting pr-review bound to d97cf5a. |
…handling cdkd evaluates the Conditions section, the resource-level Condition: key, and Fn::If / Fn::Equals / Fn::And / Fn::Or / Fn::Not itself (no CloudFormation engine underneath). The existing `conditions` fixture has no verify.sh and only exercised a single Fn::And + one conditionally-created S3 bucket + an Fn::If bucket name, leaving condition-gated resource creation, Fn::If -> AWS::NoValue property omission, and Fn::Or / Fn::Not without an end-to-end real-AWS backstop. The new CdkdConditionsIfExample stack (cheap: 3x SSM Parameter + 1x SNS Topic, no VPC / NAT) closes the gap: - Conditions section combining Fn::Equals on a CfnParameter with Fn::And (IsPremiumPrimary), Fn::Or (IsPremiumOrSecondary), and Fn::Not (IsSecondaryRegion). - TWO resources with a Condition: key (PremiumOnlyParam on the bare Fn::Equals condition, PremiumPrimaryParam on the Fn::And condition) so a resource is created in one parameter setting and absent in another. - An always-created parameter whose Value is an Fn::If branch (TierLabelParam). - An SNS topic whose DisplayName is Fn::If(IsPremium, 'Premium Notifications', AWS::NoValue) plus two tag values driven by Fn::If. The Tier CfnParameter default is read from CDK context (-c tier=premium |basic) at synth, since cdkd has no deploy-time --parameter flag, so flipping the context is the param-flip mechanism. verify.sh (BSD/macOS-portable: no grep -P, no date -d, real exit codes, explicit pass line) runs three phases against real AWS: - Phase 1 (-c tier=premium): Fn::If property branch reached AWS (TierLabelParam Value == tier-is-premium), both condition-gated parameters PRESENT, SNS DisplayName SET to Premium Notifications, Fn::If tag values / Fn::Or tag value reached AWS. - Phase 2 (-c tier=basic, redeploy in place): Fn::If branch FLIPPED on AWS (tier-is-basic), both condition-gated parameters now ABSENT, SNS DisplayName genuinely OMITTED on AWS (Fn::If -> AWS::NoValue), tag values flipped. - Phase 3: destroy + assert every named resource is NOT-FOUND on AWS and the state file is gone. New scenario tag conditions-and-if in KNOWN_SCENARIOS (scripts/build-scenario-coverage-matrix.ts); coverage matrices regenerated; changelog entry added. Test-only, no src/ change. Not yet run against real AWS.
…e on redeploy (#840) The new conditions-and-if integ surfaced a real cdkd gap, not a test artifact. Synthing the fixture both ways confirms CloudFormation does NOT strip condition-gated resources at synth time: CDK emits PremiumOnlyParam into Resources carrying `Condition: IsPremium` in BOTH `-c tier=premium` and `-c tier=basic`, differing only in the Tier parameter default that flips the condition. cdkd evaluated the Conditions section for Fn::If resolution but never consulted the resource-level `Condition:` key, so a condition-false resource was created on first deploy AND never deleted when its condition flipped (it stayed in the desired set and diffed as NO_CHANGE). That is exactly the integ's Phase 2 FAIL: PremiumOnlyParam STILL EXISTS after the basic redeploy. Fix: the deploy engine now prunes every resource whose `Condition:` resolved to false (new TemplateParser.filterResourcesByCondition) right after evaluating the Conditions section, so the whole downstream pipeline (type/property validation, DAG build, diff, provisioning) operates on the CFn-effective resource set. A condition-false resource is never created, and one present in prior state but condition-excluded from the effective template flows through the diff's existing "present in state, absent from desired -> DELETE" path exactly as CloudFormation removes it. A resource whose `Condition:` names an unevaluated/unknown condition is kept (absent-from-map is not === false), never silently dropped. Tests: - 5 filterResourcesByCondition unit tests (tests/unit/analyzer/template-parser.test.ts): false->removed, true->kept, no-Condition->kept, unknown-condition->kept, preserves Conditions/Outputs and does not mutate input. - 1 diff-calculator unit test (tests/unit/analyzer/diff-calculator.test.ts): resource in state but absent from the pruned template -> DELETE. Docs: architecture.md diff comparison section, .claude/rules/analyzer.md, and a changelog-cdkd.md entry. Closes #840
…tions (Fn::And/Or/Not), order-independent (#840) The #840 fix (filterResourcesByCondition + effectiveTemplate) pruned condition-gated resources whose simple Fn::Equals condition evaluated false, but a resource gated on a COMPOSITE condition such as IsPremiumPrimary = Fn::And[{Condition: IsPremium}, {Condition: IsPrimaryRegion}] was not pruned in the basic tier: the conditions-and-if integ re-validation reported "PremiumPrimaryParam STILL EXISTS but should be removed". Root cause (two compounding bugs in evaluateConditions / resolveValue): 1. Reference resolution: resolveValue had NO case for a {Condition: X} named-condition reference. Inside Fn::And/Or/Not the inner {Condition: IsPremium} fell through to the generic "not an intrinsic, recurse object properties" branch and produced the object {Condition: "IsPremium"} -- a truthy value -- so And(false, true) wrongly evaluated to And(truthy, truthy) = true, and the gated resource was never pruned. 2. Evaluation order: evaluateConditions iterated the Conditions block in declaration order with no dependency ordering, so a composite condition declared before the conditions it references would see them unevaluated. Fix: - Add a {Condition: X} case to resolveValue (single-key, string-valued guard so a resource property literally named "Condition" alongside siblings is not misdetected) that delegates to resolveConditionReference. - Rewrite evaluateConditions to evaluate lazily/recursively with memoization (the result map doubles as the memo cache) and an in-progress set as a cycle guard, threading a conditionResolver hook onto the context so nested {Condition: Y} references recurse through the same evaluator. Evaluation is now dependency-ordered, not declaration-ordered. A detected cycle or an undeclared reference downgrades to false (warn) rather than aborting the deploy, matching the prior per-condition error tolerance and Fn::If's not-found behavior. Verified: in the basic tier IsPremium=false -> IsPremiumPrimary=And(false,true)=false -> PremiumPrimaryParam pruned. Fn::Or / Fn::Not composites referencing other conditions also resolve correctly. Unit tests (tests/unit/deployment/intrinsic-functions.test.ts): Fn::And of two {Condition: X} refs across all four truth combinations; declaration-order independence (composite declared before its referenced conditions); Fn::Or / Fn::Not composites; nested composite-referencing-composite; circular-reference guard; undeclared-reference warn-and-false; and the "Condition" property false-positive guard. Closes #840
…ditionResolver presence
The `{Condition: <name>}` named-condition reference branch in
`resolveValue` fired for every property, not just inside a condition
definition. A resource/output property whose value was exactly a
single-key `{ "Condition": "<string>" }` object was silently coerced to a
boolean via `resolveConditionReference` — and in normal property context
(no `conditionResolver`, name absent from `context.conditions`) that
returns `false`, corrupting the property.
The reference form is only reachable during `evaluateConditions`, the
sole code path that threads a `conditionResolver` hook onto the context.
Gate the branch on `context.conditionResolver` being present so a
resource property literally named `Condition` is never coerced to false;
it now falls through and resolves as an ordinary object, exactly as
before the #840 change. The single-key + `typeof string` guards remain
as defense in depth. Composite-condition behavior is unaffected (it
always runs with `conditionResolver` set).
Adds a unit test pinning that a single-key `{ Condition: "Foo" }`
property resolved in normal context stays a plain object and is not
`false` (review fix).
go-to-k
force-pushed
the
test/conditions-and-if
branch
from
June 13, 2026 15:40
d97cf5a to
5f897e7
Compare
|
🎉 This PR is included in version 0.221.4 🎉 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.
Summary
Fixes two related cdkd defects around CloudFormation Conditions (issue #840), plus a failure-seeking integ and unit tests.
1. Composite conditions referencing other named conditions resolve correctly and order-independently
src/deployment/intrinsic-function-resolver.ts+src/analyzer/template-parser.ts. AConditionssection entry can compose other named conditions via{Condition: X}insideFn::And/Fn::Or/Fn::Not(for exampleIsPremiumPrimary: { Fn::And: [ { Condition: IsPremium }, { Condition: IsPrimaryRegion } ] }). cdkd previously did not resolve the{Condition: X}reference form when evaluating composite conditions, and the result also depended on the order conditions appeared in the template. Conditions are now evaluated so that a{Condition: X}reference resolves to the already-evaluated boolean of conditionXregardless of declaration order, matching CloudFormation semantics.2. A resource whose
Condition:flipstrue -> falseon a redeploy is now pruned / deletedsrc/analyzer/template-parser.ts(newfilterResourcesByCondition) +src/deployment/deploy-engine.ts. CloudFormation does not strip condition-gated resources at synth time -- CDK emits a resource carrying aCondition:key intoResourcesregardless of the condition value. cdkd evaluated theConditionssection forFn::Ifresolution but never consulted the resource-levelCondition:key, so a condition-false resource was created on first deploy and never deleted when its condition flipped (it stayed in the desired set and diffed asNO_CHANGE). The deploy engine now prunes every resource whoseCondition:resolved tofalse(viaTemplateParser.filterResourcesByCondition) immediately after evaluating theConditionssection, so the whole downstream pipeline (type/property validation, DAG build, diff, provisioning) operates on the CFn-effective resource set: a condition-false resource is never created, and one present in prior state but condition-excluded from the effective template falls through the diff's existing "present in state, absent from desired -> DELETE" path exactly as CloudFormation removes it. A resource whoseCondition:names an unknown/unevaluated condition is kept (treated as present, not silently dropped).Tests
conditions-and-ifinteg (tests/integration/conditions-and-if/**): a cheap stack (3xAWS::SSM::Parameter+ 1xAWS::SNS::Topic, no VPC / NAT) whoseConditionssection combinesFn::Equalson aCfnParameterwithFn::And/Fn::Or/Fn::Not, carries two resources with aCondition:key, an always-created parameter whoseValueis anFn::Ifbranch, and an SNS topic whoseDisplayNameisFn::If(IsPremium, ..., AWS::NoValue).verify.shruns three phases against real AWS: Phase 1 (-c tier=premium) asserts theFn::Ifbranch, both condition-gated parameters present, and the SNSDisplayNameset; Phase 2 (-c tier=basic, in-place redeploy) asserts theFn::Ifbranch flipped, both condition-gated parameters now absent (resource removed because itsCondition:went false), and theDisplayNamegenuinely omitted (AWS::NoValue); Phase 3 destroys and asserts every named resource is gone and the state file removed.filterResourcesByConditionunit tests (tests/unit/analyzer/template-parser.test.ts): false -> removed, true -> kept, no-Condition -> kept, unknown-condition -> kept, preserves Conditions+Outputs and does not mutate input.tests/unit/analyzer/diff-calculator.test.ts): a resource in state but absent from the pruned template diffs asDELETE.{Condition: X}resolution unit tests covering the order-independent evaluation.Validation
/run-integ conditions-and-ifpassed (deploy + redeploy flip + destroy clean, 0 orphans).bench-cdk-samplebroad integ ran clean in this worktree (deploy + destroy, 0 orphans), so theinteg-broadandinteg-destroygates are satisfied.Closes #840