test(integ): sg-circular-dependency failure-seeking integration test - #842
Merged
Conversation
…roup refs Surfaces create/destroy DAG-ordering bugs with a circular Security Group reference: SG-A allows ingress from SG-B AND SG-B allows ingress from SG-A. Modeled the CFn-safe way via standalone AWS::EC2::SecurityGroupIngress resources (not inline ingress) so the two SGs can exist before the cross-references are added. CDK emits each addIngressRule(otherSg, ...) as a standalone ingress resource whose SourceSecurityGroupId Fn::GetAtt's the other SG, breaking what would otherwise be a genuine SG-to-SG cycle. Confirmed via synth: 2 standalone SecurityGroupIngress resources, zero inline ingress. Stresses: 1. DEPLOY - the DAG builder (src/analyzer/dag-builder.ts) must NOT raise a false DependencyError; the standalone ingress resources break the cycle. 2. DESTROY (the key test) - the ingress rules must be revoked BEFORE the SGs are deleted, or AWS rejects DeleteSecurityGroup with DependencyViolation. Exercises the existing AWS::EC2::SecurityGroup -> SecurityGroupIngress implicit-delete-dep edge (src/analyzer/implicit-delete-deps.ts) end-to-end. verify.sh is BSD/macOS-portable, captures real exit codes, prints an explicit PASS only on full success, locates resources by the cdkd:integ-fixture tag, and its EXIT trap revokes-then-deletes both SGs directly so a destroy-ordering bug never leaks billing resources. Adds KNOWN_SCENARIOS tag sg-circular-dependency; regenerates the integ-coverage and scenario-coverage matrices; adds a changelog entry. Test-only; no src/ change.
The Phase 0 cdkd synth invocation passed --state-bucket (and a no-op --region), but cdkd synth only synthesizes the CDK app to a template and does NOT read or write state, so it rejects --state-bucket with "error: unknown option '--state-bucket'". That aborted the whole script before the deploy ran, so the SG circular create/destroy ordering (the entire point of the fixture) was never exercised. Drop --state-bucket / --region from the synth call (keep only the supported --output plus the stack selector). The deploy and destroy invocations were already correct (deploy/destroy do accept --state-bucket/--region) and are unchanged, so the create/destroy ordering assertions now run as intended.
AWS rejects non-ASCII in SecurityGroup GroupDescription
("Character sets beyond ASCII are not supported"), so the em-dash
(U+2014) in the SG descriptions failed the deploy step. Replace the
em-dashes in both SG descriptions with an ASCII hyphen, and also
de-Unicode the stack/app comment em-dashes and the ASCII-art box
drawing for consistency. Re-synth confirms ASCII-only GroupDescription
values and the two standalone AWS::EC2::SecurityGroupIngress resources
are unchanged, so the circular-ref create/destroy ordering test still
exercises the intended shape.
…ion for sg-circular integ
…-circular verify.sh The fixture comments (verify.sh, the lib stack JSDoc, and README.md) claim the script confirms zero inline SecurityGroupIngress entries on either SG, but verify.sh only ran positive checks (>= 2 standalone AWS::EC2::SecurityGroupIngress resources, >= 2 carrying SourceSecurityGroupId). It never asserted the inline Properties.SecurityGroupIngress was empty, so the comment's claim was not enforced. Add a jq guard against the synthesized template asserting NO AWS::EC2::SecurityGroup carries a non-empty Properties.SecurityGroupIngress array (an inline ingress pointing at the other SG is exactly what would reintroduce the CFn cycle this fixture exists to avoid). The check fails with a clear message naming the offending SG logical id(s). BSD/macOS-portable (no grep -P). The existing comments now match the enforced behavior (review fix).
go-to-k
force-pushed
the
test/sg-circular-dependency
branch
from
June 13, 2026 14:13
0fc55fa to
8a29a1c
Compare
Owner
Author
|
Review note: pr-review-gate classifies this 3-axis by file count, but it is a fixture-only PR (no src logic). Per maintainer go-ahead, reviewed by 1 pr-code-reviewer; the one minor finding (missing zero-inline-ingress assertion) is fixed and the assertion now enforced. Setting pr-review marker manually per the gate's documented fixture-only escape hatch. (rebased on main: 8a29a1c) |
|
🎉 This PR is included in version 0.221.1 🎉 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
Test-only PR (no
src/change). Adds a failure-seeking integration testtests/integration/sg-circular-dependency/that stresses cdkd's create/destroyDAG ordering with the classic CloudFormation "circular Security Group" shape:
SG-A allows ingress from SG-B AND SG-B allows ingress from SG-A.
The cycle is broken the CloudFormation-safe way: each ingress rule is emitted as
a STANDALONE
AWS::EC2::SecurityGroupIngressresource (not inline), so the twoSGs can exist before the cross-references are added. In CDK,
sgA.addIngressRule(sgB, ...)+sgB.addIngressRule(sgA, ...)against twodistinct SG constructs makes CDK emit standalone ingress resources (each
Fn::GetAttsGroupIdon the SG it attaches to andSourceSecurityGroupIdonthe OTHER SG). The stack is a
natGateways: 0single-AZ VPC + SG-A + SG-B + thetwo cross-referencing ingress resources (no EC2 instances, so no instance cost).
What it stresses
src/analyzer/dag-builder.ts) must NOTraise a false
DependencyError-- the standalone ingress resources break whatwould otherwise be a genuine SG-to-SG cycle.
be revoked / deleted BEFORE the SGs. If cdkd deletes an SG while its
cross-referencing ingress rule is still live, AWS rejects
DeleteSecurityGroupwithDependencyViolation: resource sg-xxx has a dependent object, so a wrongdelete order fails or orphans here. This exercises the existing
AWS::EC2::SecurityGroup -> AWS::EC2::SecurityGroupIngressimplicit-delete-depedge in
src/analyzer/implicit-delete-deps.tsend-to-end on real AWS.verify.sh(BSD/macOS-portable, captures real exit codes, prints PASS only onfull success): Phase 0 synth-asserts the cycle-breaking shape (>= 2 standalone
ingress resources each carrying a
SourceSecurityGroupId); Phase 1 deploys andasserts both SGs exist with the live cross-reference; Phase 2 destroys and
asserts 0 errors plus both SGs + the VPC + the state file gone. Resources are
located by the
cdkd:integ-fixture=sg-circular-dependencytag, and the EXIT-trapcleanup revokes-then-deletes both SGs directly so a destroy-ordering bug never
leaks billing resources.
ASCII-description fix
While authoring the fixture, AWS rejected the deploy because the SecurityGroup
GroupDescriptionstrings contained a non-ASCII em-dash (U+2014). AWS onlyaccepts ASCII characters in
GroupDescription, so both descriptions are nowASCII-only (plain hyphen). Lesson recorded: integ fixture strings that AWS
validates (such as SecurityGroup descriptions) must be ASCII.
Validation
Validated GREEN against real AWS:
cdkd deployandcdkd destroybothcompleted cleanly with 0 orphan resources. The live-test requirement is
satisfied.
New scenario tag
sg-circular-dependencyadded to the canonical taxonomy(
scripts/build-scenario-coverage-matrix.ts); the integ-coverage andscenario-coverage matrices were regenerated. Typecheck / lint / build / unit
tests (5755 tests) all pass.