From c77ab4b2570965f06e148a29e560c57b5afb903b Mon Sep 17 00:00:00 2001 From: scttfrdmn <3011922+scttfrdmn@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:13:55 -0700 Subject: [PATCH] feat(volume): real modal.CloudBucketMount -> S3 mount support (calque#91 Workstream A) Replaces CloudBucketMount's leak-only treatment with a real mountpoint-s3 mount against the script's own S3 bucket: pyast.py extracts bucket_name/ key_prefix/read_only from an inline CloudBucketMount(...) volumes= value, parse.go decodes them into ir.Function/Class.CloudBucketMounts, internal/plan/cloudbucketmount.go resolves+renders the mount-s3 shell lines, bootstrap.go splices them in before @enter runs, and RealRunPolicy's new extraBuckets param grants the instance role access to the script's own bucket (separate from calque's --bucket staging area). secret=/bucket_endpoint_url=/requester_pays=/force_path_style= are each leaked distinctly as unhonored. Dict/Queue/NetworkFileSystem/ App.include remain leak-only; NetworkFileSystem is a separate, later workstream. --- cmd/calque/fleetrun.go | 21 ++++- cmd/calque/realrun.go | 40 ++++++++- docs/modal-compatibility-matrix.md | 13 ++- internal/exec/bootstrap.go | 18 ++++ internal/exec/bootstrap_demo_test.go | 58 ++++++++++++ internal/ir/ir.go | 58 ++++++++---- internal/parse/parse.go | 99 +++++++++++++++++--- internal/parse/parse_test.go | 63 +++++++++++-- internal/plan/cloudbucketmount.go | 119 +++++++++++++++++++++++++ internal/plan/cloudbucketmount_test.go | 113 +++++++++++++++++++++++ internal/plan/iam.go | 31 ++++++- internal/plan/iam_test.go | 51 ++++++++++- testdata/scripts/cloud_bucket_mount.py | 19 ++++ tools/pyast/pyast.py | 99 ++++++++++++++++++-- 14 files changed, 742 insertions(+), 60 deletions(-) create mode 100644 internal/plan/cloudbucketmount.go create mode 100644 internal/plan/cloudbucketmount_test.go create mode 100644 testdata/scripts/cloud_bucket_mount.py diff --git a/cmd/calque/fleetrun.go b/cmd/calque/fleetrun.go index dba9f99..405b862 100644 --- a/cmd/calque/fleetrun.go +++ b/cmd/calque/fleetrun.go @@ -247,6 +247,18 @@ func fleetRun(o realOpts, shards int) (err error) { // shard shares the SAME resolved mounts, since they all drive the // same picked unit's own body. Computed once, outside the loop. shardVolumeSync, shardVolumeCommit := volumeSpecsForApp(app, o.bucket, rep) + // calque#91 Workstream A: the same real modal.CloudBucketMount(...) + // wiring realrun.go added — see cloudBucketMountSpecsForApp's doc + // comment. Every shard shares the SAME resolved mounts (they all drive + // the same picked unit's own body), passed into runShard's own + // BootstrapConfig/IAM setup below (D4's dedicated-fallback-instance + // path). NOTE: unlike VolumeSync/VolumeCommit, this does NOT currently + // reach the D2 shared-worker-pool path (ProvisionFleetWorkers/ + // buildFleetWorkerBootstrapCommand, internal/pool/fleet_provision.go) — + // that path builds its own bootstrap script independent of + // calexec.BootstrapConfig and, same as this pre-existing Volume + // plumbing, has no CloudBucketMount mounting either; out of scope here. + shardCloudBucketMountLines, shardCloudBucketMountBuckets := cloudBucketMountSpecsForApp(app, rep) var wg sync.WaitGroup for i := range shs { if err := calexec.WriteManifestBody(ctx, s3c, calexec.RunLayout{ @@ -380,7 +392,7 @@ func fleetRun(o realOpts, shards int) (err error) { waitForQuotaHeadroom(ctx, cfg, inst, o.region, o.spot, safeRep) } fmt.Fprintf(os.Stderr, "[fleet] shard %d failed (%v); re-driving once on a fresh instance\n", shs[i].ID, shardErrs[i]) - m, serr := runShard(ctx, s3c, ec2c, spawnClient, o, shs[i], places, pricePerHr, tgt, shardBody, shardHostMode, safeRep, shardVolumeSync, shardVolumeCommit) + m, serr := runShard(ctx, s3c, ec2c, spawnClient, o, shs[i], places, pricePerHr, tgt, shardBody, shardHostMode, safeRep, shardVolumeSync, shardVolumeCommit, shardCloudBucketMountLines, shardCloudBucketMountBuckets) measurements[i], shardErrs[i] = m, serr if serr != nil { safeRep.Addf(leak.PrimAcquire, leak.KindSemanticGap, o.model, 0, @@ -581,7 +593,7 @@ const fleetWorkerIdleTimeout = 1 * time.Minute // pointer across shards would race on that mutation). func runShard(ctx context.Context, s3c *s3.Client, ec2c *ec2.Client, spawnClient *spawnaws.Client, o realOpts, sh calexec.Shard, places []plan.Placement, pricePerHr float64, baseTgt *target.Target, body calexec.ManifestBody, hostMode bool, rep *syncReport, - volumeSync, volumeCommit []calexec.VolumeSyncSpec) (measure.Measurement, error) { + volumeSync, volumeCommit []calexec.VolumeSyncSpec, cloudBucketMountLines, cloudBucketMountBuckets []string) (measure.Measurement, error) { shardLayout := calexec.RunLayout{ Bucket: o.bucket, ArtifactPfx: "fleet/" + o.runID + "/artifacts", ManifestKey: sh.ManifestKey, ResultPrefix: sh.ResultPrefix, SummaryKey: sh.SummaryKey, LogKey: sh.LogKey, @@ -593,11 +605,14 @@ func runShard(ctx context.Context, s3c *s3.Client, ec2c *ec2.Client, spawnClient BaseImage: "vllm/vllm-openai:latest", Bucket: o.bucket, ArtifactPrefix: shardLayout.ArtifactPfx, ManifestKey: shardLayout.ManifestKey, WorkerDir: hostWorkerDir, Region: o.region, LogKey: shardLayout.LogKey, HostMode: hostMode, ModelEnv: o.model, + CloudBucketMountLines: cloudBucketMountLines, } // calque#148: see realrun.go's identical fix — without this, the // dedicated fallback instance has no credentials for its own // bootstrap's aws s3 cp/sync calls, including its own failure log. - iamProfile, err := plan.RealRunInstanceProfile(ctx, spawnClient, o.region, o.bucket) + // calque#91 Workstream A: also grants access to the script's OWN + // CloudBucketMount bucket(s), if any (cloudBucketMountBuckets). + iamProfile, err := plan.RealRunInstanceProfile(ctx, spawnClient, o.region, o.bucket, cloudBucketMountBuckets...) if err != nil { return measure.Measurement{}, fmt.Errorf("shard %d set up IAM instance profile: %w", sh.ID, err) } diff --git a/cmd/calque/realrun.go b/cmd/calque/realrun.go index 165d542..7cc014a 100644 --- a/cmd/calque/realrun.go +++ b/cmd/calque/realrun.go @@ -344,6 +344,14 @@ func realRun(o realOpts) (err error) { // before calque#79) gets an empty slice both ways — byte-for-byte // unchanged behavior. volumeSync, volumeCommit := volumeSpecsForApp(app, o.bucket, rep) + // calque#91 Workstream A: a script's REAL modal.CloudBucketMount(...) + // mounts (its OWN S3 bucket, mounted live via mountpoint-s3 — NOT + // calque's --bucket staging area the way an ordinary Volume is) resolve + // into shell lines spliced into the bootstrap script, plus the distinct + // bucket names the instance's IAM role needs read/write/list access to. + // A script with no CloudBucketMounts (the vast majority) gets an empty + // slice both ways — byte-for-byte unchanged behavior. + cloudBucketMountLines, cloudBucketMountBuckets := cloudBucketMountSpecsForApp(app, rep) // calque#148, widened: bootstrap.go's host-mode branch ALWAYS // provisions a uv-managed venv now (not just when --pip supplies real // deps), so warmd must ALWAYS invoke that SAME venv's interpreter for @@ -384,6 +392,7 @@ func realRun(o realOpts) (err error) { PipPackages: o.pipPackages, PythonVersion: o.pythonVersion, StageFiles: o.stageFiles, RegistryRef: registryRef, BuildDockerfile: buildDockerfile, BuildTag: buildTag, + CloudBucketMountLines: cloudBucketMountLines, } // calque#134/#178: when --script named a real parsed unit, carry its @@ -423,8 +432,10 @@ func realRun(o realOpts) (err error) { // its own bootstrap script makes — not even for uploading its OWN // bootstrap log on failure, which is why a bootstrap failure on this // path was previously totally silent (no log, no error, just a - // timeout at the deadline). Scoped to just this run's own bucket. - iamProfile, err := plan.RealRunInstanceProfile(ctx, spawnClient, o.region, o.bucket) + // timeout at the deadline). Scoped to just this run's own bucket, plus + // (calque#91 Workstream A) any distinct bucket(s) the script's own + // resolved CloudBucketMount(s) reference. + iamProfile, err := plan.RealRunInstanceProfile(ctx, spawnClient, o.region, o.bucket, cloudBucketMountBuckets...) if err != nil { return fmt.Errorf("set up IAM instance profile: %w", err) } @@ -560,6 +571,31 @@ func volumeSpecsForApp(app ir.App, bucket string, rep *leak.Report) (sync, commi return sync, commit } +// cloudBucketMountSpecsForApp resolves app's REAL modal.CloudBucketMount(...) +// mounts (calque#91 Workstream A) into the already-rendered shell lines +// spliced into BootstrapConfig.CloudBucketMountLines, plus the distinct S3 +// bucket names (the SCRIPT'S OWN buckets, not calque's --bucket staging +// area) the instance's IAM role needs read/write/list access to — mirrors +// volumeSpecsForApp's factoring (a pure function, no ctx/S3, so it's +// unit-testable without a real script/S3 client). A script with no +// CloudBucketMounts (the vast majority) returns (nil, nil) — byte-for-byte +// the same as the hardcoded nil, nil this replaces. +func cloudBucketMountSpecsForApp(app ir.App, rep *leak.Report) (lines []string, buckets []string) { + mounts := plan.ResolveCloudBucketMounts(app, rep) + if len(mounts) == 0 { + return nil, nil + } + lines = plan.MountCommands(mounts) + seen := map[string]bool{} + for _, m := range mounts { + if !seen[m.BucketName] { + seen[m.BucketName] = true + buckets = append(buckets, m.BucketName) + } + } + return lines, buckets +} + func emitK(o realOpts, inst string, perItem []float64, enterSec float64, occ calexec.OccupancyRaw, acq plan.Acquired, priceHr float64) error { rates, err := cost.LoadRates(o.ratesFP) if err != nil { diff --git a/docs/modal-compatibility-matrix.md b/docs/modal-compatibility-matrix.md index f9df061..d16975a 100644 --- a/docs/modal-compatibility-matrix.md +++ b/docs/modal-compatibility-matrix.md @@ -6,6 +6,10 @@ ports to AWS **unchanged**. This document is the single most direct answer to "does calque support my script." **Provenance:** +- Updated the §E `modal.CloudBucketMount` row (2026-08-14, calque#91 + Workstream A): moved from ⬜ (not modeled — leak only) to ✅ (a real + mountpoint-s3 mount against the script's own S3 bucket). See the row + itself for the file-by-file implementation summary. - Verified against calque v0.5.0 (2026-08-14) — updated the App-level defaults row for calque#174 (image= per-function resolution fix). Also fixed the §H `modal.Cron`/`modal.Period` rows (calque#149 doc @@ -138,7 +142,7 @@ real gap — should not stay this way) · ⬜ not present at all. | `modal.Volume.from_name(...)` + `volumes={mount: vol}` | **Not a live shared filesystem** — snapshot-at-container-start, explicit `.commit()`/`.reload()` for cross-container visibility, last-write-wins on concurrent same-file writes (documented, expected data loss). | 🔥 | ✅ maps to a deterministic S3 prefix, real delta-sync before `@enter`, real end-of-run commit write-back. | calque's model (sync-before-run, commit-after-run) matches Modal's snapshot-at-start semantics reasonably well for the common case; **mid-run `.reload()`** (re-sync during execution) is correctly leaked as unreproduced. | — | | `.commit()` / `.reload()` call sites | End-of-run persistence / mid-run re-read. | 🔥 (wherever Volumes are used) | ✅ `.commit()` honored as real end-of-run write-back. 🟨 `.reload()` leaked as unreproduced. | — | — | | `modal.NetworkFileSystem` (deprecated, being removed) | **Live-shared** filesystem — no commit/reload cycle, closer to EFS/NFS than Volume's snapshot model. | 🧊 (deprecated, Modal steers users to Volume) | ⬜ | If a real script still uses this, calque's Volume→S3-prefix mapping is the WRONG model (S3 has no live-shared-write semantics) — this would need an EFS-shaped mapping instead, not a Volume-shaped one. | [#91](https://github.com/spore-host/calque/issues/91) | -| `modal.CloudBucketMount` | Direct S3/R2/GCS mount via `mountpoint-s3` — no append writes, no seek+write, must open in truncate mode, no rename. | 🧊 | ⬜ | A script using this directly against real S3 is a DIFFERENT (and more restrictive) primitive than Volume — calque's Volume mapping doesn't cover it. | [#91](https://github.com/spore-host/calque/issues/91) | +| `modal.CloudBucketMount` | Direct S3/R2/GCS mount via `mountpoint-s3` — no append writes, no seek+write, must open in truncate mode, no rename. | 🧊 | ✅ (calque#91 Workstream A) a real `CloudBucketMount(bucket_name, key_prefix=, read_only=)` used INLINE as a `volumes=` value (the real Modal idiom — constructed directly in the dict, not assigned to a variable first) resolves to a real mountpoint-s3 mount against the SCRIPT'S OWN S3 bucket: `tools/pyast/pyast.py`'s `_cloud_bucket_mount` extracts `bucket_name`/`key_prefix`/`read_only`, `internal/parse/parse.go` decodes them into `ir.Function`/`ir.Class.CloudBucketMounts`, and `internal/plan/cloudbucketmount.go`'s `MountCommands` renders the on-instance `mount-s3` invocation (spliced into the bootstrap script before `@enter` runs, via `internal/exec.BootstrapConfig.CloudBucketMountLines`); `internal/plan.RealRunPolicy`'s `extraBuckets` param grants the instance role read/write/list on that bucket, separate from calque's own `--bucket` staging area. `secret=` is recognized but NOT honored (the instance's own IAM role is used instead) — leaked distinctly. `bucket_endpoint_url=`/`requester_pays=`/`force_path_style=` are NOT supported — leaked distinctly; mounting is against AWS S3 with default settings only. A `bucket_name` that isn't a string literal still falls back to the pre-existing "recognized but not modeled" leak. | R2/GCS-backed CloudBucketMounts (`bucket_endpoint_url=`) are NOT reproduced — AWS S3 only. No live-Modal-managed credential rotation via `secret=`; the instance's own IAM role is the only credential path. | [#91](https://github.com/spore-host/calque/issues/91) (Workstream A closed; NetworkFileSystem is a separate, later workstream) | | `modal.Dict` | Distributed KV store, cloudpickle values, 7-day inactivity TTL, capped `.len()` at 100,000. | 🧊 | ⬜ not modeled, but [#151](https://github.com/spore-host/calque/issues/151) closed the failure mode: a bare reference to a module-level `Dict.from_name(...)` constant used to ship verbatim and crash at runtime with a confusing Modal SDK auth error — it's now refused with a clear leak naming the construct instead. | — | [#91](https://github.com/spore-host/calque/issues/91) | | `modal.Queue` | FIFO **per-partition only**, 24h partition auto-expiry. | 🧊 | ⬜ not modeled; same [#151](https://github.com/spore-host/calque/issues/151) honest-refusal fix applies to a bare reference to a `Queue.from_name(...)` constant. | — | [#91](https://github.com/spore-host/calque/issues/91) | @@ -312,11 +316,14 @@ a generic "unmodeled arg" message. live-verified end-to-end — [#98](https://github.com/spore-host/calque/issues/98) (closed). 12. Lower-priority/rare, still open: `modal.Dict`/`Queue`, - `@modal.batched`, `modal.NetworkFileSystem`, `modal.CloudBucketMount`, + `@modal.batched`, `modal.NetworkFileSystem`, `App.include`/`.deploy`/`.run` lifecycle nuances. (`cloud=` closed separately, calque#91's own §C fix; `modal.Cron`/`Period` object-form *recognition* also closed under calque#91 — see §H — though actually - executing on a schedule remains out of scope.) + executing on a schedule remains out of scope. `modal.CloudBucketMount` + is now REAL — see §E — closed as calque#91 Workstream A; + `modal.NetworkFileSystem` is a separate, larger workstream planned for + later, not attempted here.) [#91](https://github.com/spore-host/calque/issues/91) Not individually filed (genuinely low-priority/narrow; revisit if real usage diff --git a/internal/exec/bootstrap.go b/internal/exec/bootstrap.go index 2ef2a08..ef3cc2b 100644 --- a/internal/exec/bootstrap.go +++ b/internal/exec/bootstrap.go @@ -94,6 +94,16 @@ type BootstrapConfig struct { // content-addressing property internal/image already documents for a // future ECR push path). Ignored when BuildDockerfile is false. BuildTag string + // CloudBucketMountLines are already-rendered shell lines (calque#91 + // Workstream A) that mount every resolved modal.CloudBucketMount(...) + // via mountpoint-s3 — the caller (cmd/calque/realrun.go) builds these via + // plan.MountCommands(plan.ResolveCloudBucketMounts(app, rep)), so this + // package never needs to import internal/plan (no import-cycle risk). + // Spliced in right after the artifact sync, before either the HostMode + // or docker-mode run invocation — the mount must be live before @enter + // runs, in either mode. nil/empty (the default) is a no-op, reproducing + // prior behavior byte-for-byte for every script with no CloudBucketMount. + CloudBucketMountLines []string } // ecrHostname matches an ECR registry hostname, e.g. @@ -172,6 +182,14 @@ func (b BootstrapConfig) Command() string { } } + // calque#91 Workstream A: mount every resolved modal.CloudBucketMount(...) + // via mountpoint-s3 BEFORE either the HostMode or docker-mode run + // invocation below — the mount must be live before @enter runs, in + // either mode. Empty (the default) is a no-op. + if len(b.CloudBucketMountLines) > 0 { + lines = append(lines, b.CloudBucketMountLines...) + } + if b.HostMode { // Smoke test / real-AWS host-mode: run warmd directly on the host — // no docker, no GPU-container layer. Isolates acquisition + diff --git a/internal/exec/bootstrap_demo_test.go b/internal/exec/bootstrap_demo_test.go index 9bad075..aa4ee0f 100644 --- a/internal/exec/bootstrap_demo_test.go +++ b/internal/exec/bootstrap_demo_test.go @@ -268,3 +268,61 @@ func TestBootstrapCommandDockerModeWithNonECRRegistryRefNoLogin(t *testing.T) { t.Errorf("non-ECR RegistryRef should still be pulled (anonymously); got:\n%s", cmd) } } + +// TestBootstrapCommandSplicesCloudBucketMountLinesDockerMode (calque#91 +// Workstream A) proves CloudBucketMountLines are spliced into Command()'s +// output AFTER the artifact sync but BEFORE the docker run invocation — +// the mount must be live before @enter runs. +func TestBootstrapCommandSplicesCloudBucketMountLinesDockerMode(t *testing.T) { + c := BootstrapConfig{ + BaseImage: "vllm/vllm-openai:latest", Bucket: "b", ArtifactPrefix: "runs/x/art", + ManifestKey: "runs/x/manifest.json", Region: "us-west-2", + CloudBucketMountLines: []string{"mkdir -p /data", "mount-s3 my-bucket /data"}, + } + cmd := c.Command() + for _, w := range []string{"mkdir -p /data", "mount-s3 my-bucket /data"} { + if !strings.Contains(cmd, w) { + t.Errorf("missing %q in:\n%s", w, cmd) + } + } + syncIdx := strings.Index(cmd, "aws s3 cp --recursive") + mountIdx := strings.Index(cmd, "mount-s3 my-bucket /data") + runIdx := strings.Index(cmd, "docker run") + if syncIdx == -1 || mountIdx == -1 || runIdx == -1 { + t.Fatalf("missing expected markers in:\n%s", cmd) + } + if syncIdx >= mountIdx || mountIdx >= runIdx { + t.Errorf("expected order artifact-sync < cloud-bucket-mount < docker-run; got:\n%s", cmd) + } +} + +// TestBootstrapCommandSplicesCloudBucketMountLinesHostMode is the HostMode +// sibling: the mount must be live before warmd itself runs (host mode has +// no docker run invocation at all). +func TestBootstrapCommandSplicesCloudBucketMountLinesHostMode(t *testing.T) { + c := BootstrapConfig{ + Bucket: "b", ArtifactPrefix: "runs/x/art", ManifestKey: "runs/x/manifest.json", + Region: "us-west-2", HostMode: true, + CloudBucketMountLines: []string{"mkdir -p /data", "mount-s3 my-bucket /data"}, + } + cmd := c.Command() + mountIdx := strings.Index(cmd, "mount-s3 my-bucket /data") + warmdIdx := strings.Index(cmd, "warmd run --manifest") + if mountIdx == -1 || warmdIdx == -1 { + t.Fatalf("missing expected markers in:\n%s", cmd) + } + if mountIdx > warmdIdx { + t.Errorf("cloud-bucket-mount lines must run BEFORE warmd; got:\n%s", cmd) + } +} + +// TestBootstrapCommandNoCloudBucketMountLinesUnchanged proves the default +// (empty CloudBucketMountLines) reproduces prior behavior byte-for-byte: no +// mount-s3 anything appears at all. +func TestBootstrapCommandNoCloudBucketMountLinesUnchanged(t *testing.T) { + c := BootstrapConfig{BaseImage: "vllm/vllm-openai:latest", Bucket: "b", ArtifactPrefix: "runs/x/art", ManifestKey: "runs/x/manifest.json", Region: "us-west-2"} + cmd := c.Command() + if strings.Contains(cmd, "mount-s3") { + t.Errorf("no CloudBucketMountLines set — must not emit any mount-s3 reference; got:\n%s", cmd) + } +} diff --git a/internal/ir/ir.go b/internal/ir/ir.go index 5f1b970..503621b 100644 --- a/internal/ir/ir.go +++ b/internal/ir/ir.go @@ -187,6 +187,16 @@ type Config struct { Cloud string // cloud= ("aws"/"gcp"/"oci"/"auto"); recorded, not honored (calque#91) } +// CloudBucketMount is one modal.CloudBucketMount(...) used inline as a +// volumes= value (calque#91) — mounts the USER'S OWN S3 bucket directly +// via mountpoint-s3, not calque's own --bucket staging area the way an +// ordinary Volume does. +type CloudBucketMount struct { + BucketName string + KeyPrefix string + ReadOnly bool +} + // Function is an @app.function (or, when embedded in a Class, an @method). type Function struct { Name string @@ -197,17 +207,24 @@ type Function struct { // ONE globally-picked image (App.Image) regardless of which image= // var it actually referenced — a function with its OWN explicit // image= could silently get a DIFFERENT function's image. - Image Image - GPU string // raw from source, e.g. "H100" or "A100:8" — guarded/rewritten in §7 - Volumes map[string]string // mount path -> Modal volume name (from_name) - Timeout int // seconds; 0 if unset - Config Config // portable decorator config (cpu/memory/retries/secrets/schedule/region) - IsMap bool // is this callable's .map() invoked anywhere in the script? - Invoke InvokeKind // how the callable is invoked (map/starmap/for_each/remote); §C - EntryKind EntryKind // execution shape: batch (default) or serve (§F) - Body string // verbatim payload, shipped to the worker - Args []string // verbatim parameter names, incl. self/cls (calque#92: needed to reconstruct a .local()-referenced sibling's call signature) - ItemArg string // first non-self parameter name — the per-item arg the warm runner binds + Image Image + GPU string // raw from source, e.g. "H100" or "A100:8" — guarded/rewritten in §7 + Volumes map[string]string // mount path -> Modal volume name (from_name) + // CloudBucketMounts is every modal.CloudBucketMount(...) used INLINE as a + // volumes= value on this callable (calque#91 Workstream A) — mounts the + // USER'S OWN S3 bucket directly via mountpoint-s3, not calque's own + // --bucket staging area the way an ordinary Volume mount is. Keyed by + // mount path, disjoint from Volumes above (a given mount path is either + // an ordinary Volume or a CloudBucketMount, never both). + CloudBucketMounts map[string]CloudBucketMount + Timeout int // seconds; 0 if unset + Config Config // portable decorator config (cpu/memory/retries/secrets/schedule/region) + IsMap bool // is this callable's .map() invoked anywhere in the script? + Invoke InvokeKind // how the callable is invoked (map/starmap/for_each/remote); §C + EntryKind EntryKind // execution shape: batch (default) or serve (§F) + Body string // verbatim payload, shipped to the worker + Args []string // verbatim parameter names, incl. self/cls (calque#92: needed to reconstruct a .local()-referenced sibling's call signature) + ItemArg string // first non-self parameter name — the per-item arg the warm runner binds // LocalCalls are the leaf names of sibling callables THIS function's own // body references via .local() (calque#92) — a property of the body, not // of how this function itself is invoked (distinct from Invoke/IsMap). @@ -271,14 +288,17 @@ type Class struct { // Image is THIS class's own resolved image (calque#174) — see // Function.Image's doc comment; the same App->class->method // resolution chain gpu=/volumes= already used is extended to image=. - Image Image - GPU string - Volumes map[string]string - Timeout int - Config Config // portable decorator config (§B) - EnterBody string // @modal.enter body — runs ONCE in the warm runner (§6) - HasExit bool // @modal.exit() present (calque#86); teardown is not reproduced - Methods []Function // @modal.method bodies + Image Image + GPU string + Volumes map[string]string + // CloudBucketMounts mirrors Function.CloudBucketMounts (calque#91 + // Workstream A) — see its doc comment. + CloudBucketMounts map[string]CloudBucketMount + Timeout int + Config Config // portable decorator config (§B) + EnterBody string // @modal.enter body — runs ONCE in the warm runner (§6) + HasExit bool // @modal.exit() present (calque#86); teardown is not reproduced + Methods []Function // @modal.method bodies // EnterLocalCalls are sibling callables the @enter body itself references // via .local() (calque#92) — EnterBody is a bare string with no other Function // to carry this on. diff --git a/internal/parse/parse.go b/internal/parse/parse.go index 6c49e85..39a9267 100644 --- a/internal/parse/parse.go +++ b/internal/parse/parse.go @@ -819,13 +819,16 @@ func buildFn(f pyFunc, script string, rep *leak.Report, invokes map[string]ir.In // The function-config decorator is the one named "*.function" (or "*.method" // for class methods); enter/method markers carry no gpu/volumes. for _, d := range f.Decorators { - gpu, vols, timeout, cfg := readConfigKwargs(d.Kwargs, leak.PrimGPU, f.Name, script, d.Lineno, rep) + gpu, vols, cbm, timeout, cfg := readConfigKwargs(d.Kwargs, leak.PrimGPU, f.Name, script, d.Lineno, rep) if gpu != "" { fn.GPU = gpu } if vols != nil { fn.Volumes = vols } + if cbm != nil { + fn.CloudBucketMounts = cbm + } if timeout != 0 { fn.Timeout = timeout } @@ -836,8 +839,8 @@ func buildFn(f pyFunc, script string, rep *leak.Report, invokes map[string]ir.In func buildClass(c pyClass, script string, rep *leak.Report, invokes map[string]ir.InvokeKind, items map[string][]any, defaultVolumes map[string]string, defaultSecrets []string, out pyOut, appImage ir.Image) ir.Class { cls := ir.Class{Name: c.Name, Line: c.Lineno} - gpu, vols, timeout, cfg := readConfigKwargs(c.ClsKwargs, leak.PrimGPU, c.Name, script, c.Lineno, rep) - cls.GPU, cls.Volumes, cls.Timeout, cls.Config = gpu, vols, timeout, cfg + gpu, vols, cbm, timeout, cfg := readConfigKwargs(c.ClsKwargs, leak.PrimGPU, c.Name, script, c.Lineno, rep) + cls.GPU, cls.Volumes, cls.CloudBucketMounts, cls.Timeout, cls.Config = gpu, vols, cbm, timeout, cfg // calque#168: App-level volumes=/secrets= inherited if the CLASS itself // declares none — before a method's own class->method fallback below. applyAppDefaults(&cls.Volumes, &cls.Config.Secrets, defaultVolumes, defaultSecrets) @@ -890,6 +893,9 @@ func buildClass(c pyClass, script string, rep *leak.Report, invokes map[string]i if method.Volumes == nil { method.Volumes = cls.Volumes } + if method.CloudBucketMounts == nil { + method.CloudBucketMounts = cls.CloudBucketMounts + } if len(method.Config.Secrets) == 0 { method.Config.Secrets = cls.Config.Secrets } @@ -922,8 +928,10 @@ var autoscalingKwargs = map[string]bool{ // readConfigKwargs pulls gpu/volumes/timeout + the portable Config kwargs // (cpu/memory/retries/secrets/schedule/region, §B) out of a decorator's kwargs. // Autoscaling kwargs are recognized and leaked as deferred (§4/§1, M10/S1); -// anything else it can't model becomes a generic leak (§10). -func readConfigKwargs(kwargs map[string]json.RawMessage, _ leak.Primitive, owner, script string, line int, rep *leak.Report) (gpu string, vols map[string]string, timeout int, cfg ir.Config) { +// anything else it can't model becomes a generic leak (§10). cbm is calque#91 +// Workstream A's real CloudBucketMount->S3-mount resolution, alongside the +// pre-existing plain-Volume vols map — see decodeVolumesAndCloudBucketMounts. +func readConfigKwargs(kwargs map[string]json.RawMessage, _ leak.Primitive, owner, script string, line int, rep *leak.Report) (gpu string, vols map[string]string, cbm map[string]ir.CloudBucketMount, timeout int, cfg ir.Config) { for k, raw := range kwargs { switch k { case "gpu": @@ -950,12 +958,7 @@ func readConfigKwargs(kwargs map[string]json.RawMessage, _ leak.Primitive, owner timeout = n } case "volumes": - if m, ok := decodeStringMap(raw); ok { - vols = m - } else { - rep.Addf(leak.PrimVolume, leak.KindUnsupportedArg, script, line, - "%s: volumes= not a {str:str} map (%s)", owner, string(raw)) - } + vols, cbm = decodeVolumesAndCloudBucketMounts(raw, owner, script, line, rep) case "cpu": // cpu= is cores (int or float) in Modal; a [request, limit] list also // occurs (mirrors memory=) — take the request (first) element and leak @@ -1044,7 +1047,79 @@ func readConfigKwargs(kwargs map[string]json.RawMessage, _ leak.Primitive, owner "%s: unmodeled decorator arg %q=%s", owner, k, string(raw)) } } - return gpu, vols, timeout, cfg + return gpu, vols, cbm, timeout, cfg +} + +// decodeVolumesAndCloudBucketMounts decodes a volumes= kwarg's raw JSON into +// its two disjoint per-mount-path shapes (calque#91 Workstream A): the +// pre-existing plain-string {mount_path: volume_var_name} map (vols, an +// ordinary modal.Volume.from_name(...) mount) and the new +// {mount_path: {"__cloud_bucket_mount__": {...}}} shape pyast emits for a +// modal.CloudBucketMount(...) call used inline as a volumes= value (cbm, a +// real S3 mount). A raw value that's neither — including pyast's own +// "recognized but not modeled" __unparsed__ fallback for a CloudBucketMount +// whose bucket_name wasn't a string literal, or any OTHER unmodeled +// construct — is silently absent from both maps; that specific case already +// gets its own leak from pyast's helper_leaks (surfaced separately in +// build(), see the "pyast helper flagged" leak), so no second, redundant +// leak is emitted here. A volumes= value that isn't even a JSON object at +// all (e.g. a bare unparseable expression) leaks exactly like before this +// change. +func decodeVolumesAndCloudBucketMounts(raw json.RawMessage, owner, script string, line int, rep *leak.Report) (map[string]string, map[string]ir.CloudBucketMount) { + var m map[string]json.RawMessage + if err := json.Unmarshal(raw, &m); err != nil { + rep.Addf(leak.PrimVolume, leak.KindUnsupportedArg, script, line, + "%s: volumes= not a {str:str} map (%s)", owner, string(raw)) + return nil, nil + } + var vols map[string]string + var cbm map[string]ir.CloudBucketMount + for mountPath, rawVal := range m { + if s, ok := decodeString(rawVal); ok { + if vols == nil { + vols = map[string]string{} + } + vols[mountPath] = s + continue + } + if mount, ok := decodeCloudBucketMount(rawVal); ok { + if cbm == nil { + cbm = map[string]ir.CloudBucketMount{} + } + cbm[mountPath] = mount + } + // Neither shape: pyast's own helper_leaks already named this (see doc + // comment above) — no redundant leak here. + } + if vols == nil && cbm == nil { + rep.Addf(leak.PrimVolume, leak.KindUnsupportedArg, script, line, + "%s: volumes= not a {str:str} map (%s)", owner, string(raw)) + } + return vols, cbm +} + +// decodeCloudBucketMount decodes one volumes= dict value's +// {"__cloud_bucket_mount__": {"bucket_name": ..., "key_prefix": ..., +// "read_only": ...}} shape (calque#91 Workstream A; see pyast.py's +// _cloud_bucket_mount) into ir.CloudBucketMount. Returns (_, false) for any +// other shape, including a plain string (an ordinary Volume mount, handled +// by the caller instead) and pyast's {"__unparsed__": ...} fallback marker. +func decodeCloudBucketMount(raw json.RawMessage) (ir.CloudBucketMount, bool) { + var wrapper struct { + CBM *struct { + BucketName string `json:"bucket_name"` + KeyPrefix string `json:"key_prefix"` + ReadOnly bool `json:"read_only"` + } `json:"__cloud_bucket_mount__"` + } + if err := json.Unmarshal(raw, &wrapper); err != nil || wrapper.CBM == nil || wrapper.CBM.BucketName == "" { + return ir.CloudBucketMount{}, false + } + return ir.CloudBucketMount{ + BucketName: wrapper.CBM.BucketName, + KeyPrefix: wrapper.CBM.KeyPrefix, + ReadOnly: wrapper.CBM.ReadOnly, + }, true } // ---- small decode helpers (kwargs are heterogeneous JSON) ---- diff --git a/internal/parse/parse_test.go b/internal/parse/parse_test.go index ca7055b..2d52004 100644 --- a/internal/parse/parse_test.go +++ b/internal/parse/parse_test.go @@ -1123,13 +1123,19 @@ func TestParseScheduleObjectForms(t *testing.T) { } // TestParseRareConstructsAreTaggedNotSilent (calque#91): modal.Dict/Queue/ -// NetworkFileSystem.from_name(...), an inline modal.CloudBucketMount(...) used -// as a volumes= value, and App.include(...) must each fire a DISTINCT, named -// leak — before this fix, the first three vanished entirely (no visit_Assign -// branch matched them) and CloudBucketMount was silently miscategorized as an -// ordinary Volume mount (no leak at all). None of these are modeled; this only -// proves each is now a clean grep hit instead of silence or a false -// classification. +// NetworkFileSystem.from_name(...) and App.include(...) must each fire a +// DISTINCT, named leak — before this fix, all three vanished entirely (no +// visit_Assign branch matched them). None of these are modeled; this only +// proves each is now a clean grep hit instead of silence. +// +// modal.CloudBucketMount is NOT asserted here anymore (calque#91 Workstream +// A): it moved from "recognized but not modeled" to a REAL, resolved S3 +// mount — rare_constructs.py's own CloudBucketMount("my-bucket", secret=None) +// usage has a literal bucket_name and an explicit secret=None (a no-op, not a +// real secret= request), so it now resolves cleanly with ZERO leak at all, +// the same as an ordinary Volume mount. See +// TestParseCloudBucketMountResolves for the positive (modeled) case, using +// testdata/scripts/cloud_bucket_mount.py instead. func TestParseRareConstructsAreTaggedNotSilent(t *testing.T) { r, args := runner(t) rep := &leak.Report{} @@ -1143,7 +1149,6 @@ func TestParseRareConstructsAreTaggedNotSilent(t *testing.T) { "modal.Dict": true, "modal.Queue": true, "modal.NetworkFileSystem": true, - "modal.CloudBucketMount": true, "App.include": true, } for _, l := range rep.Leaks { @@ -1158,6 +1163,48 @@ func TestParseRareConstructsAreTaggedNotSilent(t *testing.T) { } } +// TestParseCloudBucketMountResolves (calque#91 Workstream A) proves the +// POSITIVE case: a real modal.CloudBucketMount(...) used inline as a +// volumes= value, with a literal bucket_name/key_prefix/read_only, resolves +// to ir.Function.CloudBucketMounts — a real S3 mount via mountpoint-s3 — not +// a leak. testdata/scripts/cloud_bucket_mount.py is the fixture. +func TestParseCloudBucketMountResolves(t *testing.T) { + r, args := runner(t) + rep := &leak.Report{} + script, _ := filepath.Abs("../../testdata/scripts/cloud_bucket_mount.py") + + app, err := Parse(context.Background(), script, rep, r, args...) + if err != nil { + t.Fatalf("Parse: %v", err) + } + + fn, ok := app.FindFunction("use_bucket_mount") + if !ok { + t.Fatalf("function %q not found in parsed app", "use_bucket_mount") + } + mount, ok := fn.CloudBucketMounts["/data"] + if !ok { + t.Fatalf("CloudBucketMounts[%q] missing; got %+v", "/data", fn.CloudBucketMounts) + } + if mount.BucketName != "my-real-bucket" { + t.Errorf("BucketName = %q, want %q", mount.BucketName, "my-real-bucket") + } + if mount.KeyPrefix != "foo/" { + t.Errorf("KeyPrefix = %q, want %q", mount.KeyPrefix, "foo/") + } + if !mount.ReadOnly { + t.Error("ReadOnly = false, want true") + } + if fn.Volumes != nil { + t.Errorf("Volumes = %+v, want nil (this mount is a CloudBucketMount, not an ordinary Volume)", fn.Volumes) + } + for _, l := range rep.Leaks { + if strings.Contains(l.Detail, "CloudBucketMount") { + t.Errorf("unexpected CloudBucketMount leak for a fully-resolved literal mount: %+v", l) + } + } +} + // TestParseModalBatchedDecoratorLeaks (calque#91): @modal.batched(...) had // ZERO recognition at all before this fix — unlike its four from_name/ // CloudBucketMount siblings tested above (TestParseRareConstructsAreTaggedNotSilent), diff --git a/internal/plan/cloudbucketmount.go b/internal/plan/cloudbucketmount.go new file mode 100644 index 0000000..f2a35a8 --- /dev/null +++ b/internal/plan/cloudbucketmount.go @@ -0,0 +1,119 @@ +package plan + +import ( + "fmt" + "sort" + + "github.com/spore-host/calque/internal/ir" + "github.com/spore-host/calque/internal/leak" +) + +// CloudBucketMount plumbing (calque#91 Workstream A). +// +// Modal's CloudBucketMount(bucket_name, ...), used inline as a volumes= value, +// mounts the USER'S OWN S3 bucket directly into the container via +// mountpoint-s3 — a fundamentally different shape from an ordinary +// Volume.from_name(...) mount (see volume.go): a Volume is calque's own +// staging area, synced through the run's --bucket; a CloudBucketMount is the +// script's OWN bucket, mounted live, with no calque-owned S3 prefix and no +// download/commit sync step. Real writes through mountpoint-s3 are already +// live against S3 — there is nothing to "commit" back the way a Volume needs. + +// CloudBucketMountResolved is one resolved CloudBucketMount: the in-container +// mount path plus the real S3 bucket/prefix/read-only flag from the script's +// own CloudBucketMount(...) call. +type CloudBucketMountResolved struct { + MountPath string + BucketName string + KeyPrefix string + ReadOnly bool +} + +// ResolveCloudBucketMounts collects every modal.CloudBucketMount(...) mounted +// by the app's classes/functions, deduped by mount path — mirrors +// ResolveVolumes' exact collect/conflict/sort shape (see volume.go), applied +// to the sibling CloudBucketMounts map instead of Volumes. A mount path +// claimed by two DIFFERENT CloudBucketMounts (different bucket, prefix, or +// read-only flag) is a conflict we leak rather than guess through, same as +// ResolveVolumes. +func ResolveCloudBucketMounts(app ir.App, rep *leak.Report) []CloudBucketMountResolved { + seen := map[string]ir.CloudBucketMount{} // mountPath -> resolved mount + collect := func(owner string, mounts map[string]ir.CloudBucketMount, line int) { + for mountPath, m := range mounts { + if m.BucketName == "" { + continue + } + if prev, ok := seen[mountPath]; ok && prev != m { + rep.Addf(leak.PrimVolume, leak.KindUnhandledCase, app.Script, line, + "%s: mount path %q maps to two different CloudBucketMounts (%+v vs %+v); mount overlap not modeled", + owner, mountPath, prev, m) + continue + } + seen[mountPath] = m + } + } + for _, c := range app.Classes { + collect(c.Name, c.CloudBucketMounts, c.Line) + } + for _, f := range app.Functions { + collect(f.Name, f.CloudBucketMounts, f.Line) + } + + // Deterministic order (stable plans / stable tests). + paths := make([]string, 0, len(seen)) + for p := range seen { + paths = append(paths, p) + } + sort.Strings(paths) + + out := make([]CloudBucketMountResolved, 0, len(paths)) + for _, p := range paths { + m := seen[p] + out = append(out, CloudBucketMountResolved{ + MountPath: p, + BucketName: m.BucketName, + KeyPrefix: m.KeyPrefix, + ReadOnly: m.ReadOnly, + }) + } + return out +} + +// MountCommands returns the shell lines to mount each resolved +// CloudBucketMount via mountpoint-s3 (the AWS Labs FUSE driver Modal's own +// CloudBucketMount is itself built on) — idempotent install check matching +// this repo's existing `command -v X >/dev/null || (sudo apt-get update && +// sudo apt-get install -y X)` idiom (internal/exec/bootstrap.go), then +// `mkdir -p` the mount path, then `mount-s3 ` with +// `--read-only`/`--prefix ` when set. mountpoint-s3 ships as a +// .deb for Debian/Ubuntu-family AMIs (the DL AMI family this repo already +// targets); there is no apt package name for it (it's not in Debian/Ubuntu's +// own repos), so the fallback branch downloads AWS's published .deb directly +// instead of `apt-get install`. +func MountCommands(mounts []CloudBucketMountResolved) []string { + var lines []string + for _, m := range mounts { + lines = append(lines, + // mountpoint-s3 has no apt/dnf package; install AWS's own published + // .deb the first time this runs (idempotent: skipped once mount-s3 + // is already on PATH). + `command -v mount-s3 >/dev/null || (curl -LsSf -o /tmp/mount-s3.deb https://s3.amazonaws.com/mountpoint-s3-release/latest/x86_64/mount-s3.deb && sudo apt-get install -y /tmp/mount-s3.deb)`, + fmt.Sprintf("mkdir -p %s", m.MountPath), + mountCommand(m), + ) + } + return lines +} + +// mountCommand renders one `mount-s3 [--prefix +// ] [--read-only]` invocation — mountpoint-s3's real CLI flags. +func mountCommand(m CloudBucketMountResolved) string { + cmd := fmt.Sprintf("mount-s3 %s %s", m.BucketName, m.MountPath) + if m.KeyPrefix != "" { + cmd += fmt.Sprintf(" --prefix %s", m.KeyPrefix) + } + if m.ReadOnly { + cmd += " --read-only" + } + return cmd +} diff --git a/internal/plan/cloudbucketmount_test.go b/internal/plan/cloudbucketmount_test.go new file mode 100644 index 0000000..036e303 --- /dev/null +++ b/internal/plan/cloudbucketmount_test.go @@ -0,0 +1,113 @@ +package plan + +import ( + "strings" + "testing" + + "github.com/spore-host/calque/internal/ir" + "github.com/spore-host/calque/internal/leak" +) + +func TestResolveCloudBucketMountsDeduped(t *testing.T) { + rep := &leak.Report{} + app := ir.App{ + Script: "s.py", + Classes: []ir.Class{ + {Name: "Scorer", CloudBucketMounts: map[string]ir.CloudBucketMount{ + "/data": {BucketName: "my-bucket", KeyPrefix: "foo/", ReadOnly: true}, + }}, + }, + Functions: []ir.Function{ + {Name: "download", CloudBucketMounts: map[string]ir.CloudBucketMount{ + "/data": {BucketName: "my-bucket", KeyPrefix: "foo/", ReadOnly: true}, + }}, + }, + } + mounts := ResolveCloudBucketMounts(app, rep) + if len(mounts) != 1 { + t.Fatalf("mounts = %d, want 1 (same mount at same path, deduped)", len(mounts)) + } + m := mounts[0] + if m.MountPath != "/data" || m.BucketName != "my-bucket" || m.KeyPrefix != "foo/" || !m.ReadOnly { + t.Errorf("mount = %+v", m) + } + if rep.Len() != 0 { + t.Errorf("identical mounts should not leak: %+v", rep.Leaks) + } +} + +// TestCloudBucketMountConflictLeaks: two DIFFERENT CloudBucketMounts (here, +// different bucket names) at one mount path is a conflict surfaced, not +// guessed through — mirrors TestVolumeConflictLeaks. +func TestCloudBucketMountConflictLeaks(t *testing.T) { + rep := &leak.Report{} + app := ir.App{ + Script: "s.py", + Classes: []ir.Class{ + {Name: "A", CloudBucketMounts: map[string]ir.CloudBucketMount{"/mnt": {BucketName: "bucket-one"}}, Line: 5}, + {Name: "B", CloudBucketMounts: map[string]ir.CloudBucketMount{"/mnt": {BucketName: "bucket-two"}}, Line: 9}, + }, + } + ResolveCloudBucketMounts(app, rep) + if rep.Len() == 0 { + t.Error("two different CloudBucketMounts at one mount path should leak a conflict") + } +} + +func TestNoCloudBucketMountsNoMounts(t *testing.T) { + rep := &leak.Report{} + app := ir.App{Script: "s.py", Classes: []ir.Class{{Name: "C"}}} + if got := ResolveCloudBucketMounts(app, rep); len(got) != 0 { + t.Errorf("no CloudBucketMounts -> no mounts, got %+v", got) + } +} + +// TestMountCommandsShape proves MountCommands emits an idempotent +// mount-s3 install check, a mkdir, and a mount-s3 invocation carrying the +// real bucket name/mount path — mirrors TestSyncUsesSyncNotCp's shape- +// assertion style for the Volume sibling. +func TestMountCommandsShape(t *testing.T) { + mounts := []CloudBucketMountResolved{{MountPath: "/data", BucketName: "my-bucket"}} + lines := MountCommands(mounts) + joined := strings.Join(lines, "\n") + if !strings.Contains(joined, "command -v mount-s3") { + t.Errorf("must idempotently check for mount-s3, got:\n%s", joined) + } + if !strings.Contains(joined, "mkdir -p /data") { + t.Errorf("must create the mount path, got:\n%s", joined) + } + if !strings.Contains(joined, "mount-s3 my-bucket /data") { + t.Errorf("must mount the real bucket at the mount path, got:\n%s", joined) + } + if strings.Contains(joined, "--read-only") || strings.Contains(joined, "--prefix") { + t.Errorf("no read_only/key_prefix set — must not emit those flags, got:\n%s", joined) + } +} + +// TestMountCommandsReadOnlyAndPrefix proves the --read-only/--prefix flags +// are rendered when the resolved mount asked for them. +func TestMountCommandsReadOnlyAndPrefix(t *testing.T) { + mounts := []CloudBucketMountResolved{{MountPath: "/data", BucketName: "my-bucket", KeyPrefix: "foo/", ReadOnly: true}} + lines := MountCommands(mounts) + joined := strings.Join(lines, "\n") + if !strings.Contains(joined, "mount-s3 my-bucket /data --prefix foo/ --read-only") { + t.Errorf("expected --prefix and --read-only on the mount-s3 line, got:\n%s", joined) + } +} + +// TestMountCommandsOrder proves mkdir happens before the mount-s3 call, and +// the install check happens before both. +func TestMountCommandsOrder(t *testing.T) { + mounts := []CloudBucketMountResolved{{MountPath: "/data", BucketName: "my-bucket"}} + lines := MountCommands(mounts) + joined := strings.Join(lines, "\n") + installIdx := strings.Index(joined, "command -v mount-s3") + mkdirIdx := strings.Index(joined, "mkdir -p /data") + mountIdx := strings.Index(joined, "mount-s3 my-bucket /data") + if installIdx == -1 || mkdirIdx == -1 || mountIdx == -1 { + t.Fatalf("missing expected lines in:\n%s", joined) + } + if installIdx >= mkdirIdx || mkdirIdx >= mountIdx { + t.Errorf("expected order install < mkdir < mount, got indices %d, %d, %d in:\n%s", installIdx, mkdirIdx, mountIdx, joined) + } +} diff --git a/internal/plan/iam.go b/internal/plan/iam.go index cdb05ac..2d3f59d 100644 --- a/internal/plan/iam.go +++ b/internal/plan/iam.go @@ -31,7 +31,15 @@ import ( // actual image-layer read actions are scoped to account+region, not to // one specific repo, since the resolved registry ref varies per script // and isn't known when the role is created. -func RealRunPolicy(account, region, bucket string) string { +// +// calque#91 Workstream A: extraBuckets grants read/write/list on every +// DISTINCT bucket a --script real run's resolved modal.CloudBucketMount(...) +// mounts reference — the SCRIPT'S OWN bucket, not calque's own --bucket +// staging area (bucket above). Each distinct name in extraBuckets gets the +// same S3 statement SHAPE as bucket's own grant above (GetObject/PutObject + +// ListBucket/GetBucketLocation), scoped to that bucket only. nil/empty (the +// default, every pre-#91 caller) reproduces prior behavior byte-for-byte. +func RealRunPolicy(account, region, bucket string, extraBuckets []string) string { obj := fmt.Sprintf("arn:aws:s3:::%s/*", bucket) bkt := fmt.Sprintf("arn:aws:s3:::%s", bucket) ecrRepos := fmt.Sprintf("arn:aws:ecr:%s:%s:repository/*", region, account) @@ -41,6 +49,19 @@ func RealRunPolicy(account, region, bucket string) string { `{"Effect":"Allow","Action":["ecr:GetAuthorizationToken"],"Resource":["*"]}`, fmt.Sprintf(`{"Effect":"Allow","Action":["ecr:BatchGetImage","ecr:GetDownloadUrlForLayer"],"Resource":[%q]}`, ecrRepos), } + seen := map[string]bool{bucket: true} + for _, b := range extraBuckets { + if b == "" || seen[b] { + continue + } + seen[b] = true + extraObj := fmt.Sprintf("arn:aws:s3:::%s/*", b) + extraBkt := fmt.Sprintf("arn:aws:s3:::%s", b) + stmts = append(stmts, + fmt.Sprintf(`{"Effect":"Allow","Action":["s3:GetObject","s3:PutObject"],"Resource":[%q]}`, extraObj), + fmt.Sprintf(`{"Effect":"Allow","Action":["s3:ListBucket","s3:GetBucketLocation"],"Resource":[%q]}`, extraBkt), + ) + } return `{"Version":"2012-10-17","Statement":[` + strings.Join(stmts, ",") + `]}` } @@ -72,7 +93,11 @@ func RealRunPolicy(account, region, bucket string) string { // IAM role per DISTINCT bucket ever passed to --bucket (bounded by how // many buckets a caller actually uses, not by run count: two runs against // the SAME bucket still correctly share one role, as before). -func RealRunInstanceProfile(ctx context.Context, client *spawnaws.Client, region, bucket string) (string, error) { +// +// extraBuckets (calque#91 Workstream A) threads through to RealRunPolicy — +// see its doc comment. nil/empty (the default, every pre-#91 caller) +// reproduces prior behavior byte-for-byte. +func RealRunInstanceProfile(ctx context.Context, client *spawnaws.Client, region, bucket string, extraBuckets ...string) (string, error) { account, err := client.GetAccountID(ctx) if err != nil { return "", fmt.Errorf("resolve account id: %w", err) @@ -80,7 +105,7 @@ func RealRunInstanceProfile(ctx context.Context, client *spawnaws.Client, region profile, err := client.CreateOrGetInstanceProfile(ctx, spawnaws.IAMRoleConfig{ RoleName: roleNameForBucket(bucket), TrustServices: []string{"ec2"}, - InlinePolicyJSON: RealRunPolicy(account, region, bucket), + InlinePolicyJSON: RealRunPolicy(account, region, bucket, extraBuckets), }) if err != nil { return "", fmt.Errorf("set up real-run IAM instance profile: %w", err) diff --git a/internal/plan/iam_test.go b/internal/plan/iam_test.go index c888a7d..35ac45e 100644 --- a/internal/plan/iam_test.go +++ b/internal/plan/iam_test.go @@ -11,7 +11,7 @@ import ( // instance real-run policy: it must grant read/write on the run's own // bucket only, and must produce valid JSON. func TestRealRunPolicy_ScopesToTheGivenBucketOnly(t *testing.T) { - policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket") + policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", nil) var doc map[string]any if err := json.Unmarshal([]byte(policy), &doc); err != nil { @@ -34,7 +34,7 @@ func TestRealRunPolicy_ScopesToTheGivenBucketOnly(t *testing.T) { // bucket — no queue ARN, since a single-instance real run has no SQS // queue at all. func TestRealRunPolicy_DoesNotCollideWithPoolOrFleetPolicy(t *testing.T) { - policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket") + policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", nil) if strings.Contains(policy, "sqs:") { t.Errorf("RealRunPolicy must not grant any SQS action (no queue involved); got %s", policy) } @@ -48,7 +48,7 @@ func TestRealRunPolicy_DoesNotCollideWithPoolOrFleetPolicy(t *testing.T) { // that specific action, it isn't resource-scopable); the layer-read // actions are scoped to this account+region's own repos. func TestRealRunPolicy_GrantsECRPull(t *testing.T) { - policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket") + policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", nil) var doc struct { Statement []struct { @@ -88,6 +88,51 @@ func TestRealRunPolicy_GrantsECRPull(t *testing.T) { } } +// TestRealRunPolicy_GrantsExtraBuckets (calque#91 Workstream A) proves each +// DISTINCT bucket in extraBuckets gets its own read/write/list statements, +// scoped to THAT bucket, separate from the run's own --bucket grant — for a +// --script real run whose resolved modal.CloudBucketMount(...) mounts +// reference the script's OWN bucket(s), not calque's staging area. +func TestRealRunPolicy_GrantsExtraBuckets(t *testing.T) { + policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", []string{"my-real-bucket", "other-real-bucket"}) + + var doc map[string]any + if err := json.Unmarshal([]byte(policy), &doc); err != nil { + t.Fatalf("RealRunPolicy did not produce valid JSON: %v\n%s", err, policy) + } + for _, want := range []string{"calque-runs-bucket", "my-real-bucket", "other-real-bucket"} { + if !strings.Contains(policy, want) { + t.Errorf("policy missing bucket %q; got %s", want, policy) + } + } + if !strings.Contains(policy, `arn:aws:s3:::my-real-bucket/*`) || !strings.Contains(policy, `arn:aws:s3:::my-real-bucket"`) { + t.Errorf("policy missing object+bucket ARNs for my-real-bucket; got %s", policy) + } +} + +// TestRealRunPolicy_ExtraBucketsDedupedAndSkipsRunBucket proves a duplicate +// name in extraBuckets (or one matching the run's own bucket) doesn't emit +// a second, redundant statement pair. +func TestRealRunPolicy_ExtraBucketsDedupedAndSkipsRunBucket(t *testing.T) { + policy := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", []string{"my-real-bucket", "my-real-bucket", "calque-runs-bucket"}) + if got := strings.Count(policy, "my-real-bucket"); got != 2 { // object ARN + bucket ARN, once each + t.Errorf("my-real-bucket should appear exactly twice (object+bucket ARN), got %d times in %s", got, policy) + } + if got := strings.Count(policy, "calque-runs-bucket"); got != 2 { + t.Errorf("calque-runs-bucket should still appear exactly twice (its own grant, not duplicated by extraBuckets), got %d times in %s", got, policy) + } +} + +// TestRealRunPolicy_NilExtraBucketsUnchanged proves the default (nil +// extraBuckets) reproduces prior behavior byte-for-byte. +func TestRealRunPolicy_NilExtraBucketsUnchanged(t *testing.T) { + withNil := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", nil) + withEmpty := RealRunPolicy("111122223333", "us-east-1", "calque-runs-bucket", []string{}) + if withNil != withEmpty { + t.Errorf("nil and empty extraBuckets should produce identical policies:\nnil: %s\nempty: %s", withNil, withEmpty) + } +} + // TestRoleNameForBucket_DifferentBucketsGetDifferentRoles (calque#167) proves // the actual fix: two real runs against different buckets no longer share // one mutable role whose inline policy PutRolePolicy replaces wholesale on diff --git a/testdata/scripts/cloud_bucket_mount.py b/testdata/scripts/cloud_bucket_mount.py new file mode 100644 index 0000000..b96a227 --- /dev/null +++ b/testdata/scripts/cloud_bucket_mount.py @@ -0,0 +1,19 @@ +"""cloud_bucket_mount.py — calque#91 Workstream A fixture: a real +modal.CloudBucketMount(...) used inline as a volumes= value, the actual +Modal idiom (constructed directly in the volumes= dict, never assigned to a +variable first). Unlike testdata/scripts/rare_constructs.py's own +CloudBucketMount usage (which stays leak-only in THAT fixture, proving the +"recognized but not modeled" tag still exists for a genuinely unresolvable +case), this fixture's bucket_name/key_prefix/read_only are all plain string/ +bool literals, so it must resolve to a REAL S3 mount via mountpoint-s3 — +ir.Function.CloudBucketMounts — not a leak. +""" + +import modal + +app = modal.App("cloud-bucket-mount-fixture") + + +@app.function(volumes={"/data": modal.CloudBucketMount("my-real-bucket", key_prefix="foo/", read_only=True)}) +def use_bucket_mount(x): + return x diff --git a/tools/pyast/pyast.py b/tools/pyast/pyast.py index 8af71a5..e04b6c3 100755 --- a/tools/pyast/pyast.py +++ b/tools/pyast/pyast.py @@ -74,7 +74,75 @@ def _decorator_name(node: ast.AST) -> str: return ".".join(_attr_chain(target)) -def _volumes_map(node: ast.AST, leaks: list[dict[str, Any]] | None = None) -> dict[str, str] | None: +def _cloud_bucket_mount(node: ast.Call, leaks: list[dict[str, Any]] | None) -> dict[str, Any] | None: + """Extract a `CloudBucketMount(bucket_name, ...)` call's kwargs into the + `{"__cloud_bucket_mount__": {...}}` wire shape (calque#91 Workstream A) — + a REAL S3 mount, not an ordinary Volume. `bucket_name` is the constructor's + first positional arg or its own `bucket_name=` kwarg; `key_prefix=` and + `read_only=` are best-effort extracted alongside it. + + Returns None (extraction genuinely failed) only when `bucket_name` isn't a + plain string literal — the caller falls back to the pre-existing + "recognized but not modeled" leak in that case, same posture as before this + construct was modeled at all. + + `secret=` (a real Modal kwarg for non-AWS-role credential injection) and + `bucket_endpoint_url=`/`requester_pays=`/`force_path_style=` are each + separately, informationally leaked when present — calque mounts against + AWS S3 with the instance's own IAM role and default settings only, so a + script relying on any of these needs a distinct signal that its specific + request wasn't honored, even though the mount itself DID resolve. + """ + bucket_name: str | None = None + if node.args: + bucket_name = _const_str(node.args[0]) + key_prefix: str | None = None + read_only = False + saw_secret = False + saw_other_unhonored = False + for kw in node.keywords: + if kw.arg == "bucket_name": + bucket_name = _const_str(kw.value) or bucket_name + elif kw.arg == "key_prefix": + key_prefix = _const_str(kw.value) + elif kw.arg == "read_only": + try: + v = ast.literal_eval(kw.value) + except (ValueError, SyntaxError): + v = None + if isinstance(v, bool): + read_only = v + elif kw.arg == "secret" and not (isinstance(kw.value, ast.Constant) and kw.value.value is None): + saw_secret = True + elif kw.arg in ("bucket_endpoint_url", "requester_pays", "force_path_style"): + if not (isinstance(kw.value, ast.Constant) and kw.value.value in (None, False)): + saw_other_unhonored = True + + if bucket_name is None: + return None + + if leaks is not None: + if saw_secret: + leaks.append( + { + "where": "modal.CloudBucketMount(secret=)", + "detail": "CloudBucketMount(secret=...) credential is not honored — the instance's own IAM role is used instead (calque#91)", + "lineno": getattr(node, "lineno", 0), + } + ) + if saw_other_unhonored: + leaks.append( + { + "where": "modal.CloudBucketMount(bucket_endpoint_url=/requester_pays=/force_path_style=)", + "detail": "CloudBucketMount's bucket_endpoint_url=/requester_pays=/force_path_style= are not supported — mounting against AWS S3 with default settings only (calque#91)", + "lineno": getattr(node, "lineno", 0), + } + ) + + return {"__cloud_bucket_mount__": {"bucket_name": bucket_name, "key_prefix": key_prefix, "read_only": read_only}} + + +def _volumes_map(node: ast.AST, leaks: list[dict[str, Any]] | None = None) -> dict[str, Any] | None: """Extract `volumes={"/mount": vol_handle}` as {mount_path: volume_var_name}. The keys are string literals (mount paths); the values are Volume *variables* @@ -82,21 +150,38 @@ def _volumes_map(node: ast.AST, leaks: list[dict[str, Any]] | None = None) -> di to match IR §14 `Volumes map[string]string // mount path -> volume name`. Returns None if this isn't a dict we can map. - calque#91: a value that's a direct `CloudBucketMount(...)`/`NetworkFileSystem(...)` - call (not a Volume.from_name()-derived variable) is a DIFFERENT, unmodeled - construct — before this check existed it fell into the generic `ast.unparse(v)` - branch below and was silently treated as an ordinary Volume mount, with no - leak distinguishing it at all. When leaks is supplied, tag it there instead. + calque#91 Workstream A: a value that's a direct `CloudBucketMount(...)` call + (not a Volume.from_name()-derived variable) is a DIFFERENT, now-MODELED + construct — a real S3 mount, not calque's own --bucket staging area the way + an ordinary Volume mount is. When it extracts cleanly, the mount path's value + is `{"__cloud_bucket_mount__": {...}}` (see _cloud_bucket_mount) instead of a + plain string, distinguishable from the ordinary `{mount_path: var_name}` + shape. When extraction genuinely fails (bucket_name isn't a string literal), + or the value is some OTHER unmodeled call (e.g. NetworkFileSystem(...) used + the same way), it falls back to the pre-existing "recognized but not + modeled" leak — before this construct was distinguished at all, it fell + into the generic `ast.unparse(v)` branch below and was silently treated as + an ordinary Volume mount with no leak whatsoever. """ if not isinstance(node, ast.Dict): return None - out: dict[str, str] = {} + out: dict[str, Any] = {} for k, v in zip(node.keys, node.values): key = _const_str(k) if k is not None else None if key is None: continue if isinstance(v, ast.Name): out[key] = v.id + elif isinstance(v, ast.Call) and _attr_chain(v.func)[-1:] == ["CloudBucketMount"]: + cbm = _cloud_bucket_mount(v, leaks) + if cbm is not None: + out[key] = cbm + continue + if leaks is not None: + leaks.append( + {"where": "modal.CloudBucketMount", "detail": "CloudBucketMount(...) used as a volumes= value, recognized but not modeled — bucket_name is not a plain string literal (calque#91)", "lineno": getattr(v, "lineno", 0)} + ) + out[key] = ast.unparse(v) else: if leaks is not None and isinstance(v, ast.Call): construct = _unsupported_construct_call(_attr_chain(v.func))