chore(docs): document feature flags and the canonicalAssetHashes scheme#324
chore(docs): document feature flags and the canonicalAssetHashes scheme#324vincenthsh wants to merge 4 commits into
Conversation
archiveSync, hashPath, and copySync all walked with fs.statSync, which follows symlinks: shared targets were duplicated N times into archives (15 MB -> 34 MB on a real pnpm Lambda bundle), symlink entries were never emitted, and circular links (legitimate in pnpm node_modules) crashed synth with ELOOP -- for every asset type, since hashPath runs in the TerraformAsset constructor. Walk with lstatSync instead and treat symlinks first-class, restoring the behavior cdktf gets from archiver (dropped in open-constructs#95 when archiver was replaced by yazl, kept broken through the fflate rewrite in open-constructs#148): - archiveSync emits real symlink entries (readlink target as STORED data, S_IFLNK unix mode in the external attrs, host os Unix), now also preserves regular-file modes (executable bits were stripped), and pins entry mtimes to a fixed date so archives are byte-reproducible (fflate defaulted to Date.now() per synth) - hashPath hashes a symlink by its target path, so retargeting still changes the asset hash without following (or double-counting) links - copySync recreates symlinks for AssetType.DIRECTORY assets Fixes open-constructs#320 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ware copier Review follow-ups on open-constructs#321, downscoped per review to the compatibility-preserving hash only (canonical hashing moved to open-constructs#322): - hashPath fed symlink targets and file contents into one unframed stream, so a file containing 'foo' and a symlink targeting 'foo' could produce the same assetHash for materially different artifacts. The scheme now keeps the legacy content hash byte-identical for symlink-free trees and, only when symlinks exist, combines it with separately-framed symlink metadata (relative path + target) under a tagged outer hash. - TerraformModuleAsset dropped its private copier, which routed directory symlinks into copyFileSync (EISDIR) before the fixed walkers could run; it now shares the symlink-aware copySync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduces the canonical entry-framed asset hash from open-constructs#322 as an opt-in FUTURE_FLAGS entry (on for new projects, opt-in for existing ones, default at the next major). The scheme is modeled on git trees and Nix NAR and frames everything that affects the emitted artifact: - files: 'F <mode> <relPath>NUL<size>NUL' + content, where mode is the octal permission mask archiveSync preserves in zip external attributes - symlinks: 'L <mode> <relPath>NUL<target length>NUL' + target - directories: 'D <relPath>NUL' including empty ones (copySync materializes them); no mode, since neither emitter preserves directory permissions Entries are framed in sorted order with /-separated relative paths, so renames, entry-boundary shifts, permission changes, empty-directory changes, file-vs-symlink swaps, and symlink retargeting all change the hash -- closing the metadata hole identified in review (0644 -> 0755 previously changed archive bytes but not the hash). The legacy scheme remains the default for existing projects and is untouched by the flag. Closes open-constructs#322 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/feature-flags.md covering the FUTURE_FLAGS lifecycle and the canonicalAssetHashes flag from open-constructs#323: why the legacy hash is blind to renames, entry boundaries, permissions, and empty directories; how the canonical representation frames the complete metadata model (the git-tree/Nix-NAR lesson from open-constructs#322 -- payload framing alone is not canonical); the one-time migration impact of opting in; and why early adoption ahead of the next major is recommended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for documenting this, but this material belongs in the user-facing The replacement should be usage- and upgrade-oriented rather than primarily documenting the internal hash record format. In particular, it should explain:
Because these are potentially breaking asset-behavior changes behind a feature flag, most of the migration guidance should go into the 0.24.0 release/upgrade guide, with the reusable feature-flag usage material linked from the relevant asset documentation. There is already an open docs PR for the Node.js 22.19+ prerequisite: cdk-terrain-docs#28. Its thread explicitly notes that the full |
|
closing as this goes to |
### Description Closes #320. `TerraformAsset` walked source trees with `fs.statSync`, which follows symlinks. On symlinked trees (e.g. pnpm's `node_modules`) this meant: - **Archive bloat**: a target reachable through N symlink paths was copied N times into the zip (15 MB → 34 MB on a real Lambda bundle for byte-identical logical content). - **No symlink fidelity**: archives contained zero symlink entries — links became full copies. - **Hard synth crash**: circular symlinks (legitimate in pnpm trees) hit `ELOOP`. Notably this crashed for **every** asset type, not just `ARCHIVE`, because `hashPath` runs in the `TerraformAsset` constructor. This is a fork regression, not inherited from cdktf: upstream archives with `archiver`, which walks with `lstat` and emits real symlink entries (cycles are structurally impossible there). The regression entered when `archiver` was replaced by yazl in #95 — yazl has no symlink awareness at all (`addFile` stats-and-follows) — and survived the yazl → fflate rewrite in #148, which kept the hand-rolled `statSync` walk. ### The fix All three walkers in `packages/cdktn/src/private/fs.ts` now use `lstatSync` and treat symlinks first-class, restoring `archiver`/cdktf parity: - **`archiveSync`** emits real symlink entries: the `readlink` target as STORED entry data with `S_IFLNK | perms` in the unix external attributes and version-made-by host = Unix — exactly what `archiver` produces and what Info-ZIP `unzip`, Go `archive/zip`, and the AWS Lambda runtime (verified live on `nodejs22.x` in #320) recreate as symlinks. Never recursing through links makes cycles unreachable by construction. fflate 0.8.2 supports this natively via per-file `[data, { os, attrs, level }]` tuples (attrs must be caller-pre-shifted, `(mode << 16) >>> 0`). - **`hashPath`** hashes a symlink by its target path instead of following it — retargeting a link still changes the asset hash, but shared targets are no longer double-counted and cycles no longer crash the constructor. - **`copySync`** (`AssetType.DIRECTORY`) recreates symlinks with `fs.symlinkSync`. Two latent zip-metadata gaps fixed along the way (both also `archiver`/cdktf parity): - Regular-file unix modes are now preserved (executable bits were silently stripped — matters for Lambda binaries and `.bin` scripts). - Entry mtimes are pinned to a fixed 1980 date, making archives byte-reproducible across synths (fflate defaults `mtime` to `Date.now()`, so every synth previously produced different zip bytes — perpetual drift for anyone hashing the archive, e.g. `filebase64sha256`). The pin uses the local-time `Date` constructor deliberately: fflate encodes DOS dates from local getters and rejects years < 1980, so a UTC midnight date would underflow to 1979 in timezones west of UTC. Prior art considered and rejected: `terraform-provider-archive` always dereferences and has no cycle detection — it accumulated the opt-in `exclude_symlink_directories` band-aid (v2.4.0, hashicorp/terraform-provider-archive#183; follow-up defect fixed in v2.4.2, #298) and still `ELOOP`s on cycles. Reverting to `archiver` would reintroduce the `execSync` child-process bridge that #148 removed (its API is async-only). ### Hash compatibility (updated after review — downscoped) Review surfaced a domain collision in the first cut of the `hashPath` change (file bytes and symlink-target bytes shared one unframed stream). Per the requested downscope this PR now carries only the compatibility-preserving resolution (reviewer's option 3): - the legacy content hash is kept byte-identical for symlink-free trees (proven by a test that recomputes the historical md5 independently); - symlink metadata (relative path + target) is framed into a separate digest and combined under a tagged outer hash **only when symlinks exist** — so hashes still change only for symlink-bearing trees, which today either bloat 2-3× or crash outright. The canonical entry-framed scheme and its `canonicalAssetHashes` feature flag moved to the stack: design issue #322 → implementation #323 → docs #324. `TerraformModuleAsset` stays in scope per review: it shares the corrected copier (its private copier sent directory symlinks into `copyFileSync` → `EISDIR`) with a regression test. ### Testing - `packages/cdktn/test/archive-symlink.test.ts`: 10 tests — the issue's repros (file/dir symlink preservation via system-`unzip` round-trip + `lstat`, dedup of shared targets, `ELOOP` on cycles for both `archiveSync` and `hashPath`, hash-changes-on-retarget), `copySync` symlink recreation, executable-bit preservation, and byte-identical archives across runs. 5 of the 6 core repros fail on `main`. - Full `cdktn` package suite green (453 passed; the one pre-existing `matchers.test.ts` → `toPlanSuccessfully` failure shells out to a real `terraform plan` and fails identically without this change). - Issue #320's literal repro script against the built lib: `zipinfo` shows `lrwxr-xr-x ... stor` entries for `link-a`/`link-b`, one payload copy (538-byte zip vs 3× 200 KiB before), and the cyclic tree archives to a 120-byte zip instead of aborting synth. ### Checklist - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation if applicable - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works if applicable - [x] New and existing unit tests pass locally with my changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Description
Docs follow-up promised in #321/#323. Stacked on #323 (which is stacked on #321) — only the last commit (
chore(docs): document feature flags and the canonicalAssetHashes scheme) is new here; the rest collapses as the base PRs merge, and I'll rebase after each.Adds
docs/feature-flags.md:FUTURE_FLAGSlifecycle (cdktn initenables for new projects, existing projects opt in viacdktf.json, graduation at the next major, no opt-out flags) and a table of all current flags;canonicalAssetHashes(feat(lib): canonical asset hashes behind the canonicalAssetHashes feature flag #323): the legacy hash's structural blind spots (renames, entry boundaries, permissions, empty directories), and why canonical hashing is the recommended future default;archiveSyncemits) and explicit directory records including empty ones;cdktfStaticModuleAssetHashas a pinning escape hatch) and the case for adopting early (re-deploy on your schedule; trustworthy change detection; byte-reproducible archives endingfilebase64sha256drift).Note:
docs/README.mdcurrently declares this directory deprecated in favor of upstream terraform.io docs — that pointer is stale for the fork, and this page starts rebuilding fork-native docs. Happy to move the page if maintainers prefer a different docs home.Checklist
🤖 Generated with Claude Code