Skip to content

Commit 4f2491e

Browse files
authored
fix(cloudfront): map CFn->SDK casing/shapes and merge UpdateDistribution config (#1372)
1 parent c4206e6 commit 4f2491e

9 files changed

Lines changed: 954 additions & 23 deletions

File tree

.claude/skills/check/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Run these sequentially and report results:
2323
this copy exists because `/check` is usually the FIRST skill run in a
2424
fresh worktree.
2525

26-
1. `vp check --fix` — typecheck + lint + Prettier formatting, with auto-fix. **Use this, not `vp run lint:fix`**: the CI workflow runs `vp check` (which includes Prettier), and `lint:fix` does NOT touch Prettier formatting — so a `lint:fix`-only run passes locally but CI fails with `Formatting issues found` on the same branch. See memory rule `feedback_vp_check_vs_lint_fix.md` for the underlying gotcha and PR #363 for a concrete trap.
26+
1. `vp check --fix` — typecheck + lint + Prettier formatting, with auto-fix. Then `vp run check` — the EXACT command CI's `check-build-test` job runs. The two are NOT equivalent: `vp check --fix` has passed 0-errors while `vp run check` failed with a TS7053 error on the same tree (observed 2026-08-09 on PR #1372 — an implicit-any union index the --fix invocation never surfaced, plus a `no-base-to-string` that --fix reported as a warning but CI failed as an error). Run both; CI parity comes from the second. **Use this, not `vp run lint:fix`**: the CI workflow runs `vp check` (which includes Prettier), and `lint:fix` does NOT touch Prettier formatting — so a `lint:fix`-only run passes locally but CI fails with `Formatting issues found` on the same branch. See memory rule `feedback_vp_check_vs_lint_fix.md` for the underlying gotcha and PR #363 for a concrete trap.
2727
2. `vp run typecheck:test` — type-checks `tsconfig.test.json` (the `tests/**` project). **`vp check` above only type-checks `tsconfig.json` (src/** + types/**), which excludes `**/*.test.ts`** — so a wrong `import type` or a stale mock shape in a test file would pass `vp check` AND `vp test` (whose "Type Errors" line only covers `*.test-d.ts`). This step is what makes test-file type errors fail locally the same way CI now fails them (issue #1133).
2828
3. `vp run build`
2929
4. `vp run test`

docs/_generated/integ-last-run.tsv

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ rollback-failure-injection 2026-07-24T10:50:16Z PASS 368 verify.sh 0724b sweep P
224224
rollback-sqs-cooldown 2026-07-25T05:24:26Z PASS 180 verify.sh #1220 echo-before-assert on steps 4/9/10, destroy clean
225225
route53 2026-07-22T07:59:10Z PASS verify.sh TTL-refresh sweep; HostedZone/GeoProximity/CidrRouting backfills, 6 del 0 err, hosted zone + state gone
226226
s3-asset-deploy 2026-07-26T19:45:58Z PASS 58 verify.sh 0727b sweep-b12 staleness re-run (rc=0); account clean
227-
s3-cloudfront 2026-07-30T23:04:11Z PASS 225 verify.sh 1316 delete waits out propagating disable, 5 del/0 err
227+
s3-cloudfront 2026-08-08T16:49:03Z PASS 223 verify.sh PR1372 post-rebase+lint-fix run; destroy clean 0 orphans
228228
s3-directory-bucket 2026-08-02T15:38:47Z PASS 50 verify.sh re-run for #1347 wontdo-comment PR; guard + clean destroy, 0 orphans
229229
s3-event-notification 2026-07-21T05:28:05Z PASS 79 verify.sh rc ok, orph clean
230230
s3-lifecycle 2026-07-21T14:28:43Z PASS 58 verify.sh rc ok, orph clean

docs/changelog-cdkd.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ The CLAUDE.md `## Known Limitations` section retains the load-bearing summary
1616

1717
---
1818

19+
**Recently Implemented** (2026-08-09):
20+
- ✅ **CloudFront Distribution: CFn -> SDK casing/shape maps + UpdateDistribution read-modify-write merge** (issues #1370, #1371 — both externally reported) — `src/provisioning/providers/cloudfront-distribution-provider.ts`, plus unit tests in `tests/unit/provisioning/cloudfront-distribution-provider.test.ts` and an UPDATE phase + casing assertions added to the `tests/integration/s3-cloudfront/` fixture. **(#1370)** `convertToSdkFormat` passed most `DistributionConfig` keys through verbatim, but three `ViewerCertificate` members (`AcmCertificateArn` / `SslSupportMethod` / `IamCertificateId`) and top-level `IPV6Enabled` differ in ACRONYM CASING between the CFn schema and the CloudFront API (`ACMCertificateArn` / `SSLSupportMethod` / `IAMCertificateId` / `IsIPV6Enabled`), and `Restrictions.GeoRestriction` carries a bare CFn `Locations` array where the SDK wants `{ RestrictionType, Quantity, Items }` — so a custom-domain distribution failed to create ("Your ViewerCertificate is missing one of ACMCertificateArn, IAMCertificateId, or CloudFrontDefaultCertificate"), IPv6 silently ended up disabled, and a geo restriction never reached AWS. Both directions are now mapped (`convertToSdkFormat` + the `convertToCfnFormat` inverse, so `cdkd drift` compares in CFn spelling with no phantom drift). The pre-flight property-coverage check cannot catch this class (it compares TOP-LEVEL properties only; `DistributionConfig` is handled) — the nested-sub-property audit remains tracked under the #1160/#1225 umbrella. **(#1371)** `update()` documented itself as merging the current config but actually sent the template's `DistributionConfig` verbatim (only `CallerReference` was reused), while `UpdateDistribution` is a read-modify-write API expecting the COMPLETE config — so EVERY update of a distribution failed on the first required-on-update member the template legitimately omits ("WebACLId is missing for the resource"). The new `mergeUpdateConfig` merges per top-level member: template is the authority for members it carries, the live config fills members it never templated, and a member REMOVED since the previous template resets to its CloudFormation default (`REMOVAL_RESET_DEFAULTS`, sourced from the registry schema's `default` annotations + empty-list wrappers + the documented Logging-off shape) — members with no documented default (e.g. `IsIPV6Enabled` / `Staging`) keep their live value with a WARN instead of a silent guess. `convertToSdkFormat` also stopped mutating its input's nested `Logging` object (state-sourced `previousProperties` now flow through it). The live UPDATE run then surfaced a SECOND required-field layer: the API accepts SPARSE `Origins` / cache-behavior sub-shapes on CREATE but rejects them on UPDATE ("The 'OriginCustomHeaders' field is missing" / "OriginReadTimeout is required for updates" / "The parameter SmoothStreaming flag is missing" / field-level-encryption-id / allowed-method-settings / lambda-function-associations), so `mergeUpdateConfig` now runs `completeRequiredUpdateFields` — an absent-only fill of the empirically probed required set (per origin: `OriginPath` '' + `CustomHeaders` empty + CustomOriginConfig read/keepalive timeouts 30/5; per cache behavior: `SmoothStreaming` false, `FieldLevelEncryptionId` '', `LambdaFunctionAssociations`/`FunctionAssociations` empty, `TrustedSigners`/`TrustedKeyGroups` disabled, `AllowedMethods` GET/HEAD default with nested `CachedMethods`) — validated by driving the exact payload to a live `UpdateDistribution` success. The probe also exposed two more members of the casing/shape family, fixed in both directions: the CFn template spelling `CustomOriginConfig.OriginSSLProtocols` (capital SSL) never matched the converter's `OriginSslProtocols` lookup (the protocol list was silently dropped on every create), and the CFn SIBLING `CachedMethods` array was never nested inside the SDK's `AllowedMethods` wrapper (`revertCacheBehavior` has always hoisted it back out — the forward direction was simply missing). The first live run of the extended fixture immediately caught a third defect the shape fix newly exposed: CloudFront does not preserve the submitted order of `GeoRestriction` country codes (template `[JP, US]` read back `[US, JP]`), so the positional drift compare fired guaranteed phantom drift on any multi-country restriction — `getDriftUnorderedPaths` now declares `DistributionConfig.Restrictions.GeoRestriction.Locations` as an unordered set (the FSx `WindowsConfiguration.Aliases` precedent, issue #1096 mechanism). Live-verified end-to-end by the extended `s3-cloudfront` integ: create asserts `IsIPV6Enabled=true` + the geo allowlist reached AWS, drift reports clean on the fresh deploy, the new Phase 1.5 re-deploys with `CDKD_TEST_UPDATE=true` (comment + geo change) through the merge — a phase that failed unconditionally before #1371 — and destroy completes clean.
21+
1922
**Recently Implemented** (2026-08-03):
2023
- ✅ **The `cdkd rollback` plan preview stops unwinding a record for a delete it will refuse; dead `supportsFinalSnapshot` removed (issue [#1368](https://github.com/go-to-k/cdkd/issues/1368))** — `src/cli/commands/rollback.ts`, `src/provisioning/final-snapshot.ts`. **Before:** #1366 taught the plan LABEL to say `the rollback will REFUSE it` for a Snapshot shape cdkd cannot snapshot, but `applyPlanToPreview` / `applyFailedPlanToPreview` still deleted that record from the preview state — and the preview state is what the NEXT (older) journal segment's plan is classified against, so an older segment's real work was downgraded to `skip — already reverted` in the one preview the user reads before typing `y`. Second-order: the per-item route stamping (#1366) reads the same preview state, so the dropped record also made a later item fall back to the journaled route — the #1366 defect one layer up. **Now:** both appliers take `skipFinalSnapshot` (as the label functions already did) and consult the SAME `refusesFinalSnapshot` predicate with the SAME route, so the label and the previewed state cannot disagree by construction; under `--skip-final-snapshot` nothing is refused and the unwind is unconditional, matching the executor. **Also:** `supportsFinalSnapshot` had zero `src/` callers (never re-exported from `src/index.ts`, so not a public-API break) and was deleted; its test block is re-homed onto the type SETS as a STRONGER fence — the union pinned against the literal CloudFormation-documented Snapshot-capable list, plus an explicit DISJOINTNESS assertion that nothing had before (load-bearing since #1366: `finalSnapshotMechanism` tests the atomic set first, so a type in both would silently take the atomic arm and never reach the pre-delete snapshot). **Tests:** 4 MULTI-SEGMENT CLI cases (a refused completed CREATE and a refused `--revert-failed` CREATE each keep their record so the older segment is not mislabelled; `--skip-final-snapshot` and a snapshottable shape both still unwind — the carve-out is refusal-only) + 3 re-homed set fences. Binding-proofed: neutering the carve-out fails 2, and adding `AWS::EC2::Volume` to the atomic set fails the union + disjointness fences (and the #1366 matrix cases).
2124
- ✅ **Rollback events report the route the delete took, and the plan preview stops promising a snapshot it will refuse (issue [#1366](https://github.com/go-to-k/cdkd/issues/1366))** — `src/provisioning/final-snapshot.ts` (new pure `finalSnapshotMechanism(type, route)` -> `atomic-delete-parameter` / `pre-delete-snapshot` / `refuse-cc-routed` / `refuse-unsupported-type`, plus the `refusesFinalSnapshot` predicate), `src/deployment/rollback-executor.ts` (`prepareCreateRollbackFinalSnapshot` switches on that matrix instead of re-deriving it; the four CREATE-rollback arms emit the EFFECTIVE route; plan items carry `effectiveProvisionedBy`), `src/cli/commands/rollback.ts` (shared `snapshotNote` helper for both label functions). **Before:** (1) both delete sites resolved the routing layer via `effectiveProvisionedBy` (state record first) and routed the provider lookup with it, but the emitted `ROLLBACK_RESOURCE_*` event still carried `op.provisionedBy` — so when a legacy journal entry disagreed with the state record, `cdkd events` named a layer the delete did not use; (2) `actionLabel` / `failedActionLabel` rendered `[DeletionPolicy Snapshot — final snapshot, then delete]` for EVERY Snapshot-policy resource, including the cc-api-routed atomic types and no-mechanism types the executor was about to REFUSE, and the preview dropped their records as if the delete would happen. **Now:** the event reports the route actually used (resolved BEFORE the record is dropped on the orphan arms, where the authoritative side is about to vanish), and the preview reads the same matrix the replay runs — a shape that will be refused is labelled `cdkd cannot snapshot this resource; the rollback will REFUSE it (re-run with --skip-final-snapshot to delete without one)`. `--skip-final-snapshot` still wins over both (nothing is refused under the opt-out). **Design note:** the refusal verdict lives in the LABEL layer, not as a new `refuse-*` action kind from the classifier — whether a Snapshot shape is refused also depends on `--skip-final-snapshot`, a CLI flag the pure classifier deliberately cannot see, and the label functions already take it. **Tests:** 4 matrix units in `final-snapshot.test.ts` (per-route atomic polarity, route-agnostic pre-delete types, unsupported arm, the predicate pinned in both directions), 7 in `rollback-executor.test.ts` (event route on completed delete / orphan / failed delete / failed orphan — each with the journal and the record DELIBERATELY disagreeing, which is the only shape that can observe the bug — plus the two plan-stamping cases and the legacy fallback), 5 CLI plan-label units. Binding-proofed three ways: neutering `refusesFinalSnapshot` fails 4, reverting the event route fails 4, reverting the plan stamping fails 4. **Scoped out:** `deploy-engine.ts`'s `prepareFinalSnapshotForDelete` still carries a third copy of the matrix — routing it through the new helper is the obvious consolidation but that file is in the `integ-destroy` + `integ-broad` gate scope, so a pure refactor there would force a broad real-AWS integ; left for whenever that file is next touched for a behavior reason.

0 commit comments

Comments
 (0)