feat(lib): Initial Asset Pipeline implementation - #339
Conversation
|
Before recommending implementation fixes, could we clarify the intended end-state for asset publication? I traced the current PR at What we evaluated
The new source/location interfaces could support either of these materially different models:
Which lifecycle is intended?
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 recommendationThere is currently a split identity path:
Consequently, a no-op-looking option such as I recommend one shared, parameterized internal fingerprinting path for existing
For compatibility, applications without 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? |
|
This PR CDKTN's asset implementation follows the core principles from these RFCs of AWS CDK: ✅ Content-addressable hashing (RFC 0092) Where the key differences from AWS CDK are:
|
|
Now going into depth for the questions more specifically. Test Coverage: This PR adds comprehensive tests that validate the current independent Hash Paths Confirmed:
The tests confirm your observation: enabling Agreement on Next Steps
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. |
|
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
This package is jsii‑generated for TypeScript, Python, Java, .NET, and Go ( 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 categories1. One fingerprinting core shared between the existing path and An explicit The existing
The 2. Correctly implement the advertised
This should match the documented intent now:
3. Preserve existing symlink safety and enforce The new walker regresses the The The safe alternativeIf finishing the unification in this PR is too large for the current scope, the alternative is to temporarily remove the advanced public API: the Test coverageThe current tests now pass, but several encode the dual‑path behavior and would need rewriting for the unified destination:
The unification work should replace these with behavior‑focused tests: exclusions affect the actual synthesized artifact, 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. |
|
I'll look at the PR specifics a bit later, but some initial high level thoughts.
|
User Stories can be seen in TerraConstructs:
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
|
|
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. |
|
@sakul-learning, following suggestions, the unified fingerprinting has been implemented in d212d56 and |
d53e8c0 to
cf8c2e2
Compare
|
I have created the following draft PRs to show how the simplification (or addition) would look like for consumers: |
cf8c2e2 to
24ebe84
Compare
|
oops, sorry rebase flipped the signed commits, my bad |
|
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:
And:
|
Review update —
|
|
these could be existing bugs in the TerraConstructs btw - sorry for that 😅 |
24ebe84 to
6d8ff78
Compare
|
All 5 implementation blockers have been resolved:
Files ModifiedCore Implementation:
Tests:
|
|
Thanks for working through the review feedback—this update addresses the practical merge blockers identified in the earlier review:
The configured checks are also green on I no longer see a practical merge blocker. The following are explicitly non-blocking follow-up opportunities:
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. |
6d8ff78 to
ce7c20c
Compare
- 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>
ce7c20c to
0c8578c
Compare
|
Based on discussion with @jsteinich, I decided to split the Asset Pipeline feature into multiple PRs:
|
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
SOURCE- based on source content (default)OUTPUT- based on bundled outputCUSTOM- user-provided hash, used verbatimcanonicalAssetHashes) for deterministic, entry-framed hashing modeled on git treesprivate/fs.tsand take an optional exclusion predicate, so exclusions, symlink handling and directory framing cannot drift between the plain and staged code pathsarchiveSyncemits it (no directory records), so the hash tracks the emitted artifact.zip,.tar,.tar.gz,.tgzfilescdktn:assetHashSaltcontext keyDocker Bundling
ILocalBundlinginterfaceBIND_MOUNT- direct host path mounts (default, faster)VOLUME_COPY- Docker volumes with copy (works with remote/shared Docker)BIND_MOUNT) - the host source is mounted read-only so bundlers cannot modify itARCHIVED- single archive fileNOT_ARCHIVED- directory of filesSINGLE_FILE- single non-archive fileAUTO_DISCOVER- automatic detectionconsistencyflag is only emitted when explicitly requestedcdktf.out/assetsonly ever contains real assetstry/finallywraps 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.VOLUME_COPYhelper container uses a pinned tag, overridable viaCDKTN_BUNDLING_HELPER_IMAGESecurity & Validation
../traversal to ensure Dockerfiles stay within build contextassetHashnames the staged file, so it must match[A-Za-z0-9_.-]+; traversal sequences are rejected rather than resolved into a pathtypethat the staged asset cannot satisfy is rejected on both the plain and the staged code pathsBIND_MOUNT, bundlers cannot modify or delete host source files.VOLUME_COPYinstead never bind-mounts a host path into the bundling container at all.Test Coverage
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-opexcludeleaving the hash unchanged, exclusions applied to bundled output, symlink fidelity (including dangling and directory links),extraHashcomposing with a salt, output-directory cleanliness, staged file extensions, custom-hash and type-override validation,App.outdirprecedence, and bundling running once per distinct asset--security-opt, and mount consistency../paths, plus positive cases that assert the resultingdocker buildinvocation⏸️ Intentionally Deferred
To maintain simplicity and avoid new dependencies.
Not Implemented from AWS CDK/TerraConstructs:
esbuilddependencyexcludesupports exact paths,*.extsuffixes 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
AssetStagingwhencanonicalAssetHashesis enabled, because archive framing is now applied correctly. This affects hash values only, not artifact contents.assetHashcontaining/,..or other unsafe characters now throws where it was previously interpolated into the staged path.docker runargv changed: mount consistency is no longer forced onto volumes, and--security-optis omitted when unset.VOLUME_COPYbundling 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:#f3e5f5Component 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)Checklist