Skip to content

Commit 6360e20

Browse files
vincenthshclaude
andauthored
fix(lib)!: preserve symlinks in TerraformAsset walkers (#321)
### 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>
1 parent a899b7c commit 6360e20

4 files changed

Lines changed: 386 additions & 38 deletions

File tree

packages/cdktn/src/private/fs.ts

Lines changed: 90 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,33 @@
33
import * as fs from "fs";
44
import * as path from "path";
55
import * as crypto from "crypto";
6-
import { zipSync } from "fflate";
6+
import { strToU8, zipSync } from "fflate";
7+
import type { ZipOptions } from "fflate";
78
import { assetCanNotCreateZipArchive } from "../errors";
89

910
const HASH_LEN = 32;
1011

12+
// Unix file-type bits for zip external attributes (st_mode upper nibble)
13+
const S_IFLNK = 0o120000;
14+
const S_IFREG = 0o100000;
15+
const PERM_MASK = 0o7777;
16+
// "version made by" host byte; unzip only honors unix mode attrs when set
17+
const ZIP_OS_UNIX = 3;
18+
// Pinned so archives are byte-reproducible across synths. fflate encodes
19+
// DOS dates from local-time getters and rejects years outside 1980-2099,
20+
// so this must be a local-time 1980 date (a UTC one underflows to 1979 in
21+
// timezones west of UTC).
22+
const ZIP_ENTRY_MTIME = new Date(1980, 0, 1);
23+
24+
/**
25+
* Zip external attributes for a unix mode: file-type and permission bits
26+
* belong in the high 16 bits of the 32-bit field.
27+
* @param mode - unix st_mode bits (type | permissions)
28+
*/
29+
function zipAttrs(mode: number): number {
30+
return (mode << 16) >>> 0;
31+
}
32+
1133
// Full implementation at https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/copy-sync.js
1234
/**
1335
* Copy a file or directory. The directory can have contents and subfolders.
@@ -21,11 +43,12 @@ export function copySync(src: string, dest: string) {
2143
*/
2244
function copyItem(p: string) {
2345
const sourcePath = path.resolve(src, p);
24-
const stat = fs.statSync(sourcePath);
25-
if (stat.isFile()) {
46+
const stat = fs.lstatSync(sourcePath);
47+
if (stat.isSymbolicLink()) {
48+
fs.symlinkSync(fs.readlinkSync(sourcePath), path.resolve(dest, p));
49+
} else if (stat.isFile()) {
2650
fs.copyFileSync(sourcePath, path.resolve(dest, p));
27-
}
28-
if (stat.isDirectory()) {
51+
} else if (stat.isDirectory()) {
2952
walkSubfolder(p);
3053
}
3154
}
@@ -51,52 +74,99 @@ export function copySync(src: string, dest: string) {
5174
*/
5275
export function archiveSync(src: string, dest: string) {
5376
try {
54-
const files: Record<string, Uint8Array> = {};
77+
const files: Record<string, [Uint8Array, ZipOptions]> = {};
5578
const walk = (dir: string, prefix: string) => {
5679
for (const entry of fs.readdirSync(dir)) {
5780
const full = path.join(dir, entry);
5881
const zipPath = prefix ? `${prefix}/${entry}` : entry;
59-
if (fs.statSync(full).isDirectory()) {
82+
const stat = fs.lstatSync(full);
83+
if (stat.isSymbolicLink()) {
84+
// Store the link target as the entry data with S_IFLNK attrs so
85+
// extractors recreate the symlink instead of a copy of the target.
86+
// Never recursing through links also makes cycles unreachable.
87+
files[zipPath] = [
88+
strToU8(fs.readlinkSync(full)),
89+
{
90+
os: ZIP_OS_UNIX,
91+
attrs: zipAttrs(S_IFLNK | (stat.mode & PERM_MASK)),
92+
level: 0,
93+
},
94+
];
95+
} else if (stat.isDirectory()) {
6096
walk(full, zipPath);
6197
} else {
62-
files[zipPath] = fs.readFileSync(full);
98+
files[zipPath] = [
99+
fs.readFileSync(full),
100+
{
101+
os: ZIP_OS_UNIX,
102+
attrs: zipAttrs(S_IFREG | (stat.mode & PERM_MASK)),
103+
},
104+
];
63105
}
64106
}
65107
};
66108
walk(src, "");
67-
fs.writeFileSync(dest, zipSync(files, { level: 9 }));
109+
fs.writeFileSync(
110+
dest,
111+
zipSync(files, { level: 9, mtime: ZIP_ENTRY_MTIME }),
112+
);
68113
} catch (err: any) {
69114
throw assetCanNotCreateZipArchive(src, dest, err);
70115
}
71116
}
72117

73118
/**
74119
* Compute a stable MD5 hash of a file or directory's contents.
75-
* Directories are hashed by recursively folding each file's contents into
76-
* the same digest, in directory-listing order.
120+
* File contents fold into one digest in directory-listing order, exactly as
121+
* earlier releases did, so trees without symlinks keep their historical
122+
* hashes. Symlinks are hashed by their metadata (path + target) instead of
123+
* being followed, so shared targets are not double-counted and cycles cannot
124+
* recurse; that metadata goes into a second digest, and only when symlinks
125+
* exist are the two combined under a tagged outer hash — the tag keeps
126+
* symlink metadata in a separate domain from file bytes, so a file
127+
* containing `foo` can never collide with a symlink targeting `foo`.
77128
* @param src - path to a file or directory to hash
78129
* @returns uppercased hex digest, truncated to HASH_LEN characters
79130
*/
80131
export function hashPath(src: string): string {
81-
const hash = crypto.createHash("md5");
132+
const content = crypto.createHash("md5");
133+
const links = crypto.createHash("md5");
134+
let linkCount = 0;
82135

83136
/**
84-
* Walk `p`, feeding any file contents into the enclosing hash accumulator.
137+
* Walk `p`, feeding file contents and symlink metadata into the enclosing
138+
* accumulators.
85139
* @param p - path to walk
140+
* @param relPath - path of `p` relative to the walk root, `/`-separated
141+
* @param isRoot - follow a symlink only at the root, matching how the
142+
* asset's own path is resolved when it is read
86143
*/
87-
function hashRecursion(p: string) {
88-
const stat = fs.statSync(p);
89-
if (stat.isFile()) {
90-
hash.update(fs.readFileSync(p));
144+
function hashRecursion(p: string, relPath: string, isRoot = false) {
145+
const stat = isRoot ? fs.statSync(p) : fs.lstatSync(p);
146+
if (stat.isSymbolicLink()) {
147+
links.update(`${relPath}\0${fs.readlinkSync(p)}\0`);
148+
linkCount++;
149+
} else if (stat.isFile()) {
150+
content.update(fs.readFileSync(p));
91151
} else if (stat.isDirectory()) {
92152
fs.readdirSync(p).forEach((filename) =>
93-
hashRecursion(path.resolve(p, filename)),
153+
hashRecursion(
154+
path.resolve(p, filename),
155+
relPath ? `${relPath}/${filename}` : filename,
156+
),
94157
);
95158
}
96159
}
97160

98-
hashRecursion(src);
99-
return hash.digest("hex").slice(0, HASH_LEN).toUpperCase();
161+
hashRecursion(src, "", true);
162+
if (linkCount === 0) {
163+
return content.digest("hex").slice(0, HASH_LEN).toUpperCase();
164+
}
165+
const outer = crypto.createHash("md5");
166+
outer.update("cdktn/asset-hash/symlinks/v1\0");
167+
outer.update(content.digest("hex"));
168+
outer.update(links.digest("hex"));
169+
return outer.digest("hex").slice(0, HASH_LEN).toUpperCase();
100170
}
101171

102172
/**

packages/cdktn/src/terraform-module-asset.ts

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import * as os from "os";
99
import * as fs from "fs";
1010
import { TerraformStack } from "./terraform-stack";
1111
import { AssetType, TerraformAsset } from "./terraform-asset";
12-
import { hashPath } from "./private/fs";
12+
import { copySync, hashPath } from "./private/fs";
1313

1414
const TERRAFORM_MODULE_ASSET_SYMBOL = Symbol.for("cdktf.TerraformModuleAsset");
1515

@@ -127,19 +127,3 @@ export function findLowestCommonPath(paths: string[]): string | undefined {
127127
const relativePath = path.relative(process.cwd(), absolutePathPrefix);
128128
return relativePath === "" ? "." : relativePath;
129129
}
130-
131-
/**
132-
* Copies a file or directory recursively
133-
* @param from
134-
* @param to
135-
*/
136-
function copySync(from: string, to: string) {
137-
if (fs.lstatSync(from).isDirectory()) {
138-
fs.mkdirSync(to, { recursive: true });
139-
for (const file of fs.readdirSync(from)) {
140-
copySync(path.join(from, file), path.join(to, file));
141-
}
142-
} else {
143-
fs.copyFileSync(from, to);
144-
}
145-
}

0 commit comments

Comments
 (0)