fix(lib)!: preserve symlinks in TerraformAsset walkers#321
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>
|
flagged as breaking change as it changes the checksums / SHA created by the
Altho I'm not sure if we want this (or it should be possible to opt-in / default opt-out to this
|
sakul-learning
left a comment
There was a problem hiding this comment.
Review summary
The symlink fix itself is well targeted, and the new tests pass the behavior-oriented/value assessment: each core test exercises an observable regression rather than implementation text. In particular, the extraction tests prove real symlink fidelity; the shared-target size test catches archive materialization/bloat; the circular-link tests protect all three walkers from ELOOP; retargeting checks asset invalidation; and the directory-copy test proves links are recreated rather than dereferenced. The executable-mode test protects a real Lambda/CLI artifact property, while the byte-identical archive test protects deterministic output (subject to resolving the maintainer tradeoff already raised about forcing mtimes). These tests are included by the normal cdktn Nx/Jest target, and the configured full run passed: 39 suites, 454 tests, and 288 snapshots. The focused validations suite and nx build cdktn also passed.
I found one blocking correctness issue in the new hash representation: regular-file bytes and symlink-target bytes occupy the same unframed hash domain, so materially different artifacts can receive the same assetHash and synthesized path. See the inline comment for options and compatibility tradeoffs.
I also think this small PR should cover TerraformModuleAsset's separate source copier. packages/cdktn/src/terraform-module-asset.ts:136-144 sends every non-directory, including a directory symlink, to copyFileSync, which can fail with EISDIR before the corrected TerraformAsset walkers are reached. Reusing the corrected helper (or adding equivalent symlink handling) plus one module-asset regression would address the same class of synthesis failures without materially expanding the PR. GitHub cannot attach a review comment to those unchanged lines, so this item is recorded here and alongside the shared copier.
No workflow files changed, and I found no security concern in the touched code. Ponytail/artifact-value pass: the core regression tests earn their maintenance cost; no speculative abstraction or low-signal static test was added.
…ware copier Review follow-ups on open-constructs#321: - 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 default 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 under a tagged outer hash. - New canonicalAssetHashes feature flag (FUTURE_FLAGS, on for new projects): every entry is hashed as (type, relative path, payload size, payload) in sorted order -- the construction git trees and Nix NAR use -- which also makes renames, entry-boundary shifts, and file-vs-symlink swaps visible to the hash. Graduates to the default at the next major. - 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 and the flag-aware hashPath. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keeping the pin, on prior art: Go module zips do exactly this — the Two things bound the blast radius here: If someone does turn up with a real need for source mtimes we can add a knob then, but I'd rather not grow the JSII surface for it preemptively. |
Requested downscope: keep option 3 here; move canonical hashing outThe compatibility-preserving option 3 now resolves the blocker from the earlier review: trees without symlinks keep their historical content hash, while symlink-bearing trees combine separately framed symlink metadata under a tagged outer digest. That is sufficient for #320 and should remain in this PR. Please remove the additional The reason to separate it is not only scope. The current canonical representation has a metadata hole: it frames file/symlink type, relative path, payload length, and payload, but regular-file records omit the mode that The Git/Nix precedent did not expose this problem because their canonical formats serialize more than payload framing:
So the framing pattern was borrowed without the complete metadata model that makes those formats canonical. #322 captures the needed contract and regressions for modes, empty directories, paths, entry boundaries, and symlink metadata. The |
…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>
0d8c055 to
ff817b2
Compare
|
Downscoped as requested — the branch was force-pushed so this PR is now exactly: the three symlink-aware walkers, the option-3 compatibility-preserving hash, and the shared The follow-up is a stack, review-ready:
Thanks for the git/Nix observation — you were right that the first draft borrowed the framing without the object model that makes those formats canonical. |
There was a problem hiding this comment.
Approved after the downscope and successful checks on ff817b2. The option-3 hash fix and TerraformModuleAsset copier regression are in scope; canonical hashing remains correctly split into the stacked follow-up.
Documentation follow-up for open-constructs/cdk-terrain-docs: please explain the deterministic ZIP timestamp policy and cite golang.org/x/mod/zip as precedent (its format says “File permissions and timestamps are also ignored”). Phrase the comparison precisely: TerraformAsset archives similarly avoid meaningful source timestamps by pinning ZIP mtimes, but unlike Go module ZIPs they preserve regular-file permission bits and support symlinks.
Description
Closes #320.
TerraformAssetwalked source trees withfs.statSync, which follows symlinks. On symlinked trees (e.g. pnpm'snode_modules) this meant:ELOOP. Notably this crashed for every asset type, not justARCHIVE, becausehashPathruns in theTerraformAssetconstructor.This is a fork regression, not inherited from cdktf: upstream archives with
archiver, which walks withlstatand emits real symlink entries (cycles are structurally impossible there). The regression entered whenarchiverwas replaced by yazl in #95 — yazl has no symlink awareness at all (addFilestats-and-follows) — and survived the yazl → fflate rewrite in #148, which kept the hand-rolledstatSyncwalk.The fix
All three walkers in
packages/cdktn/src/private/fs.tsnow uselstatSyncand treat symlinks first-class, restoringarchiver/cdktf parity:archiveSyncemits real symlink entries: thereadlinktarget as STORED entry data withS_IFLNK | permsin the unix external attributes and version-made-by host = Unix — exactly whatarchiverproduces and what Info-ZIPunzip, Goarchive/zip, and the AWS Lambda runtime (verified live onnodejs22.xin TerraformAsset: AssetType.ARCHIVE follows symlinks (statSync), duplicating content and crashing on cyclic links #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).hashPathhashes 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 withfs.symlinkSync.Two latent zip-metadata gaps fixed along the way (both also
archiver/cdktf parity):.binscripts).mtimetoDate.now(), so every synth previously produced different zip bytes — perpetual drift for anyone hashing the archive, e.g.filebase64sha256). The pin uses the local-timeDateconstructor 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-archivealways dereferences and has no cycle detection — it accumulated the opt-inexclude_symlink_directoriesband-aid (v2.4.0, hashicorp/terraform-provider-archive#183; follow-up defect fixed in v2.4.2, #298) and stillELOOPs on cycles. Reverting toarchiverwould reintroduce theexecSyncchild-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
hashPathchange (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 canonical entry-framed scheme and its
canonicalAssetHashesfeature flag moved to the stack: design issue #322 → implementation #323 → docs #324.TerraformModuleAssetstays in scope per review: it shares the corrected copier (its private copier sent directory symlinks intocopyFileSync→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-unzipround-trip +lstat, dedup of shared targets,ELOOPon cycles for botharchiveSyncandhashPath, hash-changes-on-retarget),copySyncsymlink recreation, executable-bit preservation, and byte-identical archives across runs. 5 of the 6 core repros fail onmain.cdktnpackage suite green (453 passed; the one pre-existingmatchers.test.ts→toPlanSuccessfullyfailure shells out to a realterraform planand fails identically without this change).zipinfoshowslrwxr-xr-x ... storentries forlink-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
🤖 Generated with Claude Code