Skip to content

chore(docs): document feature flags and the canonicalAssetHashes scheme#324

Closed
vincenthsh wants to merge 4 commits into
open-constructs:mainfrom
vincenthsh:docs/canonical-asset-hashes-flag
Closed

chore(docs): document feature flags and the canonicalAssetHashes scheme#324
vincenthsh wants to merge 4 commits into
open-constructs:mainfrom
vincenthsh:docs/canonical-asset-hashes-flag

Conversation

@vincenthsh

Copy link
Copy Markdown
Contributor

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:

  • the FUTURE_FLAGS lifecycle (cdktn init enables for new projects, existing projects opt in via cdktf.json, graduation at the next major, no opt-out flags) and a table of all current flags;
  • a full section on 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;
  • the canonical representation itself, explicitly incorporating the review lesson from feat(lib): define a complete canonical asset-hash representation #322: git trees and Nix NAR are canonical because they serialize a complete metadata model — modes and directory records included — not payload framing alone. The documented record table covers file/symlink permission masks (the exact bits archiveSync emits) and explicit directory records including empty ones;
  • migration impact (one-time asset-path change and re-deploy on opt-in, cdktfStaticModuleAssetHash as a pinning escape hatch) and the case for adopting early (re-deploy on your schedule; trustworthy change detection; byte-reproducible archives ending filebase64sha256 drift).

Note: docs/README.md currently 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

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • My changes generate no new warnings

🤖 Generated with Claude Code

vincenthsh and others added 4 commits July 11, 2026 15:00
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>
@sakul-learning

Copy link
Copy Markdown
Contributor

Thanks for documenting this, but this material belongs in the user-facing open-constructs/cdk-terrain-docs repository rather than the deprecated docs/ directory in this implementation repository. I suggest closing this PR and opening a focused docs PR there.

The replacement should be usage- and upgrade-oriented rather than primarily documenting the internal hash record format. In particular, it should explain:

  • how to enable canonicalAssetHashes and which asset paths/types it affects;
  • the user-visible problem it addresses and why asset hashes/paths can change;
  • the impact and risks of enabling it, including the one-time asset path changes, uploads, and resource redeployments;
  • the risks of not enabling it before the behavior becomes the default;
  • the broader symlink-handling fix: what bug it fixes, when the regression was introduced, and the migration impact for projects coming from upstream CDKTF, where symlinks in archive assets worked;
  • which behavior is behind the feature flag versus which symlink corrections apply independently of it.

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 upgrade-guide-v0-24.mdx work should be included after the release PR merges, so please coordinate this canonical-hash/symlink migration guidance with that 0.24 documentation work rather than creating a second documentation home here.

@vincenthsh

Copy link
Copy Markdown
Contributor Author

closing as this goes to open-constructs/cdk-terrain-docs

@vincenthsh vincenthsh closed this Jul 12, 2026
so0k pushed a commit that referenced this pull request Jul 12, 2026
### 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants