Skip to content

feat(lib): Initial Asset Pipeline implementation - #339

Draft
eduardomourar wants to merge 10 commits into
open-constructs:mainfrom
eduardomourar:feat/basic-asset-pipeline
Draft

feat(lib): Initial Asset Pipeline implementation#339
eduardomourar wants to merge 10 commits into
open-constructs:mainfrom
eduardomourar:feat/basic-asset-pipeline

Conversation

@eduardomourar

@eduardomourar eduardomourar commented Jul 19, 2026

Copy link
Copy Markdown

Related issue

Fully Resolves:

Partially Addresses:

Description

Implements a simplified asset staging and bundling pipeline for CDKTN, following patterns from AWS CDK and TerraConstructs while avoiding unnecessary dependencies.

✅ Implemented Features

Core Asset Staging
  • AssetStaging construct for staging files and directories
  • Hash calculation with multiple strategies:
    • SOURCE - based on source content (default)
    • OUTPUT - based on bundled output
    • CUSTOM - user-provided hash, used verbatim
  • Canonical hashing - Feature flag (canonicalAssetHashes) for deterministic, entry-framed hashing modeled on git trees
  • Single fingerprinting implementation - both hash schemes live in private/fs.ts and take an optional exclusion predicate, so exclusions, symlink handling and directory framing cannot drift between the plain and staged code paths
  • Archive-aware framing - a directory staged as a zip is hashed the way archiveSync emits it (no directory records), so the hash tracks the emitted artifact
  • Archive detection for .zip, .tar, .tar.gz, .tgz files
  • Asset caching - staged assets are content-keyed and reused; identical assets in one synth stage (and bundle) once
  • Exclusion patterns - filter files during staging, applied identically to the hash and the staged copy
  • Extra hash - cache busting mechanism, composes with the cdktn:assetHashSalt context key
  • Symlink fidelity - symlinks are staged as symlinks rather than dereferenced; dangling links and links to directories are handled without failing
Docker Bundling
  • Local bundling with Docker fallback via ILocalBundling interface
  • Two bundling modes:
    • BIND_MOUNT - direct host path mounts (default, faster)
    • VOLUME_COPY - Docker volumes with copy (works with remote/shared Docker)
  • Docker command construction with support for:
    • Custom images (registry or build from Dockerfile with path validation)
    • Commands, entrypoints, environment variables
    • User, network, platform, security options
    • Working directory and additional volumes
    • Read-only input mount (BIND_MOUNT) - the host source is mounted read-only so bundlers cannot modify it
  • Bundling output types:
    • ARCHIVED - single archive file
    • NOT_ARCHIVED - directory of files
    • SINGLE_FILE - single non-archive file
    • AUTO_DISCOVER - automatic detection
  • Opt-in mount consistency - the macOS-only consistency flag is only emitted when explicitly requested
  • Scratch isolation - bundling works in a temp directory outside the output tree, so cdktf.out/assets only ever contains real assets
  • Resource cleanup - try/finally wraps resource creation as well as bundling, and each teardown step (helper container, input volume, output volume) is attempted independently so one failure cannot strand the others. Cleanup failures are reported as construct warnings instead of failing synthesis.
  • Pinned helper image - the VOLUME_COPY helper container uses a pinned tag, overridable via CDKTN_BUNDLING_HELPER_IMAGE
Security & Validation
  • Dockerfile path validation - Rejects ../ traversal to ensure Dockerfiles stay within build context
  • Custom hash validation - a custom assetHash names the staged file, so it must match [A-Za-z0-9_.-]+; traversal sequences are rejected rather than resolved into a path
  • Asset type validation - an explicit type that the staged asset cannot satisfy is rejected on both the plain and the staged code paths
  • Read-only input mount - in BIND_MOUNT, bundlers cannot modify or delete host source files. VOLUME_COPY instead never bind-mounts a host path into the bundling container at all.
  • Network option forwarding - Consistent network behavior across both bundling modes
Test Coverage
  • 705 tests total (+20), 300 snapshots for regression detection
  • Comprehensive patterns from AWS CDK tests
  • Unit tests for Docker command construction
  • Integration tests with Docker stubs for end-to-end workflows
  • asset-staging-regression.test.ts - 18 cases, each pinning a behavior that is invisible to the rest of the suite: archive hash framing, a no-op exclude leaving the hash unchanged, exclusions applied to bundled output, symlink fidelity (including dangling and directory links), extraHash composing with a salt, output-directory cleanliness, staged file extensions, custom-hash and type-override validation, App.outdir precedence, and bundling running once per distinct asset
  • Docker argv assertions covering read-only input mounts, network forwarding, --security-opt, and mount consistency
  • Cleanup failure coverage - asserts teardown still runs when bundling fails, and that a failure removing one resource does not skip the rest
  • Dockerfile path validation - negative cases for absolute and ../ paths, plus positive cases that assert the resulting docker build invocation
  • Docker stub output is written per Jest worker, so parallel runs cannot clobber each other's recordings
  • Edge cases: empty dirs, special chars, symlinks, binary files

⏸️ Intentionally Deferred

To maintain simplicity and avoid new dependencies.

Not Implemented from AWS CDK/TerraConstructs:
  • esbuild bundling - would require esbuild dependency
  • Asset synthesis integration - cloud assembly manifest, stack metadata
  • Asset publishing - S3 upload, publishing manifest
  • Disk-based caching - the staging cache is in-memory and per-synth; cross-run caching and complex invalidation are out of scope
  • Full glob exclusions - exclude supports exact paths, *.ext suffixes and directories. **, ?, character classes and ! negation would need a glob dependency and are documented as unsupported.

Rationale: These features add significant complexity and external dependencies that aren't needed for the initial use case. The current implementation provides all core functionality (staging, hashing, bundling) with extensible interfaces for future enhancements.

🔄 Behavior notes for reviewers

  • Asset hashes change for directory assets staged through AssetStaging when canonicalAssetHashes is enabled, because archive framing is now applied correctly. This affects hash values only, not artifact contents.
  • A custom assetHash containing /, .. or other unsafe characters now throws where it was previously interpolated into the staged path.
  • Emitted docker run argv changed: mount consistency is no longer forced onto volumes, and --security-opt is omitted when unset.
  • VOLUME_COPY bundling has been exercised against the Docker stub only; a real-Docker smoke test would be worthwhile before relying on it in CI.
Architecture Diagram
flowchart TD
    %% User-facing entry point
    subgraph PUBLIC["Public API (terraform-asset.ts)"]
        TA["TerraformAsset"]
        CFG["TerraformAssetConfig<br/>path · type · assetHash<br/>exclude · extraHash · bundling · assetHashType"]
    end

    CFG --> TA
    TA -->|"advanced features<br/>detected?"| GATE{exclude ∨ extraHash<br/>∨ bundling ∨ assetHashType}

    GATE -->|"no"| LEGACY["Legacy Path<br/>statSync → hashPath → assetHash<br/>(backwards-compatible)"]
    GATE -->|"yes"| AS

    %% ──────────────────────────────────────────
    subgraph STAGING["Asset Staging (asset-staging.ts)"]
        AS["AssetStaging construct"]
        CACHE{"stagingCache<br/>hit?"}
        AS --> CACHE
        CACHE -->|"yes"| REUSE["Reuse cached StagedAsset"]
        CACHE -->|"no"| B_GATE{"bundling<br/>configured?"}
        B_GATE -->|"no"| RESOLVE
        B_GATE -->|"yes"| BUNDLEPHASE["Run Bundler"]
        BUNDLEPHASE --> RESOLVE["resolvePackaging()<br/>ARCHIVED · NOT_ARCHIVED<br/>AUTO_DISCOVER · SINGLE_FILE"]
        RESOLVE --> HASHPHASE["calculateHash()"]
        HASHPHASE --> COPYPHASE["copyAsset() → asset.{hash}{ext}"]
    end

    %% ──────────────────────────────────────────
    subgraph BUNDLING["Bundling Subsystem (bundling.ts + private/asset-staging.ts)"]
        BUNDLEPHASE --> LOCAL{"ILocalBundling<br/>tryBundle()?"}
        LOCAL -->|"true"| LOCALOUT["Local output dir"]
        LOCAL -->|"false"| DOCKER["Docker bundling"]
        DOCKER --> ACCESS{"BundlingFileAccess"}
        ACCESS -->|"BIND_MOUNT"| BIND["AssetBundlingBindMount<br/>mount source + output dirs"]
        ACCESS -->|"VOLUME_COPY"| VOL["AssetBundlingVolumeCopy<br/>create volumes → helper container<br/>→ cp in → run → cp out → cleanup"]
        BIND --> DIMG["DockerImage.run()"]
        VOL --> DIMG
        DIMG --> DEXEC["dockerExec()<br/>spawn docker CLI"]
    end

    %% ──────────────────────────────────────────
    subgraph HASHING["Hashing (private/fs.ts)"]
        HASHPHASE --> HTYPE{"AssetHashType"}
        HTYPE -->|"SOURCE"| HSRC["Hash original source tree"]
        HTYPE -->|"OUTPUT"| HOUT["Hash bundled output"]
        HTYPE -->|"CUSTOM"| HCUST["Use verbatim<br/>(validated safe chars)"]
        HSRC --> SCHEME{"canonical<br/>feature flag?"}
        HOUT --> SCHEME
        SCHEME -->|"yes"| CANONICAL["canonicalHashPath<br/>git-tree style framing<br/>type·size·name per entry"]
        SCHEME -->|"no"| LEGACYH["legacyHashPath<br/>concatenate file bytes<br/>+ symlink metadata"]
        CANONICAL --> SALT["Apply extraHash + salt<br/>→ MD5 truncated 32 chars"]
        LEGACYH --> SALT
    end

    %% ──────────────────────────────────────────
    subgraph SYNTH["Synthesis (_onSynthesize)"]
        TA -->|"addCustomSynthesis"| EMIT["Emit to stack outdir"]
        EMIT --> ETYPE{"AssetType"}
        ETYPE -->|"FILE"| FCOPY["copyFileSync"]
        ETYPE -->|"DIRECTORY"| DCOPY["copySync (recursive)"]
        ETYPE -->|"ARCHIVE"| ACOPY["archiveSync (zip)<br/>or copyFileSync if pre-zipped"]
    end

    %% Styling
    style PUBLIC fill:#e1f5fe
    style STAGING fill:#fff3e0
    style BUNDLING fill:#fce4ec
    style HASHING fill:#e8f5e9
    style SYNTH fill:#f3e5f5
Loading
Component Interaction (Sequence)
sequenceDiagram
    participant User as User Code
    participant TA as TerraformAsset
    participant AS as AssetStaging
    participant B as Bundler (Docker/Local)
    participant H as hashPath (private/fs)
    participant S as Synthesis

    User->>TA: new TerraformAsset(scope, id, config)
    alt simple path (no advanced features)
        TA->>H: hashPath(sourcePath, {canonical, archive})
        H-->>TA: assetHash
    else advanced path
        TA->>AS: new AssetStaging(scope, id, props)
        alt bundling configured
            AS->>B: local.tryBundle() || Docker run
            B-->>AS: bundled output dir
        end
        AS->>AS: resolvePackaging(output)
        AS->>H: hashPath(source|output, {canonical, archive, exclude})
        H-->>AS: baseHash
        AS->>AS: fold extraHash + salt → final assetHash
        AS->>AS: copyAsset → staging dir
        AS-->>TA: staging.assetHash, packaging, absoluteStagedPath
    end

    Note over TA,S: Later, during app.synth()
    S->>TA: _onSynthesize(session)
    TA->>S: copy staged/source → stack outdir (file/dir/zip)
Loading

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
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

@eduardomourar
eduardomourar requested a review from a team as a code owner July 19, 2026 03:15
@sakul-learning

Copy link
Copy Markdown
Contributor

Before recommending implementation fixes, could we clarify the intended end-state for asset publication? I traced the current PR at a3893fe, compared it with the TerraConstructs/base asset flow referenced in the PR body, and exercised the exported staging API with local bundlers.

What we evaluated

  • The configured repository checks pass: validations.test.ts (31/31), nx build cdktn, and nx test cdktn (583/583).
  • Black-box staging exercises confirmed SINGLE_FILE selection and OUTPUT sensitivity, but also reproduced contract gaps visible in AssetStaging: SOURCE and OUTPUT both reach calculateHash() with finalSourcePath, so a SOURCE hash follows bundled output rather than remaining a source identity; ARCHIVED does not reject empty output, a non-archive file, or multiple output entries.
  • In TerraConstructs/base, local AssetStaging prepares the artifact, while IAssetManager/AwsAssetManager synthesizes aws_s3_object, docker_image, and docker_registry_image resources. Terraform/provider execution performs the upload/build/push during apply, with those operations represented in Terraform state.
  • This PR stops at local synthesis-time staging. Its body explicitly defers cloud-assembly integration and “Asset publishing – S3 upload, publishing manifest.” The four commit messages do not link a larger design/RFC or identify which component will eventually own publication.

The new source/location interfaces could support either of these materially different models:

  1. Terraform-managed publication: preserve the lifecycle boundary used by TerraConstructs, with a provider-specific asset manager synthesizing S3/ECR/Docker resources and a stack integration point for generated or existing storage.
  2. CLI-managed publication: emit an asset manifest and have CDKTN TypeScript/CLI code upload to S3 and build/push to ECR before Terraform runs.

Which lifecycle is intended?

  • Should file uploads and image pushes remain Terraform resources tracked in Terraform state, as in TerraConstructs/base?
  • Or is the long-term plan a CDKTN publisher that performs those side effects directly?
  • Are both intended behind pluggable asset-manager backends?
  • If Terraform-managed publication is intended, should CDKTN gain an equivalent asset-manager seam and stack integration point, or is that expected to remain in provider-specific libraries such as TerraConstructs?

My recommendation is to preserve the Terraform-managed lifecycle unless there is already agreement to change ownership. That would retain the boundary used by the referenced implementation and avoid adding a second side-effecting publication path without an explicit design decision.

If direct CLI publication is the goal—or if both models must coexist—I suggest creating/linking a focused scope RFC before extending these interfaces. It should lock down at least: publication ownership and sequencing; manifest schema/versioning; destination/bootstrap discovery; composition with existing S3/ECR infrastructure; credentials and cross-account behavior; Terraform-state boundaries; retries and partial-failure recovery; cleanup/retention; and migration compatibility.

Hashing/content-fingerprinting recommendation

There is currently a split identity path:

  • ordinary TerraformAsset uses private/fs.ts hashPath(), with legacy versus canonical traversal selected by the canonicalAssetHashes feature flag;
  • advanced TerraformAsset options route through AssetStaging, whose independent full lowercase SHA-256 walker follows symlinks via statSync and has different path, mode, directory, exclusion, and framing behavior;
  • the existing hashPath() contract returns an uppercase 32-character MD5-derived identity, including when canonical entry framing is enabled.

Consequently, a no-op-looking option such as exclude: [] or explicit assetHashType: SOURCE switches both the implementation and externally visible asset identity. CUSTOM also changes meaning: ordinary TerraformAsset.assetHash is used directly, while AssetStaging normalizes it through SHA-256.

I recommend one shared, parameterized internal fingerprinting path for existing TerraformAsset and the new staging/asset-management flow, with canonicalAssetHashes continuing to select the compatibility behavior. The shared core should receive the effective filtered tree, artifact mode, and transformation metadata rather than maintaining AssetStaging.hashPath() separately:

  • SOURCE: fingerprint the filtered source tree plus an RFC-defined, stable description of transformation inputs; it is a source identity, not necessarily the emitted bytes. Non-serializable local bundlers need an explicit invalidation contract, likely via extraHash.
  • OUTPUT: fingerprint the final selected file/directory/archive after bundling and output validation.
  • CUSTOM: define whether the supplied value is used verbatim or normalized, then apply that rule consistently across ordinary and advanced usage.

For compatibility, applications without canonicalAssetHashes should retain legacy identities. Applications with the flag should use the canonical framed traversal already used by TerraformAsset. If SHA-256 is desired, that should be a new/versioned fingerprint-scheme boundary rather than an incidental consequence of enabling exclusions or bundling.

Would you be open to linking an existing design document if one exists, or creating a focused RFC to resolve publication ownership and the fingerprint contract before the deferred publishing work proceeds?

@eduardomourar

Copy link
Copy Markdown
Author

This PR CDKTN's asset implementation follows the core principles from these RFCs of AWS CDK:

Content-addressable hashing (RFC 0092)
Pluggable bundling (RFC 0092)
App-specific staging (RFC 0513)
Separation of staging and deployment (RFC 0513)

Where the key differences from AWS CDK are:

  1. No Cloud Assembly: CDKTN doesn't generate AWS-specific manifests
  2. No Asset Publishing: Terraform handles file references directly (no S3/ECR upload)
  3. Simpler Destinations: Assets are referenced locally in Terraform JSON
  4. No Bootstrap Stack: Terraform doesn't require pre-provisioned resources
  5. State vs Templates: Terraform state scanning instead of CloudFormation template scanning

@eduardomourar

eduardomourar commented Jul 19, 2026

Copy link
Copy Markdown
Author

Now going into depth for the questions more specifically.

Test Coverage: This PR adds comprehensive tests that validate the current independent AssetStaging.hashPath() behavior. These serve as the regression baseline for future unification work.

Hash Paths Confirmed:

  • Path 1: TerraformAsset via src/private/fs.ts (MD5, uppercase, canonical flag)
  • Path 2: AssetStaging via src/asset-staging.ts:310 (SHA-256, lowercase, independent)

The tests confirm your observation: enabling exclude or bundling switches hashing paths, changing asset identity.

Agreement on Next Steps

  1. Terraform-managed publication is the intended model (deferred to provider libraries like TerraConstructs). At a later point in time both publication methods can be used if needed.
  2. Unified fingerprinting should be addressed:
    • Shared fingerprinting core for both TerraformAsset and AssetStaging
    • Feature flag strategy preserving canonicalAssetHashes behavior
    • Migration path for existing applications
    • Hash algorithm selection (MD5 vs SHA-256)

The current implementation establishes core functionality with comprehensive test coverage. The unification work will use these tests as the baseline while maintaining backward compatibility through feature flags.

@so0k, let me know which recommendation I should include as part of this PR otherwise I will have in a follow-up one.

@sakul-learning

Copy link
Copy Markdown
Contributor

Thanks for the detailed breakdown confirming the two hash paths and agreeing that unified fingerprinting is the right direction. The question you raised — whether to include the unification in this PR or defer it to a follow-up — has a clear answer based on the public API surface the PR introduces.

The unification needs to happen in this PR if the advanced public API is committed now. Here's the specific evidence.

Why deferring the hashing unification creates a compatibility problem

packages/cdktn/src/terraform-asset.ts:133-150 routes to AssetStaging — the new, independent SHA-256 path — whenever any of exclude, extraHash, bundling, or assetHashType is truthy. That means adding exclude: [] or explicit AssetHashType.SOURCE (both semantically equivalent to the default behavior) changes:

  1. Asset identity — from the existing 32‑character uppercase MD5‑derived hash (with the existing deterministic file‑tree traversal at private/fs.ts:120-260) to a 64‑character lowercase SHA‑256 hash produced by a separate walker that follows symlinks via statSync (asset-staging.ts:310-329).

  2. Published artifact type — a directory without bundling infers AssetType.DIRECTORY on the existing path (terraform-asset.ts:167-170) but AssetType.ARCHIVE on the AssetStaging path (terraform-asset.ts:154‑160, because staging reports ZIP_DIRECTORY at asset-staging.ts:225‑234). A consumer referencing a directory path now receives archive.zip.

  3. Custom hash semantics — on the existing path, TerraformAsset.assetHash is used verbatim (terraform-asset.ts:171‑176). On the AssetStaging path, that same value is normalized through SHA‑256 (asset-staging.ts:280‑287). The test at terraform-asset.test.ts:244‑258 explicitly asserts the normalized form rather than preserving the supplied value.

This package is jsii‑generated for TypeScript, Python, Java, .NET, and Go (package.json:31‑66). The new exports are public via index.ts:44‑46. Once released, every one of the behaviors above becomes a cross‑language compatibility commitment that cannot change without a major version or feature gate — even though the current implementation is already agreed to be a staging baseline rather than the intended final form.

If the dual‑path API ships first and a follow‑up flattens it, the follow‑up must handle: different hash formats, different hash algorithms, different artifact types, different symlink behavior, and different custom‑hash normalization depending on a now‑public configuration flag. That's not a refactor; it's a compatibility migration.

Three blocking categories

1. One fingerprinting core shared between the existing path and AssetStaging

An explicit exclude: [] or AssetHashType.SOURCE should preserve existing hash format, algorithm, artifact type, and symlink safety unless the user has opted into different behavior through a feature flag.

The existing canonicalAssetHashes feature flag at packages/cdktn/src/features.ts is the natural compatibility boundary:

  • Flag off: retain current TerraformAsset identities.
  • Flag on: use the already‑existing canonical framed traversal (private/fs.ts:120‑260) for both the existing path and the new AssetStaging path.

The AssetStaging hash walker at asset-staging.ts:310‑329 should be replaced by a call to a shared internal primitive that respects the feature flag, rather than maintaining an independent implementation.

2. Correctly implement the advertised SOURCE / OUTPUT / CUSTOM contracts

asset-staging.ts:120‑162 bundles first, then passes finalSourcePath (the bundle output) to calculateHash(). At lines 274‑307, every non‑CUSTOM mode hashes that same path. A bundled SOURCE hash is therefore computed from the output, not the source — the only difference from OUTPUT is that JSON.stringify(props.bundling) is included.

This should match the documented intent now:

  • SOURCE: fingerprint the filtered source tree plus explicitly defined, stable transformation inputs.
  • OUTPUT: fingerprint the final emitted artifact after bundling and output validation.
  • CUSTOM: apply one consistent verbatim‑or‑normalized rule across both the existing and AssetStaging paths.

3. Preserve existing symlink safety and enforce BundlingOutput contracts

The new walker regresses the lstatSync()‑based behavior at private/fs.ts. Symlinks should be preserved as link entries rather than followed with statSync, which can escape the source tree or recurse through cycles — the identical issue fixed in 6360e202.

The BundlingOutput.ARCHIVED and SINGLE_FILE contracts (bundling.ts:101‑124) should either be enforced (validate and throw on invalid output) or intentionally revised. The current implementation silently converts invalid output to ZIP_DIRECTORY, and the tests at bundling.test.ts:494‑522 / 553‑581 codify that contradiction rather than flagging it.

The safe alternative

If finishing the unification in this PR is too large for the current scope, the alternative is to temporarily remove the advanced public API: the exclude, extraHash, bundling, and assetHashType fields from TerraformAsset, the AssetStaging class, the BundlingOptions types, their index.ts exports, and the corresponding tests. They can be reintroduced in a follow‑up that delivers the unified pipeline with correct contracts and a compatible migration path — and that follow‑up won't be burdened with a dual‑path compatibility migration.

Test coverage

The current tests now pass, but several encode the dual‑path behavior and would need rewriting for the unified destination:

  • terraform-asset.test.ts:147‑150 uses assetHashType: 0, but AssetHashType is string‑valued; 0 is falsy, so this does not exercise the advanced SOURCE path.
  • SOURCE vs OUTPUT tests assert only toBeDefined() or hash length, not the advertised source‑vs‑output contract.
  • Archive tests write arbitrary text to a filename ending in .zip and test suffix classification, not a usable archive.
  • Symlink tests silently return as passing if symlinkSync fails, and assert identical hashes for different source paths — freezing symlink‑following behavior that contradicts the existing safety baseline.
  • The Docker fallback test at bundling.test.ts:316‑341 accepts either success or any thrown error, so it cannot fail.

The unification work should replace these with behavior‑focused tests: exclusions affect the actual synthesized artifact, SOURCE and OUTPUT respond to different changes, exclude: [] is a no‑op, symlinks are non‑escaping, invalid output modes throw, and a valid single archive is preserved byte‑for‑byte.

Relative to 0.24.0

@so0k mentioned above that 0.24.0 is targeted this week and the team can aggregate merges for the cycle after. The safest sequencing given that timeline: cut 0.24.0 without this PR, then merge the corrected version early in the next cycle, once the unification and contract fixes are in place.

@jsteinich — would appreciate your read on the public‑API compatibility risks above and the preferred sequencing relative to 0.24.0.

@jsteinich

Copy link
Copy Markdown
Contributor

I'll look at the PR specifics a bit later, but some initial high level thoughts.

  • This is a conceptually large expansion. We shouldn't rush it out and should make sure that we are happy with both the API surface and the commitment to the functionality. Following along with AWS CDK designs is useful, but there's more thought needed to determine if they make sense for CDKTN.
  • Would be nice to have some user stores and/or specific deployment patterns to use in guiding the planning and development.
  • An asset pipeline has implications beyond the initial creation. Assets have lifecycles which are important to think about up front.
  • How does this fit into bigger picture integration with multiple different higher level construct libraries?

@so0k

so0k commented Jul 19, 2026

Copy link
Copy Markdown
Contributor
  • Would be nice to have some user stores and/or specific deployment patterns to use in guiding the planning and development.

User Stories can be seen in TerraConstructs:

  • An asset pipeline has implications beyond the initial creation. Assets have lifecycles which are important to think about up front.

TerraConstructs leverages Terraform State for lifecycle management (unlike the AWSCDK CLI which implements it's own asset manifest). @eduardomourar indicated the same goal would be in follow up PRs

  • How does this fit into bigger picture integration with multiple different higher level construct libraries?
  • Asset Pipeline is available in AWSCDK in case CDK Terrain can integrate with them through a dedicated "bridge-synthesis-package", their CLI keeps control of that and CDK Terrain just piggy backs
  • This is more useful for GCP/Azure which also have object storage (buckets) and container registries, Adding support for Asset Pipelines that can build/bundle and stage Assets (and then dedicated L2 libraries like TerraConstructs/gcp or TerraConstructs/azure could just implement the Cloud Provider specific implementation of the exposed interfaces.

@eduardomourar

eduardomourar commented Jul 20, 2026

Copy link
Copy Markdown
Author

In addition to everything already mentioned, I have been successfully running my team's deployment using TerraConstructs in Production for the past 7 months. I know that my setup is very straightforward with multiple Lambda functions (written in JavaScript/Node.js and Rust) that have zip files from S3 as source asset. Still, the developer experience has been similar to my past work with Lambda using the AWS CDK.

@eduardomourar

eduardomourar commented Jul 20, 2026

Copy link
Copy Markdown
Author

@sakul-learning, following suggestions, the unified fingerprinting has been implemented in d212d56 and BundlingOutput contract issues are fixed in 31607d8. In order to avoid compatibility issues, only MD5‑derived hash is now used following current behavior from TerraformAsset. Feel free to review and validate according to what was previously discussed and agreed.

@eduardomourar
eduardomourar force-pushed the feat/basic-asset-pipeline branch 2 times, most recently from d53e8c0 to cf8c2e2 Compare July 23, 2026 23:28
@eduardomourar

Copy link
Copy Markdown
Author

I have created the following draft PRs to show how the simplification (or addition) would look like for consumers:

@so0k
so0k force-pushed the feat/basic-asset-pipeline branch from cf8c2e2 to 24ebe84 Compare July 24, 2026 14:38
@so0k

so0k commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

oops, sorry rebase flipped the signed commits, my bad

@eduardomourar

Copy link
Copy Markdown
Author

I see linting errors not related to my change somehow. I will try to rebase again and see I can fix most of the errors like this one:

/Users/admin/projects/public/cdk-terrain/packages/cdktn/src/tokens/private/resolve.ts
189:3 error Definition for rule 'no-instanceof/no-instanceof' was not found no-instanceof/no-instanceof

And:

/Users/admin/projects/public/cdk-terrain/packages/cdktn/src/tokens/lazy.ts
150:1 error Definition for rule 'jsdoc/require-jsdoc' was not found jsdoc/require-jsdoc

@sakul-learning

Copy link
Copy Markdown
Contributor

Review update — 24ebe840

Verdict: REQUEST_CHANGES

I reviewed head 24ebe840 against its current base. The dependency update appears valid. The following commands passed in my checkout:

pnpm exec jest packages/cdktn/test/validations.test.ts --runInBand
→ 1 suite, 41 tests, 10 snapshots passed

pnpm exec nx build cdktn
→ jsii: 0 errors, 0 unsilenced warnings

pnpm exec nx test cdktn --runInBand
→ 49 suites, 673 tests, 300 snapshots passed

The following implementation blockers remain:

  1. packages/cdktn/src/asset-staging.ts:383 reads the wrong canonical-hash context key. features.ts defines CANONICAL_ASSET_HASHES as "canonicalAssetHashes", and TerraformAsset reads that constant, but AssetStaging queries "cdktn:canonicalAssetHashes". Advanced staging therefore ignores the established feature flag. Please use the shared constant and add a regression test that enables it through the normal context key.

  2. packages/cdktn/src/private/asset-staging.ts:68-72 exposes /asset-input read-write. DockerImage.run() renders the source volume as <source>:/asset-input:delegated; delegated controls consistency and does not make the mount read-only. A bundler can modify or delete the original source tree. The input mount should be read-only while /asset-output remains writable, with a test asserting both modes.

  3. DockerImage.fromBuild() does not enforce that options.file stays within contextPath. packages/cdktn/src/bundling.ts:344-360 rejects absolute paths, but accepts values such as ../Dockerfile, despite documenting the file as relative to the build context. Please resolve both paths, reject a Dockerfile outside the resolved context, and add a ../Dockerfile regression test.

  4. AssetBundlingVolumeCopy.run() leaks Docker resources after failures. packages/cdktn/src/private/asset-staging.ts:180-205 creates volumes and a helper container, performs the copies and bundle, and only then cleans up, with no try/finally. Any intermediate failure can leave the helper or named volumes behind. Please use try/finally so cleanup is attempted after every failure, and ensure that a failed removal of one resource does not prevent attempting the remaining removals. Add a test for a failed bundle or copy path.

  5. VOLUME_COPY silently drops BundlingOptions.network. The bind-mount path forwards network at private/asset-staging.ts:79; the VOLUME_COPY image.run() call at lines 186-200 omits it. Selecting a file-transfer mechanism should not change independent networking behavior. Please forward the option and cover it in the VOLUME_COPY tests.

  6. cdktn:lint currently fails on PR-local code. packages/cdktn/src/bundling.ts:457-460 uses require("os") and require("fs"). The lint target reports @typescript-eslint/no-require-imports warnings on both lines, and --max-warnings=0 turns them into a failure. Please use ordinary top-level fs and os imports instead.

The linked TerraConstructs and Azure draft consumers show intended downstream use of the staging/bundling abstraction, so I no longer consider the abstraction speculative. They do not mitigate the runtime issues above.

Non-blocking test-maintenance note: the new staging tests run in the normal cdktn suite and cover meaningful Docker orchestration, but the fixed /tmp/docker-stub* files and Bash command parsing remain concurrency/platform-fragile. The suite also repeats many happy-path assertions while missing failure cleanup, VOLUME_COPY network forwarding, read-only input, Dockerfile traversal, and the actual canonical context key. A smaller workflow-focused set covering those boundaries would be more durable.

@so0k

so0k commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

these could be existing bugs in the TerraConstructs btw - sorry for that 😅

@eduardomourar
eduardomourar force-pushed the feat/basic-asset-pipeline branch from 24ebe84 to 6d8ff78 Compare July 24, 2026 21:28
@eduardomourar

Copy link
Copy Markdown
Author

All 5 implementation blockers have been resolved:

  1. ✅ Canonical hash context key - Now uses CANONICAL_ASSET_HASHES constant from features.ts
  2. ✅ Read-only /asset-input mount - Added readOnly field to DockerVolume, input mounts set to read-only
  3. ✅ Dockerfile path validation - Validates Dockerfiles stay within build context, rejects ../ traversal
  4. ✅ Cleanup on failure - Added try/finally with independent error handling for container and volume cleanup
  5. ✅ Network forwarding in VOLUME_COPY - Both BIND_MOUNT and VOLUME_COPY now consistently forward network option

Files Modified

Core Implementation:

  • packages/cdktn/src/asset-staging.ts - Fixed canonical hash key, added import for feature flag constant
  • packages/cdktn/src/bundling.ts - Added readOnly field to DockerVolume, Dockerfile path validation
  • packages/cdktn/src/private/asset-staging.ts - Read-only input mounts, try/finally cleanup, network forwarding

Tests:

  • packages/cdktn/test/asset-staging.test.ts - +3 tests for canonical hash feature flag
  • packages/cdktn/test/bundling.test.ts - +5 tests for Dockerfile path validation
  • packages/cdktn/test/staging.test.ts - +4 tests for read-only mounts, network forwarding, cleanup resilience
  • All existing staging tests updated to expect ,ro flag on input mounts

@sakul-learning

Copy link
Copy Markdown
Contributor

Thanks for working through the review feedback—this update addresses the practical merge blockers identified in the earlier review:

  • AssetStaging now uses the shared CANONICAL_ASSET_HASHES key.
  • The default BIND_MOUNT source is read-only while the output remains writable.
  • Dockerfile paths that traverse outside the permitted context are rejected.
  • VOLUME_COPY now forwards network.
  • The require() lint issue in this change is fixed.

The configured checks are also green on 6d8ff78:

validations: 1 suite, 41 tests, 10 snapshots passed
nx build cdktn: 0 errors
nx test cdktn: 49 suites, 684 tests, 300 snapshots passed
git diff --check: passed

I no longer see a practical merge blocker. The following are explicitly non-blocking follow-up opportunities:

  1. AssetBundlingVolumeCopy.run() enters try/finally only after prepareVolumes() and startHelperContainer(). A failure creating the second volume or starting the helper can still leave already-created resources behind. Also, cleanVolumes() removes both volumes sequentially, so failure removing the input volume prevents attempting the output-volume removal.

  2. A few new tests do not yet distinguish the fixes from the previous implementation:

    • the canonical tests only assert that a hash exists;
    • the test named for VOLUME_COPY read-only behavior only asserts --volumes-from; it would be clearer to rename or remove it, because the host-source read-only behavior applies to BIND_MOUNT;
    • the cleanup-resilience test covers successful cleanup rather than failure cleanup;
    • the positive Dockerfile tests catch broad exceptions instead of asserting the expected build invocation.

The updated BIND_MOUNT command assertions, VOLUME_COPY network test, and negative Dockerfile traversal tests do provide useful regression coverage.

Overall: nice progress—core behavior is in good shape, and I’m comfortable treating the remaining cleanup/test-quality items as follow-ups rather than holding up the asset-pipeline introduction.

@eduardomourar
eduardomourar force-pushed the feat/basic-asset-pipeline branch from 6d8ff78 to ce7c20c Compare July 27, 2026 21:43
- Replace runDockerBundling() with direct DockerImage.run() calls in AssetStaging
- Add DockerVolume and DockerVolumeConsistency interfaces for volume mount configuration
- Add DockerRunOptions interface to standardize container execution parameters
- Move dockerExec() to private/asset-staging.ts for internal use
- Add BUNDLING_INPUT_DIR and BUNDLING_OUTPUT_DIR static constants to AssetStaging
- Update BundlingOptions to accept DockerImage objects instead of string image names
- Add support for additional volumes and volumesFrom in bundling configuration
- Update bundling test fixtures and matchers to work with new DockerImage pattern
- Improve consistency handling for Docker volumes on macOS with DELEGATED mode
…improvements

- Add readOnly property to DockerVolume interface for mounting volumes as read-only
- Validate Dockerfile path stays within build context to prevent escapes
- Improve volume mount mode construction to support read-only consistency options
- Add network option support to volume copy bundling strategy
- Implement comprehensive error handling in bundling cleanup with try-finally
- Replace magic string with CANONICAL_ASSET_HASHES constant for feature flag
- Add test coverage for canonical asset hashes feature flag behavior
- Ensure all cleanup operations attempt execution even if individual steps fail
…iene

Packaging now resolves before hashing, so archive framing and the staged
file extension are no longer derived from unset state. Exclusions route
through a single shared walker in private/fs.ts, removing the duplicated
hash implementations that disagreed with it on directory records and
traversal order.

Staging preserves symlinks instead of dereferencing them, applies
exclusions relative to the copied tree, keeps scratch directories out of
the assets outdir, and reuses staged results so bundling runs once per
distinct asset. Custom hashes and type overrides are validated, salt now
composes with extraHash, and cleanup of VOLUME_COPY resources covers the
setup phase with each step attempted independently.

Co-Authored-By: Claude <noreply@anthropic.com>
@eduardomourar

Copy link
Copy Markdown
Author

Based on discussion with @jsteinich, I decided to split the Asset Pipeline feature into multiple PRs:

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.

TerraformAsset: add bundling feature Allow to ignore files from TerraformAsset Helper for using local files

4 participants