diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md new file mode 100644 index 00000000..e4e67b71 --- /dev/null +++ b/.cursor/BUGBOT.md @@ -0,0 +1,127 @@ +# Bugbot guide — tracebloc/cli + +## Context + +Public Go CLI (Apache-2.0), shipped as **signed 8-platform releases** (cosign keyless, +verified by `scripts/install.sh`). Customers run it on their own machines against their own +Kubernetes to operate a self-hosted secure environment. It talks to a public HTTPS backend +(`internal/api`) and to an in-cluster jobs-manager (`internal/submit`), and shells out to +`kubectl`/`helm`/`docker`. + +Two things make this repo unusual and should shape every finding: + +1. **Its exit codes are a scripting contract** — customers branch on them + (`internal/cli/exitcodes.go`: "the numeric values are FROZEN"). +2. **`make ci` mirrors CI exactly.** The `Makefile` header states it outright: "divergence + between local and CI is the bug this file exists to prevent." Tool versions are pinned in + lockstep with `.github/workflows/build.yml`. + +## Always flag + +- **A command that reports success it hasn't earned.** Exiting 0 is not the same as + succeeding. The reference pattern is `classifyPushOutcome` + (`internal/cli/data_ingest_output.go:25`): a Job that exits 0 but whose summary reports row + failures returns `"completed_with_failures"` + `exitIngestFailed`, *not* `"succeeded"` — its + comment cites this class explicitly. `internal/doctor` carries the same idea in + `StatusUnknown`: a check that ran but cannot back a green prints a neutral line rather than a + false ✔. Flag any new multi-step command where a partial failure collapses into success, and + any path where the `--output-json` status and the process exit code can disagree. + +- **An exit code that isn't a named constant from `internal/cli/exitcodes.go`**, a repurposed + numeric value, or a new failure path returning generic `1` when a specific code already + exists. Every non-test `&exitError{}` names its code. + +- **A prompt whose cancellation produces no visible feedback.** `errInteractiveCancelled` + (`internal/cli/interactive.go:26`) must print a `Cancelled — …` line via the Printer and then + return cleanly — see `resources_set.go:219`, `data_delete.go:233`, + `data_ingest_local.go:103`, `data_ingest_cluster.go:339`. Watch for + **`mapClientErr` (`internal/cli/client.go:858`), which maps it straight to `nil` with no + output at all** — a Ctrl-C routed through it exits 0 in total silence, right next to a + declined-answer branch that *does* print (`client.go:334`, `delete.go:196`). Check this at + every new or changed prompt site. Signals are wired centrally via `signal.NotifyContext` + (`cmd/tracebloc/main.go:58`) so deferred cleanup runs — a bare handler skips it and breaks + `push.Stage`'s cleanup contract. Interrupted-but-clean paths exit 130. + +- **HTTP 426 treated as anything other than a hard stop.** It is detected centrally + (`internal/api/client.go`, `parseUpgradeRequired` → `*UpgradeRequiredError`) so every caller + degrades to the same actionable "run `tracebloc upgrade`" message — see `auth.go:336`, + `doctor.go:119`, `client_status.go:129`, `delete.go:151,218`, `client.go:253`. Flag a new API + consumer that retries through it, frames it as a transient outage, or folds it into a generic + error. A too-old CLI never recovers by waiting, so `--wait` loops must fail fast on it. + +- **Verification that degrades to a warning.** In `scripts/install.sh` the SHA256 compare + aborts when no hashing tool is present, and `verify_cosign_signature()` bootstraps a pinned, + checksum-verified cosign (`COSIGN_VERSION=v2.4.1`) rather than skipping; the only bypass is an + explicit `TRACEBLOC_ALLOW_UNVERIFIED=1` with a loud warning. A previous "warn + continue + + still print ✓ matches" branch was caught as *both* a security regression and a dishonest log. + Also flag any `--version` / `RELEASE_VERSION` use that skips `validate_version_tag` before URL + interpolation. `tracebloc upgrade` and host prep must keep delegating to this verified script + instead of reimplementing verification in Go. + +- **An external call with no ceiling.** Backend HTTP: `defaultTimeout = 30 * time.Second` + (`internal/api/client.go:31`). In-cluster submit: `SubmitTimeout` + (`internal/submit/client.go:21`). Doctor probes: `httpProbeTimeout = 8s`. Every shell-out uses + `exec.CommandContext`. Flag a bare `exec.Command` in non-test code, an `http.Client{}` with no + `Timeout`, or a watch/poll loop with no deadline. + +- **A missing empty / nil / zero guard on anything crossing a boundary** (user input, API + response, cluster state). There is no shared validator — the convention is a colocated + `validate*` func: `internal/push/spec.go:100` (`ValidateTableName`), + `internal/cli/interactive.go:537-568`. Two specifics: a bare Enter yields `""` and must not be + treated as a real path (`validateDatasetPath` documents exactly this); and pagination must + fail loudly on an unparseable `next` link rather than silently truncating the list + (`internal/api/client.go`, `nextPath`). Where "empty" and "unknown" are different answers, + prefer a three-valued return (`internal/cluster/discover.go:302`). + +- **A cross-repo contract change that only lands on one side.** `scripts/.data-ingestors-ref`, + `scripts/.client-ref` and `scripts/.backend-ref` pin upstream refs deliberately so an + unrelated upstream commit can't red every open PR. Flag a hand-edit to a generated artifact + (`internal/schema/*.json`, `internal/api/testdata/*.json`, + `internal/push/testdata/parity/goldens.json`, `internal/cli/testdata/golden/*.golden`) that + doesn't also bump and re-sync its pin, and any change to a chart assumption (discovery labels, + jobs-manager port, PVC mount path) that doesn't update `scripts/chart-invariants` — a chart + rename otherwise ships green in both repos and breaks discovery in the field. + +- **Output that breaks the style contract** (`STYLE.md`): all colour goes through + `internal/ui`'s Printer — never inline an escape or brand hex outside `internal/ui` + (`scripts/check-style.sh` greps for it). Colour is never load-bearing: headings carry bold, + alerts carry a glyph, so the output still reads under `NO_COLOR`, in a pipe, and for a + colour-blind reader. User-facing copy follows the terminology table ("secure environment", + "ingest", "delete", "Online/Offline", "collaborators", "task"); only the workspace → secure + environment swap is grep-enforced, the rest is review judgement. A new user-facing string + almost always needs its golden regenerated: + `TB_UPDATE_GOLDEN=1 go test ./internal/cli/ -run TestCopyCatalog`. + +- **Errors that lose their type.** `%w` wrapping is the house convention (~325 sites), with + typed errors for the cases callers branch on: `APIError`, `UpgradeRequiredError`, + `SubmitError`, `WatchError`, `exitError`, `noParentReleaseError`. Flag string-matching on an + error message where `errors.Is`/`errors.As` applies. + +## Known non-issues — do not flag + +- **`.golangci.yml` does not gate CI.** `golangci-lint` is never invoked in a workflow (its + `staticcheck`/`unused` are disabled there for runner OOM reasons); the blocking Lint job runs + pinned standalone binaries — `errcheck`, `gofmt -s`, `goimports`, `ineffassign`, `misspell`, + `staticcheck`, plus `deadcode-check.sh`, `file-budget.sh`, `check-style.sh`. Don't infer + coverage from that file. +- **`staticcheck` runs `-checks all,-ST1005` deliberately** — do not flag error-string + capitalisation or punctuation. It is a tracked, intentional exclusion (cli#279). +- `internal/submit/client.go:78` — `InsecureSkipVerify` is intentional for cluster-internal + traffic with no recognisable CA, documented in place and marked `//nolint:gosec`. It is the + only `nolint` in the repo. +- `scripts/deadcode-allowlist.txt` entries are verified false positives (Stringers reached only + through `fmt` reflection; test-only parity harnesses that must live in production source). +- `test/integration/*` uses 30s–5min timeouts because it drives a real cluster — not the + production timeout convention. +- `mutation.yml` and `head-drift-canary.yml` are advisory and never gate a merge. +- No `vendor/` directory — the module cache is used on purpose. +- `// style-guard: allow` is a defined escape hatch but is currently used nowhere; if one + appears, it is a novel exception worth scrutiny rather than an accepted pattern. + +## Tone + +Direct. Name the file and line. Give a concrete fix, not "consider". State the customer-visible +consequence — what they see, and which exit code they get — not just the code smell. + +This repo is **public**: never put a customer name, internal hostname, or internal-only ticket +detail in a finding. A bare `tracebloc/backend#NNNN` reference is fine. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 97c7f3c2..cf6b56bc 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -2,7 +2,7 @@ ## Related - + ## Type of change - [ ] Feature @@ -19,3 +19,4 @@ - [ ] `go build ./...`, `go vet`, and the Lint job's checks pass locally - [ ] Terminal output follows [STYLE.md](../STYLE.md) — Printer tones (no hardcoded colour/emoji), "secure environment" not "workspace"; `bash scripts/check-style.sh` passes - [ ] No secrets / credentials in the diff +- [ ] Cross-repo issues use `Fixes tracebloc/#N` — a bare `repo#N` closes nothing diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b1374a67..15123e47 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,7 +55,7 @@ This repo follows the same conventions as the rest of the tracebloc org: fix(#150): handle empty kubeconfig gracefully ``` -- **PR body** should include a `Closes #N` line (on its own line) for any ticket the PR fully resolves. GitHub auto-closes the issue on merge. The `feat(#N):` convention in the title is for kanban tracking; `Closes #N` in the body is what triggers auto-close. +- **PR body** should include a `Closes #N` line (on its own line) for any ticket the PR fully resolves. The `feat(#N):` convention in the title is for kanban tracking; the body line is what links the issue. Owner-qualify anything in another repo — `Fixes tracebloc/backend#123` — because a bare `backend#123` only cross-references and closes nothing. And since GitHub fires closing keywords only on merges into the default branch (`main`), a PR merged to `develop` won't auto-close on its own: confirm the ticket, and close it by hand if needed. - **One PR per ticket** when practical. Roll-up sync PRs (`Sync develop → main for vX.Y.Z release`) are an exception. diff --git a/cmd/tracebloc/main.go b/cmd/tracebloc/main.go index 978cde41..29ef9f00 100644 --- a/cmd/tracebloc/main.go +++ b/cmd/tracebloc/main.go @@ -59,11 +59,23 @@ func main() { syscall.SIGINT, syscall.SIGTERM) defer stop() - err := cli.NewRootCmd(cli.BuildInfo{ + executed, err := cli.NewRootCmd(cli.BuildInfo{ Version: version, GitSHA: gitSHA, BuildDate: buildDate, - }).ExecuteContext(ctx) + }).ExecuteContextC(ctx) + + // F1: after the command runs, a quiet once-a-day nudge if a newer release + // exists (best-effort; silent on dev builds, off a terminal, in CI, or with + // TRACEBLOC_NO_UPDATE_CHECK). Printed before the error handling so on success + // it's the last line, and on failure the error stays the most prominent one. + // Skipped right after `tracebloc upgrade`: that command swaps this very + // binary, so the still-running old process would otherwise nudge about the + // release it just installed. + if !cli.SkipUpdateNudge(executed) { + cli.MaybeNotifyUpdate(version, os.Stderr) + } + if err == nil { return } diff --git a/docs/rfcs/0001-cli-auth-and-client-provisioning.md b/docs/rfcs/0001-cli-auth-and-client-provisioning.md index ff49b697..7f1ccab7 100644 --- a/docs/rfcs/0001-cli-auth-and-client-provisioning.md +++ b/docs/rfcs/0001-cli-auth-and-client-provisioning.md @@ -1,5 +1,11 @@ # RFC 0001 — Browser-based auth & one-command client provisioning +> **Qualified ID: `RFC-CLI-0001`.** Cite this document by its qualified ID, never as +> a bare "RFC 0001" — `backend` and `client` each have their own 0001. Note that +> most existing bare `RFC-0001` references in the `backend`, `cli` and `client` +> codebases mean **this** document. Org-wide index: `docs/rfcs/README.md` in +> `tracebloc/backend` (private). +> > **Status: ACCEPTED — implemented.** The design in this RFC shipped in > **CLI v0.4.0** ([cli#107](https://github.com/tracebloc/cli/pull/107)); the > tracking epic ([cli#54](https://github.com/tracebloc/cli/issues/54)) is closed. diff --git a/docs/rfcs/0002-data-ingest-flow.md b/docs/rfcs/0002-data-ingest-flow.md index 1a3f6814..df4207bd 100644 --- a/docs/rfcs/0002-data-ingest-flow.md +++ b/docs/rfcs/0002-data-ingest-flow.md @@ -1,5 +1,9 @@ # RFC 0002 — `tracebloc data ingest`: flow, terminology & task taxonomy +> **Qualified ID: `RFC-CLI-0002`.** Cite this document by its qualified ID, never as +> a bare "RFC 0002" — `backend` also has a 0002 (platform cost & autoscaling). +> Org-wide index: `docs/rfcs/README.md` in `tracebloc/backend` (private). +> > **Status: DRAFT — for discussion.** Owner: @LukasWodka. Last updated: 2026-07-07. > > This RFC captures the redesign of the `tracebloc data ingest` user diff --git a/docs/rfcs/0003-storage-and-offboard-hygiene.md b/docs/rfcs/0003-storage-and-offboard-hygiene.md new file mode 100644 index 00000000..2f21dfc4 --- /dev/null +++ b/docs/rfcs/0003-storage-and-offboard-hygiene.md @@ -0,0 +1,607 @@ +# RFC 0003 — The secure environment: dataset storage, offboard hygiene & the boundary + +> **Qualified ID: `RFC-CLI-0003`.** Cite this document by its qualified ID, never as +> a bare "RFC 0003" — `backend` also has a 0003 (configurable preprocessing & +> imputation). Note that the existing bare `RFC-0003` references in the `client` +> chart templates and `docs/SEAL-CHECK.md` mean **this** document. Org-wide index: +> `docs/rfcs/README.md` in `tracebloc/backend` (private). +> +> **Status: DECIDED — v2.3; decisions D1–D20 locked (D1–D15 2026-07-22; +> D16–D20 2026-07-23). Execution tickets filed and cross-linked in §10/§12; +> D16–D20 (per-dataset isolation) are decided but not yet ticketed, with +> open items O5–O7 out for reviewer feedback.** Owner: @LukasWodka. +> Co-design & validation: @saadqbal. +> +> **v2.2 → v2.3.** Adds the **per-dataset data-isolation model** (new §7bis, +> D16–D20): one immutable `ds_` table per ingestion, correction +> by delete (no version chain), and isolation enforced by a grant-scoped +> definer-view instead of a `WHERE ingestor_id` filter — and **splits the +> multi-org composition layer** (data spaces, federated dataset combination) +> into its own RFC, "Data spaces & federated dataset composition" (drafting +> in `backend`), which builds on these primitives. +> +> **v2.1 same-day errata:** the §3.1/§6.4 weight-lifecycle claims were +> re-verified against the code — **weights do not rest durably in the +> environment** (per-cycle scratch only; the durable store is +> platform-side). D8 re-aimed accordingly; O1 decided → D15. +> +> **v1 → v2.** v1 (2026-07-21) framed two problems — offboard hygiene and +> dataset storage — and left §7 as a decision menu. v2 records the decisions +> taken (with prototype validation on a real install, client#368), folds in +> the review feedback, and widens the lens to what those decisions were +> always serving: a precise, honest definition of the **secure environment** +> — its boundary (§1), threat model (§2), model-IP stance (§6), in-cluster +> walls (§7), and per-substrate conformance (§8). +> +> Section map from v1: §2→§3, §3→§4, §4→§5, §5→§9, §6→§11, §7→§10 +> (now a decision log), §8→§12, §9→§13. +> +> Related: RFC-0001 (auth & client provisioning), RFC-0002 (data ingest flow +> & terminology), terminology source-of-truth (v2 `TERMINOLOGY.md`, docs +> `main`). Forward: **Data spaces & federated dataset composition** RFC +> (drafting in `backend`) builds on the §7bis isolation primitives. Design +> ticket: backend#1151. Storage prototype: client#368 +> (flag-gated, draft). Offboard honesty fix: cli#389. Spans `cli`, `client`, +> `client-runtime`, `backend`. + +## 1. The secure environment — definition + +A **secure environment** is a sealed enclave on infrastructure the customer +controls — a laptop (k3d), a cloud cluster (EKS/AKS/OpenShift), or bare +metal; the promise is the same in every form. It has **exactly three +ingress channels, one egress channel, and nothing sideways**: + +| Direction | Channel | Carries | Transport | +|---|---|---|---| +| In | **Data ingester** | raw datasets, from sources the customer configures | ingest jobs (local files / network share) | +| In | **tracebloc control plane** | models, weights, experiment instructions | backend API + Service Bus (TLS) | +| In | **tracebloc software** | container images, chart, CLI | registries — all images digest-pinned | +| Out | **tracebloc control plane** | trained weights, metrics, status/logs | Service Bus + allowlisted FQDNs (TLS) | + +Everything else is sealed: the environment does not reach into host +directories (§5), nothing on the host casually reads the environment's data +(§5), and workloads inside cannot reach arbitrary networks (§8). The +environment only dials out — no inbound ports are required. + +**Wording discipline.** Externally we say: *defined, auditable ingress and +egress — and raw data is never an egress channel.* We do **not** say +"air-gapped": the environment *requires* outbound connectivity (it +authenticates to the backend to obtain its Service Bus credentials; no +backend egress ⇒ experiments sit Pending), so a literal air-gap claim fails +the first serious security review. "Almost air-gapped" is internal +shorthand only. + +## 2. Threat model — protect what, from whom + +The environment protects **two assets for two parties**: the data owner's +datasets, and the model provider's model IP. Adversary by adversary: + +| Adversary | Datasets protected by | Model IP protected by | Status | +|---|---|---|---| +| External network attacker | TLS everywhere; outbound-only; NetworkPolicy | same | enforced | +| Other services/users on the host | node-local storage — no host files, no 777, no bind-mount (§5) | same | decided (D1/D2) | +| Vendor training code in-cluster | egress lockdown (§8.1) + dataset-scoped mounts + scoped DB grants (§7) | n/a — it *is* the model | built-inert / decided | +| tracebloc itself | raw data never leaves; only weights + defined metrics egress | n/a | by design | +| Cloud provider (cloud form) | customer-run infra; encrypted PVs; TEE later | TEE later (§6.5) | partial | +| **Environment owner (root)** | out of scope — the data is theirs | hygiene + watermark/audit now (§6.3–6.4); TEE phase 2 (§6.5) | decided (D7/D8) | + +**Honest ceilings — never claim past these:** + +1. **The machine owner has root, Docker, and the disk.** On classical + hardware, nothing that executes there can be made absolutely + inaccessible to them. For datasets that is fine (the data is theirs); + for model IP, see §6. +2. **The sanctioned exit carries data-derived information.** Weights and + metrics are functions of the training data — that is the product. The + defensible claim is "**raw data never leaves**", not "nothing derived + from your data leaves". Residual leakage (memorization, membership + inference) is a known property of federated learning; differential + privacy / secure aggregation are future options, out of scope here. +3. **Tampering can be made detectable, not impossible** (owner-root + again). Dataset integrity & score reproducibility is deliberately a + separate RFC (§11). + +## 3. How it works today (verified 2026-07-21; evidence refreshed 2026-07-22) + +### 3.1 Where data and weights physically live +- MySQL runs **in-cluster**; ingested datasets are tables in the + `training_test_datasets` database. +- **Model weights do not rest durably in the environment** (verified + against the code 2026-07-22). Each training pod downloads its current + weights from the backend at cycle start (ZIP transport envelope → a + framework file, e.g. `.pkl`/`.safetensors`), writes them **only into its + own per-experiment scratch** (`EXPERIMENT_SCRATCH_PATH`, an emptyDir that + dies with the pod; legacy images fall back to the image filesystem), and + uploads the result back to the backend after the cycle. The durable, + cross-cycle weight store is **platform-side** (tracebloc's averaging + share: `{edge}_{exp}_{cycle}_weights.pkl` under `SHARE_PATH`) — outside + the environment. N concurrent trainings = N pods, each holding only its + own current file; nothing accumulates in the environment. +- The MySQL PersistentVolume uses a **hostPath** under + `/tracebloc//mysql`; the local k3d cluster **bind-mounts + `~/.tracebloc → /tracebloc`** (`HOST_DATA_DIR`, default + `$HOME/.tracebloc`). Net effect: dataset tables are host files. +- Dataset *files* can additionally come from `HOST_DATASET_DIR` (a network + mount by design, backend#743), bind-mounted **cluster-wide** (`@all`) at + `/tracebloc-data` — a wider window than a single ingest needs (§7). + +### 3.2 Permissions (corrected from v1) +- `_ensure_tracebloc_dirs` chmods **the `logs`/`mysql`/`data` subdirs** to + `777` — not all of `~/.tracebloc`, and `values.yaml` is spared. v1 + overstated the blast radius. +- On the hostPath model that 777 is **load-bearing** (the host user writes + into the shared dirs, and kubelet does not apply fsGroup to hostPath + volumes) — so v1 §4.6's "can ship independently" was wrong. The 777 + disappears **with Option C**, not before it (D2). + +### 3.3 Install & upgrade behavior +- Install does `mkdir -p` with **no existing-data guard** — leftover data + is silently adopted. Flat and per-release layouts coexist across + versions; a machine that has seen both accumulates both (Problem A). +- On re-run, the installer **reuses** an existing cluster ("already exists" + → use it; `cluster start`) — it does not recreate. In-place upgrades + therefore keep data; only an explicit `cluster delete` destroys it. This + fact is load-bearing for D1 and D4. + +### 3.4 Offboard (`tracebloc delete`) +- Teardown order: revoke credential → clear active-client pointer → Helm + uninstall → k3d cluster delete → prune images → remove host data dir → + remove self. Flags: `--yes`, `--keep-data`, `--force`. +- The host-data wipe is now **verified before printing ✔** (cli#389) — + a nil `RemoveAll` is no longer treated as proof. +- `HOST_DATASET_DIR` is never touched (by design — shared network mount). + +### 3.5 Egress today (added in v2) +- The training NetworkPolicy ships **enabled**, allowing DNS, in-cluster + MySQL, the requests-proxy, and the egress gateway — **plus a + `0.0.0.0/0:443` rule** (`networkPolicy.training.allowExternalHttps` + defaults to `true`). The squid egress gateway (FQDN allowlist: backend + + App Insights) ships **inert** (`egressProxy.routeWorkloads: false`). +- Translation: the lockdown is **built but not flipped** — today a training + pod can still reach any external host on :443 (§8.1). +- Enforcement tooling exists: a `helm test` probe verifies the CNI actually + blocks egress, and a reachability check verifies required backend egress + works. + +### 3.6 Spawned-job hardening today +- New-architecture images run with `readOnlyRootFilesystem`, write weights + and scratch to a pod-scoped `emptyDir` (`EXPERIMENT_SCRATCH_PATH` — dies + with the pod), and get read-only shared mounts. **Legacy images are + carved out** of parts of this (they write inside the image filesystem). + +### 3.7 The 2026-07-21 reproduction (unchanged from v1) +- "Delete isn't deleting" was **wrong**: `tracebloc delete --force --yes` + fully wiped `~/.tracebloc` (0 survivors). The earlier "survival" was a + flat/per-release **transition artifact** across a version bump, plus a + second CLI binary in `/usr/local/bin`. The real gaps: alternate/legacy + locations, and the installer silently re-adopting whatever it finds. + +## 4. Problem A — offboard leaves a clean slate → DECIDED + +**What we want:** "delete" means the environment's data is gone — current +version and leftovers — so a reinstall starts empty. UX expectation and +privacy expectation at once. + +**Scope constraint (unchanged):** never a machine-wide wipe. A machine can +host multiple environments, and `HOST_DATASET_DIR` may be a shared network +mount other tools use. Clean-slate is scoped to the environment being +offboarded. + +**Decision (D3):** +1. **Installer leftover-guard** — if data is present at install time, stop + and make the user choose (reuse / wipe / new dir) instead of silently + adopting. This is the guard that prevents the bug that was actually hit, + and it doubles as the migration prompt (D4). +2. **Drop the heavy multi-path wipe** proposed in v1 — under node-local + storage (§5) there are no host data paths left to chase. `delete` keeps + config/token cleanup and **verifies the wipe before printing ✔** + (shipped as cli#389). +3. `--keep-data` stays as the explicit opt-out. A `--purge-all` (enumerate, + confirm, never default) is deferred until someone actually needs it. + +## 5. Problem B — where datasets live → DECIDED: Option C (node-local) + +**The principle (unchanged from v1): separate restart-persistence from +delete-persistence.** Laptops reboot and data must survive; offboard/delete +means destroyed. The hostPath bind-mount conflated the two by making data +survive everything — which is exactly what made offboard fragile (Problem +A) *and* put data outside the environment boundary (Problem B). + +The v1 menu, with the decision marked: + +| Option | Restart-persists | Delete wipes it | Host exposure | +|---|---|---|---| +| A. Status quo — bind-mount `~/.tracebloc`, 777 subdirs | ✅ | ⚠️ only if delete chases every path | ❌ world-readable files in `$HOME` | +| B. Docker managed named volume (env-scoped) | ✅ | ✅ if removed on offboard | ✅ | +| **C. Node-local storage (k3s `local-path`, inside the k3d node) — CHOSEN (D1)** | ✅ (node container persists across stop/start) | ✅ (dies with `cluster delete`) | ✅ not visible as host files | +| D. C + encryption at rest | ✅ | ✅ | ✅✅ — **phase 2** (D5) | + +**Why C:** datasets and MySQL move onto k3s's built-in `local-path` +provisioner *inside the k3d node*. Delete destroys the data by +construction; there is no browsable host folder; the 777 goes away by +construction (D2); restarts keep working (the node container and its +Docker volume survive stop/start); and upgrades keep data because the +installer **reuses** the cluster (§3.3). The v1 objection to C — +"loses survive-delete+recreate" — dissolves: delete+recreate is precisely +the case where data *should* die. + +**Validated on a real credentialed dev install (client#368, 2026-07-22):** +single-node cluster, all PVCs Bound on `local-path`, zero hostPath PVs; +real jobs-manager-spawned ingest jobs mounted the shared PVC and registered +datasets against the backend; stop/start preserved data; `cluster delete` +destroyed the node, its Docker volume, and all data; nothing under +`~/.tracebloc`. The prototype run also caught and fixed a real leak (a +second `_ensure_tracebloc_dirs` call site still creating empty 777 dirs). + +**Constraint C1 — single-node.** `local-path` is RWO/WaitForFirstConsumer, +and spawned Jobs must land on the node that holds the volume, so node-local +pins the cluster to a **single node** — it forces both `AGENTS=0` **and** +`SERVERS=1`. Dropping the agents is not enough on its own: k3s server nodes +are schedulable, so `SERVERS>1` would still yield multiple nodes a Job could +land on away from the volume; the prototype (client#368) had to pin both. +Since the defaults are `SERVERS=1`/`AGENTS=1` today, **flipping the storage +default also changes the default local topology from two nodes to one** — +call this out in release notes. (k3s agents on one Docker host provide no +real isolation, so nothing of value is lost.) + +**Out of scope for now:** `HOST_DATASET_DIR` (network-mount ingest sources) +stays on the hostpath path; combining it with node-local is a follow-up. + +**Migration (D4): no data-copier.** `--reuse-values` upgrades keep existing +installs on their current `~/.tracebloc`; they move to C on a clean +delete + reinstall — consistent with "delete means gone". The leftover-guard +(D3) ensures data is never silently stranded or adopted. + +**Encryption at rest (D5): phase 2.** For local installs, recommend host +full-disk encryption (FileVault/LUKS); in cloud, encrypted PVs. App-level +crypto for the volume is not warranted for a local dev tool unless the +threat model changes — and the real answer to "protected from a local +admin" is §6.5, not filesystem crypto. + +**Decided (D15, 2026-07-22):** node-local flips from flag-gated +(`TB_STORAGE_MODE`) to **default** for local installs — the step that +actually delivers this RFC to users — after one green end-to-end +**training run** on node-local (the only client#368-checklist item not yet +exercised; it was blocked by an orthogonal dev auth issue, not by +storage). The single-node topology change (C1) goes in the release notes. + +## 6. Model IP protection — two-sided trust (new in v2) + +### 6.1 Goal +The environment owner must not be able to access the vendor models that +run inside their environment. This completes the marketplace trust story in +both directions — the data owner's data is protected from the vendor, +**and** the vendor's model is protected from the data owner. Almost nobody +in the FL space offers the second half; it is worth building toward +deliberately. + +### 6.2 The ceiling — state it before designing around it +To execute, weights must exist in plaintext in RAM/VRAM, and any decryption +key must be present in the environment. Root can dump process memory, GPU +memory, or the disk at any moment *during* execution — so at-rest +encryption alone, obfuscation, and delete-after-run all raise the bar +without changing the outcome. **On classical hardware, prevention against +owner-root is impossible; only trusted execution environments (§6.5) +change that.** Everything in 6.3–6.4 is deterrence, detection, and +minimization — valuable, and never to be sold as prevention. + +### 6.3 Now — watermarking + audit (D7) +Fingerprint delivered weights **per environment** (traitor tracing): a +leaked model is attributable to the environment it leaked from, which makes +the contractual protection enforceable. Log model-delivery events. This is +detection and legal recourse, not prevention — and it is what makes 6.4 +credible commercially. + +### 6.4 Now — runtime hygiene, maximum practical (D8) +1. **Weights enter at runtime over TLS, never baked into images** (already + true — images stay generic per task type; `docker save` yields no IP). +2. **The active working copy** lives in pod-scoped scratch + (`EXPERIMENT_SCRATCH_PATH`, an `emptyDir`) and dies with the pod — + already true on new-architecture images; close the legacy-image + carve-out (D11). +3. **At rest in the environment — corrected in v2.1 after code + verification.** The edge has **no durable weight store to encrypt**: + weights arrive per cycle, exist only in the pod's own scratch and + process memory, and the durable store is platform-side (§3.1). v2's + proposal to envelope-encrypt an in-environment weight DB targeted a + store that does not exist — the architecture is already stronger than + that proposal assumed. What remains on the edge is **lifecycle + hygiene**: `ttlSecondsAfterFinished` on finished Jobs (an emptyDir + survives until its pod object is deleted), closing the legacy + image-filesystem fallback (D11), offboard's existing image prune + (§3.4), and — optional, marginal against the §6.2 ceiling — encrypting + the scratch file with a per-pod in-memory key. +4. **The platform-side store is where envelope encryption + + crypto-shredding apply.** The durable `{edge}_{exp}_{cycle}_weights.pkl` + files under the averaging share are opaque bytes: encrypt on receipt, + decrypt in memory for averaging, per-experiment key in the backend's + KMS, discard the key on experiment deletion → every copy (including + backups/snapshots) becomes unrecoverable, instantly and verifiably. + Feasible as a contained change at the existing weight-file I/O choke + points (deserialization is already confined via `safe_unpickle` on both + edge and averaging). This protects vendor IP **on tracebloc + infrastructure** and makes experiment deletion verifiable — but it is a + **platform ticket (backend/averaging), not part of the environment + boundary** this RFC defines. + +Stated ceiling: root can still capture plaintext *during* execution — 6.4 +stops casual and after-the-fact access, not a determined owner. That gap +belongs to 6.5. + +### 6.5 Phase 2 — confidential computing (TEE) (D14) +AMD SEV-SNP / Intel TDX confidential VMs plus NVIDIA H100/H200 +confidential-computing mode encrypt memory with keys held in silicon; the +host, hypervisor, and root **cannot** read enclave memory. Remote +attestation lets tracebloc act as key broker: weights decrypt **only** +inside a measured, attested enclave. This is the one mechanism that +actually delivers "the owner cannot access the model" — and it also +strengthens the *data* story in cloud deployments (data protected from the +cloud provider). Azure sells confidential GPU VMs today, so a pilot on +cloud-form environments is the natural first step **when build capacity +allows — explicitly deferred for now** (decided 2026-07-22). On-prem +follows where customer hardware supports it; local k3d never gets it. +Bridge until then: **sensitivity tiering** — vendors can flag crown-jewel +models to run only on TEE-attested environments; commodity/open-backbone +models run anywhere. + +### 6.6 Rejected +Homomorphic encryption / MPC for training: orders of magnitude too slow for +deep learning at this scale. Named here so it doesn't resurface. + +## 7. In-cluster walls — dataset & experiment scoping (new in v2, DECIDED) + +Being inside the box must not mean seeing everything in the box. The +precise promise is "no service **outside the sanctioned training flow** +touches the data" — and the sanctioned flow is *per experiment*. A training +job must reach exactly its own dataset and its own artifacts, nothing else. + +1. **Dataset-scoped mounts (D9).** Jobs-manager currently mounts the whole + shared data PVC into spawned jobs. Near term: mount only the job's + dataset directory (`subPath`), read-only for training. End state under + Option C: **one PVC per dataset** — `local-path` dynamic provisioning + makes per-dataset PVCs cheap — so a job physically cannot see other + datasets. This also narrows the cluster-wide `HOST_DATASET_DIR` window + where that path is used. +2. **The database layer (D10).** Tabular and time-series datasets are + MySQL tables, and the training NetworkPolicy allows training→MySQL — + PVC scoping alone does not cover them. Adopt **per-experiment DB + credentials whose grants cover only that experiment's dataset tables** + (and its own weight rows, which composes with §6.4): short-lived, + injected at job start, revoked after. +3. **Finish spawned-pod hardening (D11).** Close the legacy-image + read-only carve-out; set `automountServiceAccountToken: false` on + spawned jobs; keep `readOnlyRootFilesystem` + read-only shared mounts as + the floor. + +## 7bis. Per-dataset isolation — immutable tables & grant-scoped views (new in v2.3, DECIDED) + +§7 scopes *access* to a dataset (mounts, DB grants, pod hardening). This +section sharpens *what a dataset physically is*, so the scoping rests on +structure rather than on a filter a job could sidestep. Today +tabular/time-series ingests land as rows in a **shared** table tagged by +`ingestor_id`, and every read narrows with `WHERE ingestor_id IN (…)` (§3.1; +tracebloc-engine `core/utils/database.py:243-361`). A `WHERE` clause is a +convention, not a wall — one dropped predicate or one `SELECT *` and a job +reads its neighbours. The model below turns the convention into physics. + +1. **One immutable table per ingestion (D16).** Every ingest creates a + **brand-new physical table** named `ds_`; the ingestor id + *is* the physical identity. The dataset name the user chose is a + **user-facing label only** (metadata), never the table name. Ten ingests + of the same name and schema produce **ten tables**, not ten appends into + one. Isolation stops being a runtime filter and becomes a property of the + schema: a job that can only name `ds_` cannot phrase a query + that reaches another dataset. + +2. **Immutable — no append, no in-place edit (D17; enforced by D19).** A + table, once written by an ingestion, is never appended to or edited. + Re-ingesting the same name+schema **creates a new table**; it does not + reuse the old one. This is enforced by **removing the ingestor's + reuse/append path**: `data-ingestors/tracebloc_ingestor/database.py:275-357` + today returns/reflects an existing table when the name and feature schema + match (the "return existing table if already created" / "check if table + exists in database" branch) — that branch is deleted, and ingestion + always creates a fresh `ds_`. The schema-drift guard that + lives in that branch (the "stale table from an earlier ingestion" error) + becomes moot once every ingestion owns a private table name. + +3. **Correction = delete, not versioning (D17).** There is **no version + chain**. A wrong ingestion is corrected by **dropping its table** and + re-ingesting — a new id, a new `ds_`, the bad one gone — + which keeps the model honest with the RFC's "delete means gone" stance + (§4/§5) instead of accreting history. **Referential cleanup is part of + the drop:** removing a table must invalidate every view and every + data-space reference pointing at it (O7). A definer-view left dangling + over a dropped base table is both a broken read path and a latent leak. + +4. **Isolation = a grant-scoped access handle, never raw-table access + (D18).** A training pod is never granted access to base tables at large. + For MySQL it is granted `SELECT` **only** on a **definer-rights view** + (degenerate case: on its single `ds_` table) — never on any + other dataset's base table. Because the grant does not name the + neighbours, a malicious or buggy `SELECT * FROM ds_` **cannot + even reference** a table it holds no grant on; it fails at + permission-check time, not at a `WHERE` clause the job controls. + + *View vs. table, for the reader:* a **table** stores rows on disk; a + **view** is a stored `SELECT` exposing only chosen rows/columns of one or + more tables, holding no data of its own; a **definer-rights view** + executes with the privileges of the user that *created* it, so you can + `GRANT SELECT` on the view to the training user **without** granting that + user any privilege on the underlying base tables. The view becomes the + only name the job holds; the base tables stay invisible. + + For **file/image** datasets the analogue is the **per-dataset PVC / + `subPath` mount** already chosen as the D9 end state (client-runtime#203) + — same principle, different substrate: the handle is a mount, not a view. + And this **composes with the per-experiment DB grant (D10, + backend#1181)**: that grant now targets the experiment's **view/table + handle** rather than the shared table, and still excludes the platform + metadata DB. §7 and §7bis are the same wall from two sides — §7 scopes + the grant, §7bis scopes the object the grant points at. + +5. **Grandfather — new ingests only (D20).** The model applies to **new + ingestions**. Existing shared multi-ingestor tables **stay as they are** + — no migration, no backfill into per-ingestion tables — mirroring the + storage-migration stance (D4: no data-copier; installs move forward on + the natural boundary, here the next ingest). The client read path + therefore keeps tolerating both shapes through the transition (O6). + +**Scope boundary — what this section does *not* own.** The primitive here is +deliberately **single-environment**: it makes one dataset an isolated, +access-scoped object inside one secure environment. The **multi-org +composition** layer — *data spaces*, federated combination of datasets +across environments, the schema/compatibility engine that decides when two +datasets may train together, and cross-org authorization — is carved into +its **own RFC, "Data spaces & federated dataset composition"** (drafting in +`backend`), which **builds on** these primitives (a data space is composed +out of exactly these access-scoped units). Ownership split: **this RFC owns +the isolation primitive; the data-spaces RFC owns the composition over it.** + +## 8. Boundary enforcement & conformance (new in v2) + +### 8.1 Egress lockdown — flip it (D6, separate ticket) +The outbound wall exists (training NetworkPolicy, squid FQDN-allowlist +gateway, enforcement probe, backend-reachability check) but **ships +permissive**: `allowExternalHttps: true` keeps a `0.0.0.0/0:443` rule and +`routeWorkloads: false` leaves the gateway inert. Until the per-fleet flip +(verify gateway → route workloads → drop the 443 rule), "nothing gets out" +is not an enforced property for training pods on :443. Tracked as its own +ticket; this RFC records the dependency: **the golden-box claim is gated on +that flip** more than on anything else in this document. + +### 8.2 The seal check (D12) +Productize the existing probes into **one conformance suite** — the +egress-enforcement probe, the required-backend-reachability check, and +storage checks (post-C: no hostPath PVs, PVCs on the expected class, +nothing under `~/.tracebloc`) — runnable at install, at upgrade, and on +demand, surfaced pass/fail in the CLI. Design stance the chart already +takes: **silent non-protection is worse than explicit disabling.** An +environment that cannot enforce a guarantee is explicitly marked *unsealed* +— never silently claimed sealed. + +### 8.3 The guarantee matrix (D12) +Enforcement differs per substrate; the matrix is the honest artifact a +customer security review can quote. To be filled precisely as part of D12 +(cells: enforced / conditional / recommended / not available): + +| Guarantee | k3d local | EKS | AKS | OpenShift | bare metal | +|---|---|---|---|---|---| +| Storage inside cluster boundary (§5) | decided (C) | native PV | native PV | native PV | native PV | +| NetworkPolicy egress enforcement | verify (k3s embedded) | conditional (CNI mode) | conditional (CNI) | native (OVN) | conditional (CNI) | +| Encryption at rest | host FDE (recommended) | encrypted EBS | encrypted disks | platform | site policy | +| Confidential compute (§6.5) | not available | phase 2 | phase 2 (pilot) | phase 2 | hardware-dependent | + +### 8.4 Verify local enforcement (D12) +Do not assume k3d enforces NetworkPolicy — verify the k3s-embedded +controller blocks egress on a local install and fold that probe into the +seal check. + +## 9. Messaging reconciliation + +- The **channel list in §1 is the quotable definition** of the secure + environment — align docs, website, and sales material with it and with + the terminology source-of-truth. +- Keep the strong "within the secure environment" wording — Option C makes + it literally true for local storage. Internal precision: node-local means + *not-host-visible + dies-with-cluster*, not *cryptographically secure*. +- Never "air-gapped" in external copy (§1). +- Model-IP claims: "technically protected" only where TEE-attested (§6.5, + phase 2). Until then the accurate sentence is: *runtime-hygienic, + watermarked, and contractually protected.* + +## 10. Decision log (2026-07-22, Lukas + Asad) + +| # | Decision (v1 ref) | Outcome | +|---|---|---| +| D1 | Local storage target (§7.2) | **Option C — node-local**, validated in client#368 → client#367 | +| D2 | Drop `chmod 777` (§7.3) | **Yes — by construction with C** (load-bearing on hostpath, cannot ship separately) | +| D3 | Offboard clean-slate (§7.1) | **Leftover-guard yes; heavy multi-path wipe no; verify-before-✔** (guard: client#376; verify: cli#389) | +| D4 | Migration (§7.5) | **No data-copier**; guard prevents stranding; installs move to C on delete+reinstall | +| D5 | At-rest encryption (§7.4) | **Phase 2**; recommend host FDE; keep wording honest | +| D6 | Egress lockdown flip | **client-runtime#199**; golden-box claim gated on it (§8.1) | +| D7 | Model IP — now | **Watermarking + audit** (§6.3) → backend#1183 | +| D8 | Weights at rest | **Edge: nothing rests by architecture (verified) — lifecycle hygiene (client-runtime#200); platform-side envelope encryption + crypto-shredding (backend#1182)** (§6.4) | +| D9 | Dataset scoping | **Scoped mounts → per-dataset PVCs under C** (§7.1) → client-runtime#203 | +| D10 | DB scoping | **Per-experiment DB credentials, table-scoped grants** (§7.2) → backend#1181 | +| D11 | Pod hardening | **Close legacy carve-out; no SA token; read-only floor** (§7.3) → client-runtime#201 + #202 | +| D12 | Conformance | **Guarantee matrix + seal check + verify k3d enforcement** (§8.2–8.4) → backend#1184 + cli#393 | +| D13 | Score/tamper integrity | **Separate RFC** — versioning+integrity root cause, not storage (§11) → backend#1185 | +| D14 | Confidential compute | **Phase 2 — deferred for capacity**; Azure confidential-GPU pilot first; tiering as bridge (§6.5) | +| D15 | Node-local default (was O1) | **Flip node-local to default for local installs** after one green training run on node-local (gate: backend#1180); checklist on client#367 | +| D16 | Table-per-ingestion (§7bis) | **One immutable `ds_` table per ingest** — the ingestor id is the physical identity; the user's dataset name is a label only (same name+schema → N tables) | +| D17 | Immutability + correction (§7bis) | **No append, no in-place edit; re-ingest = new table; correction = drop + re-ingest, no version chain** — the drop must cascade to dependent views / data-space refs (O7) | +| D18 | Isolation enforcement (§7bis) | **Grant-scoped access handle, not raw-table access** — MySQL: `SELECT` on a definer-rights view (or the single `ds_` table) only; files: per-dataset PVC/`subPath` (D9); composes with the per-experiment grant (D10, backend#1181) | +| D19 | Ingestor append-disable (§7bis) | **Remove the reuse/append path** — `data-ingestors/tracebloc_ingestor/database.py:275-357` (reuse-existing-table branch) deleted; ingestion always creates a fresh table | +| D20 | Grandfather (§7bis) | **New ingests only; existing shared multi-ingestor tables stay as-is; no migration/backfill** (mirrors D4) | + +Open items: + +| # | Question | Recommendation | +|---|---|---| +| O2 | Platform-side weight retention: when does crypto-shred fire on the averaging-share store (experiment completion? grace window? audit needs?) | Shred at completion + configurable grace window — decided inside backend#1182 | +| O4 | Watermarking mechanics & owner (backend work) | Scope inside backend#1183 | +| O5 | Storage abstraction beyond MySQL — should the RFC define "a dataset = an isolated, access-scoped unit" with a per-backend physical form (MySQL table + definer-view; file/folder PVC; future other DBs) rather than binding the concept to MySQL? | Open (reviewer feedback) — lean toward a backend-neutral definition; confirm the per-substrate forms | +| O6 | View mechanics at scale — many tables + views (UNION / indexing / perf), and how the training read path moves from the client-built `WHERE ingestor_id IN (…)` (tracebloc-engine `core/utils/database.py:243-361`) to reading a backend-provisioned view/table handle | Open (reviewer feedback) — needs a read-path / perf design pass | +| O7 | Delete / referential-cleanup semantics — cascade a table drop to its views and any data-space references (D17) | Open (reviewer feedback) — resolve jointly with the data-spaces RFC (references cross the boundary) | + +Resolved since v2: **O1 → D15** (decided). **O3** dissolved by the +verified architecture — there is no in-environment at-rest weight store +needing key custody; platform-side keys live in the backend's KMS. + +## 11. Non-goals + +- Remote/cloud PV storage model — unchanged (already on proper PVs on + customer infrastructure). +- Restart-persistence — unchanged (laptops reboot; data survives). +- **Dataset integrity & score reproducibility** — fingerprint at ingest, + verify at scoring, bind every score to the dataset fingerprint. Genuinely + important for the benchmark product, but its root cause is verifiable + *versioning*, not storage location; it gets its **own RFC** (D13) rather + than blurring this one. +- Cryptographic training (HE/MPC) — rejected (§6.6). Differential privacy / + secure aggregation — future, separate. + +## 12. Rollout sequence + +1. Land this RFC with the §10 decision log. Execution epic: backend#1151. +2. cli#389 (verify-before-✔) and client#368 (flag-gated C) merge on their + own review tracks. +3. Build the **installer leftover-guard** (D3 — client#376). +4. **Egress-lockdown flip** (D6 — client-runtime#199), per fleet. +5. One green **training run on node-local** (gate: backend#1180) → flip the + default (D15 — checklist on client#367), single-node topology change in + release notes. +6. **Scoping & hygiene** (D8–D11) as backend#1151 children: job-TTL + + scratch hygiene (client-runtime#200); platform-side weight-store + encryption + crypto-shred (backend#1182); scoped mounts + (client-runtime#203); per-experiment DB grants (backend#1181); + pod-hardening completion (client-runtime#201, #202); watermarking + (D7 — backend#1183). +7. **Seal check + guarantee matrix** (D12 — backend#1184, CLI surfacing + cli#393). +8. Phase 2 when capacity allows: TEE pilot on Azure confidential GPU (D14); + at-rest encryption revisit (D5). Docs/messaging: backend#1185 (integrity + RFC), backend#1186 (§9 alignment). + +## 13. Appendix — evidence (refreshed 2026-07-22; line numbers as of that date) + +- `client/scripts/lib/common.sh:386` — `HOST_DATA_DIR="${HOST_DATA_DIR:-$HOME/.tracebloc}"` (v1 cited :329 — drifted) +- `client/scripts/lib/common.sh:392` — `HOST_DATASET_DIR="${HOST_DATASET_DIR:-}"` (v1 cited :335) +- `client/scripts/lib/common.sh` (post-#368) — `SERVERS="${SERVERS:-1}"`/`AGENTS="${AGENTS:-1}"` (default two-node local topology), and `TB_STORAGE_MODE=node-local` forces **both** `AGENTS=0` **and** `SERVERS=1` for the single-node pin (§5 C1) +- `client/scripts/lib/cluster.sh:28-56` — `_ensure_tracebloc_dirs`: `mkdir -p` with no existing-data guard; `chmod -R 777` scoped to `logs`/`mysql`/`data` subdirs only, `values.yaml` spared +- `client/scripts/lib/cluster.sh:197`, `:355-356` — cluster **reuse** on re-run (`cluster start`; "already exists → Using existing cluster") +- `client/scripts/lib/cluster.sh:312` — `-v "${HOST_DATASET_DIR}:/tracebloc-data@all"` (cluster-wide dataset-source mount) +- `client/client/values.yaml` — `networkPolicy.training.{enabled,allowExternalHttps,enforcementProbeHost,clusterCidrs}`, `egressProxy.{enabled,routeWorkloads}`, `egressReachabilityCheck` (lockdown built, ships permissive; §3.5/§8.1) +- `client-runtime/jobs_manager.py` (~:825-885) — `EXPERIMENT_SCRATCH_PATH` emptyDir scratch, `readOnlyRootFilesystem`, read-only shared mounts; legacy-image carve-out (~:77, :829) +- `tracebloc-engine/core/weights/base.py:117-146, :340` — per-cycle weight + download (backend → ZIP envelope → `{scratch}/{exp}_{model}_weights.`) + and upload back to the backend; stale-sibling cleanup between formats +- `tracebloc-engine/core/utils/general.py:21-33` — `get_experiment_path()`: + `EXPERIMENT_SCRATCH_PATH` (emptyDir) or legacy image-filesystem fallback +- `averaging-service/service/safe_unpickle.py` — the durable, platform-side + weight store: `{edge}_{exp}_{cycle}_weights.pkl` under `SHARE_PATH`; + deserialization confined on both sides (edge: backend#946) +- `cli/internal/cli/delete.go` — teardown order; wipe verified before ✔ as of cli#389 +- Validation evidence — client#368 comments (2026-07-22): single-node node-local install on dev; PVCs on `local-path`; real ingest-jobs sharing the data PVC; stop/start survives; delete destroys volume + data; prototype fix `5e4ea45` (second `_ensure_tracebloc_dirs` call site) +- Repro (2026-07-21): `tracebloc delete --force --yes` → `~/.tracebloc` gone, 0 `.ibd` survivors; earlier "survival" = flat/per-release transition artifact + second CLI binary diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f9f38703..9943e9ee 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -115,12 +115,12 @@ produces that code. | Code | Meaning | Produced by | Constant | |------|---------|-------------|----------| -| `0` | Success — includes `--dry-run` completing, a guided run you cancelled cleanly, and `doctor` passing with warnings only | all commands | `exitOK` | +| `0` | Success — includes `--dry-run` completing, any prompt you declined or cancelled with Ctrl-C (nothing was started, and the CLI prints a `Cancelled — …` line saying so), and `doctor` passing with warnings only | all commands | `exitOK` | | `1` | Generic failure with no more specific bucket (also any error without an explicit code) | `login`, `client …`, `delete`, mistyped commands | `exitFailure` | | `2` | Your input didn't validate: schema validation failed (spec synthesized from flags, or your YAML), an unsupported/unknown `--task`, a task-scoped flag applied to the wrong task, an invalid dataset name, or a resource size that doesn't fit the machine | `data ingest`, `data validate`, `data delete`, `resources set` | `exitBadInput` | -| `2` | One or more checks failed | `doctor` | `exitChecksFailed` | -| `3` | Local environment problem: kubeconfig couldn't be loaded, the dataset path is missing or unreadable, the local layout is wrong, a YAML file didn't parse, or a prompt was needed but the run is non-interactive (`--no-input` / `--output-json` / no TTY) | `data ingest`, `data validate`, `data list`, `data delete`, `doctor`, `resources`, `resources set` | `exitLocalEnv` | -| `4` | Cluster reachable but no tracebloc client found in the namespace — or its shared storage / dataset list is missing, so the target can't be confirmed | `data ingest`, `data list`, `data delete`, `cluster info`, `resources`, `resources set` | `exitNoWorkspace` | +| `2` | One or more checks failed — for `client status --seal`: the environment is unsealed (a conformance check failed), or unknown (the chart ships no conformance checks, so the seal couldn't be verified) | `doctor`, `client status --seal` | `exitChecksFailed` | +| `3` | Local environment problem: kubeconfig couldn't be loaded, the dataset path is missing or unreadable, the local layout is wrong, a YAML file didn't parse, or a prompt was needed but the run is non-interactive (`--no-input` / `--output-json` / no TTY) | `data ingest`, `data validate`, `data list`, `data delete`, `doctor`, `resources`, `resources set`, `client status --seal` | `exitLocalEnv` | +| `4` | Cluster reachable but no tracebloc client found in the namespace — or its shared storage / dataset list is missing, so the target can't be confirmed | `data ingest`, `data list`, `data delete`, `cluster info`, `resources`, `resources set`, `client status --seal` | `exitNoWorkspace` | | `5` | Auth: the ingestor SA token couldn't be obtained, or jobs-manager rejected it (401/403) | `data ingest`, `cluster info` | `exitAuth` | | `5` | No dataset by that name on this client (nothing to delete) | `data delete` | `exitNoSuchDataset` | | `6` | Destination table already exists — re-run with `--overwrite` to replace it, or pick a different `--name` | `data ingest` | `exitTableExists` | @@ -129,7 +129,7 @@ produces that code. | `7` | The cluster couldn't be queried for its datasets | `data list` | `exitQueryFailed` | | `8` | jobs-manager rejected the submitted run (a non-auth 4xx/5xx), or the port-forward to it couldn't be set up | `data ingest` | `exitSubmitFailed` | | `9` | The ingestion Job exited non-zero, completed with row-level failures the summary panel reports, or its outcome couldn't be determined / followed | `data ingest` | `exitIngestFailed` | -| `130` | You hit Ctrl-C at an interactive prompt (128+SIGINT) | interactive prompts | `exitInterrupted` | +| `130` | You hit Ctrl-C while something was already running — the sign-in wait, `client status --wait`, the seal check, or an installer re-run (128+SIGINT). Ctrl-C at a *question* is `0` instead: nothing had started | `login`, `client status --wait`, `client status --seal`, `upgrade`, `prepare-host` | `exitInterrupted` | ## Still stuck? diff --git a/internal/api/client.go b/internal/api/client.go index 2f27589e..e295189e 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -172,9 +172,8 @@ func (e *UpgradeRequiredError) Error() string { floor = ">= " + e.MinVersion } return fmt.Sprintf( - "this tracebloc CLI is too old for the server (requires %s). Upgrade to the "+ - "latest release — re-run the tracebloc install script, or download it from "+ - "https://github.com/tracebloc/cli/releases/latest — then retry.", + "this tracebloc CLI is too old for the server (requires %s). Update it — run "+ + "`tracebloc upgrade` (or re-run the install script) — then retry.", floor, ) } diff --git a/internal/cli/cancel_test.go b/internal/cli/cancel_test.go new file mode 100644 index 00000000..d6e18e3f --- /dev/null +++ b/internal/cli/cancel_test.go @@ -0,0 +1,146 @@ +// The cancellation contract, in one table. Every interactive prompt in the CLI +// can be backed out of two ways — Ctrl-C, or an answer that means "no" — and both +// must produce the SAME thing: a visible "Cancelled — …" line, exit 0, and no +// side effect. This file is the drift guard for that (backend#1253, the test +// convention proposed for this finding class in backend#930). +// +// Why it exists: `client create` and `delete` used to map Ctrl-C straight to a +// nil error, so aborting at the prompt exited 0 having printed nothing about it — +// byte-for-byte indistinguishable from a completed run for anything reading the +// stream, and inconsistent with the declined-answer branch sitting right beside +// it. Asserting the exit code alone would not have caught that; every row here +// asserts the exit code AND the user-visible output. +// +// Adding a prompt? Add a row. Both prompt doubles are shared, and the "did it act +// anyway" probe keeps a row honest: a printed note over a completed side effect +// would be a worse lie than silence. + +package cli + +import ( + "context" + "net/http" + "path/filepath" + "strings" + "testing" + + "github.com/tracebloc/cli/internal/ui" +) + +// runner drives one command to its prompt, using the injected prompt double. +type runner func(*ui.Printer, prompter) error + +// sideEffectProbe reports whether the command took its irreversible action +// despite the cancellation (a provision POST, an offboard revoke/teardown). +type sideEffectProbe func() bool + +// setUpClientCreate wires `tracebloc client create` against a fake backend that +// records whether a client was ever POSTed. No --yes and a prompter present, so +// the run reaches the "Provision this client?" confirm. +func setUpClientCreate(t *testing.T) (runner, sideEffectProbe) { + t.Helper() + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) // no existing clients on the account + }) + signInAs(t, "Lab", "lab@example.com") + return func(p *ui.Printer, pr prompter) error { + return runClientCreate(context.Background(), p, pr, clientCreateOpts{}) + }, func() bool { return posted } +} + +// setUpDelete wires `tracebloc delete` (offboard this machine) with a live +// client to remove and every teardown seam faked, so the run reaches the +// typed-client-name confirmation and any teardown step is recorded, not real. +func setUpDelete(t *testing.T) (runner, sideEffectProbe) { + t.Helper() + revoked := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { + revoked = true + } + _, _ = w.Write([]byte(`{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + return func(p *ui.Printer, pr prompter) error { + return runDelete(context.Background(), p, pr, deleteOpts{}) + }, func() bool { return revoked || len(fn.calls) > 0 } +} + +// TestPromptCancellation_IsVisibleAndCleanExit: for every prompt a user can back +// out of, the CLI prints why it stopped and exits 0 — and does not act. +// +// Exit 0 (not 130) is the convention: nothing was started, so there is no +// interrupted operation to report. exitInterrupted is reserved for a Ctrl-C that +// cuts short work already in flight — see exitcodes.go and cleanCancel. +func TestPromptCancellation_IsVisibleAndCleanExit(t *testing.T) { + // declineClientCreate answers "No" at the confirm (the Ctrl-C row's twin). + no := false + + cases := []struct { + name string + // how the user backed out, for failure messages. + how string + // setUp wires the command's world; pr is what the user "did". + setUp func(*testing.T) (runner, sideEffectProbe) + pr prompter + // wantOut is the line the user must see. The Ctrl-C and declined rows of + // one command share it wherever the reason is the same — `delete`'s + // mismatch row names the reason, which is more, never less. + wantOut string + }{ + { + name: "client create/ctrl-c at the confirm", + how: "Ctrl-C at \"Provision this client?\"", + setUp: setUpClientCreate, + pr: cancellingPrompter{}, + wantOut: "Cancelled — nothing was provisioned.", + }, + { + name: "client create/answered no at the confirm", + how: "answering \"No\" at \"Provision this client?\"", + setUp: setUpClientCreate, + pr: &fakePrompter{confirm: &no}, + wantOut: "Cancelled — nothing was provisioned.", + }, + { + name: "delete/ctrl-c while typing the name", + how: "Ctrl-C at the typed-name confirmation", + setUp: setUpDelete, + pr: cancellingPrompter{}, + wantOut: "Cancelled — nothing was removed.", + }, + { + name: "delete/typed a name that didn't match", + how: "typing the wrong client name", + setUp: setUpDelete, + pr: typedNamePrompter{reply: "wrong-name"}, + wantOut: "Cancelled — the name didn't match. Nothing was removed.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + run, sideEffected := tc.setUp(t) + out := &strings.Builder{} + err := run(ui.New(out, ui.WithColor(false)), tc.pr) + + if got := ExitCodeFromError(err); got != exitOK { + t.Errorf("exit code after %s = %d, want %d (backing out is a choice, not a failure): %v", + tc.how, got, exitOK, err) + } + if !strings.Contains(out.String(), tc.wantOut) { + t.Errorf("%s printed no cancellation note — a silent exit 0 is indistinguishable from success.\nwant a line containing: %q\ngot:\n%s", + tc.how, tc.wantOut, out.String()) + } + if sideEffected() { + t.Errorf("%s must not act: the command went ahead anyway", tc.how) + } + }) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index 72d936f2..cd662a41 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -12,7 +12,6 @@ import ( "path/filepath" "strconv" "strings" - "time" "github.com/spf13/cobra" @@ -334,12 +333,17 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien renderClientReview(p, name, namespace, location, clusterID) ok, cerr := pr.Confirm("Provision this client?", true) if cerr != nil { - return mapClientErr(cerr) + // Ctrl-C here is the same choice as answering "No" below, so it + // gets the same note and the same clean exit — never a silent + // exit 0 that a script reads as "provisioned" (backend#1253). + // Logged unconditionally: for a cancel the reason IS "cancelled + // by user", and a real terminal failure should say so too. + ilog.Logf("stopped at the confirm prompt: %v", cerr) + return mapPromptErr(p, cerr, "nothing was provisioned.") } if !ok { ilog.Logf("cancelled by user at the confirm prompt") - p.Hintf("Cancelled.") - return nil + return cleanCancel(p, "nothing was provisioned.") } } else if pr == nil && !opts.yes && opts.credentialFile == "" && !willAdopt { // Non-interactive with no way to confirm AND no --credential-file: a fresh @@ -758,184 +762,6 @@ func runClientList(ctx context.Context, p *ui.Printer) error { return nil } -func newClientStatusCmd() *cobra.Command { - var wait bool - var timeout time.Duration - cmd := &cobra.Command{ - Use: "status", - Short: "Show whether tracebloc can see this machine's client (online)", - Long: `Report tracebloc's view of this machine's active client — online, offline, -or pending. With --wait, poll until tracebloc reports it online (exit 0) or the -timeout elapses (non-zero), to confirm the client connected after setup.`, - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { - // --timeout only governs the --wait poll; accepting it alone would be a - // silent no-op, so reject it rather than mislead. - if cmd.Flags().Changed("timeout") && !wait { - return &exitError{code: exitFailure, err: errors.New("--timeout has no effect without --wait")} - } - return runClientStatus(cmd.Context(), printerFor(cmd), wait, timeout) - }, - } - cmd.Flags().BoolVar(&wait, "wait", false, "poll until tracebloc reports this client online") - cmd.Flags().DurationVar(&timeout, "timeout", 120*time.Second, "with --wait, give up after this long") - return cmd -} - -// clientStatusPollInterval is how often --wait re-checks the backend. A const, -// not a seam: tests inject through pollAfter (which ignores the duration and -// fires instantly), so the value never needs overriding. -const clientStatusPollInterval = 3 * time.Second - -func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time.Duration) error { - client, cfg, err := authedClient() - if err != nil { - return &exitError{code: exitFailure, err: err} - } - active := cfg.Current().ActiveClientID - if active == "" { - return &exitError{code: exitFailure, err: errors.New( - "no active client on this machine — run `tracebloc client create` (or re-run the installer) first")} - } - - // One-shot: report the current state and exit 0 (informational). - if !wait { - st, found, lerr := lookupClientStatus(ctx, client, active) - if lerr != nil { - return &exitError{code: exitFailure, err: lerr} - } - if !found { - return &exitError{code: exitFailure, err: fmt.Errorf( - "active client %s isn't in your account — run `tracebloc client create` "+ - "(or re-run the installer) to provision this machine", active)} - } - p.Section("Client status") - p.Field("state", clientStateLabel(st)) - return nil - } - - // --wait: poll the same source `client list` renders until online or timeout. - // The backend-reported status is the honest Online signal (RFC-0001 §8.5); a - // local rollout check can't tell whether tracebloc can actually see the client. - sp := p.Spinner("Waiting for tracebloc to confirm…", "") - defer sp.Stop() // leak-proof net for every return; the online path Stops explicitly before its ✔ - deadline := time.Now().Add(timeout) - var lastErr error // most recent transient (retryable) error, for the timeout message - lastState := -1 // most recent successfully-read status; -1 = none read yet - var ue *api.UpgradeRequiredError - var apiErr *api.APIError - for { - st, found, lerr := lookupClientStatus(ctx, client, active) - switch { - case errors.As(lerr, &ue): - // 426 (CLI too old) won't recover by waiting — surface the upgrade signal. - return &exitError{code: exitFailure, err: lerr} - case errors.As(lerr, &apiErr) && (apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden): - // A revoked/expired/forbidden token (401/403) won't recover by waiting — - // the client itself may be online. Fail fast, point at sign-in. Note 429 - // and 5xx stay transient (below): those DO recover on retry. - return &exitError{code: exitFailure, err: errors.New( - "tracebloc rejected your credentials while waiting — run `tracebloc login`, then retry")} - case lerr != nil: - lastErr = lerr // transient (5xx / 429 / network) — keep waiting, remember why - case !found: - // The active client isn't in the account (deleted / wrong account) — no - // amount of waiting surfaces it. Fail fast, matching the one-shot path. - return &exitError{code: exitFailure, err: fmt.Errorf( - "active client %s isn't in your account — run `tracebloc client create` "+ - "(or re-run the installer) to provision this machine", active)} - case st == clientStatusOnline: - sp.Stop() - p.Successf("tracebloc can see this client.") - return nil - default: - // A good poll (present, not online yet) supersedes any earlier transient - // error, so a later timeout reports the real state, not a stale failure. - lastErr, lastState = nil, st - } - - // --timeout caps how long we WAIT: stop once the budget is spent, and clamp - // the sleep to what remains so total runtime doesn't overshoot by a poll - // cycle. A poll begun within budget is still honored (online → ✔ above). - remaining := time.Until(deadline) - if remaining <= 0 { - switch { - case lastErr != nil: - return &exitError{code: exitFailure, err: fmt.Errorf( - "timed out after %s waiting for tracebloc to report this client online; "+ - "the last status check failed: %v", timeout, lastErr)} - case lastState >= 0: - return &exitError{code: exitFailure, err: fmt.Errorf( - "timed out after %s waiting for tracebloc to report this client online (last state: %s). "+ - "Run `tracebloc doctor` to diagnose, or re-run the installer.", timeout, clientStateLabel(lastState))} - default: - return &exitError{code: exitFailure, err: fmt.Errorf( - "timed out after %s before tracebloc could confirm this client — retry, "+ - "or run `tracebloc doctor`.", timeout)} - } - } - wait := clientStatusPollInterval - if wait > remaining { - wait = remaining - } - select { - case <-ctx.Done(): - return &exitError{code: exitInterrupted} // Ctrl-C: exit quietly (no "Error: context canceled") - case <-pollAfter(wait): - } - } -} - -// lookupClientStatus fetches the active client directly and returns its backend -// status code. found=false means no such client (deleted, or signed into the -// wrong account). A lookup error is returned verbatim so --wait can treat it as -// transient and retry. Fetches the single client by id (GET /edge-device/{id}/) -// rather than listing the whole account — the home-screen heartbeat runs this -// under a ~1.2s budget, and paging every client blew it (cli#338). -func lookupClientStatus(ctx context.Context, client *api.Client, active string) (status int, found bool, err error) { - id, err := strconv.Atoi(active) - if err != nil { - // A non-numeric active id can never match a backend client, so report it - // as not-found — exactly what the old ListClients+match path did — rather - // than a permanent error. A --wait loop fail-fasts on a missing client but - // treats errors as transient, so returning an error here would make it - // poll a permanent parse failure to the timeout (Bugbot: poll/retry loops - // must fail-fast on non-transient errors). - return 0, false, nil - } - c, err := client.GetClient(ctx, id) - if err != nil { - return 0, false, err - } - if c == nil { - return 0, false, nil // 404 — no such client - } - return c.Status, true, nil -} - -// EdgeDevice.status codes mirrored from the backend (metaApi User.py). -const ( - clientStatusOffline = 0 - clientStatusOnline = 1 - clientStatusPending = 2 -) - -// clientStateLabel maps the backend status code to a TTY/CI-safe word. Plain -// text (not an emoji glyph) on purpose — flag/emoji glyphs mojibake in CI logs -// and Windows consoles (RFC-0001 §12 watch-item). -func clientStateLabel(status int) string { - switch status { - case clientStatusOnline: - return "online" - case clientStatusOffline: - return "offline" - case clientStatusPending: - return "pending" - default: - return "unknown" - } -} - // setActiveClient points this env's profile at c, caching its namespace and // display name alongside the id so the data commands can bind to the active // client's cluster (§7.3) without a backend round-trip. Callers Save() after. @@ -1034,14 +860,6 @@ func emailLocalPart(email string) string { return email } -// mapClientErr turns a cancelled interactive prompt into a clean exit. -func mapClientErr(err error) error { - if errors.Is(err, errInteractiveCancelled) { - return nil - } - return &exitError{code: exitFailure, err: err} -} - // randHex returns nbytes of crypto-random data hex-encoded. func randHex(nbytes int) string { b := make([]byte, nbytes) diff --git a/internal/cli/client_status.go b/internal/cli/client_status.go new file mode 100644 index 00000000..4f1d5547 --- /dev/null +++ b/internal/cli/client_status.go @@ -0,0 +1,241 @@ +// `tracebloc client status` — tracebloc's view of this machine's client +// (online / offline / pending, with --wait to poll), plus the --seal mode that +// verifies the environment's protections via the chart's conformance checks +// (seal.go). Split out of client.go, which sits at its file-budget ceiling. + +package cli + +import ( + "context" + "errors" + "fmt" + "net/http" + "strconv" + "time" + + "github.com/spf13/cobra" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/ui" +) + +func newClientStatusCmd() *cobra.Command { + var wait, seal bool + var timeout time.Duration + var kubeconfigPath, contextOverride, nsOverride string + cmd := &cobra.Command{ + Use: "status", + Short: "Show whether tracebloc can see this machine's client (online)", + Long: `Report tracebloc's view of this machine's active client — online, offline, +or pending. With --wait, poll until tracebloc reports it online (exit 0) or the +timeout elapses (non-zero), to confirm the client connected after setup. + +With --seal, verify the environment's protections instead: run the chart's +conformance checks (its helm tests — the seal check, RFC-0003) against this +machine's secure environment and report the verdict with per-check detail: + + sealed every conformance check passed + unsealed a check failed or couldn't run — a protection is not enforced + unknown the chart ships no conformance checks, so nothing was verified + +Only a fully-passed suite exits 0; an environment that can't be verified is +never claimed sealed. + +Exit codes with --seal: + 0 sealed — every conformance check passed + 2 unsealed (a check failed), or unknown (the chart has no checks) + 3 kubeconfig could not be loaded / cluster unreachable + 4 cluster reachable but no tracebloc client found there`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + // --wait watches the backend's view; --seal interrogates the cluster. + // Two different questions — refuse the ambiguous combination. + if wait && seal { + return &exitError{code: exitFailure, err: errors.New("--wait and --seal are separate modes — run them one at a time")} + } + // --timeout governs the --wait poll and the per-check budget under + // --seal; accepting it alone would be a silent no-op, so reject it + // rather than mislead. Same for the cluster-targeting flags, which + // only the seal check (a cluster-side operation) consumes. + if cmd.Flags().Changed("timeout") && !wait && !seal { + return &exitError{code: exitFailure, err: errors.New("--timeout has no effect without --wait or --seal")} + } + for _, name := range []string{"kubeconfig", "context", "namespace"} { + if cmd.Flags().Changed(name) && !seal { + return &exitError{code: exitFailure, err: fmt.Errorf("--%s has no effect without --seal", name)} + } + } + if seal { + return runSealCheck(cmd.Context(), printerFor(cmd), + cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride}, + timeout) + } + return runClientStatus(cmd.Context(), printerFor(cmd), wait, timeout) + }, + } + cmd.Flags().BoolVar(&wait, "wait", false, "poll until tracebloc reports this client online") + cmd.Flags().DurationVar(&timeout, "timeout", 120*time.Second, + "with --wait, give up after this long; with --seal, the time budget per check") + cmd.Flags().BoolVar(&seal, "seal", false, + "verify this environment's protections: run the chart's conformance checks and report sealed / unsealed") + addKubeconfigFlags(cmd, &kubeconfigPath, &contextOverride, + "with --seal: "+kubeconfigFlagUsage, + "with --seal: "+contextFlagUsage) + addNamespaceFlag(cmd, &nsOverride, "with --seal: "+namespaceFlagUsage) + return cmd +} + +// clientStatusPollInterval is how often --wait re-checks the backend. A const, +// not a seam: tests inject through pollAfter (which ignores the duration and +// fires instantly), so the value never needs overriding. +const clientStatusPollInterval = 3 * time.Second + +func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time.Duration) error { + client, cfg, err := authedClient() + if err != nil { + return &exitError{code: exitFailure, err: err} + } + active := cfg.Current().ActiveClientID + if active == "" { + return &exitError{code: exitFailure, err: errors.New( + "no active client on this machine — run `tracebloc client create` (or re-run the installer) first")} + } + + // One-shot: report the current state and exit 0 (informational). + if !wait { + st, found, lerr := lookupClientStatus(ctx, client, active) + if lerr != nil { + return &exitError{code: exitFailure, err: lerr} + } + if !found { + return &exitError{code: exitFailure, err: fmt.Errorf( + "active client %s isn't in your account — run `tracebloc client create` "+ + "(or re-run the installer) to provision this machine", active)} + } + p.Section("Client status") + p.Field("state", clientStateLabel(st)) + return nil + } + + // --wait: poll the same source `client list` renders until online or timeout. + // The backend-reported status is the honest Online signal (RFC-0001 §8.5); a + // local rollout check can't tell whether tracebloc can actually see the client. + sp := p.Spinner("Waiting for tracebloc to confirm…", "") + defer sp.Stop() // leak-proof net for every return; the online path Stops explicitly before its ✔ + deadline := time.Now().Add(timeout) + var lastErr error // most recent transient (retryable) error, for the timeout message + lastState := -1 // most recent successfully-read status; -1 = none read yet + var ue *api.UpgradeRequiredError + var apiErr *api.APIError + for { + st, found, lerr := lookupClientStatus(ctx, client, active) + switch { + case errors.As(lerr, &ue): + // 426 (CLI too old) won't recover by waiting — surface the upgrade signal. + return &exitError{code: exitFailure, err: lerr} + case errors.As(lerr, &apiErr) && (apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden): + // A revoked/expired/forbidden token (401/403) won't recover by waiting — + // the client itself may be online. Fail fast, point at sign-in. Note 429 + // and 5xx stay transient (below): those DO recover on retry. + return &exitError{code: exitFailure, err: errors.New( + "tracebloc rejected your credentials while waiting — run `tracebloc login`, then retry")} + case lerr != nil: + lastErr = lerr // transient (5xx / 429 / network) — keep waiting, remember why + case !found: + // The active client isn't in the account (deleted / wrong account) — no + // amount of waiting surfaces it. Fail fast, matching the one-shot path. + return &exitError{code: exitFailure, err: fmt.Errorf( + "active client %s isn't in your account — run `tracebloc client create` "+ + "(or re-run the installer) to provision this machine", active)} + case st == clientStatusOnline: + sp.Stop() + p.Successf("tracebloc can see this client.") + return nil + default: + // A good poll (present, not online yet) supersedes any earlier transient + // error, so a later timeout reports the real state, not a stale failure. + lastErr, lastState = nil, st + } + + // --timeout caps how long we WAIT: stop once the budget is spent, and clamp + // the sleep to what remains so total runtime doesn't overshoot by a poll + // cycle. A poll begun within budget is still honored (online → ✔ above). + remaining := time.Until(deadline) + if remaining <= 0 { + switch { + case lastErr != nil: + return &exitError{code: exitFailure, err: fmt.Errorf( + "timed out after %s waiting for tracebloc to report this client online; "+ + "the last status check failed: %v", timeout, lastErr)} + case lastState >= 0: + return &exitError{code: exitFailure, err: fmt.Errorf( + "timed out after %s waiting for tracebloc to report this client online (last state: %s). "+ + "Run `tracebloc doctor` to diagnose, or re-run the installer.", timeout, clientStateLabel(lastState))} + default: + return &exitError{code: exitFailure, err: fmt.Errorf( + "timed out after %s before tracebloc could confirm this client — retry, "+ + "or run `tracebloc doctor`.", timeout)} + } + } + wait := clientStatusPollInterval + if wait > remaining { + wait = remaining + } + select { + case <-ctx.Done(): + return &exitError{code: exitInterrupted} // Ctrl-C: exit quietly (no "Error: context canceled") + case <-pollAfter(wait): + } + } +} + +// lookupClientStatus fetches the active client directly and returns its backend +// status code. found=false means no such client (deleted, or signed into the +// wrong account). A lookup error is returned verbatim so --wait can treat it as +// transient and retry. Fetches the single client by id (GET /edge-device/{id}/) +// rather than listing the whole account — the home-screen heartbeat runs this +// under a ~1.2s budget, and paging every client blew it (cli#338). +func lookupClientStatus(ctx context.Context, client *api.Client, active string) (status int, found bool, err error) { + id, err := strconv.Atoi(active) + if err != nil { + // A non-numeric active id can never match a backend client, so report it + // as not-found — exactly what the old ListClients+match path did — rather + // than a permanent error. A --wait loop fail-fasts on a missing client but + // treats errors as transient, so returning an error here would make it + // poll a permanent parse failure to the timeout (Bugbot: poll/retry loops + // must fail-fast on non-transient errors). + return 0, false, nil + } + c, err := client.GetClient(ctx, id) + if err != nil { + return 0, false, err + } + if c == nil { + return 0, false, nil // 404 — no such client + } + return c.Status, true, nil +} + +// EdgeDevice.status codes mirrored from the backend (metaApi User.py). +const ( + clientStatusOffline = 0 + clientStatusOnline = 1 + clientStatusPending = 2 +) + +// clientStateLabel maps the backend status code to a TTY/CI-safe word. Plain +// text (not an emoji glyph) on purpose — flag/emoji glyphs mojibake in CI logs +// and Windows consoles (RFC-0001 §12 watch-item). +func clientStateLabel(status int) string { + switch status { + case clientStatusOnline: + return "online" + case clientStatusOffline: + return "offline" + case clientStatusPending: + return "pending" + default: + return "unknown" + } +} diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index 433fabde..37560f17 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -325,11 +325,29 @@ func TestCopyCatalog(t *testing.T) { ) // ── 08 client ────────────────────────────────────────────────────────────────── + // The seal check (client status --seal) renders through the pure + // renderSealResult, so all three verdicts are pinned as stable screens: the + // live run streams the same header/check/verdict pieces (plus a per-check + // spinner line, catalogued in the backstop). + sealSealed := sealModel{envName: "lukas-macbook", checks: []sealCheck{ + {name: "egress-enforcement", passed: true}, + {name: "backend-reachability", passed: true}, + }} + sealUnsealed := sealModel{envName: "lukas-macbook", checks: []sealCheck{ + {name: "egress-enforcement", passed: false, + detail: "job failed: BackoffLimitExceeded", + hint: "see why: kubectl logs -n lukas-macbook job/lukas-macbook-egress-enforcement-check"}, + {name: "backend-reachability", passed: true}, + }} + sealUnknown := sealModel{envName: "lukas-macbook"} clientFile := doc( "tb client — register / list / inspect environments", - "What you see under `tb client`. `tb client create` shows a review (below) before\nit registers a new secure environment. `tb client list` / `tb client status` read\nlive backend state, so they aren't stable screens — their strings are in\nzz-all-strings.golden.", + "What you see under `tb client`. `tb client create` shows a review (below) before\nit registers a new secure environment. `tb client status --seal` verifies the\nenvironment's protections by running the chart's conformance checks (the seal\ncheck) — all three verdicts are shown: sealed, unsealed (with the per-check\nfailure + fix hint), and unknown (a chart with no checks is honestly NOT called\nsealed). `tb client list` / plain `tb client status` read live backend state, so\nthey aren't stable screens — their strings are in zz-all-strings.golden.", []run{ {"tb client create # review, before you confirm", rndr(func(p *ui.Printer) { renderClientReview(p, "lukas-macbook", "lukas-macbook", "DE", "a1b2c3d4") })}, + {"tb client status --seal # sealed — every conformance check passed", rndr(func(p *ui.Printer) { renderSealResult(p, sealSealed) })}, + {"tb client status --seal # unsealed — a protection is not enforced", rndr(func(p *ui.Printer) { renderSealResult(p, sealUnsealed) })}, + {"tb client status --seal # unknown — this chart ships no conformance checks", rndr(func(p *ui.Printer) { renderSealResult(p, sealUnknown) })}, }, []run{ {"tracebloc client --help", help("client")}, @@ -358,18 +376,35 @@ func TestCopyCatalog(t *testing.T) { []run{{"tracebloc version --help", help("version")}}, ) + upgradeFile := doc( + "tb upgrade — update to the latest release", + "What you see when you run `tb upgrade`. On Linux/macOS it re-runs the\nofficial installer (signature-verified) to update the CLI and your secure\nenvironment together, so they never drift apart. On Windows a running .exe is\nlocked and install.ps1 is CLI-only, so it prints the command to run in a fresh\nshell instead. The update-check nudge and the \"CLI too old\" (426) message both\npoint here. The installer's own live output streams during the run (not a stable\nscreen); its copy is in the client repo's installer catalog.", + nil, + []run{{"tracebloc upgrade --help", help("upgrade")}}, + ) + + // ── 12 prepare-host ────────────────────────────────────────────────────────── + prepareHostFile := doc( + "tb prepare-host — one-time admin step so a non-admin can install", + "What you see when you run `tb prepare-host` — the one-time administrator step\nthat readies a shared / HPC host so a non-admin user can then install tracebloc\nwith no root. It re-runs the installer's verified prepare-host step; the\nprivileged prep + its progress stream from the installer (not CLI copy). Only the\n--help is byte-exact below.", + nil, + []run{{"tracebloc prepare-host --help", help("prepare-host")}}, + ) + files := map[string]string{ - "00-home.golden": homeFile, - "01-data-ingest.golden": dataIngestFile, - "02-data-list.golden": dataListFile, - "03-data-delete.golden": dataDeleteFile, - "04-resources.golden": resourcesFile, - "05-doctor.golden": doctorFile, - "06-delete.golden": deleteFile, - "07-login.golden": loginFile, - "08-client.golden": clientFile, - "09-cluster.golden": clusterFile, - "10-version.golden": versionFile, + "00-home.golden": homeFile, + "01-data-ingest.golden": dataIngestFile, + "02-data-list.golden": dataListFile, + "03-data-delete.golden": dataDeleteFile, + "04-resources.golden": resourcesFile, + "05-doctor.golden": doctorFile, + "06-delete.golden": deleteFile, + "07-login.golden": loginFile, + "08-client.golden": clientFile, + "09-cluster.golden": clusterFile, + "10-version.golden": versionFile, + "11-upgrade.golden": upgradeFile, + "12-prepare-host.golden": prepareHostFile, "zz-all-strings.golden": "every user-facing string in the source (AST-harvested — all arguments to the\n" + "Printer methods + errors.New/fmt.Errorf/fmt.Sprintf, plus the text/remedy\n" + "fields of healthLine{} and doctor.Result{} literals, both \"…\" and `…` raw\n" + @@ -393,6 +428,11 @@ func TestCopyCatalog(t *testing.T) { }, "02-data-list.golden": {"tracebloc data list --help"}, "05-doctor.golden": {"Connected to tracebloc", "Everything looks good"}, + "08-client.golden": { // the seal check's three verdicts (cli#393) + "Sealed — all 2 conformance checks passed", + "Unsealed — 1 of 2 conformance checks failed", + "Seal unknown — this chart ships no conformance checks", + }, } for name, needles := range mustRender { got := files[name] @@ -498,12 +538,24 @@ func (c *catalogPrompter) Confirm(label string, def bool) (bool, error) { // Both "…" and `…` raw strings; deduped + sorted. func harvestMessages(t *testing.T) []string { t.Helper() + // Package-local helpers that print through the Printer on the caller's + // behalf. Their note argument is user-facing copy no Printer-argument scan + // would see, and the helper supplies the sentence's opening — so harvest the + // ASSEMBLED line the user reads, not the bare clause (backend#1253). + copyHelperPrefix := map[string]string{ + "cleanCancel": "Cancelled — ", + "mapPromptErr": "Cancelled — ", + } methods := map[string]bool{ "Successf": true, "Warnf": true, "Errorf": true, "Infof": true, "Hintf": true, "Detailf": true, "Para": true, "Section": true, "PromptHint": true, "PromptHeader": true, "PromptStep": true, "WarnLine": true, "CrossLine": true, "CheckLine": true, "Step": true, "Action": true, "Stat": true, "Field": true, "MenuRow": true, "Banner": true, "Command": true, + // Spinner — live-wait lines ("Waiting for tracebloc to confirm…", + // "Checking …") print as a static line on non-TTY runs, so their + // copy is user-facing too. + "Spinner": true, // prompter seam (survey) — question labels + help text for every guided // flow (ingest, client create, delete), incl. flows not driven as a screen. "Input": true, "Select": true, "Confirm": true, @@ -536,7 +588,7 @@ func harvestMessages(t *testing.T) []string { } seen := map[string]struct{}{} - collect := func(exprs []ast.Expr) { + collect := func(prefix string, exprs []ast.Expr) { for _, arg := range exprs { lit, ok := arg.(*ast.BasicLit) if !ok || lit.Kind != token.STRING { @@ -551,7 +603,7 @@ func harvestMessages(t *testing.T) []string { if len([]rune(s)) < 4 || !strings.ContainsAny(s, "abcdefghijklmnopqrstuvwxyz") { continue } - seen[s] = struct{}{} + seen[prefix+s] = struct{}{} } } fset := token.NewFileSet() @@ -567,16 +619,21 @@ func harvestMessages(t *testing.T) []string { ast.Inspect(f, func(n ast.Node) bool { switch node := n.(type) { case *ast.CallExpr: + if id, ok := node.Fun.(*ast.Ident); ok { + if prefix, isHelper := copyHelperPrefix[id.Name]; isHelper { + collect(prefix, node.Args) + } + } if isCopyCall(node) { - collect(node.Args) + collect("", node.Args) } case *ast.CompositeLit: if isCopyStruct(node.Type) { for _, el := range node.Elts { if kv, ok := el.(*ast.KeyValueExpr); ok { - collect([]ast.Expr{kv.Value}) + collect("", []ast.Expr{kv.Value}) } else { - collect([]ast.Expr{el}) + collect("", []ast.Expr{el}) } } } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 6cf6049b..7b1e6d38 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -229,25 +229,27 @@ undone — re-ingesting the data is the only way back.`) "refusing to delete without confirmation: pass --yes or run on a terminal")} } p.PromptHint("This drops the table and removes the files listed above — there's no undo. Pass --yes next time to skip this prompt.") + // Both ways out of this prompt — Ctrl-C and an explicit "no" — report the + // same outcome: the shared cancellation note, one "declined" JSON object, + // exit 0. One closure so the pair can't drift apart. + declined := func() error { + if a.OutputJSON { + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) + jsonEmitted = true + } + return cleanCancel(p, "nothing was deleted.") + } ok, err := a.Prompter.Confirm(fmt.Sprintf("Delete %q and its files?", matched), false) if err != nil { + // Not mapPromptErr: a prompt that genuinely fails here is a + // local-environment problem (exit 3), not the generic exit 1. if errors.Is(err, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was deleted.") - if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) - jsonEmitted = true - } - return nil + return declined() } return &exitError{code: exitLocalEnv, err: err} } if !ok { - p.Infof("Cancelled — nothing was deleted.") - if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) - jsonEmitted = true - } - return nil + return declined() } } diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index 48a96649..4a82987c 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -88,8 +88,11 @@ func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clu return nil, "", false, aerr } if !proceed { - a.Printer.Infof("Cancelled — %q was left as-is; nothing was ingested.", existingTable) - return nil, "", true, nil + // Reached by both a declined replace and a Ctrl-C at that prompt + // (existingTableAction folds them into proceed=false), so one note + // covers both — via the shared cleanCancel. + return nil, "", true, cleanCancel(a.Printer, + "%q was left as-is; nothing was ingested.", existingTable) } a.Overwrite = true } diff --git a/internal/cli/data_ingest_local.go b/internal/cli/data_ingest_local.go index a4a394a5..a28436b4 100644 --- a/internal/cli/data_ingest_local.go +++ b/internal/cli/data_ingest_local.go @@ -102,8 +102,8 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus if a.Interactive && a.Prompter != nil { if err := runInteractive(a.Printer, a.Prompter, a, a.TaskSet); err != nil { if errors.Is(err, errInteractiveCancelled) { - a.Printer.Infof("Cancelled — nothing was ingested.") - return nil, nil, nil, true, nil + // cleanCancel prints the shared note and returns the clean exit. + return nil, nil, nil, true, cleanCancel(a.Printer, "nothing was ingested.") } // A typed exitError from a guided step (e.g. the path-existence // guard, which runInteractive runs before the family sniff) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index a88e2131..c2b3b163 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -31,6 +31,7 @@ var ( var ( osExecutable = os.Executable osRemoveAll = os.RemoveAll + osStat = os.Stat ) // deleteOpts bundles the `tracebloc delete` flags. @@ -43,6 +44,9 @@ type deleteOpts struct { namespace string } +// deleteCmdName is the top-level offboard command's name. +const deleteCmdName = "delete" + // newDeleteCmd wires the TOP-LEVEL `tracebloc delete` — offboarding this machine // (RFC-0001 §7.10). It is deliberately NOT under `client` and NOT // `client delete --uninstall`: on the single-machine CLI the host owns exactly @@ -63,8 +67,15 @@ type deleteOpts struct { func newDeleteCmd() *cobra.Command { var o deleteOpts cmd := &cobra.Command{ - Use: "delete", - Short: "Offboard this machine from tracebloc (revoke, uninstall, reclaim disk)", + Use: deleteCmdName, + // Suppress the post-command update nudge/cache-write: offboarding removes + // the CLI and (on the default path) wipes ~/.tracebloc, so nudging to + // upgrade a just-removed binary is nonsensical and the cache write would + // resurrect the just-wiped data dir. The annotation (not a name match) means + // `data delete` is unaffected — only this top-level command opts in + // (Bugbot #397, #404). See SkipUpdateNudge. + Annotations: map[string]string{skipUpdateNudgeAnnotation: "true"}, + Short: "Offboard this machine from tracebloc (revoke, uninstall, reclaim disk)", Long: `Removes tracebloc from this machine: revokes the machine credential, uninstalls the Helm release, deletes the local cluster, reclaims the tracebloc container images, and clears local state — then removes the CLI itself. @@ -184,11 +195,14 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er p.PromptHint("This is irreversible. Type the client name to confirm, or leave blank to cancel.") typed, perr := pr.Input(fmt.Sprintf("Type %q to offboard this machine", name), "", "", nil) if perr != nil { - return mapClientErr(perr) + // Ctrl-C mid-typing is the same "no" the mismatch branch below + // handles — same visible note, same clean exit. It used to return + // nil unprinted, so an aborted offboard looked exactly like a + // completed one to anything reading the stream (backend#1253). + return mapPromptErr(p, perr, "nothing was removed.") } if strings.TrimSpace(typed) != name { - p.Infof("Cancelled — the name didn't match. Nothing was removed.") - return nil + return cleanCancel(p, "the name didn't match. Nothing was removed.") } } @@ -382,7 +396,19 @@ func removeHostDataDir() error { if err != nil { return err } - return osRemoveAll(dir) + if err := osRemoveAll(dir); err != nil { + return err + } + // Verify the directory is actually gone before the caller prints "✔ Removed". + // A nil RemoveAll is not proof the tree is absent — a racing writer, a mount, + // or a masked partial failure can leave it present — and claiming a clean wipe + // we didn't achieve is exactly the offboard-hygiene gap RFC-0003 flags. + if _, statErr := osStat(dir); statErr == nil { + return fmt.Errorf("%s still present after removal", dir) + } else if !errors.Is(statErr, os.ErrNotExist) { + return fmt.Errorf("verifying removal of %s: %w", dir, statErr) + } + return nil } // hostDataDirDisplay is the data dir for a user-facing hint; falls back to the diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index 9b9948a0..48ec1dae 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -39,6 +39,7 @@ type fakeNodeboot struct { pruneErr error removedPaths []string removeErr map[string]error // path → error to return from osRemoveAll + statPresent bool // when true, osStat reports the data dir still exists after rm executable string executableErr error // Kubeconfig/context the uninstall seam was handed — so a test can prove the @@ -50,7 +51,7 @@ type fakeNodeboot struct { func (f *fakeNodeboot) install(t *testing.T) { t.Helper() origU, origT, origP := uninstallChart, teardownCluster, pruneImages - origExe, origRm := osExecutable, osRemoveAll + origExe, origRm, origStat := osExecutable, osRemoveAll, osStat uninstallChart = func(_ context.Context, ns, kubeconfig, kubeContext string) error { f.calls = append(f.calls, "uninstall:"+ns) f.uninstallKubeconfig, f.uninstallContext = kubeconfig, kubeContext @@ -78,9 +79,19 @@ func (f *fakeNodeboot) install(t *testing.T) { } return nil } + // osStat backs removeHostDataDir's verify-before-success. Default: the data dir + // is gone (ErrNotExist) so a clean wipe reports success. statPresent flips it to + // "still there" so a test can prove the CLI does NOT claim ✔ on an unverified wipe. + osStat = func(path string) (os.FileInfo, error) { + f.calls = append(f.calls, "stat:"+path) + if f.statPresent { + return nil, nil + } + return nil, os.ErrNotExist + } t.Cleanup(func() { uninstallChart, teardownCluster, pruneImages = origU, origT, origP - osExecutable, osRemoveAll = origExe, origRm + osExecutable, osRemoveAll, osStat = origExe, origRm, origStat }) } @@ -402,6 +413,36 @@ func TestDelete_WipeFails_StillClearsPointer(t *testing.T) { } } +// A nil RemoveAll is not proof the wipe happened: if the data dir is still present +// afterwards, delete must NOT print "✔ Removed" — it must warn and flag degraded, +// so offboard never claims a clean slate it didn't achieve (RFC-0003). +func TestDelete_WipeUnverified_DoesNotClaimSuccess(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): + w.WriteHeader(http.StatusOK) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + exe := filepath.Join(t.TempDir(), "tracebloc") + // osRemoveAll returns nil (no removeErr), but osStat reports the dir still present. + fn := &fakeNodeboot{executable: exe, statPresent: true} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("offboard: %v", err) + } + if strings.Contains(out.String(), "Removed local tracebloc data and config") { + t.Errorf("must NOT claim a clean wipe when the dir is still present:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Couldn't remove local data") { + t.Errorf("expected the unverified-wipe warning, got:\n%s", out.String()) + } +} + // When a teardown step leaves real state behind, the closing line must NOT claim a // clean offboard — it should flag that some cleanup didn't complete. func TestDelete_TeardownFailure_HonestClosing(t *testing.T) { diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 328041b3..3fd64411 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "os" + "runtime" "strings" "time" @@ -19,10 +20,16 @@ import ( "github.com/tracebloc/cli/internal/ui" ) +// installerURL is the single source of truth for the installer script URL. +// Everything that downloads or points at the installer (installCmd here, +// prepareHostInstallerCmd in prepare_host.go) derives from this so a URL change +// updates every path at once. +const installerURL = "https://tracebloc.io/i.sh" + // installCmd is the one-line installer we point people at when there's no // secure environment on this machine, or a component needs reinstalling. Kept in // one place so every remedy says the same thing. -const installCmd = "bash <(curl -fsSL https://tracebloc.io/i.sh)" +const installCmd = "bash <(curl -fsSL " + installerURL + ")" // doctorRunFn is a test seam over doctor.Run (the cluster-side probe). Tests // inject a fixed []doctor.Result so the roll-up + render can be exercised with a @@ -291,6 +298,24 @@ func tokenLabel(tok tokenState) string { // stays in --verbose. When the owner isn't connected — for ANY reason — a // healthy local cluster still can't run training, so readiness degrades honestly // to "can't check" rather than showing a reassuring ✔ next to a Connected ✖. +// computeRemedy is the "not enough compute" remedy for the host OS (#400). On +// Windows the default Docker backend is WSL2, where Docker Desktop has NO +// Resources→Memory slider — the VM's memory comes from %UserProfile%\.wslconfig. +// macOS keeps the slider; bare Linux has no Docker Desktop at all. Every +// variant ends with the drift fix: size runs to the machine (backend#1236's +// install-time auto-size can go stale when a machine shrinks). +func computeRemedy(goos string) string { + resize := fmt.Sprintf("size runs to this machine with `%s resources set max`", launcher()) + switch goos { + case "windows": + return fmt.Sprintf("Free some up, give Docker more memory (WSL2 backend: `[wsl2] memory=…` in %%UserProfile%%\\.wslconfig then `wsl --shutdown`; Hyper-V backend: Docker Desktop → Settings → Resources → Advanced), or %s.", resize) + case "darwin": + return fmt.Sprintf("Free some up, raise the machine's allocation in Docker Desktop → Settings → Resources → Advanced, or %s.", resize) + default: + return fmt.Sprintf("Free some up on this machine, or %s.", resize) + } +} + func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready healthLine) { by := make(map[string]doctor.Result, len(results)) for _, r := range results { @@ -364,7 +389,7 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // the "Everything looks good" verdict (Bugbot). ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment can't start yet.", - fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. Free some up in Docker Desktop → Resources, then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", launcher(), launcher())} + fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. %s Then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", computeRemedy(runtime.GOOS), launcher(), launcher())} case by["Image pull secret"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — the training images can't be pulled.", @@ -388,7 +413,7 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready case by["Node capacity"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — not enough free compute to start a training.", - "Free some up, or raise the machine's allocation in Docker Desktop → Resources."} + computeRemedy(runtime.GOOS)} default: ready = healthLine{doctor.StatusOK, "Ready to run training", ""} } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 7eaffae7..0712bdba 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -311,8 +311,33 @@ func TestSummarizeDoctor(t *testing.T) { t.Run("no compute → ready Fail", func(t *testing.T) { _, r := summarizeDoctor(with(allOK, "Node capacity", doctor.StatusFail), tokenOK) - if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "Docker Desktop") { - t.Errorf("want ready Fail with a raise-allocation remedy, got %v remedy=%q", r.status, r.remedy) + // GOOS-independent invariant: every variant carries the resize fix + // (#400 — the old string pinned "Docker Desktop", wrong on WSL2/linux). + if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "resources set max") { + t.Errorf("want ready Fail with a resize remedy, got %v remedy=%q", r.status, r.remedy) + } + }) + + // computeRemedy (#400): the compute remedy must match the host's actual + // memory lever — Docker Desktop's Resources slider does not exist on the + // WSL2 backend, and bare Linux has no Docker Desktop at all. + t.Run("computeRemedy per GOOS", func(t *testing.T) { + win := computeRemedy("windows") + if !strings.Contains(win, ".wslconfig") || !strings.Contains(win, "Hyper-V") { + t.Fatalf("windows remedy must name both levers: %q", win) + } + mac := computeRemedy("darwin") + if !strings.Contains(mac, "Docker Desktop → Settings → Resources → Advanced") { + t.Fatalf("darwin remedy keeps the slider: %q", mac) + } + lin := computeRemedy("linux") + if strings.Contains(lin, "Docker Desktop") || strings.Contains(lin, "wslconfig") { + t.Fatalf("linux remedy must not name Docker Desktop/WSL: %q", lin) + } + for _, r := range []string{win, mac, lin} { + if !strings.Contains(r, "resources set max") { + t.Fatalf("every remedy carries the resize fix: %q", r) + } } }) diff --git a/internal/cli/exitcodes.go b/internal/cli/exitcodes.go index ba6db4fa..e5a2191c 100644 --- a/internal/cli/exitcodes.go +++ b/internal/cli/exitcodes.go @@ -17,8 +17,10 @@ package cli // constant per MEANING sharing the value, so each construction site stays // honest and the docs table maps number → per-command meaning. const ( - // exitOK: success. Includes --dry-run completing, a guided run the - // user cancelled cleanly, and doctor passing with warnings only. + // exitOK: success. Includes --dry-run completing, any prompt the user + // declined or cancelled with Ctrl-C (always with a visible "Cancelled — + // …" note — see cleanCancel in interactive.go), and doctor passing with + // warnings only. exitOK = 0 // exitFailure: generic failure with no more specific bucket (cobra @@ -83,8 +85,16 @@ const ( // couldn't be determined / followed within the watch window. exitIngestFailed = 9 - // exitInterrupted: the user hit Ctrl-C at an interactive prompt - // (128+SIGINT, the shell convention). Emitted silent (err == nil) so - // main() prints no "Error:" line on the way out. + // exitInterrupted: the user hit Ctrl-C while an operation was already in + // flight — the sign-in wait, `client status --wait`, the seal suite, or an + // installer re-run (upgrade / prepare-host) — 128+SIGINT, the shell + // convention. Emitted silent (err == nil) so main() prints no "Error:" + // line on the way out. + // + // NOT for a cancelled PROMPT. Backing out at a question starts nothing, so + // every prompt site reports that through cleanCancel instead: a visible + // "Cancelled — …" note and exitOK (interactive.go, backend#1253). This + // comment used to claim the prompt case, contradicting exitOK above and + // every call site. exitInterrupted = 130 ) diff --git a/internal/cli/home.go b/internal/cli/home.go index d4be98c2..b39de5e7 100644 --- a/internal/cli/home.go +++ b/internal/cli/home.go @@ -489,7 +489,11 @@ func realProbeEnv(ctx context.Context) envProbe { // the remembered-name fallback) — and skip the cluster I/O entirely, which // also keeps the common unprovisioned re-entry instant. if !binding.applied { - return envProbe{local: localNoRelease} + // #401: an empty pointer isn't proof of "no environment" — the Windows + // installer never writes it. localEnvFallback adopts a release only on + // a LOCAL (loopback/k3d) cluster, so the shared-cluster guarantee above + // is preserved; everything else still reads as no-release. + return localEnvFallback(ctx) } resolved, err := loadClusterFn(opts) if err != nil { @@ -592,25 +596,25 @@ func machineCapacity(ctx context.Context, cs kubernetes.Interface) (computeInfo, return sumCapacity(nodes.Items) } -// sumCapacity is the pure summation behind machineCapacity — split out so the -// rounding + GPU logic is unit-testable without a clientset. +// sumCapacity picks the LARGEST Ready node (CPU-major, like +// resources.LargestReadyNode) — never a sum: k3d's server+agent are the same +// physical machine, and summing double-counted it (#399). func sumCapacity(nodes []corev1.Node) (computeInfo, bool) { var cpuMilli, memBytes, gpu int64 - ready := 0 + found := false for i := range nodes { - n := nodes[i] - if !nodeReady(n) { + if !nodeReady(nodes[i]) { continue } - ready++ - alloc := n.Status.Allocatable - cpuMilli += alloc.Cpu().MilliValue() - memBytes += alloc.Memory().Value() - if q, ok := alloc[gpuResource]; ok { - gpu += q.Value() + alloc := nodes[i].Status.Allocatable + cm, mb := alloc.Cpu().MilliValue(), alloc.Memory().Value() + if found && (cm < cpuMilli || (cm == cpuMilli && mb <= memBytes)) { + continue } + q := alloc[gpuResource] // zero Quantity when absent -> gpu 0 + found, cpuMilli, memBytes, gpu = true, cm, mb, q.Value() } - if ready == 0 { + if !found { return computeInfo{}, false } return computeInfo{ @@ -645,22 +649,8 @@ func sanitizeInvoked(argv0 string) string { return binTracebloc } -// tbAliasAvailable reports whether a real tracebloc-owned `tb` alias sits next to -// this binary (the installer symlinks it there, cli#142). Reuses delete.go's -// aliasStatus so "is it ours" is judged exactly as offboarding judges it — we -// only advertise `tb` when it genuinely points at this CLI. -func tbAliasAvailable() bool { - exe, err := osExecutable() - if err != nil { - return false - } - tb := filepath.Join(filepath.Dir(exe), binTB) - if tb == exe { - return false - } - _, ours := aliasStatus(tb, exe) - return ours -} +// tbAliasAvailable moved to home_local_fallback.go (#401): it now also accepts +// the Windows tb.cmd shim, which the symlink-only test could never match. // ── Rendering (pure) ── diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go new file mode 100644 index 00000000..405660fc --- /dev/null +++ b/internal/cli/home_local_fallback.go @@ -0,0 +1,118 @@ +package cli + +// Home-screen fallbacks for machines the provisioning pointer never reached +// (#401). Split from home.go to respect its file budget. + +import ( + "context" + "net" + "net/url" + "os" + "path/filepath" + "strings" + + "github.com/tracebloc/cli/internal/cluster" +) + +// localEnvFallback answers "is there a secure environment on THIS machine?" +// when the active-client pointer is empty — the state every pre-#388 Windows +// install is in permanently, because only `client create` writes the pointer +// and the Windows installer never ran it. Field case: `doctor` said "Ready to +// run training" while home said "No secure environment on this machine yet". +// +// The ownership gate in realProbeEnv exists so a status screen never greets a +// SHARED cluster's unrelated client as yours (§7.5). This fallback keeps that +// guarantee by adopting a discovered release only when the kubeconfig's server +// is LOCAL (loopback / host.docker.internal / a k3d wildcard bind) — a cluster +// that is this machine by definition, so whatever tracebloc release runs there +// is this machine's environment. Remote/shared clusters return the same honest +// no-release the gate always produced, and every error degrades to no-release +// (never "offline" — an unprovisioned machine with no reachable local cluster +// most likely has no environment at all). +func localEnvFallback(ctx context.Context) envProbe { + resolved, err := loadClusterFn(cluster.KubeconfigOptions{}) + if err != nil { + return envProbe{local: localNoRelease} + } + if !isLocalServerURL(resolved.ServerURL) { + return envProbe{local: localNoRelease} + } + resolved.RestConfig.Timeout = homeProbeTimeout + cs, err := newClientsetFn(resolved) + if err != nil { + return envProbe{local: localNoRelease} + } + // Namespace-only discovery — never the cluster-wide scan, mirroring the + // gate's no-silent-retarget rule even on a local cluster. + release, nsUsed, err := discoverRelease(ctx, nil, cs, resolved.Namespace, false) + if err != nil { + return envProbe{local: localNoRelease} + } + ep := envProbe{name: release.ReleaseName} + if jobsManagerReady(ctx, cs, nsUsed, release) { + ep.local = localLive + } else { + ep.local = localDegraded + } + if ep.local == localLive { + if c, ok := machineCapacity(ctx, cs); ok { + ep.compute, ep.hasCompute = c, true + } + } + return ep +} + +// isLocalServerURL reports whether a kubeconfig server URL points at THIS +// machine. Covers loopback names/addresses, the wildcard binds k3d writes when +// no host is pinned, and Docker Desktop's host alias (the same signals +// doctor's reachability remedy keys on). +func isLocalServerURL(serverURL string) bool { + u, err := url.Parse(serverURL) + if err != nil { + return false + } + host := u.Hostname() + switch strings.ToLower(host) { + case "localhost", "host.docker.internal", "0.0.0.0", "::": + return true + } + if ip := net.ParseIP(host); ip != nil { + return ip.IsLoopback() || ip.IsUnspecified() + } + return false +} + +// tbAliasAvailable reports whether a real tracebloc-owned `tb` alias sits next +// to this binary, so examples/remedies can echo the short name. On unix the +// installer symlinks `tb` → tracebloc and delete.go's aliasStatus judges +// ownership exactly as offboarding does. On Windows symlinks need admin, so +// install.ps1 writes a `tb.cmd` shim instead — a regular file the symlink test +// can never accept (#401): ours = the shim invokes this binary. +func tbAliasAvailable() bool { + exe, err := osExecutable() + if err != nil { + return false + } + dir := filepath.Dir(exe) + tb := filepath.Join(dir, binTB) + if tb != exe { + if _, ours := aliasStatus(tb, exe); ours { + return true + } + } + return tbCmdAliasOurs(dir, exe) +} + +// tbCmdAliasOurs reports whether a tb.cmd shim in dir invokes THIS binary by +// its full path (install.ps1 writes an absolute target — the same ownership +// bar aliasStatus applies to symlinks). Matching just the basename would claim +// any third-party shim that merely mentions "tracebloc", or one invoking a +// different tracebloc at another path (Bugbot). Case-insensitive: .cmd is a +// Windows artifact and NTFS paths are case-insensitive. +func tbCmdAliasOurs(dir, exe string) bool { + b, err := os.ReadFile(filepath.Join(dir, binTB+".cmd")) + if err != nil { + return false + } + return strings.Contains(strings.ToLower(string(b)), strings.ToLower(filepath.Clean(exe))) +} diff --git a/internal/cli/home_local_fallback_test.go b/internal/cli/home_local_fallback_test.go new file mode 100644 index 00000000..b8e38949 --- /dev/null +++ b/internal/cli/home_local_fallback_test.go @@ -0,0 +1,158 @@ +package cli + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" + + "github.com/tracebloc/cli/internal/cluster" +) + +// fallbackRelease seeds the objects DiscoverParentRelease keys on (mirrors +// internal/cluster's discovery fixtures) plus a Ready jobs-manager so the +// probe classifies localLive. +func fallbackRelease(ns string) []interface{} { + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tracebloc-jobs-manager", + Namespace: ns, + Labels: map[string]string{ + "app.kubernetes.io/name": "client", + "app.kubernetes.io/instance": "tracebloc", + "app.kubernetes.io/managed-by": "Helm", + "app.kubernetes.io/version": "1.9.5", + "helm.sh/chart": "client-1.9.5", + }, + }, + Status: appsv1.DeploymentStatus{ReadyReplicas: 1, Replicas: 1}, + } + svc := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: "jobs-manager", Namespace: ns}} + return []interface{}{dep, svc} +} + +func stubFallbackSeams(t *testing.T, serverURL string, cs kubernetes.Interface, loadErr error) { + t.Helper() + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + if loadErr != nil { + return nil, loadErr + } + return &cluster.ResolvedConfig{ + Namespace: "tracebloc", + ServerURL: serverURL, + RestConfig: &rest.Config{Host: serverURL}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + if cs == nil { + return nil, errors.New("no clientset expected on this path") + } + return cs, nil + } +} + +// #401: an empty active-client pointer must not read as "no environment" when +// a tracebloc release runs on a LOCAL cluster (the pre-#388 Windows install +// state: doctor said Ready, home said run-the-installer). +func TestLocalEnvFallback_AdoptsLocalRelease(t *testing.T) { + o := fallbackRelease("tracebloc") + cs := fake.NewClientset(o[0].(*appsv1.Deployment), o[1].(*corev1.Service)) + stubFallbackSeams(t, "https://127.0.0.1:6550", cs, nil) + ep := localEnvFallback(context.Background()) + if ep.local != localLive || ep.name != "tracebloc" { + t.Fatalf("=> %+v, want localLive named tracebloc", ep) + } +} + +// The §7.5 ownership guarantee survives: a REMOTE cluster in the kubeconfig is +// never adopted without the pointer — same honest no-release as before, and +// the clientset is never even dialed. +func TestLocalEnvFallback_RemoteClusterStaysGated(t *testing.T) { + stubFallbackSeams(t, "https://k8s.corp.example:6443", nil, nil) + if ep := localEnvFallback(context.Background()); ep.local != localNoRelease { + t.Fatalf("=> %+v, want localNoRelease (gate holds for remote clusters)", ep) + } +} + +func TestLocalEnvFallback_NoKubeconfigIsNoRelease(t *testing.T) { + stubFallbackSeams(t, "", nil, errors.New("no kubeconfig")) + if ep := localEnvFallback(context.Background()); ep.local != localNoRelease { + t.Fatalf("=> %+v, want localNoRelease", ep) + } +} + +func TestIsLocalServerURL(t *testing.T) { + local := []string{ + "https://127.0.0.1:6550", + "https://localhost:6443", + "https://[::1]:6443", + "https://0.0.0.0:6443", // k3d wildcard bind + "https://host.docker.internal:6550", + } + remote := []string{ + "https://10.2.3.4:6443", + "https://k8s.corp.example:443", + "https://192.168.1.20:6443", + "", "not a url", + } + for _, u := range local { + if !isLocalServerURL(u) { + t.Errorf("isLocalServerURL(%q) = false, want true", u) + } + } + for _, u := range remote { + if isLocalServerURL(u) { + t.Errorf("isLocalServerURL(%q) = true, want false", u) + } + } +} + +// #401: the Windows installer writes a tb.cmd shim (symlinks need admin); the +// alias check must accept it so remedies echo `tb` on Windows too. +func TestTbCmdAliasOurs(t *testing.T) { + dir := t.TempDir() + exe := filepath.Join(dir, "tracebloc.exe") + if tbCmdAliasOurs(dir, exe) { + t.Fatal("no shim present must be false") + } + shim := "@echo off\r\n\"" + exe + "\" %*\r\n" + if err := os.WriteFile(filepath.Join(dir, "tb.cmd"), []byte(shim), 0o755); err != nil { + t.Fatal(err) + } + if !tbCmdAliasOurs(dir, exe) { + t.Fatal("a shim invoking this binary must be ours") + } + if err := os.WriteFile(filepath.Join(dir, "tb.cmd"), []byte("@echo off\r\nsome-other-tool %*\r\n"), 0o755); err != nil { + t.Fatal(err) + } + if tbCmdAliasOurs(dir, exe) { + t.Fatal("a shim invoking a different tool is not ours") + } + // Bugbot: mentioning the name is not ownership — neither a comment that + // says "tracebloc" nor an invocation of a DIFFERENT tracebloc install. + if err := os.WriteFile(filepath.Join(dir, "tb.cmd"), + []byte("@echo off\r\nrem tracebloc helper\r\nsome-other-tool %*\r\n"), 0o755); err != nil { + t.Fatal(err) + } + if tbCmdAliasOurs(dir, exe) { + t.Fatal("a shim merely mentioning tracebloc is not ours") + } + other := filepath.Join(dir, "elsewhere", "tracebloc.exe") + if err := os.WriteFile(filepath.Join(dir, "tb.cmd"), + []byte("@echo off\r\n\""+other+"\" %*\r\n"), 0o755); err != nil { + t.Fatal(err) + } + if tbCmdAliasOurs(dir, exe) { + t.Fatal("a shim invoking a different tracebloc install is not ours") + } +} diff --git a/internal/cli/home_probe_helpers_test.go b/internal/cli/home_probe_helpers_test.go index b0c91cb2..2a53a41e 100644 --- a/internal/cli/home_probe_helpers_test.go +++ b/internal/cli/home_probe_helpers_test.go @@ -68,8 +68,9 @@ func TestJobsManagerReady(t *testing.T) { }) } -// TestMachineCapacity pins home.go:565 (0%): sum Ready nodes' allocatable, and -// return ok=false when the node list can't be read. +// TestMachineCapacity pins home.go's capacity view: the LARGEST Ready node's +// allocatable (never a sum — k3d's server+agent are one machine, #399), and +// ok=false when the node list can't be read. func TestMachineCapacity(t *testing.T) { ctx := context.Background() t.Run("ready node -> compute + ok", func(t *testing.T) { @@ -78,6 +79,13 @@ func TestMachineCapacity(t *testing.T) { t.Error("a Ready node must yield compute") } }) + t.Run("two identical k3d nodes -> 1x the machine, not 2x (#399)", func(t *testing.T) { + a, b := capNode("server-0", "12", "7Gi", false), capNode("agent-0", "12", "7Gi", false) + ci, ok := machineCapacity(ctx, fake.NewSimpleClientset(&a, &b)) + if !ok || ci.CPU != 12 || ci.MemGiB != 7 { + t.Errorf("=> %+v ok=%v, want cpu=12 mem=7 (largest node, not 24/14)", ci, ok) + } + }) t.Run("node list error -> ok=false", func(t *testing.T) { cs := fake.NewSimpleClientset() cs.PrependReactor("list", "nodes", func(ktesting.Action) (bool, runtime.Object, error) { diff --git a/internal/cli/home_test.go b/internal/cli/home_test.go index f5949f82..a36a758e 100644 --- a/internal/cli/home_test.go +++ b/internal/cli/home_test.go @@ -612,18 +612,19 @@ func capNode(name, cpu, mem string, notReady bool, gpu ...string) corev1.Node { // TestSumCapacity: Ready-node summation, GiB/CPU rounding, GPU aggregation, and // the all-NotReady → omit contract. func TestSumCapacity(t *testing.T) { - t.Run("sums ready nodes, rounds, counts GPUs", func(t *testing.T) { + t.Run("largest ready node wins, rounds, carries its GPUs (#399)", func(t *testing.T) { got, ok := sumCapacity([]corev1.Node{ - capNode("a", "7900m", "16Gi", false, "1"), // 7.9 CPU → 8 - capNode("b", "4", "16Gi", false, "1"), // +4 CPU, +1 GPU - capNode("gone", "8", "32Gi", true), // NotReady → ignored + capNode("a", "7900m", "16Gi", false, "1"), // 7.9 CPU → largest (CPU-major) → 8 + capNode("b", "4", "16Gi", false, "1"), // smaller — must not be added + capNode("gone", "8", "32Gi", true), // NotReady → ignored even though biggest }) if !ok { t.Fatal("expected ok") } - // 7900m + 4000m = 11900m → 11.9 → 12; 32 GiB total; 2 GPU. - if got != (computeInfo{CPU: 12, MemGiB: 32, GPU: 2}) { - t.Fatalf("got %+v, want {12 32 2}", got) + // Largest Ready node only: 7900m → 7.9 → 8; 16 GiB; its 1 GPU — never + // the 12/32/2 cross-node sum (k3d's nodes are one physical machine). + if got != (computeInfo{CPU: 8, MemGiB: 16, GPU: 1}) { + t.Fatalf("got %+v, want {8 16 1}", got) } }) t.Run("no GPU dimension when none present", func(t *testing.T) { diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 1c56c729..c388483a 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -21,8 +21,9 @@ import ( // testable without a pseudo-terminal — the same trick kubernetes.Interface // uses to let cluster code run against a fake clientset. // errInteractiveCancelled is returned when the user declines the -// confirm prompt or hits Ctrl-C. It's control flow, not a failure: -// runDataIngest maps it to a clean exit (0) with a "Cancelled" note. +// confirm prompt or hits Ctrl-C. It's control flow, not a failure — +// every site reports it through cleanCancel / mapPromptErr below: a +// visible "Cancelled — …" note and a clean exit (0). var errInteractiveCancelled = errors.New("cancelled by user") type prompter interface { @@ -103,6 +104,48 @@ func mapErr(err error) error { return err } +// cleanCancel is the ONE place a cancelled prompt is reported to the user. It +// prints the CLI's cancellation line — "Cancelled — ." — and returns +// nil, which ExitCodeFromError maps to exitOK. +// +// Exit 0 is the convention every prompting command follows (data ingest, data +// delete, resources set, client create, delete): backing out at a question is a +// user choice, and nothing was started, so there is no failure to report. +// exitInterrupted (130) is for the OTHER Ctrl-C — the one that interrupts work +// already in flight (the sign-in wait, `client status --wait`, the seal suite, an +// installer re-run), where an operation really was cut short. See exitcodes.go. +// +// nothing says what did NOT happen ("nothing was changed."), and takes format +// args for the sites that name the thing they left alone. The prefix lives here +// so no site invents its own wording, and the argument is required so no site can +// report a cancellation without saying what it left untouched. +func cleanCancel(p *ui.Printer, nothing string, a ...any) error { + p.Infof("Cancelled — %s", fmt.Sprintf(nothing, a...)) + return nil +} + +// mapPromptErr maps a prompter error to the CLI's exit contract, so a prompt can +// neither fail nor be cancelled silently. Ctrl-C (errInteractiveCancelled, from +// mapErr above) goes through cleanCancel — the same visible note and the same +// exit 0 the site's declined-answer branch produces. Anything else is a real +// prompt failure: exit 1. +// +// The Printer and the note are in the signature deliberately. The bug this +// replaced mapped the cancellation straight to nil, so Ctrl-C exited 0 with no +// output at all — a script could not tell it apart from a completed run +// (backend#1253). Handling the sentinel now costs you a note; printing nothing +// is no longer reachable. +// +// Sites whose non-cancel error needs a code other than exitFailure keep their own +// errors.Is check and call cleanCancel directly — the printing still funnels +// through one place. +func mapPromptErr(p *ui.Printer, err error, nothing string, a ...any) error { + if errors.Is(err, errInteractiveCancelled) { + return cleanCancel(p, nothing, a...) + } + return &exitError{code: exitFailure, err: err} +} + // isInteractiveTTY reports whether we can run a guided prompt flow: // both stdin (we read answers) and stdout (we draw prompts) must be a // real terminal. Piped input, redirected output, or CI all fail this diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go new file mode 100644 index 00000000..d946aeba --- /dev/null +++ b/internal/cli/prepare_host.go @@ -0,0 +1,215 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "regexp" + "runtime" + "strings" + "time" + + "github.com/spf13/cobra" +) + +// prepareHostUserRe validates the researcher username before we pass it to the +// installer as TB_PREPARE_USER. Conservative Linux-username shape: starts +// alphanumeric, then letters/digits/._- (usermod quotes it, but reject nonsense +// early with a clear error rather than a confusing failure deep in the installer). +var prepareHostUserRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,31}$`) + +// installerRunScript builds the bash program that downloads the cosign-verified +// installer to a temp file and runs THAT — optionally with a subcommand (e.g. +// "prepare-host"). Shared by `tracebloc upgrade` (no subcommand, full install) +// and `tracebloc prepare-host`, so both stay on one download-then-execute idiom. +// +// We run a downloaded FILE rather than `curl … | bash`. Two reasons, both Bugbot +// (#394, #397): +// - stdin: with `curl | bash`, the inner bash reads its *program* from the +// pipe, so the installer's stdin is no longer the terminal. Any interactive +// prompt (sign-in, or which non-admin user gets runtime access) would get +// EOF. Running a downloaded file leaves stdin on the TTY. +// - fail-closed: `set -e` + `curl -o` makes a failed download (network/DNS/HTTP +// error) abort with a non-zero status instead of silently running nothing. +// (`curl | bash` swallowed this — bash read empty stdin and exited 0.) +// +// The download uses `--tlsv1.2` — the TLS 1.2 floor scripts/install.sh enforces +// on every security-sensitive fetch — so this privileged installer download can +// never negotiate a weaker protocol (Bugbot #397). The temp file is removed on +// exit. The URL comes from installerURL (doctor.go) so every installer path +// shares one source and can't drift (Bugbot #394/#397). +func installerRunScript(subcommand string) string { + run := `bash "$tmp"` + if subcommand != "" { + run += " " + subcommand + } + return `set -e +tmp="$(mktemp)" +trap 'rm -f "$tmp"' EXIT +curl -fsSL --tlsv1.2 ` + installerURL + ` -o "$tmp" +` + run +} + +// prepareHostInstallerCmd runs the official installer's admin-only prepare-host +// step. Like `tracebloc upgrade`, this deliberately delegates to the verified +// installer (cosign-checked) instead of re-implementing any privileged host prep +// in the CLI — the privileged surface stays in one audited place. See +// installerRunScript for why we download-then-execute rather than pipe. +var prepareHostInstallerCmd = installerRunScript("prepare-host") + +// prepareHostManualHint is the copy-pasteable command we show if the automated +// run fails. Built from installCmd (doctor.go) — the single shared bootstrap +// idiom — so a URL/idiom change updates every hint at once (Bugbot #394); we +// only append the prepare-host subcommand. installCmd uses process substitution +// (bash <(curl …)), which keeps stdin on the terminal for interactive prompts. +// When a researcher username was given we prefix TB_PREPARE_USER= so a +// copy-pasted retry still grants access — otherwise the manual fallback would +// silently do less than the original request (Bugbot #394). +func prepareHostManualHint(user string) string { + if user != "" { + return "TB_PREPARE_USER=" + user + " " + installCmd + " prepare-host" + } + return installCmd + " prepare-host" +} + +// prepareHostEnv is the child's environment: the parent's, but with any ambient +// TB_PREPARE_USER stripped, then set to user only when a username was given. +// Stripping matters — the no-username path promises it grants no access, so a +// pre-set TB_PREPARE_USER in the admin's shell must not silently make the +// installer grant it anyway (Bugbot #394). +func prepareHostEnv(user string) []string { + parent := os.Environ() + env := make([]string, 0, len(parent)+1) + for _, kv := range parent { + if strings.HasPrefix(kv, "TB_PREPARE_USER=") { + continue + } + env = append(env, kv) + } + if user != "" { + env = append(env, "TB_PREPARE_USER="+user) + } + return env +} + +// prepareHostCmd builds the exec.Cmd that runs the installer. +// +// It deliberately does NOT put the installer in its own process group. The +// installer is interactive (prepare-host may prompt, e.g. which non-admin user +// gets runtime access), and stdin is the TTY — a child in a *background* process +// group that reads the terminal gets SIGTTIN and hangs (Bugbot #394). Staying in +// the CLI's foreground group means prompts work AND a terminal Ctrl-C delivers +// SIGINT to the whole pipeline (the `bash -c`, the `curl`, and the `bash "$tmp"` +// prepare-host child) in one go — no orphaned privileged work. +// +// WaitDelay bounds teardown on a *programmatic* cancel (parent shutdown / a +// SIGTERM to the CLI alone): CommandContext SIGKILLs the process and, after the +// delay, force-closes the I/O pipes so Wait can't block forever behind a child +// that traps signals. We rely on the default SIGKILL rather than a custom +// SIGINT-only Cancel (which a privileged child could ignore, hanging Wait). +func prepareHostCmd(ctx context.Context) *exec.Cmd { + c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) + c.WaitDelay = 5 * time.Second + return c +} + +// installerRunInterrupted reports whether an installer run (upgrade or +// prepare-host) ended because the user aborted, so the caller can exit quietly +// (130) instead of framing it as a failed install. ctx.Err() catches a cancel +// the signal handler already propagated — but on a terminal Ctrl-C the child can +// die and c.Run() can return BEFORE NotifyContext flips ctx.Err() (a race), so +// also treat bash's 130 (128+SIGINT) exit as an interrupt (Bugbot #394/#397). +func installerRunInterrupted(ctx context.Context, runErr error) bool { + if ctx.Err() != nil { + return true + } + var ee *exec.ExitError + if errors.As(runErr, &ee) { + return ee.ExitCode() == exitInterrupted + } + return false +} + +// prepareHostUnsupportedOnOS reports whether prepare-host can't run on this OS. +// The step readies a Linux server / HPC login node (container runtime, docker +// group) — a Unix-only concept — and shells out to bash/curl/mktemp. On Windows +// that would fail with a cryptic missing-bash error and a Unix-only retry hint, +// so we stop early with a clear message instead (mirrors upgrade's Windows +// handling; Bugbot #394). +func prepareHostUnsupportedOnOS(goos string) bool { return goos == "windows" } + +// newPrepareHostCmd builds `tracebloc prepare-host` — the one-time administrator +// step that readies a machine so a non-admin user can then install tracebloc +// with no root at all. +func newPrepareHostCmd() *cobra.Command { + return &cobra.Command{ + Use: "prepare-host [researcher-username]", + Short: "Prepare this machine so a non-admin user can install tracebloc (run once, as an administrator)", + Long: `Prepares a host that a non-admin user can't install on directly. + +Run this ONCE, as an administrator, on a machine where the person who will use +tracebloc has no root or sudo — a shared server, an HPC login node. It installs +the container runtime and its prerequisites. + +Pass that person's username to also grant them container-runtime (docker-group) +access, so they can then install tracebloc at Tier 0 with no administrator +rights at all: + + sudo tracebloc prepare-host alice + +Without a username it installs only the runtime + prerequisites and tells you +how to grant a user access afterwards. NOTE: the username is the RESEARCHER who +will use tracebloc — not you, the admin running this. + +It re-runs the official installer's prepare-host step (verified with cosign). It +does NOT create your secure environment or sign you in — it only prepares the +host, so it's safe to run on a shared machine. Safe to re-run.`, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + p := printerFor(cmd) + p.Newline() + if prepareHostUnsupportedOnOS(runtime.GOOS) { + // Discoverable in --help everywhere, but a no-op-with-explanation + // here rather than a cryptic missing-bash failure (Bugbot #394). + p.Para("prepare-host readies a Linux server or HPC login node so a non-admin user can install tracebloc without root — it doesn't apply to Windows. Run it as an administrator on the Unix host the researcher will use.") + p.Newline() + return nil + } + ctx := cmd.Context() + c := prepareHostCmd(ctx) + c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr + + user := "" + if len(args) == 1 { + user = args[0] + if !prepareHostUserRe.MatchString(user) { + return &exitError{code: exitBadInput, err: fmt.Errorf("invalid username %q — expected a Linux username (letters, digits, '.', '_', '-')", user)} + } + } + // The installer reads TB_PREPARE_USER to pick who gets docker-group + // access. Pass it through the environment (not the command string) so + // it can't be shell-interpreted; the installer quotes it for usermod. + // prepareHostEnv also strips any ambient TB_PREPARE_USER so the + // no-username path never grants access it says it won't. + c.Env = prepareHostEnv(user) + if user != "" { + p.Para(fmt.Sprintf("Preparing this host and granting %s container-runtime access — re-running the installer's prepare-host step (needs administrator rights once).", user)) + } else { + p.Para("Preparing this host — re-running the installer's prepare-host step (installs the container runtime and prerequisites; needs administrator rights once). Pass a researcher's username to also grant them access: tracebloc prepare-host ") + } + p.Newline() + if err := c.Run(); err != nil { + // User aborted (Ctrl-C) or the parent context was cancelled: exit + // quietly with 130 like the other cancellable paths, not a scary + // "prepare-host didn't complete — retry" (Bugbot #394). + if installerRunInterrupted(ctx, err) { + return &exitError{code: exitInterrupted} + } + return &exitError{code: exitFailure, err: fmt.Errorf("prepare-host didn't complete (%w). You can run the installer directly:\n %s", err, prepareHostManualHint(user))} + } + return nil + }, + } +} diff --git a/internal/cli/prepare_host_test.go b/internal/cli/prepare_host_test.go new file mode 100644 index 00000000..1da7d945 --- /dev/null +++ b/internal/cli/prepare_host_test.go @@ -0,0 +1,187 @@ +package cli + +import ( + "context" + "errors" + "os/exec" + "strings" + "testing" +) + +// A failed download must abort rather than run an empty script: with the old +// `curl | bash`, a curl failure left bash reading empty stdin and exiting 0, so +// the command reported success while prepare-host never ran. `set -e` + `curl +// -o ` makes the failure propagate — guard both against removal (Bugbot +// #394). +func TestPrepareHostCmdFailsClosedOnDownloadError(t *testing.T) { + if !strings.Contains(prepareHostInstallerCmd, "set -e") { + t.Fatalf("prepareHostInstallerCmd must `set -e` so a failed download aborts; got: %q", prepareHostInstallerCmd) + } + if !strings.Contains(prepareHostInstallerCmd, "curl") || !strings.Contains(prepareHostInstallerCmd, "-o ") { + t.Fatalf("prepareHostInstallerCmd must download the installer to a file (curl -o) so curl's exit is checked; got: %q", prepareHostInstallerCmd) + } + if !strings.Contains(prepareHostInstallerCmd, "prepare-host") { + t.Fatalf("prepareHostInstallerCmd should invoke the installer's prepare-host step; got: %q", prepareHostInstallerCmd) + } +} + +// Every curl in the shared installer script must pin the TLS 1.2 floor +// (--tlsv1.2), matching scripts/install.sh, so this privileged download can never +// negotiate a weaker protocol. Guards both the upgrade (no subcommand) and +// prepare-host paths since they share installerRunScript (Bugbot #397). +func TestInstallerRunScriptPinsTLSFloor(t *testing.T) { + for _, sub := range []string{"", "prepare-host"} { + script := installerRunScript(sub) + if !strings.Contains(script, "curl") { + t.Fatalf("installerRunScript(%q) must curl the installer; got: %q", sub, script) + } + if !strings.Contains(script, "--tlsv1.2") { + t.Errorf("installerRunScript(%q) must pin --tlsv1.2 on the download (matches install.sh); got: %q", sub, script) + } + } +} + +// The installer must NOT be fed to bash over a pipe: `curl | bash -s` makes the +// inner bash read its program from the pipe, stealing the installer's stdin so +// any interactive prompt in prepare-host gets EOF. We download and run a file +// instead, leaving stdin on the TTY (Bugbot #394). +func TestPrepareHostCmdDoesNotPipeIntoBash(t *testing.T) { + if strings.Contains(prepareHostInstallerCmd, "| bash") || strings.Contains(prepareHostInstallerCmd, "|bash") { + t.Errorf("prepareHostInstallerCmd must not pipe the script into bash (steals the installer's stdin); got: %q", prepareHostInstallerCmd) + } +} + +// The installer must run in the CLI's foreground process group (NOT its own): +// it's interactive and stdin is the TTY, so a backgrounded group would get +// SIGTTIN and hang on any prompt. And WaitDelay must be positive so a child that +// traps signals can't hang Wait forever after a programmatic cancel (Bugbot +// #394). SysProcAttr==nil is portable (the field is *syscall.SysProcAttr on +// every OS), so this stays a single cross-platform test. +func TestPrepareHostCmdStaysInForegroundGroup(t *testing.T) { + c := prepareHostCmd(context.Background()) + if c.SysProcAttr != nil { + t.Error("prepareHostCmd must not set SysProcAttr — a separate/background process group breaks interactive TTY prompts (SIGTTIN)") + } + if c.WaitDelay <= 0 { + t.Error("prepareHostCmd must set a positive WaitDelay so Wait can't hang forever after a cancel") + } +} + +// A user abort must be detected as an interrupt even when NotifyContext hasn't +// flipped ctx.Err() yet — bash exits 130 on SIGINT and c.Run() can return first +// (Bugbot #394). A genuine failure (exit 1) must NOT be treated as an interrupt. +func TestInstallerRunInterrupted(t *testing.T) { + // Cancelled context → interrupt regardless of the run error. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if !installerRunInterrupted(ctx, errors.New("boom")) { + t.Error("a cancelled context must be treated as an interrupt") + } + + if _, err := exec.LookPath("bash"); err != nil { + t.Skip("bash not available for the exit-code cases") + } + // Live context + bash exit 130 (128+SIGINT) → interrupt (the Ctrl-C race). + if err := exec.Command("bash", "-c", "exit 130").Run(); err == nil { + t.Fatal("expected a non-nil error from exit 130") + } else if !installerRunInterrupted(context.Background(), err) { + t.Error("exit 130 with a live context must be treated as an interrupt") + } + // Live context + a normal failure (exit 1) → NOT an interrupt. + if err := exec.Command("bash", "-c", "exit 1").Run(); err == nil { + t.Fatal("expected a non-nil error from exit 1") + } else if installerRunInterrupted(context.Background(), err) { + t.Error("exit 1 must NOT be treated as an interrupt") + } +} + +func TestPrepareHostCmdMetadata(t *testing.T) { + c := newPrepareHostCmd() + if !strings.HasPrefix(c.Use, "prepare-host") { + t.Errorf("Use = %q, want it to start with prepare-host", c.Use) + } + // MaximumNArgs(1): the optional researcher username. Zero or one arg is fine; + // two is rejected (Bugbot / Divya #377: name the researcher to grant access). + if err := c.Args(c, []string{}); err != nil { + t.Errorf("prepare-host must accept zero args: %v", err) + } + if err := c.Args(c, []string{"alice"}); err != nil { + t.Errorf("prepare-host must accept one username arg: %v", err) + } + if err := c.Args(c, []string{"alice", "bob"}); err == nil { + t.Error("prepare-host should reject more than one positional argument") + } +} + +// The researcher username is passed to the installer as TB_PREPARE_USER, so it +// must be validated: accept real Linux usernames, reject shell-metacharacter / +// empty / overlong input (Divya #377). +func TestPrepareHostUserValidation(t *testing.T) { + valid := []string{"alice", "bob123", "a.b_c-d", "R2D2", "svc_account"} + for _, u := range valid { + if !prepareHostUserRe.MatchString(u) { + t.Errorf("username %q should be valid", u) + } + } + invalid := []string{"", "-leading", ".dot", "has space", "semi;colon", "a/b", "$(whoami)", "a`b`", "toolong_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"} + for _, u := range invalid { + if prepareHostUserRe.MatchString(u) { + t.Errorf("username %q should be rejected", u) + } + } +} + +// The no-username path promises it grants no access, so a pre-set ambient +// TB_PREPARE_USER must be stripped from the child env; the username path sets +// exactly that user (replacing any ambient value, not duplicating it) — Bugbot #394. +func TestPrepareHostEnv_StripsAmbientAndSetsUser(t *testing.T) { + t.Setenv("TB_PREPARE_USER", "ambient-attacker") + + for _, kv := range prepareHostEnv("") { + if strings.HasPrefix(kv, "TB_PREPARE_USER=") { + t.Errorf("no-username env must not carry TB_PREPARE_USER, got %q", kv) + } + } + + n, got := 0, "" + for _, kv := range prepareHostEnv("alice") { + if strings.HasPrefix(kv, "TB_PREPARE_USER=") { + n++ + got = strings.TrimPrefix(kv, "TB_PREPARE_USER=") + } + } + if n != 1 || got != "alice" { + t.Errorf("username env should carry exactly TB_PREPARE_USER=alice, got n=%d val=%q", n, got) + } +} + +// On failure after `prepare-host `, the manual retry must still grant +// access (carry TB_PREPARE_USER=) — otherwise a copy-pasted retry silently +// does less than the original request. The no-username hint carries no such var +// (Bugbot #394). +func TestPrepareHostManualHint_CarriesUser(t *testing.T) { + if h := prepareHostManualHint(""); strings.Contains(h, "TB_PREPARE_USER") { + t.Errorf("no-username hint must not set TB_PREPARE_USER: %q", h) + } + h := prepareHostManualHint("alice") + if !strings.Contains(h, "TB_PREPARE_USER=alice") { + t.Errorf("username hint must carry TB_PREPARE_USER=alice so the retry still grants access: %q", h) + } + if !strings.Contains(h, "prepare-host") { + t.Errorf("hint must invoke prepare-host: %q", h) + } +} + +// prepare-host shells out to bash/curl and readies a Unix host, so it must be +// guarded on Windows (a no-op-with-explanation, not a cryptic missing-bash +// failure) — mirrors upgrade's Windows handling (Bugbot #394). +func TestPrepareHostUnsupportedOnWindows(t *testing.T) { + if !prepareHostUnsupportedOnOS("windows") { + t.Error("prepare-host must be guarded on windows") + } + for _, goos := range []string{"linux", "darwin"} { + if prepareHostUnsupportedOnOS(goos) { + t.Errorf("prepare-host must run on %s", goos) + } + } +} diff --git a/internal/cli/pure_helpers_coverage_test.go b/internal/cli/pure_helpers_coverage_test.go index c6e6a553..7e2b92b8 100644 --- a/internal/cli/pure_helpers_coverage_test.go +++ b/internal/cli/pure_helpers_coverage_test.go @@ -1,7 +1,9 @@ package cli import ( + "bytes" "errors" + "strings" "testing" "github.com/AlecAivazis/survey/v2/terminal" @@ -10,6 +12,7 @@ import ( "github.com/tracebloc/cli/internal/doctor" "github.com/tracebloc/cli/internal/resources" + "github.com/tracebloc/cli/internal/ui" ) // TestMapErr pins the interactive-cancel seam contract (interactive.go:82, @@ -29,17 +32,28 @@ func TestMapErr(t *testing.T) { } } -// TestMapClientErr pins client.go:1015 (0%): a cancelled prompt is a clean exit -// (nil); anything else becomes an exit-1 *exitError. -func TestMapClientErr(t *testing.T) { - if err := mapClientErr(errInteractiveCancelled); err != nil { +// TestMapPromptErr pins the seam's exit contract: a cancelled prompt is a clean +// exit (nil ⇒ exit 0) AND prints the shared note — the two are inseparable here, +// which is the whole point of the helper (backend#1253). Anything else becomes an +// exit-1 *exitError, with nothing printed (main() reports it). +func TestMapPromptErr(t *testing.T) { + var out bytes.Buffer + if err := mapPromptErr(ui.New(&out, ui.WithColor(false)), errInteractiveCancelled, "nothing was changed."); err != nil { t.Errorf("cancel must map to a clean nil, got %v", err) } - err := mapClientErr(errors.New("nope")) + if got := out.String(); !strings.Contains(got, "Cancelled — nothing was changed.") { + t.Errorf("cancel must print the shared note, got %q", got) + } + + out.Reset() + err := mapPromptErr(ui.New(&out, ui.WithColor(false)), errors.New("nope"), "nothing was changed.") var ee *exitError if !errors.As(err, &ee) || ee.Code() != 1 { t.Errorf("a real error must become exit 1, got %v", err) } + if out.String() != "" { + t.Errorf("a real prompt failure must not print a cancellation note, got %q", out.String()) + } } // TestWorseStatus pins the doctor verdict truth-table (doctor.go:229, was 40% — diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 2df0733b..d5146a46 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -216,9 +216,11 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // failures pass through unchanged. desired, err := decideDesired(p, pr, req, node, current, machineGPUName, machineGPUCount, machineHasGPU) if err != nil { + // Not mapPromptErr: a validation error from the wizard carries its own + // code (exit 2) and must pass through unchanged, so only the cancel is + // funnelled through the shared note. if errors.Is(err, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was changed.") - return nil + return cleanCancel(p, "nothing was changed.") } return err } @@ -273,18 +275,13 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * p.PromptHint("tracebloc keeps about 1 core and 3 GiB for itself on top of this — it fits on this machine.") proceed, cerr := pr.Confirm(fmt.Sprintf("Let each training run use up to %s?", perRunSize(desired)), true) if cerr != nil { - // Ctrl-C here is the same user choice as answering "No": print the - // same note the decline below (and a wizard interrupt above) prints, - // and exit 0 — never a silent success that hides the abort. - if errors.Is(cerr, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was changed.") - return nil - } - return mapClientErr(cerr) + // Ctrl-C here is the same user choice as answering "No": the same + // note the decline below (and a wizard interrupt above) prints, and + // exit 0 — never a silent success that hides the abort. + return mapPromptErr(p, cerr, "nothing was changed.") } if !proceed { - p.Infof("Cancelled — nothing was changed.") - return nil + return cleanCancel(p, "nothing was changed.") } } @@ -364,11 +361,27 @@ func desiredFromFlags(req setReq, current resources.Training, gpuName corev1.Res func runResourcesWizard(p *ui.Printer, pr prompter, node resources.Machine, current resources.Training, gpuName corev1.ResourceName, gpuCount int64, machineHasGPU bool) (resources.Training, error) { maxCores := resources.MaxRunCores(node) maxGiB := resources.MaxRunGiB(node) - - // (1) Current per-run budget vs the machine. + // The configured budget floored to the wizard's whole-unit domain — used to + // clamp prompt defaults and annotate the header (#398). `current` is the + // cluster-wide ceiling (chart default 8Gi), which a machine can shrink UNDER + // (the WSL2 field case: node ~6.7 GiB under a lingering 8Gi ceiling). + curCores := int(current.CPU.MilliValue() / 1000) + curGiB := int(current.Mem.Value() >> 30) + + // (1) Current per-run budget vs the machine. When the configured budget + // exceeds what this machine can still give one run, say so instead of + // printing a bare ">100%" line like "8 of 6.7 GiB" (#398). p.Section("How much of this machine a training run may use") - p.Field("CPU", fmt.Sprintf("%s of %s cores", coresNum(current.CPU), coresNum(node.CPU))) - p.Field("Memory", fmt.Sprintf("%s of %s GiB", gibNum(current.Mem), gibNum(node.Mem))) + cpuLine := fmt.Sprintf("%s of %s cores", coresNum(current.CPU), coresNum(node.CPU)) + if curCores > maxCores { + cpuLine += " — more than this machine can give one run now" + } + p.Field("CPU", cpuLine) + memLine := fmt.Sprintf("%s of %s GiB", gibNum(current.Mem), gibNum(node.Mem)) + if curGiB > maxGiB { + memLine += " — more than this machine can give one run now" + } + p.Field("Memory", memLine) if machineHasGPU { p.Field("GPU", fmt.Sprintf("%d of %d", currentGPUCount(current), gpuCount)) } @@ -411,17 +424,21 @@ func runResourcesWizard(p *ui.Printer, pr prompter, node resources.Machine, curr "overhead it can offer a training run at most %d core(s) and %d GiB. Free up "+ "resources or use a larger machine.", maxCores, maxGiB)} } + // The offered defaults are the current budget CLAMPED into each prompt's own + // valid range (#398): unclamped, a machine that shrank under the configured + // ceiling offered a default its own validator then rejected — pressing Enter + // on "(8)" answered "must be between 2 and 3". coresAns, err := pr.Input( fmt.Sprintf("CPU cores for one run (1–%d)", maxCores), "how many CPU cores a single training run may use", - coresNum(current.CPU), boundedInt(1, maxCores)) + strconv.Itoa(clampInt(curCores, 1, maxCores)), boundedInt(1, maxCores)) if err != nil { return resources.Training{}, err } memAns, err := pr.Input( fmt.Sprintf("Memory for one run in GiB (2–%d)", maxGiB), "how much memory a single training run may use, in GiB", - gibNum(current.Mem), boundedInt(2, maxGiB)) + strconv.Itoa(clampInt(curGiB, 2, maxGiB)), boundedInt(2, maxGiB)) if err != nil { return resources.Training{}, err } @@ -602,6 +619,18 @@ func sameCeiling(a, b resources.Training) bool { func coresNum(q resource.Quantity) string { return strings.TrimSuffix(resources.FormatCPU(q), " CPU") } func gibNum(q resource.Quantity) string { return strings.TrimSuffix(resources.FormatMem(q), " GiB") } +// clampInt limits v to [lo, hi]. Prompt defaults must sit inside their own valid +// range, or accepting the offer is rejected by its own validator (#398). +func clampInt(v, lo, hi int) int { + if v < lo { + return lo + } + if v > hi { + return hi + } + return v +} + func currentGPUCount(t resources.Training) int64 { if t.HasGPU { return t.GPU.Value() diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 75103c63..20407e44 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -333,6 +333,39 @@ func TestWizard_ChooseAnAmount(t *testing.T) { } } +// TestWizard_DefaultsClampedToShrunkMachine (#398): on a machine that shrank +// under the configured ceiling (the WSL2 field case: node smaller than the +// chart-default 8Gi RESOURCE_LIMITS), pressing Enter on every prompt must +// succeed — the offered defaults are clamped into their own valid ranges. +// Before the clamp, accepting the memory default "(8)" was rejected by its own +// validator ("must be between 2 and N"). +func TestWizard_DefaultsClampedToShrunkMachine(t *testing.T) { + pr := &fakePrompter{ + answers: map[string]string{ + "How much may one training run use?": "Choose an amount", + // CPU + memory prompts deliberately NOT scripted: the fake returns + // each prompt's DEFAULT — exactly "the user pressed Enter". + }, + confirm: boolPtr(true), + } + // 12-CPU / 7-GiB node → maxCores 11, maxGiB 4; current cpu=2 fits, memory + // 8Gi exceeds → default must clamp 8 → 4. + cs := csWith("12", "7Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + var buf bytes.Buffer + err := applyResourcesSet(context.Background(), ui.New(&buf, ui.WithColor(false)), pr, + setTarget(cs), cluster.KubeconfigOptions{}, setReq{dryRun: true}) + if err != nil { + t.Fatalf("Enter-on-defaults must succeed on a shrunk machine: %v", err) + } + if !strings.Contains(buf.String(), "cpu=2,memory=4Gi") { + t.Errorf("clamped defaults should apply cpu=2,memory=4Gi:\n%s", buf.String()) + } + // The header must not present the stale ceiling as a bare ">100%" fact. + if !strings.Contains(buf.String(), "more than this machine can give one run now") { + t.Errorf("header should annotate the over-ceiling budget:\n%s", buf.String()) + } +} + // TestWizard_ChooseAnAmountTooSmallMachine: on a machine too small to give a run // even the 1-core / 2-GiB minimum after tracebloc's overhead, the "Choose an // amount" path must NOT prompt an impossible range (e.g. "1–0") that rejects diff --git a/internal/cli/root.go b/internal/cli/root.go index 7954cc35..06e896c0 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -104,6 +104,10 @@ Helm, no YAML, no kubectl needed.`, // `client` and NOT `client delete --uninstall`: one machine owns one client, // so this removes tracebloc from the host and avoids colliding with `data delete`. root.AddCommand(newDeleteCmd()) + root.AddCommand(newPrepareHostCmd()) + // F1: apply an update (re-runs the verified installer). The update-check + // nudge (update_check.go) and the 426 "too old" error both point here. + root.AddCommand(newUpgradeCmd()) // Bare `tracebloc` (no subcommand) renders a status-aware home screen — // where you stand (signed in? environment live?) then the commands — diff --git a/internal/cli/seal.go b/internal/cli/seal.go new file mode 100644 index 00000000..f27c2141 --- /dev/null +++ b/internal/cli/seal.go @@ -0,0 +1,318 @@ +// The seal check — `tracebloc client status --seal` (cli#393, RFC-0003 §8.2 +// D12). Runs the chart's conformance checks (its helm-test hooks; see +// internal/helm/seal.go for the chart contract) against this machine's secure +// environment and reports an honest verdict: +// +// sealed — every conformance check passed (exit 0) +// unsealed — a check failed or couldn't run: a protection is not enforced (exit 2) +// unknown — the chart ships no conformance checks, so nothing was verified (exit 2) +// +// The honest-output principle (the same one delete's closing summary follows): +// never print a success state that wasn't verified. "Unknown" is explicitly NOT +// sealed — an environment that cannot demonstrate a guarantee is never claimed +// to hold it — and only a fully-passed suite exits 0, so scripts can gate on it. + +package cli + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/helm" + "github.com/tracebloc/cli/internal/ui" +) + +// Seams over the helm seal-check calls, so tests drive every verdict path +// without helm or a cluster — the same fn-var pattern as listDatasetsFn / +// resolveClusterTargetFn. +var ( + listTestHooksFn = helm.ListTestHooks + runHelmTestFn = helm.RunTest +) + +// sealCheck is one conformance check's outcome, ready to render. +type sealCheck struct { + name string // display name (chart's seal-check-name label, else the de-prefixed hook name) + passed bool + detail string // one-line failure detail ("" when passed) + hint string // one-line remediation pointer ("" when passed) +} + +// sealModel is everything renderSealResult needs — a pure value, so the copy +// catalog renders the sealed / unsealed / unknown screens byte-exact. +type sealModel struct { + envName string // the secure environment being verified + fallback bool // chart marks no seal-check suite; ran ALL of its helm tests instead + checks []sealCheck // empty = the chart ships no conformance checks (verdict: unknown) +} + +// sealed reports the verdict: true only when there were checks and every one +// passed. No checks at all is NOT sealed (it's unknown — nothing was verified). +func (m sealModel) sealed() bool { + if len(m.checks) == 0 { + return false + } + return m.failedCount() == 0 +} + +func (m sealModel) failedCount() int { + n := 0 + for _, c := range m.checks { + if !c.passed { + n++ + } + } + return n +} + +// runSealCheck drives the seal verdict: resolve the cluster + release the same +// way the data commands do (exit 3 unreachable, exit 4 no client — with the +// §7.3 active-client binding and its "runs on another machine" rewrite), list +// the chart's test hooks, run each one, render, and exit by the verdict. +func runSealCheck(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, timeout time.Duration) error { + binding := bindActiveClientNamespace(&opts) + target, err := resolveClusterTargetFn(ctx, p, opts, binding, false) + if err != nil { + return binding.explain(err) + } + tt := helm.TestTarget{ + Release: target.Release.ReleaseName, + Namespace: target.Resolved.Namespace, + Kubeconfig: opts.Path, + // The RESOLVED context, not the raw --context flag: with the flag + // omitted the raw value is empty and helm would fall back to its own + // ambient resolution (including $HELM_KUBECONTEXT), which can point at + // a different cluster than the one discovery just used. Same pinning + // `resources set` does (Bugbot). + KubeContext: target.Resolved.Context, + } + + hooks, herr := listTestHooksFn(ctx, tt) + if herr != nil { + // Ctrl-C while enumerating is a cancelled run, not an enumeration + // failure — exit quietly, matching the per-check loop below. Reuse + // installerRunInterrupted so a terminal Ctrl-C where helm exits 130 before + // NotifyContext flips ctx.Err() is caught here too (Bugbot #397). + if installerRunInterrupted(ctx, herr) { + return &exitError{code: exitInterrupted} + } + // Can't even enumerate the checks — that's an error, not a verdict: + // printing "unsealed" (or worse, "unknown") off a helm failure would + // misstate what we know about the environment. + return &exitError{code: exitFailure, err: fmt.Errorf( + "couldn't read the chart's conformance checks: %w", herr)} + } + suite, aux, fallback := sealSuite(hooks) + model := sealModel{envName: sealEnvName(target, binding), fallback: fallback} + + if len(suite) == 0 { + renderSealResult(p, model) // header + the unknown verdict + return &exitError{code: exitChecksFailed, err: nil} // rendered above — exit silent + } + + renderSealHeader(p, model) + for _, h := range suite { + name := sealCheckName(h, tt.Release) + sp := p.Spinner(fmt.Sprintf("Checking %s…", name), "") + // The filter carries the check plus the aux plumbing hooks (see + // helm.RunTest) — but never the other checks, so this check's verdict + // stays its own. + out, terr := runHelmTestFn(ctx, tt, append([]string{h.Name}, aux...), timeout) + sp.Stop() + // Ctrl-C (or a parent deadline) mid-suite is a cancelled run, not a + // verdict: every remaining check would "fail" on the dead context and + // render a fake Unsealed. Exit quietly, the way `status --wait` does. + // Reuse installerRunInterrupted (shared with upgrade/prepare-host): it + // also catches the race where a terminal Ctrl-C makes the helm child exit + // 130 BEFORE NotifyContext flips ctx.Err() — otherwise this check reads as + // a real failure and prints a false Unsealed (exit 2), breaking the "never + // claim a verdict you didn't verify" rule (Bugbot #397). + if installerRunInterrupted(ctx, terr) { + return &exitError{code: exitInterrupted} + } + check := sealCheck{name: name, passed: terr == nil} + if terr != nil { + check.detail = helmFailureDetail(out, terr) + check.hint = sealFailureHint(h, tt.Namespace) + } + renderSealCheckLine(p, check) + model.checks = append(model.checks, check) + } + + renderSealVerdict(p, model) + if !model.sealed() { + return &exitError{code: exitChecksFailed, err: nil} // rendered above — exit silent + } + return nil +} + +// sealSuite splits the release's test hooks into the checks to run and the +// aux plumbing to carry along on every run: +// +// - suite: the RUNNABLE hooks (Jobs/Pods) the chart labels as the seal-check +// suite; when the chart labels none (an older chart), ALL of its runnable +// helm tests, reported as a fallback so the output says which contract ran. +// - aux: the non-runnable test hooks (a check's ServiceAccount/RBAC, created +// at negative hook-weight). Never checks themselves — an SA can't "pass" — +// but helm's --filter would exclude them from a per-check run and strand +// the check without its plumbing, so every RunTest lists them too. +func sealSuite(hooks []helm.TestHook) (suite []helm.TestHook, aux []string, fallback bool) { + var runnable []helm.TestHook + for _, h := range hooks { + if h.Runnable() { + runnable = append(runnable, h) + } else { + aux = append(aux, h.Name) + } + } + for _, h := range runnable { + if h.SealCheck { + suite = append(suite, h) + } + } + if len(suite) > 0 { + return suite, aux, false + } + return runnable, aux, len(runnable) > 0 +} + +// sealEnvName names the environment under test: the active client's display +// name when the §7.3 binding chose the target, else the namespace actually +// resolved (explicit --namespace/--context, or the kubeconfig default). +func sealEnvName(target *clusterTarget, binding activeClientBinding) string { + if binding.applied && binding.name != "" { + return binding.name + } + return target.Resolved.Namespace +} + +// sealCheckName is a check's display name: the chart's seal-check-name label +// when present (the stable per-check identifier, e.g. "egress-enforcement"), +// else the hook name with the release prefix trimmed (the chart's hooks are +// named "-"). +func sealCheckName(h helm.TestHook, release string) string { + if h.SealName != "" { + return h.SealName + } + return strings.TrimPrefix(h.Name, release+"-") +} + +// sealFailureHint is the one-line remediation pointer under a failed check: +// the chart's seal-hint annotation when present, else the kubectl command that +// shows the check's own output (the probes print their diagnosis). +func sealFailureHint(h helm.TestHook, namespace string) string { + if h.SealHint != "" { + return h.SealHint + } + ref := h.Name + if strings.EqualFold(h.Kind, "Job") { + ref = "job/" + h.Name + } + return fmt.Sprintf("see why: kubectl logs -n %s %s", namespace, ref) +} + +// helmFailureDetail distills helm's combined output + error into the one-line +// reason a check failed. helm reports hook failures as a bullet list under an +// "Error:" line ("* job failed: BackoffLimitExceeded", "* timed out waiting +// for the condition"); prefer the first bullet (the specific reason), then the +// Error: line, then the raw error's first line — never empty for a failure. +func helmFailureDetail(out string, err error) string { + if err == nil { + return "" + } + var errLine, bullet string + for _, line := range strings.Split(out, "\n") { + l := strings.TrimSpace(line) + switch { + case bullet == "" && strings.HasPrefix(l, "* "): + bullet = strings.TrimSpace(strings.TrimPrefix(l, "* ")) + case errLine == "" && strings.HasPrefix(l, "Error:"): + errLine = strings.TrimSpace(strings.TrimPrefix(l, "Error:")) + } + } + detail := bullet + if detail == "" { + detail = errLine + } + if detail == "" { + detail, _, _ = strings.Cut(err.Error(), "\n") + } + return sealTrimDetail(detail) +} + +// sealTrimDetail caps a failure detail to one readable line; the hint under it +// points at the full output. +func sealTrimDetail(s string) string { + const max = 140 + r := []rune(s) + if len(r) <= max { + return s + } + return string(r[:max-1]) + "…" +} + +// ── rendering ──────────────────────────────────────────────────────────────── +// Split header / per-check line / verdict so the live path streams each check's +// result as it lands while the copy catalog renders the identical composed +// screen (renderSealResult). + +func renderSealHeader(p *ui.Printer, m sealModel) { + p.Section(fmt.Sprintf("Seal check — secure environment %q", m.envName)) + if m.fallback { + p.Infof("This chart doesn't mark a seal-check suite yet — running all of its checks instead.") + } + p.Newline() +} + +func renderSealCheckLine(p *ui.Printer, c sealCheck) { + if c.passed { + p.CheckLine("%s", c.name) + return + } + p.CrossLine("%s — %s", c.name, c.detail) + if c.hint != "" { + p.Hintf(" %s", c.hint) + } +} + +func renderSealVerdict(p *ui.Printer, m sealModel) { + if len(m.checks) == 0 { + // No conformance checks shipped — HONESTLY unknown. Never word this as + // sealed: nothing was verified. + p.Warnf("Seal unknown — this chart ships no conformance checks, so this environment's protections can't be verified. Not claiming sealed.") + p.Hintf("Upgrade your secure environment to a chart with the seal-check suite, then re-run `tracebloc client status --seal`.") + return + } + p.Newline() + // The default chart can ship a single conformance check (the enforcement + // probe only renders once the egress lockdown is enabled), so the singular + // wording is a common case, not an edge. + single := len(m.checks) == 1 + switch failed := m.failedCount(); { + case failed > 0 && single: + p.Errorf("Unsealed — the conformance check failed. This environment's protections are not enforced.") + case failed > 0: + p.Errorf("Unsealed — %d of %d conformance checks failed. This environment's protections are not all enforced.", failed, len(m.checks)) + case single: + p.Successf("Sealed — the conformance check passed. This environment's protections are enforced.") + default: + p.Successf("Sealed — all %d conformance checks passed. This environment's protections are enforced.", len(m.checks)) + } + if m.failedCount() > 0 { + p.Hintf("Fix the failing checks above, then re-run `tracebloc client status --seal` to confirm the seal.") + } +} + +// renderSealResult renders a complete seal screen from a model — what a full +// run prints, composed of the same pieces the live path streams. The copy +// catalog drives this for the sealed / unsealed / unknown states. +func renderSealResult(p *ui.Printer, m sealModel) { + renderSealHeader(p, m) + for _, c := range m.checks { + renderSealCheckLine(p, c) + } + renderSealVerdict(p, m) +} diff --git a/internal/cli/seal_test.go b/internal/cli/seal_test.go new file mode 100644 index 00000000..f4860972 --- /dev/null +++ b/internal/cli/seal_test.go @@ -0,0 +1,494 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "os/exec" + "strings" + "testing" + "time" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/helm" + "github.com/tracebloc/cli/internal/ui" +) + +// stubSealTarget fakes cluster resolution for the seal check (no kubeconfig, +// no apiserver): the release "acme" in namespace "acme" — the same fake-target +// pattern the data-delete tests use. The resolved context is deliberately +// distinct from any --context flag a test passes, so asserts can prove helm is +// pinned to the RESOLVED context, never the raw flag (Bugbot). +func stubSealTarget(t *testing.T) { + t.Helper() + orig := resolveClusterTargetFn + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _ bool) (*clusterTarget, error) { + return &clusterTarget{ + Resolved: &cluster.ResolvedConfig{Context: "resolved-ctx", Namespace: "acme"}, + Release: &cluster.ParentRelease{ReleaseName: "acme"}, + }, nil + } + t.Cleanup(func() { resolveClusterTargetFn = orig }) +} + +// stubSealHelm fakes the two helm seams: the hook listing and the per-check +// run. The run fake keys the scripted outcome on the CHECK hook (names[0]) +// and records every invocation's full filter list for aux-plumbing asserts. +type stubSealHelm struct { + hooks []helm.TestHook + listErr error + listGot helm.TestTarget + runs []string // names[0] of each run — the check under test + filters [][]string // the full names list of each run (check + aux) + runGot helm.TestTarget + runOut map[string]string + runErr map[string]error + timeouts []time.Duration +} + +func (s *stubSealHelm) install(t *testing.T) { + t.Helper() + origList, origRun := listTestHooksFn, runHelmTestFn + listTestHooksFn = func(_ context.Context, tt helm.TestTarget) ([]helm.TestHook, error) { + s.listGot = tt + return s.hooks, s.listErr + } + runHelmTestFn = func(_ context.Context, tt helm.TestTarget, names []string, timeout time.Duration) (string, error) { + s.runGot = tt + s.runs = append(s.runs, names[0]) + s.filters = append(s.filters, names) + s.timeouts = append(s.timeouts, timeout) + return s.runOut[names[0]], s.runErr[names[0]] + } + t.Cleanup(func() { listTestHooksFn, runHelmTestFn = origList, origRun }) +} + +// sealHooks is the chart's suite as docs/SEAL-CHECK.md labels it: conformance +// Jobs carrying tracebloc.io/seal-check=true + the seal-check-name identifier +// (one also carrying the CLI's optional hint annotation). +func sealHooks() []helm.TestHook { + return []helm.TestHook{ + {Kind: "Job", Name: "acme-egress-enforcement-check", SealCheck: true, SealName: "egress-enforcement", + SealHint: "ensure the CNI enforces egress NetworkPolicy, then re-run"}, + {Kind: "Job", Name: "acme-egress-reachability-check", SealCheck: true, SealName: "backend-reachability"}, + } +} + +func runSeal(t *testing.T, timeout time.Duration) (string, error) { + t.Helper() + var out bytes.Buffer + err := runSealCheck(context.Background(), ui.New(&out), cluster.KubeconfigOptions{ + Path: "/tmp/kc", Context: "kind-acme", + }, timeout) + return out.String(), err +} + +// Every check passes → Sealed, exit 0, per-check ✓ lines, and helm was pointed +// at the resolved release/namespace/kubeconfig — never the ambient context. +func TestSeal_AllPass_Sealed(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{hooks: sealHooks()} + s.install(t) + + out, err := runSeal(t, 90*time.Second) + if err != nil { + t.Fatalf("sealed run must exit 0, got: %v", err) + } + for _, want := range []string{ + `Seal check — secure environment "acme"`, + "✓ egress-enforcement", + "✓ backend-reachability", + "Sealed — all 2 conformance checks passed", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q:\n%s", want, out) + } + } + if len(s.runs) != 2 || s.runs[0] != "acme-egress-enforcement-check" || s.runs[1] != "acme-egress-reachability-check" { + t.Errorf("helm test runs = %v", s.runs) + } + // KubeContext is the RESOLVED context — never the raw --context flag + // ("kind-acme" here), which would be empty when omitted and let helm fall + // back to its own ambient resolution ($HELM_KUBECONTEXT included). + wantTT := helm.TestTarget{Release: "acme", Namespace: "acme", Kubeconfig: "/tmp/kc", KubeContext: "resolved-ctx"} + if s.listGot != wantTT || s.runGot != wantTT { + t.Errorf("helm targets = list %+v run %+v, want %+v", s.listGot, s.runGot, wantTT) + } + if s.timeouts[0] != 90*time.Second { + t.Errorf("per-check timeout = %v, want 90s", s.timeouts[0]) + } +} + +// One check fails → Unsealed (exit 2, silent — the verdict was already +// rendered), the OTHER check still runs (no first-failure abort), the failure +// line carries helm's distilled reason, and the hint is the chart's seal-hint. +func TestSeal_OneFails_Unsealed_AllChecksStillRun(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{ + hooks: sealHooks(), + runOut: map[string]string{ + "acme-egress-enforcement-check": "NAME: acme\nError: 1 error occurred:\n\t* job failed: BackoffLimitExceeded\n", + }, + runErr: map[string]error{"acme-egress-enforcement-check": errors.New("exit status 1")}, + } + s.install(t) + + out, err := runSeal(t, 0) + if got := ExitCodeFromError(err); got != exitChecksFailed { + t.Fatalf("exit code = %d, want %d", got, exitChecksFailed) + } + if !IsSilentError(err) { + t.Fatalf("verdict already rendered — the exit error must be silent, got: %v", err) + } + for _, want := range []string{ + "✗ egress-enforcement — job failed: BackoffLimitExceeded", + "ensure the CNI enforces egress NetworkPolicy, then re-run", + "✓ backend-reachability", // the suite kept going past the failure + "Unsealed — 1 of 2 conformance checks failed", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q:\n%s", want, out) + } + } + if strings.Contains(out, "Sealed — all") { + t.Errorf("a failed suite must never print the sealed verdict:\n%s", out) + } + if len(s.runs) != 2 { + t.Errorf("both checks must run despite the first failing, ran: %v", s.runs) + } +} + +// A failing check with no chart hint falls back to the kubectl-logs pointer +// (job/ for a Job hook), so the failure is still actionable. +func TestSeal_FailureHintFallsBackToKubectlLogs(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{ + hooks: []helm.TestHook{{Kind: "Job", Name: "acme-egress-reachability-check", SealCheck: true}}, + runErr: map[string]error{"acme-egress-reachability-check": errors.New("exit status 1")}, + } + s.install(t) + + out, _ := runSeal(t, 0) + if want := "see why: kubectl logs -n acme job/acme-egress-reachability-check"; !strings.Contains(out, want) { + t.Errorf("output missing the kubectl-logs hint %q:\n%s", want, out) + } +} + +// No runnable checks → honestly UNKNOWN: exit 2 (a script must not read +// "couldn't verify" as sealed), the word "sealed" only in the disclaimer. +// Both shapes: a chart with no test hooks at all, and one whose only test +// hooks are non-runnable plumbing (an SA alone verifies nothing). +func TestSeal_NoRunnableChecks_Unknown(t *testing.T) { + for name, hooks := range map[string][]helm.TestHook{ + "no hooks": nil, + "plumbing only": {{Kind: "ServiceAccount", Name: "acme-storage-assertions-sa"}}, + } { + t.Run(name, func(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{hooks: hooks} + s.install(t) + + out, err := runSeal(t, 0) + if got := ExitCodeFromError(err); got != exitChecksFailed { + t.Fatalf("exit code = %d, want %d", got, exitChecksFailed) + } + if !IsSilentError(err) { + t.Fatalf("unknown verdict is rendered, not errored — got: %v", err) + } + if !strings.Contains(out, "Seal unknown — this chart ships no conformance checks") { + t.Errorf("output missing the unknown verdict:\n%s", out) + } + if strings.Contains(out, "Sealed — all") { + t.Errorf("an unverifiable environment must never be claimed sealed:\n%s", out) + } + if len(s.runs) != 0 { + t.Errorf("nothing to run, but helm test ran: %v", s.runs) + } + }) + } +} + +// An older chart with test hooks but no seal labels → run ALL of its tests and +// say so (the fallback note), rather than claiming unknown while checks exist. +func TestSeal_UnlabeledHooks_FallbackRunsAll(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{hooks: []helm.TestHook{ + {Kind: "Job", Name: "acme-egress-enforcement-check"}, + {Kind: "Job", Name: "acme-egress-reachability-check"}, + }} + s.install(t) + + out, err := runSeal(t, 0) + if err != nil { + t.Fatalf("passing fallback suite must exit 0, got: %v", err) + } + if !strings.Contains(out, "doesn't mark a seal-check suite yet — running all of its checks") { + t.Errorf("output missing the fallback note:\n%s", out) + } + if len(s.runs) != 2 { + t.Errorf("fallback must run every test hook, ran: %v", s.runs) + } +} + +// When the chart labels a seal suite, ONLY the labelled checks run — unlabeled +// helper tests aren't part of the conformance verdict — and no fallback note. +func TestSeal_LabeledSubsetOnly(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{hooks: []helm.TestHook{ + {Kind: "Job", Name: "acme-egress-enforcement-check", SealCheck: true}, + {Kind: "Job", Name: "acme-smoke-helper"}, + }} + s.install(t) + + out, err := runSeal(t, 0) + if err != nil { + t.Fatalf("want exit 0, got: %v", err) + } + if len(s.runs) != 1 || s.runs[0] != "acme-egress-enforcement-check" { + t.Errorf("only the labelled check should run, ran: %v", s.runs) + } + if strings.Contains(out, "running all of its checks") { + t.Errorf("fallback note must not print when the chart labels a suite:\n%s", out) + } + if !strings.Contains(out, "Sealed — the conformance check passed") { + t.Errorf("verdict must count only the seal suite (singular wording for one check):\n%s", out) + } +} + +// Non-runnable test hooks — the storage check's ServiceAccount/RBAC plumbing +// at negative hook-weight — are never checks (no line, no verdict weight), but +// they must ride along in EVERY per-check filter: helm's --filter excludes +// unlisted test hooks, and running a check without its plumbing strands it and +// would report a false Unsealed. +func TestSeal_AuxPlumbingCarriedNotCounted(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{hooks: append(sealHooks(), + helm.TestHook{Kind: "ServiceAccount", Name: "acme-storage-assertions-sa"}, + helm.TestHook{Kind: "Role", Name: "acme-storage-assertions-role"}, + )} + s.install(t) + + out, err := runSeal(t, 0) + if err != nil { + t.Fatalf("want exit 0, got: %v", err) + } + if !strings.Contains(out, "Sealed — all 2 conformance checks passed") { + t.Errorf("aux hooks must not count as checks:\n%s", out) + } + if len(s.filters) != 2 { + t.Fatalf("want 2 check runs, got %v", s.filters) + } + for i, f := range s.filters { + want := []string{s.runs[i], "acme-storage-assertions-sa", "acme-storage-assertions-role"} + if strings.Join(f, ",") != strings.Join(want, ",") { + t.Errorf("run %d filter = %v, want %v (check first, then the aux plumbing)", i, f, want) + } + } +} + +// Hook enumeration failing is an ERROR (exit 1 with the cause), never a +// verdict: we don't know the environment's state, so we say neither sealed, +// unsealed, nor unknown. +func TestSeal_HookListError_NoVerdict(t *testing.T) { + stubSealTarget(t) + s := &stubSealHelm{listErr: errors.New("helm get hooks acme: exit status 1\nKubernetes cluster unreachable")} + s.install(t) + + out, err := runSeal(t, 0) + if err == nil || !strings.Contains(err.Error(), "couldn't read the chart's conformance checks") { + t.Fatalf("want the enumeration failure surfaced, got: %v", err) + } + if got := ExitCodeFromError(err); got != exitFailure { + t.Errorf("exit code = %d, want %d", got, exitFailure) + } + for _, verdict := range []string{"Sealed", "Unsealed", "Seal unknown"} { + if strings.Contains(out, verdict) { + t.Errorf("no verdict may render when the checks couldn't be read; got %q in:\n%s", verdict, out) + } + } +} + +// A cluster-resolution failure propagates with its own exit contract (here the +// §7.3 binding-miss rewrite path returns exit 4) — the seal check adds nothing. +func TestSeal_ResolveErrorPropagates(t *testing.T) { + orig := resolveClusterTargetFn + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _ bool) (*clusterTarget, error) { + return nil, &exitError{code: exitNoWorkspace, err: errors.New("no tracebloc client found in namespace \"acme\"")} + } + t.Cleanup(func() { resolveClusterTargetFn = orig }) + s := &stubSealHelm{} + s.install(t) + + _, err := runSeal(t, 0) + if got := ExitCodeFromError(err); got != exitNoWorkspace { + t.Fatalf("exit code = %d, want %d (%v)", got, exitNoWorkspace, err) + } +} + +// Ctrl-C while the hooks are being enumerated exits quietly (130), not as an +// enumeration failure — same quiet-exit contract as mid-suite below (Bugbot). +func TestSeal_CancelledDuringListing_QuietExit(t *testing.T) { + stubSealTarget(t) + ctx, cancel := context.WithCancel(context.Background()) + origList := listTestHooksFn + listTestHooksFn = func(_ context.Context, _ helm.TestTarget) ([]helm.TestHook, error) { + cancel() // the user hits Ctrl-C while `helm get hooks` runs + return nil, errors.New("context canceled") + } + t.Cleanup(func() { listTestHooksFn = origList }) + + var out bytes.Buffer + err := runSealCheck(ctx, ui.New(&out), cluster.KubeconfigOptions{}, 0) + if got := ExitCodeFromError(err); got != exitInterrupted { + t.Fatalf("exit code = %d, want %d (%v)", got, exitInterrupted, err) + } + if !IsSilentError(err) { + t.Fatalf("Ctrl-C must exit quietly, got: %v", err) + } + if out.Len() != 0 { + t.Errorf("nothing may render on a cancelled enumeration, got:\n%s", out.String()) + } +} + +// Ctrl-C mid-suite exits quietly (130) with NO verdict: the remaining checks +// didn't fail — they never ran — and a fake Unsealed would misstate the +// environment. +func TestSeal_CancelledMidSuite_NoVerdict(t *testing.T) { + stubSealTarget(t) + ctx, cancel := context.WithCancel(context.Background()) + origList, origRun := listTestHooksFn, runHelmTestFn + listTestHooksFn = func(_ context.Context, _ helm.TestTarget) ([]helm.TestHook, error) { + return sealHooks(), nil + } + ran := 0 + runHelmTestFn = func(_ context.Context, _ helm.TestTarget, _ []string, _ time.Duration) (string, error) { + ran++ + cancel() // the user hits Ctrl-C while the first check runs + return "", errors.New("context canceled") + } + t.Cleanup(func() { listTestHooksFn, runHelmTestFn = origList, origRun }) + + var out bytes.Buffer + err := runSealCheck(ctx, ui.New(&out), cluster.KubeconfigOptions{}, 0) + if got := ExitCodeFromError(err); got != exitInterrupted { + t.Fatalf("exit code = %d, want %d (%v)", got, exitInterrupted, err) + } + if !IsSilentError(err) { + t.Fatalf("Ctrl-C must exit quietly, got: %v", err) + } + if ran != 1 { + t.Errorf("no further checks may run after cancellation, ran %d", ran) + } + for _, verdict := range []string{"Sealed", "Unsealed", "Seal unknown"} { + if strings.Contains(out.String(), verdict) { + t.Errorf("no verdict may render on a cancelled run; got %q in:\n%s", verdict, out.String()) + } + } +} + +// The Ctrl-C race the ctx.Err()-only check missed: on a terminal Ctrl-C the helm +// child can exit 130 (128+SIGINT) BEFORE NotifyContext flips ctx.Err(). The +// cancel site must still treat that as a quiet interrupt (exit 130, no verdict) +// and never let the failed check render a false Unsealed. It reuses the same +// installerRunInterrupted helper as upgrade/prepare-host (Bugbot #397). +func TestSeal_CtrlCExit130BeforeCtxFlips_NoFalseUnsealed(t *testing.T) { + if _, err := exec.LookPath("bash"); err != nil { + t.Skip("bash not available to synthesize an exit-130 ExitError") + } + // A real *exec.ExitError with code 130 — exactly what helm.Runner surfaces + // (CombinedOutput returns the exec error unwrapped) when the child dies on SIGINT. + exit130 := exec.Command("bash", "-c", "exit 130").Run() + + stubSealTarget(t) + origList, origRun := listTestHooksFn, runHelmTestFn + listTestHooksFn = func(_ context.Context, _ helm.TestTarget) ([]helm.TestHook, error) { + return sealHooks(), nil + } + ran := 0 + runHelmTestFn = func(_ context.Context, _ helm.TestTarget, _ []string, _ time.Duration) (string, error) { + ran++ + // ctx is deliberately NOT cancelled — this is the race window where the + // child already exited 130 but NotifyContext hasn't flipped ctx.Err() yet. + return "", exit130 + } + t.Cleanup(func() { listTestHooksFn, runHelmTestFn = origList, origRun }) + + var out bytes.Buffer + err := runSealCheck(context.Background(), ui.New(&out), cluster.KubeconfigOptions{}, 0) + if got := ExitCodeFromError(err); got != exitInterrupted { + t.Fatalf("exit code = %d, want %d — a Ctrl-C (helm exit 130) must be a quiet interrupt, not a verdict: %v", got, exitInterrupted, err) + } + if !IsSilentError(err) { + t.Fatalf("Ctrl-C must exit quietly, got: %v", err) + } + if ran != 1 { + t.Errorf("no further checks may run after the interrupt, ran %d", ran) + } + for _, verdict := range []string{"Sealed", "Unsealed", "Seal unknown"} { + if strings.Contains(out.String(), verdict) { + t.Errorf("no verdict (esp. a false Unsealed) may render on a Ctrl-C run; got %q in:\n%s", verdict, out.String()) + } + } +} + +// helmFailureDetail distills helm's combined output into the one-line reason: +// the specific hook bullet first, then the Error: line, then the raw error — +// and never an empty string for a failure. +func TestHelmFailureDetail(t *testing.T) { + cases := []struct { + name, out string + err error + want string + }{ + {"bullet wins", "Error: 1 error occurred:\n\t* job failed: BackoffLimitExceeded\n", errors.New("exit status 1"), "job failed: BackoffLimitExceeded"}, + {"error line when no bullet", "Error: timed out waiting for the condition\n", errors.New("exit status 1"), "timed out waiting for the condition"}, + {"raw error fallback", "", errors.New("context deadline exceeded\nmore"), "context deadline exceeded"}, + {"nil error is not a failure", "whatever", nil, ""}, + } + for _, c := range cases { + if got := helmFailureDetail(c.out, c.err); got != c.want { + t.Errorf("%s: helmFailureDetail = %q, want %q", c.name, got, c.want) + } + } + if got := helmFailureDetail(strings.Repeat("* x", 1)+strings.Repeat("y", 300), errors.New("x")); len([]rune(got)) > 140 { + t.Errorf("detail not capped to one readable line: %d runes", len([]rune(got))) + } +} + +// Cobra-level flag guards: the modes are mutually exclusive, and flags that +// only one mode consumes are rejected without it instead of silently ignored. +func TestSeal_FlagGuards(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + cases := []struct { + args []string + want string + }{ + {[]string{"client", "status", "--wait", "--seal"}, "--wait and --seal are separate modes"}, + {[]string{"client", "status", "--timeout", "5s"}, "--timeout has no effect without --wait or --seal"}, + {[]string{"client", "status", "--namespace", "acme"}, "--namespace has no effect without --seal"}, + {[]string{"client", "status", "--kubeconfig", "/tmp/kc"}, "--kubeconfig has no effect without --seal"}, + {[]string{"client", "status", "--context", "kind"}, "--context has no effect without --seal"}, + } + for _, c := range cases { + _, err := runCmd(t, c.args...) + if err == nil || !strings.Contains(err.Error(), c.want) { + t.Errorf("%v: err = %v, want %q", c.args, err, c.want) + } + } +} + +// `client status --seal --timeout` is a valid pair (the per-check budget) and +// must reach the seal path, not the --timeout guard. +func TestSeal_TimeoutWithSealAccepted(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + stubSealTarget(t) + s := &stubSealHelm{hooks: sealHooks()} + s.install(t) + + if _, err := runCmd(t, "client", "status", "--seal", "--timeout", "45s"); err != nil { + t.Fatalf("--seal --timeout: %v", err) + } + if len(s.timeouts) == 0 || s.timeouts[0] != 45*time.Second { + t.Errorf("per-check timeouts = %v, want 45s", s.timeouts) + } +} diff --git a/internal/cli/testdata/golden/00-home.golden b/internal/cli/testdata/golden/00-home.golden index e2166ef9..b0c0be23 100644 --- a/internal/cli/testdata/golden/00-home.golden +++ b/internal/cli/testdata/golden/00-home.golden @@ -236,18 +236,20 @@ Usage: tracebloc [command] Available Commands: - auth Inspect tracebloc authentication state - client Provision this machine's tracebloc client - cluster Inspect the cluster the CLI is currently targeting - completion Generate the autocompletion script for the specified shell - data Manage the datasets in your secure environment - delete Offboard this machine from tracebloc (revoke, uninstall, reclaim disk) - doctor Check your secure environment is connected and ready to run training - help Help about any command - login Sign in to tracebloc in your browser (device flow) - logout Sign out (revoke the token server-side and clear it locally) - resources Show how much of this machine tracebloc may use - version Print the tracebloc CLI version, git SHA, and build date + auth Inspect tracebloc authentication state + client Provision this machine's tracebloc client + cluster Inspect the cluster the CLI is currently targeting + completion Generate the autocompletion script for the specified shell + data Manage the datasets in your secure environment + delete Offboard this machine from tracebloc (revoke, uninstall, reclaim disk) + doctor Check your secure environment is connected and ready to run training + help Help about any command + login Sign in to tracebloc in your browser (device flow) + logout Sign out (revoke the token server-side and clear it locally) + prepare-host Prepare this machine so a non-admin user can install tracebloc (run once, as an administrator) + resources Show how much of this machine tracebloc may use + upgrade Update tracebloc to the latest release + version Print the tracebloc CLI version, git SHA, and build date Flags: -h, --help help for tracebloc diff --git a/internal/cli/testdata/golden/08-client.golden b/internal/cli/testdata/golden/08-client.golden index 6ac292bf..8a94809e 100644 --- a/internal/cli/testdata/golden/08-client.golden +++ b/internal/cli/testdata/golden/08-client.golden @@ -1,9 +1,12 @@ tb client — register / list / inspect environments ================================================== What you see under `tb client`. `tb client create` shows a review (below) before -it registers a new secure environment. `tb client list` / `tb client status` read -live backend state, so they aren't stable screens — their strings are in -zz-all-strings.golden. +it registers a new secure environment. `tb client status --seal` verifies the +environment's protections by running the chart's conformance checks (the seal +check) — all three verdicts are shown: sealed, unsealed (with the per-check +failure + fix hint), and unknown (a chart with no checks is honestly NOT called +sealed). `tb client list` / plain `tb client status` read live backend state, so +they aren't stable screens — their strings are in zz-all-strings.golden. $ tb client create # review, before you confirm @@ -13,6 +16,33 @@ $ tb client create # review, before you confirm location: DE cluster: a1b2c3d4 (anchors this client — re-runs adopt it) +$ tb client status --seal # sealed — every conformance check passed + + Seal check — secure environment "lukas-macbook" + + ✓ egress-enforcement + ✓ backend-reachability + + ✔ Sealed — all 2 conformance checks passed. This environment's protections are enforced. + +$ tb client status --seal # unsealed — a protection is not enforced + + Seal check — secure environment "lukas-macbook" + + ✗ egress-enforcement — job failed: BackoffLimitExceeded + see why: kubectl logs -n lukas-macbook job/lukas-macbook-egress-enforcement-check + ✓ backend-reachability + + ✖ Unsealed — 1 of 2 conformance checks failed. This environment's protections are not all enforced. + Fix the failing checks above, then re-run `tracebloc client status --seal` to confirm the seal. + +$ tb client status --seal # unknown — this chart ships no conformance checks + + Seal check — secure environment "lukas-macbook" + + ⚠ Seal unknown — this chart ships no conformance checks, so this environment's protections can't be verified. Not claiming sealed. + Upgrade your secure environment to a chart with the seal-check suite, then re-run `tracebloc client status --seal`. + ------------------------------------------------------------ --help @@ -78,13 +108,34 @@ Report tracebloc's view of this machine's active client — online, offline, or pending. With --wait, poll until tracebloc reports it online (exit 0) or the timeout elapses (non-zero), to confirm the client connected after setup. +With --seal, verify the environment's protections instead: run the chart's +conformance checks (its helm tests — the seal check, RFC-0003) against this +machine's secure environment and report the verdict with per-check detail: + + sealed every conformance check passed + unsealed a check failed or couldn't run — a protection is not enforced + unknown the chart ships no conformance checks, so nothing was verified + +Only a fully-passed suite exits 0; an environment that can't be verified is +never claimed sealed. + +Exit codes with --seal: + 0 sealed — every conformance check passed + 2 unsealed (a check failed), or unknown (the chart has no checks) + 3 kubeconfig could not be loaded / cluster unreachable + 4 cluster reachable but no tracebloc client found there + Usage: tracebloc client status [flags] Flags: - -h, --help help for status - --timeout duration with --wait, give up after this long (default 2m0s) - --wait poll until tracebloc reports this client online + --context string with --seal: name of the kubeconfig context to use (default: kubeconfig's current-context) + -h, --help help for status + --kubeconfig string with --seal: path to the kubeconfig file (default: $KUBECONFIG, then ~/.kube/config) + -n, --namespace string with --seal: namespace where your tracebloc client is installed + --seal verify this environment's protections: run the chart's conformance checks and report sealed / unsealed + --timeout duration with --wait, give up after this long; with --seal, the time budget per check (default 2m0s) + --wait poll until tracebloc reports this client online Global Flags: --plain disable color and decorative output (also honors $NO_COLOR) diff --git a/internal/cli/testdata/golden/11-upgrade.golden b/internal/cli/testdata/golden/11-upgrade.golden new file mode 100644 index 00000000..078f8efc --- /dev/null +++ b/internal/cli/testdata/golden/11-upgrade.golden @@ -0,0 +1,31 @@ +tb upgrade — update to the latest release +========================================= +What you see when you run `tb upgrade`. On Linux/macOS it re-runs the +official installer (signature-verified) to update the CLI and your secure +environment together, so they never drift apart. On Windows a running .exe is +locked and install.ps1 is CLI-only, so it prints the command to run in a fresh +shell instead. The update-check nudge and the "CLI too old" (426) message both +point here. The installer's own live output streams during the run (not a stable +screen); its copy is in the client repo's installer catalog. + + +------------------------------------------------------------ +--help +------------------------------------------------------------ +$ tracebloc upgrade --help +Updates tracebloc to the latest release by re-running the official +installer. It verifies signatures (cosign) and replaces the CLI; on Linux/macOS +it also upgrades your secure environment's services to match, so the CLI and the +environment never drift apart. On Windows it prints the command to update the +CLI (a running executable can't replace itself, so you run it in a fresh shell). +Safe to run anytime; safe to re-run. + +Usage: + tracebloc upgrade [flags] + +Flags: + -h, --help help for upgrade + +Global Flags: + --plain disable color and decorative output (also honors $NO_COLOR) + --verbose stream detailed step-by-step progress (also via $TRACEBLOC_LOG_LEVEL=debug) diff --git a/internal/cli/testdata/golden/12-prepare-host.golden b/internal/cli/testdata/golden/12-prepare-host.golden new file mode 100644 index 00000000..ec5fcda5 --- /dev/null +++ b/internal/cli/testdata/golden/12-prepare-host.golden @@ -0,0 +1,42 @@ +tb prepare-host — one-time admin step so a non-admin can install +================================================================ +What you see when you run `tb prepare-host` — the one-time administrator step +that readies a shared / HPC host so a non-admin user can then install tracebloc +with no root. It re-runs the installer's verified prepare-host step; the +privileged prep + its progress stream from the installer (not CLI copy). Only the +--help is byte-exact below. + + +------------------------------------------------------------ +--help +------------------------------------------------------------ +$ tracebloc prepare-host --help +Prepares a host that a non-admin user can't install on directly. + +Run this ONCE, as an administrator, on a machine where the person who will use +tracebloc has no root or sudo — a shared server, an HPC login node. It installs +the container runtime and its prerequisites. + +Pass that person's username to also grant them container-runtime (docker-group) +access, so they can then install tracebloc at Tier 0 with no administrator +rights at all: + + sudo tracebloc prepare-host alice + +Without a username it installs only the runtime + prerequisites and tells you +how to grant a user access afterwards. NOTE: the username is the RESEARCHER who +will use tracebloc — not you, the admin running this. + +It re-runs the official installer's prepare-host step (verified with cosign). It +does NOT create your secure environment or sign you in — it only prepares the +host, so it's safe to run on a shared machine. Safe to re-run. + +Usage: + tracebloc prepare-host [researcher-username] [flags] + +Flags: + -h, --help help for prepare-host + +Global Flags: + --plain disable color and decorative output (also honors $NO_COLOR) + --verbose stream detailed step-by-step progress (also via $TRACEBLOC_LOG_LEVEL=debug) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 009e7131..f2341e53 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -48,6 +48,7 @@ screen. %s/%d are runtime placeholders. "%s of %s GiB" "%s of %s cores" "%s requires CLIENT_WRITE permission" +"%s still present after removal" "%s unreachable: %v" "%s · %d" "%s · Online%s" @@ -56,6 +57,7 @@ screen. %s/%d are runtime placeholders. "%s · running, but tracebloc hasn't heard from it — run %s" "%s · starting up, not ready yet — run %s" "%s ×%d" +"%s — %s" "%s, … and %d more" "%s/%s" "%s: %w" @@ -73,16 +75,19 @@ screen. %s/%d are runtime placeholders. ", %s=%s" "- %s, age %s%s" "-%02d" +"--%s has no effect without --seal" "--label-column doesn't apply to task %q — it trains on the text itself, with no label column" "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q" "--overwrite can't be combined with --idempotency-key: a reused key makes the cluster replay the previous run instead of ingesting the new data — after --overwrite's removal that would report success while loading nothing. Drop one of the two (a fresh per-run key is the default)." "--schema is empty; expected col:TYPE,col:TYPE,..." "--schema is tabular/time-series tasks only; it doesn't apply to task %q" "--time-column is time_to_event_prediction only; it doesn't apply to task %q" -"--timeout has no effect without --wait" +"--timeout has no effect without --wait or --seal" +"--wait and --seal are separate modes — run them one at a time" "0:%d" "3 GiB" "A dataset named %q already exists — replace it?" +"A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade" "A real run continues with step 2 (copy into your secure environment) and step 3 (validate and load)." "A tracebloc client is already running on this cluster — adopting it. Couldn't read the cluster identity, so its idempotency anchor was left unchanged; point --kubeconfig/--context at a cluster where kube-system is readable to stamp it." "A training run is allocated up to:" @@ -95,21 +100,25 @@ screen. %s/%d are runtime placeholders. "CSV %s has no columns" "Can't reach tracebloc from here." "Cancelled — %q was left as-is; nothing was ingested." +"Cancelled — %s" "Cancelled — nothing was changed." "Cancelled — nothing was deleted." "Cancelled — nothing was ingested." +"Cancelled — nothing was provisioned." +"Cancelled — nothing was removed." "Cancelled — the name didn't match. Nothing was removed." -"Cancelled." "Chart uninstall reported: %v" "Check on it later with: kubectl logs -f -n %s job/%s" "Check your data" "Check your network / HTTP(S)_PROXY, then run `%s doctor` again." +"Checking %s…" "Client install" "Client status" "Clients in your account" "Cluster teardown reported: %v" "Column types" "Connected to tracebloc" +"Connecting to your secure environment to submit the run" "Connecting to your secure environment…" "Copy into your secure environment" "Copying %s" @@ -128,6 +137,8 @@ screen. %s/%d are runtime placeholders. "Couldn't verify your session with the backend (%v)." "Couldn't write the support bundle: %v" "Credential written to %s (mode 0600, not shown). This machine is set to enroll as client %d." +"Ctrl-C to cancel" +"Ctrl-C to stop watching — the run keeps going on the cluster" "DB failures" "DROP TABLE IF EXISTS `%s`.`%s`" "Datasets in %s (0)" @@ -152,10 +163,13 @@ screen. %s/%d are runtime placeholders. "Ensure your kubeconfig user can list nodes." "Enter" "Everything looks good — you're ready to run training." +"Fix the failing checks above, then re-run `tracebloc client status --seal` to confirm the seal." "Follow it later with: kubectl logs -f -n %s job/%s" "For help: https://docs.tracebloc.io/create-use-case/prepare-dataset" "Found labels.csv and a %s folder — this looks like text data." -"Free some up, or raise the machine's allocation in Docker Desktop → Resources." +"Free some up on this machine, or %s." +"Free some up, give Docker more memory (WSL2 backend: `[wsl2] memory=…` in %%UserProfile%%\\.wslconfig then `wsl --shutdown`; Hyper-V backend: Docker Desktop → Settings → Resources → Advanced), or %s." +"Free some up, raise the machine's allocation in Docker Desktop → Settings → Resources → Advanced, or %s." "Full log: %s" "GPU access removed — training runs will use CPU only." "Give the path to a file or a folder — whichever holds your data:" @@ -218,11 +232,14 @@ screen. %s/%d are runtime placeholders. "Pending > %s: %v" "Pick this dataset when you set it up." "Please name the dataset." +"Preparing this host and granting %s container-runtime access — re-running the installer's prepare-host step (needs administrator rights once)." +"Preparing this host — re-running the installer's prepare-host step (installs the container runtime and prerequisites; needs administrator rights once). Pass a researcher's username to also grant them access: tracebloc prepare-host " "Private image pulls will ImagePullBackOff. Reinstall the chart with valid registry credentials." "Proceed with the ingest?" "Provision this client?" "Provisioned client %q (namespace %s)." "Provisioning didn't complete. Re-running is safe — on the same cluster it adopts the existing client instead of minting a duplicate (idempotent):" +"Reading your files" "Reading your files locally first — nothing has touched your secure environment yet — so a layout or settings problem shows up right away." "Ready for `tracebloc data ingest`." "Ready to run training" @@ -230,6 +247,7 @@ screen. %s/%d are runtime placeholders. "Ready to run training — couldn't check free compute (run with --verbose)" "Ready to run training — couldn't check your workloads (run with --verbose)" "Reclaimed tracebloc's downloaded images." +"Reclaiming the temporary copy" "Recreate it as a docker-registry secret (kubectl create secret docker-registry)." "Recreate the registry secret; its .dockerconfigjson isn't valid JSON." "Regression targets are continuous. 'bucket' groups them into ranges before they leave the cluster; 'passthrough' keeps raw values." @@ -246,6 +264,10 @@ screen. %s/%d are runtime placeholders. "Run '%s --help' for the available commands." "SELECT '%s',%s,COUNT(*),%s,%s FROM `%s`.`%s`" "SELECT table_name FROM information_schema.tables WHERE table_schema='%s' ORDER BY table_name" +"Seal check — secure environment %q" +"Seal unknown — this chart ships no conformance checks, so this environment's protections can't be verified. Not claiming sealed." +"Sealed — all %d conformance checks passed. This environment's protections are enforced." +"Sealed — the conformance check passed. This environment's protections are enforced." "Secure environment %q" "Set one up: %s" "Sign in to tracebloc" @@ -257,12 +279,13 @@ screen. %s/%d are runtime placeholders. "Signed out locally, but couldn't revoke the token server-side (%v). Revoke from the dashboard if this was a shared machine." "Signed out." "Signed-in token was rejected by the backend — run `tracebloc login`." -"Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. Free some up in Docker Desktop → Resources, then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`." +"Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. %s Then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`." "Some tracebloc images couldn't be reclaimed (harmless) — remove them later with `docker rmi $(docker images --filter=reference='ghcr.io/tracebloc/*' --format '{{.Repository}}:{{.Tag}}')`." "Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`." "Stopped following after 1 hour — the ingestion is still running and will finish on its own." "Stopped watching — the ingestion keeps running on your secure environment." "Submitted — tracebloc is validating your data and loading it into the table." +"Submitting the run" "Submitting the run — with --detach it keeps running on your secure environment after this command returns; the reconnect command is shown below." "Submitting the run, then following along as tracebloc validates your data and loads it into the table — progress streams below." "System · %d" @@ -278,6 +301,7 @@ screen. %s/%d are runtime placeholders. "The size your images already are, as WxH — tracebloc checks every image matches and never resizes. Press Enter to read it from your first image. e.g. 224x224" "The tracebloc CLI (your local data & config are kept — --keep-data)" "This CLI is out of date — update it: %s" +"This chart doesn't mark a seal-check suite yet — running all of its checks instead." "This cluster is already registered as client %q (namespace %s) — adopted it." "This drops the table and removes the files listed above — there's no undo. Pass --yes next time to skip this prompt." "This follows the run for up to an hour; a longer run keeps going on its own (or start it with --detach and check back later)." @@ -289,14 +313,22 @@ screen. %s/%d are runtime placeholders. "This will remove" "Time column" "To train or benchmark models on it, create a use case at https://ai.tracebloc.io/my-use-cases" +"To update tracebloc on Windows, run this in a new PowerShell window:" "Training results can't reach tracebloc — experiments will stall." "Try again shortly; if it persists, email support@tracebloc.io with `%s doctor --diagnose`." "Type %q to offboard this machine" "Uninstalled tracebloc." +"Unsealed — %d of %d conformance checks failed. This environment's protections are not all enforced." +"Unsealed — the conformance check failed. This environment's protections are not enforced." +"Updating tracebloc — re-running the installer (verifies signatures, then updates the CLI and your secure environment)." +"Upgrade your secure environment to a chart with the seal-check suite, then re-run `tracebloc client status --seal`." "Use the GPU for training runs?" "VARCHAR(%d)" "Validate and load" "Verify the requests-proxy is wired: kubectl set env deploy/-jobs-manager --list | grep PROXY" +"Waiting for the ingestion to start (scheduling + pulling the image)" +"Waiting for tracebloc to confirm…" +"Waiting for your browser…" "We couldn't tell from the layout — tabular = a CSV table; image = labels.csv + images/; text = labels.csv + texts/." "We infer each column's type from your CSV. Press Enter to accept, or type overrides like age:INT,price:FLOAT." "Welcome to your secure environment for AI, %s 👋" @@ -355,6 +387,7 @@ screen. %s/%d are runtime placeholders. "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit" "couldn't read capacity: %v" "couldn't read jobs-manager to resolve image pull secrets — skipping" +"couldn't read the chart's conformance checks: %w" "couldn't read this machine's capacity: %w" "cpu=%s, memory=%s" "crash-looping: %v" @@ -379,6 +412,7 @@ screen. %s/%d are runtime placeholders. "e.g. 17 for COCO pose" "e.g. age:INT,price:FLOAT" "e.g. ~/data/patients.csv or ~/data/xray/" +"empty tag_name in release response" "enter a whole number between %d and %d" "exactly %d" "exec stream against %s/%s: %w" @@ -391,6 +425,7 @@ screen. %s/%d are runtime placeholders. "generating Pod-name random suffix: %w" "generating idempotency key: %w" "generating staging-dir suffix: %w" +"github releases: HTTP %d" "how many CPU cores a single training run may use" "how much memory a single training run may use, in GiB" "http://%s.%s.svc.cluster.local:%d" @@ -410,6 +445,7 @@ screen. %s/%d are runtime placeholders. "interactive setup: %w" "internal: re-parsing synthesized spec: %w\n%s" "invalid table name %q: %w" +"invalid username %q — expected a Linux username (letters, digits, '.', '_', '-')" "jobs-manager" "jobs-manager %s returned HTTP %d: %s" "jobs-manager has no literal REQUESTS_PROXY_URL (chart too old, or it's set via a configMap/secret ref)" @@ -473,6 +509,8 @@ screen. %s/%d are runtime placeholders. "pod %s container %s restarted %d times" "port-forward allocated zero ports" "port-forward to %s/%s failed during startup: %w" +"prepare-host didn't complete (%w). You can run the installer directly:\n %s" +"prepare-host readies a Linux server or HPC login node so a non-admin user can install tracebloc without root — it doesn't apply to Windows. Run it as an administrator on the Unix host the researcher will use." "pvc path" "querying datasets: %w%s" "reading %q: %w" @@ -520,6 +558,7 @@ screen. %s/%d are runtime placeholders. "schema entry %q must be col:TYPE (e.g. age:INT,price:FLOAT)" "secret %q has an empty or malformed %s" "secret %q is type %q, not %s" +"see why: kubectl logs -n %s %s" "sent to API" "server" "service %s/%s has no selector — can't resolve to a Pod for port-forwarding" @@ -530,6 +569,7 @@ screen. %s/%d are runtime placeholders. "shared PVC: %s (%s)" "sign-in was denied in the browser" "signed in" +"size runs to this machine with `%s resources set max`" "skipped" "source" "stage Pod %s/%s did not become Ready within %s%s" @@ -578,7 +618,9 @@ screen. %s/%d are runtime placeholders. "train" "unavailable" "unknown command %q for %q" +"upgrade didn't complete (%w). You can run the installer directly:\n %s" "values:" +"verifying removal of %s: %w" "waiting for ingestor Pod: %w" "waiting for staging-cleanup pod: %w" "waiting for teardown pod: %w" @@ -590,3 +632,4 @@ screen. %s/%d are runtime placeholders. "~%s (requested; server may cap shorter)" "· %d GPU" "· %d classes" +"— this machine could give a run up to cpu=%d,memory=%dGi ('tracebloc resources set max')" diff --git a/internal/cli/update_check.go b/internal/cli/update_check.go new file mode 100644 index 00000000..8f317c48 --- /dev/null +++ b/internal/cli/update_check.go @@ -0,0 +1,242 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strconv" + "strings" + "time" + + "golang.org/x/term" + + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/ui" +) + +// Update-check: a quiet, throttled nudge so a customer who ran the installer +// once doesn't silently sit on an old CLI forever (backlog F1). We check the +// latest GitHub release at most once per updateCheckInterval, cache the result +// next to the config, and — only when a newer release exists — print one dim +// line pointing at `tracebloc upgrade`. It is strictly best-effort: it never +// blocks meaningfully (the network call is capped at updateCheckTimeout), never +// errors out, and stays silent on a dev build, off a terminal, in CI, or when +// TRACEBLOC_NO_UPDATE_CHECK is set. Applying the update is a separate, explicit +// step (`tracebloc upgrade`); we never touch the binary here. +var ( + updateCheckInterval = 24 * time.Hour + updateCheckTimeout = 2 * time.Second + // var (not const) so tests can point it at an httptest server. + latestReleaseURL = "https://api.github.com/repos/tracebloc/cli/releases/latest" +) + +const updateCacheFile = "update-check.json" + +// updateCache is the throttle state: when we last asked GitHub and what it said, +// so we hit the network at most once per updateCheckInterval. +type updateCache struct { + CheckedAt time.Time `json:"checked_at"` + Latest string `json:"latest"` // normalized, e.g. "0.9.7" (no leading v) +} + +// MaybeNotifyUpdate prints a one-line "newer version available" nudge to w when +// the running CLI is behind the latest release. Called once from main after the +// command runs. Safe to call unconditionally — it gates itself. +func MaybeNotifyUpdate(version string, w io.Writer) { + if !updateChecksAllowed(version, w) { + return + } + latest := latestReleaseVersion() + if latest == "" || !versionLess(version, latest) { + return + } + p := ui.New(w) + p.Newline() + p.Infof("A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade", latest, version) +} + +// updateChecksAllowed gates the nudge: real release build, an interactive +// terminal on w, not CI, not opted out. +func updateChecksAllowed(version string, w io.Writer) bool { + if os.Getenv("TRACEBLOC_NO_UPDATE_CHECK") != "" || os.Getenv("CI") != "" { + return false + } + if version == "" || version == "dev" || strings.HasPrefix(version, "dev") { + return false // not a release build — nothing meaningful to compare + } + f, ok := w.(*os.File) + return ok && term.IsTerminal(int(f.Fd())) +} + +// latestReleaseVersion returns the latest release version (normalized), using +// the on-disk cache when it's fresh and only hitting the network otherwise. On a +// network failure it falls back to any cached value, else "". +func latestReleaseVersion() string { + path := updateCachePath() + if c, ok := readUpdateCache(path); ok && time.Since(c.CheckedAt) < updateCheckInterval { + return c.Latest + } + // No fresh cache. If the config dir is absent, the throttle can't be persisted + // (writeUpdateCache won't recreate it — Bugbot #404), so fetching here would + // repeat on EVERY command and burn updateCheckTimeout each time. A missing dir + // also means the CLI isn't set up (fresh install) or was offboarded — nothing + // to nudge about. Skip the network check entirely. The dir-present but + // stale/unreadable case still falls through to the throttled fetch below, so + // this doesn't defeat the normal path (Bugbot #397). + if !configDirExists(path) { + return "" + } + latest, err := fetchLatestRelease(latestReleaseURL, updateCheckTimeout) + if err != nil { + if c, ok := readUpdateCache(path); ok { + // Stale, but better than nothing. Re-stamp CheckedAt to now so the + // once-per-interval throttle actually holds — otherwise the cache + // stays expired and every command re-hits the network (and eats the + // full updateCheckTimeout) while offline or rate-limited. + _ = writeUpdateCache(path, updateCache{CheckedAt: time.Now(), Latest: c.Latest}) + return c.Latest + } + return "" + } + _ = writeUpdateCache(path, updateCache{CheckedAt: time.Now(), Latest: latest}) + return latest +} + +// fetchLatestRelease reads the tag_name of the latest GitHub release, time-boxed. +func fetchLatestRelease(url string, timeout time.Duration) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return "", err + } + req.Header.Set("Accept", "application/vnd.github+json") + resp, err := http.DefaultClient.Do(req) + if err != nil { + return "", err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("github releases: HTTP %d", resp.StatusCode) + } + var body struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<16)).Decode(&body); err != nil { + return "", err + } + v := normalizeVersion(body.TagName) + if v == "" { + return "", fmt.Errorf("empty tag_name in release response") + } + return v, nil +} + +func updateCachePath() string { + dir, err := config.Dir() + if err != nil { + return "" + } + return filepath.Join(dir, updateCacheFile) +} + +// configDirExists reports whether the tracebloc config dir (the parent of the +// update cache) is present — the single gate for "can the update-check throttle +// be persisted?". When it's absent (a fresh install, or a wiped/offboarded +// ~/.tracebloc) the cache can neither be read nor written, so the caller must +// no-op: latestReleaseVersion skips the network check (so a fetch isn't repeated +// unthrottled on every command — Bugbot #397) and writeUpdateCache skips the +// write (so a throttle cache never resurrects a just-wiped dir — Bugbot #404). +// An empty path (config.Dir() failed) counts as absent. +func configDirExists(cachePath string) bool { + if cachePath == "" { + return false + } + _, err := os.Stat(filepath.Dir(cachePath)) + return err == nil +} + +func readUpdateCache(path string) (updateCache, bool) { + if path == "" { + return updateCache{}, false + } + raw, err := os.ReadFile(path) + if err != nil { + return updateCache{}, false + } + var c updateCache + if json.Unmarshal(raw, &c) != nil || c.Latest == "" { + return updateCache{}, false + } + return c, true +} + +// writeUpdateCache persists the throttle state next to the config. Best-effort +// (every caller ignores the error) and, critically, it must NOT create the +// tracebloc data directory: after `tracebloc delete` wipes ~/.tracebloc, the +// post-command update check still runs, and an MkdirAll here would resurrect the +// just-offboarded host data dir (Bugbot #397; RFC-0003 offboard hygiene). So we +// only write when the dir already exists — its lifecycle is owned by +// login/client-create and delete, never by a throttle cache. A missing dir is a +// silent no-op (the throttle simply isn't persisted until the dir exists again). +func writeUpdateCache(path string, c updateCache) error { + if !configDirExists(path) { + return nil // dir gone (fresh machine, or just-offboarded) — don't recreate it + } + raw, err := json.Marshal(c) + if err != nil { + return err + } + return os.WriteFile(path, raw, 0o600) +} + +// versionLess reports whether a is an older version than b. Dotted numeric +// (major.minor.patch); a leading "v" is ignored; a pre-release suffix ("-rc1") +// ranks BELOW the same core release, so a stable user is never nudged onto a +// pre-release of the version they already run. +func versionLess(a, b string) bool { return compareVersions(a, b) < 0 } + +func compareVersions(a, b string) int { + an, ap := splitVersion(a) + bn, bp := splitVersion(b) + for i := 0; i < 3; i++ { + if an[i] != bn[i] { + if an[i] < bn[i] { + return -1 + } + return 1 + } + } + switch { + case ap == "" && bp == "": + return 0 + case ap == "": // a is a release, b a pre-release of the same core → a is newer + return 1 + case bp == "": + return -1 + default: + return strings.Compare(ap, bp) + } +} + +func splitVersion(v string) ([3]int, string) { + v = normalizeVersion(v) + pre := "" + if i := strings.IndexAny(v, "-+"); i >= 0 { + pre, v = v[i+1:], v[:i] + } + var out [3]int + for i, part := range strings.SplitN(v, ".", 3) { + if i > 2 { + break + } + out[i], _ = strconv.Atoi(strings.TrimSpace(part)) + } + return out, pre +} + +func normalizeVersion(v string) string { return strings.TrimPrefix(strings.TrimSpace(v), "v") } diff --git a/internal/cli/update_check_test.go b/internal/cli/update_check_test.go new file mode 100644 index 00000000..989d95ed --- /dev/null +++ b/internal/cli/update_check_test.go @@ -0,0 +1,244 @@ +package cli + +import ( + "bytes" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" +) + +func TestCompareVersions(t *testing.T) { + cases := []struct { + a, b string + want int // -1 ab + }{ + {"0.9.5", "0.9.6", -1}, + {"0.9.5", "0.9.5", 0}, + {"0.9.6", "0.9.5", 1}, + {"1.0.0", "0.9.9", 1}, + {"0.10.0", "0.9.9", 1}, // numeric, not lexical (10 > 9) + {"v0.9.5", "0.9.5", 0}, // leading v ignored + {"0.9.5", "v0.9.6", -1}, + {"0.9.5-rc1", "0.9.5", -1}, // pre-release ranks below the release + {"0.9.5", "0.9.5-rc1", 1}, + {"1.2.3", "1.2", 1}, // missing patch treated as 0 + } + for _, c := range cases { + if got := compareVersions(c.a, c.b); got != c.want { + t.Errorf("compareVersions(%q,%q) = %d, want %d", c.a, c.b, got, c.want) + } + // versionLess is the strict-less wrapper. + if got := versionLess(c.a, c.b); got != (c.want < 0) { + t.Errorf("versionLess(%q,%q) = %v, want %v", c.a, c.b, got, c.want < 0) + } + } +} + +func TestUpdateCacheRoundTrip(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, updateCacheFile) + in := updateCache{CheckedAt: time.Now().Truncate(time.Second), Latest: "0.9.7"} + if err := writeUpdateCache(path, in); err != nil { + t.Fatalf("write: %v", err) + } + got, ok := readUpdateCache(path) + if !ok || got.Latest != "0.9.7" || !got.CheckedAt.Equal(in.CheckedAt) { + t.Fatalf("round-trip = %+v ok=%v, want %+v", got, ok, in) + } + // A missing / empty-Latest cache reads as not-present. + if _, ok := readUpdateCache(filepath.Join(dir, "nope.json")); ok { + t.Error("missing cache should read as absent") + } +} + +// writeUpdateCache must never create the config dir: after `tracebloc delete` +// wipes ~/.tracebloc, the post-command update check still runs, and recreating +// the dir to write the throttle cache would resurrect the just-offboarded host +// data dir. A missing dir is a silent no-op — no file, no dir (Bugbot #397). +func TestWriteUpdateCache_DoesNotResurrectMissingDir(t *testing.T) { + base := t.TempDir() + gone := filepath.Join(base, "wiped") // deliberately never created + path := filepath.Join(gone, updateCacheFile) + + if err := writeUpdateCache(path, updateCache{CheckedAt: time.Now(), Latest: "0.9.9"}); err != nil { + t.Fatalf("writeUpdateCache to a missing dir must be a silent no-op, got err: %v", err) + } + if _, err := os.Stat(gone); !os.IsNotExist(err) { + t.Errorf("writeUpdateCache must not create the missing dir %s (err=%v)", gone, err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("writeUpdateCache must not write a cache file into a missing dir: %v", err) + } + + // Sanity: with the dir present it still writes (round-trips). + if err := writeUpdateCache(filepath.Join(base, updateCacheFile), updateCache{CheckedAt: time.Now(), Latest: "1.0.0"}); err != nil { + t.Fatalf("writeUpdateCache into an existing dir must succeed: %v", err) + } + if _, ok := readUpdateCache(filepath.Join(base, updateCacheFile)); !ok { + t.Error("cache should be present after writing into an existing dir") + } +} + +func TestFetchLatestRelease(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"tag_name":"v0.9.7","name":"ignored"}`)) + })) + defer srv.Close() + got, err := fetchLatestRelease(srv.URL, 2*time.Second) + if err != nil || got != "0.9.7" { + t.Fatalf("fetchLatestRelease = %q, %v; want 0.9.7, nil", got, err) + } + + bad := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer bad.Close() + if _, err := fetchLatestRelease(bad.URL, 2*time.Second); err == nil { + t.Error("non-200 should error") + } +} + +func TestLatestReleaseVersion_FreshCacheSkipsNetwork(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + // A server that fails the test if it's ever hit — proves a fresh cache is used. + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("network hit despite a fresh cache") + })) + defer srv.Close() + swapURL(t, srv.URL) + + if err := writeUpdateCache(updateCachePath(), updateCache{CheckedAt: time.Now(), Latest: "0.9.9"}); err != nil { + t.Fatal(err) + } + if got := latestReleaseVersion(); got != "0.9.9" { + t.Errorf("latestReleaseVersion = %q, want 0.9.9 (from fresh cache)", got) + } +} + +// An ABSENT config dir (fresh install / offboarded) must SKIP the network check +// entirely — not hit GitHub on every command. writeUpdateCache can't persist the +// throttle without the dir (Bugbot #404), so an unconditional fetch would repeat +// forever and burn updateCheckTimeout each time (Bugbot #397). Distinct from the +// dir-present-but-stale case, which still fetches (throttled) below. +func TestLatestReleaseVersion_MissingConfigDirSkipsNetwork(t *testing.T) { + absent := filepath.Join(t.TempDir(), "nope") // deliberately never created + t.Setenv("TRACEBLOC_CONFIG_DIR", absent) + // A server that fails the test if it's ever hit — proves no fetch is attempted. + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("network hit despite an absent config dir — the update check must be skipped") + })) + defer srv.Close() + swapURL(t, srv.URL) + + if got := latestReleaseVersion(); got != "" { + t.Errorf("latestReleaseVersion = %q, want \"\" (skipped: no config dir)", got) + } + // The skipped check must not have resurrected the dir either (reconciles #404). + if _, err := os.Stat(absent); !os.IsNotExist(err) { + t.Errorf("update check must not create the missing config dir %s (err=%v)", absent, err) + } +} + +// The dir-present-but-no-cache case (e.g. right after login created ~/.tracebloc) +// must still fetch and then persist the throttle — the missing-dir skip must NOT +// bleed into the normal path, or the once-a-day throttle would never arm. +func TestLatestReleaseVersion_DirPresentNoCacheFetchesAndPersists(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) // dir exists; no cache file yet + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"tag_name":"v2.0.0"}`)) + })) + defer srv.Close() + swapURL(t, srv.URL) + + if got := latestReleaseVersion(); got != "2.0.0" { + t.Errorf("latestReleaseVersion = %q, want 2.0.0 (dir present, no cache → fetch)", got) + } + // The fetch must have persisted the throttle so the next call is served from cache. + if c, ok := readUpdateCache(updateCachePath()); !ok || c.Latest != "2.0.0" { + t.Errorf("throttle not persisted after a dir-present fetch: %+v ok=%v", c, ok) + } +} + +func TestLatestReleaseVersion_StaleCacheFetchesAndRewrites(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"tag_name":"v1.2.0"}`)) + })) + defer srv.Close() + swapURL(t, srv.URL) + + // Stale cache (older than the interval) → must fetch. + if err := writeUpdateCache(updateCachePath(), updateCache{CheckedAt: time.Now().Add(-48 * time.Hour), Latest: "0.0.1"}); err != nil { + t.Fatal(err) + } + if got := latestReleaseVersion(); got != "1.2.0" { + t.Errorf("latestReleaseVersion = %q, want 1.2.0 (fetched)", got) + } + // The fetch should have rewritten the cache with the fresh value + time. + if c, ok := readUpdateCache(updateCachePath()); !ok || c.Latest != "1.2.0" || time.Since(c.CheckedAt) > time.Minute { + t.Errorf("cache not refreshed: %+v ok=%v", c, ok) + } +} + +// A failed fetch must fall back to the stale cache AND re-stamp CheckedAt, so +// the once-per-interval throttle holds while offline — otherwise the cache stays +// expired and every command re-hits the network and eats the timeout. +func TestLatestReleaseVersion_FailedFetchRefreshesThrottle(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + bad := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer bad.Close() + swapURL(t, bad.URL) + + // Stale cache (older than the interval) forces a fetch, which fails. + stale := time.Now().Add(-48 * time.Hour) + if err := writeUpdateCache(updateCachePath(), updateCache{CheckedAt: stale, Latest: "0.9.9"}); err != nil { + t.Fatal(err) + } + if got := latestReleaseVersion(); got != "0.9.9" { + t.Errorf("latestReleaseVersion = %q, want 0.9.9 (stale fallback)", got) + } + // CheckedAt must now be fresh so the next call is served from cache without + // re-hitting the network. + c, ok := readUpdateCache(updateCachePath()) + if !ok || c.Latest != "0.9.9" { + t.Fatalf("cache after failed fetch = %+v ok=%v, want Latest 0.9.9", c, ok) + } + if time.Since(c.CheckedAt) > time.Minute { + t.Errorf("CheckedAt not refreshed on failed fetch: %v", c.CheckedAt) + } +} + +func TestUpdateChecksAllowed_SkipConditions(t *testing.T) { + var buf bytes.Buffer // non-*os.File → never a terminal + t.Setenv("CI", "") + t.Setenv("TRACEBLOC_NO_UPDATE_CHECK", "") + + if updateChecksAllowed("dev", os.Stderr) { + t.Error("dev build must skip") + } + if updateChecksAllowed("0.9.5", &buf) { + t.Error("non-terminal writer must skip") + } + t.Setenv("TRACEBLOC_NO_UPDATE_CHECK", "1") + if updateChecksAllowed("0.9.5", os.Stderr) { + t.Error("opt-out must skip") + } + t.Setenv("TRACEBLOC_NO_UPDATE_CHECK", "") + t.Setenv("CI", "true") + if updateChecksAllowed("0.9.5", os.Stderr) { + t.Error("CI must skip") + } +} + +// swapURL points the release check at a test server for the duration of a test. +func swapURL(t *testing.T, url string) { + t.Helper() + old := latestReleaseURL + latestReleaseURL = url + t.Cleanup(func() { latestReleaseURL = old }) +} diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go new file mode 100644 index 00000000..9c0d8b6f --- /dev/null +++ b/internal/cli/upgrade.go @@ -0,0 +1,139 @@ +package cli + +import ( + "fmt" + "os" + "os/exec" + "runtime" + + "github.com/spf13/cobra" +) + +// upgradeCmdName is the command that re-runs the installer; the update nudge +// skips itself after this command (the running process swaps its own binary and +// is stale by design afterwards — see MaybeNotifyUpdate / main). +const upgradeCmdName = "upgrade" + +// The verified update command per OS. On Linux/macOS we re-run the official +// installer ourselves: it re-downloads + cosign-verifies the release, replaces +// the CLI, and upgrades the secure environment's services to match — so we never +// re-implement (and risk diverging from) the installer's signature verification. +// We download-then-execute the installer (installerRunScript, shared with +// prepare-host) rather than `curl … | bash`: piping makes the inner bash read +// its program from the pipe, stealing the installer's stdin so its interactive +// prompts (sign-in, etc.) can't read the TTY. The URL is derived from +// installerURL (doctor.go) so it can't drift from the other installer paths +// (Bugbot #397). +// +// Windows is different: we do NOT self-exec there. A running .exe is locked, so +// install.ps1's Move-Item can't overwrite the very binary we're running, and +// install.ps1 is CLI-only (no environment). So on Windows we print the command +// for the user to run in a fresh shell instead of pretending to self-update. +const upgradeInstallerCmdWindows = "irm https://github.com/tracebloc/cli/releases/latest/download/install.ps1 | iex" + +// upgradePlan is how `upgrade` proceeds on a given OS: either exec the installer +// (Unix) or just show the user a command to run (Windows). manual is the +// copy-paste command in both cases. +type upgradePlan struct { + exec bool // run the installer ourselves (Unix) vs. print instructions (Windows) + name string // exec program (when exec) + args []string // exec args (when exec) + manual string // command to show the user +} + +// upgradePlanFor returns the upgrade plan for a GOOS. Split out from runtime.GOOS +// so it's testable on any host. +func upgradePlanFor(goos string) upgradePlan { + if goos == "windows" { + return upgradePlan{exec: false, manual: upgradeInstallerCmdWindows} + } + // Download-then-execute the verified installer (installerRunScript, shared + // with prepare-host): its `set -e`+`curl -o` fails closed on a bad download, + // and running a file (not a pipe) keeps the installer's stdin on the TTY. The + // manual hint reuses installCmd (doctor.go), the shared bootstrap idiom, so + // the URL has a single source. + return upgradePlan{ + exec: true, + name: "bash", + args: []string{"-c", installerRunScript("")}, + manual: installCmd, + } +} + +// skipUpdateNudgeAnnotation marks a command after which main must NOT run the +// post-command update nudge (nor the cache write it triggers). Commands +// self-declare it in their cobra Annotations (see newUpgradeCmd, newDeleteCmd), +// so SkipUpdateNudge needs no central name list and — critically — can't be +// fooled by a same-named subcommand: the top-level offboard `delete` carries the +// annotation while `data delete` does not, even though both leaves are named +// "delete" (Bugbot #404). +const skipUpdateNudgeAnnotation = "tracebloc.io/skip-update-nudge" + +// SkipUpdateNudge reports whether the post-command update nudge (and the cache +// write it triggers) must be suppressed for the command that just ran. Exported +// for main, which owns the nudge call. A command opts in via +// skipUpdateNudgeAnnotation; today that's: +// - upgrade: the running process swapped its own binary and now carries the +// stale compile-time version — it would nudge about the release it just +// installed. +// - delete: offboarding removed the CLI and (on the default path) wiped +// ~/.tracebloc, so the nudge is nonsensical and its cache write would +// resurrect the just-offboarded host data dir (Bugbot #397). +func SkipUpdateNudge(cmd *cobra.Command) bool { + return cmd != nil && cmd.Annotations[skipUpdateNudgeAnnotation] == "true" +} + +// newUpgradeCmd implements `tracebloc upgrade` — the apply step the update +// nudge (update_check.go) and the 426 "too old" error both point at. +func newUpgradeCmd() *cobra.Command { + return &cobra.Command{ + Use: upgradeCmdName, + // Suppress the post-command update nudge/cache-write: this process just + // swapped its own binary and is stale by design (see SkipUpdateNudge). + Annotations: map[string]string{skipUpdateNudgeAnnotation: "true"}, + Short: "Update tracebloc to the latest release", + Long: `Updates tracebloc to the latest release by re-running the official +installer. It verifies signatures (cosign) and replaces the CLI; on Linux/macOS +it also upgrades your secure environment's services to match, so the CLI and the +environment never drift apart. On Windows it prints the command to update the +CLI (a running executable can't replace itself, so you run it in a fresh shell). +Safe to run anytime; safe to re-run.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + p := printerFor(cmd) + plan := upgradePlanFor(runtime.GOOS) + p.Newline() + + if !plan.exec { + // Windows: guide, don't self-exec (see upgradePlanFor). Running + // the installer in a fresh shell means no tracebloc process holds + // the binary, so install.ps1 can replace it. + p.Para("To update tracebloc on Windows, run this in a new PowerShell window:") + p.Para(" " + plan.manual) + p.Newline() + return nil + } + + p.Para("Updating tracebloc — re-running the installer (verifies signatures, then updates the CLI and your secure environment).") + p.Newline() + + // Stream the installer straight to the user's terminal, and keep + // stdin wired so its interactive prompts (sign-in, etc.) still work. + ctx := cmd.Context() + c := exec.CommandContext(ctx, plan.name, plan.args...) + c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr + if err := c.Run(); err != nil { + // User aborted (Ctrl-C) or the parent context was cancelled: exit + // quietly with 130 like prepare-host, not a scary "upgrade didn't + // complete — retry" (Bugbot #397). + if installerRunInterrupted(ctx, err) { + return &exitError{code: exitInterrupted} + } + return &exitError{code: exitFailure, err: fmt.Errorf( + "upgrade didn't complete (%w). You can run the installer directly:\n %s", + err, plan.manual)} + } + return nil + }, + } +} diff --git a/internal/cli/upgrade_test.go b/internal/cli/upgrade_test.go new file mode 100644 index 00000000..59843896 --- /dev/null +++ b/internal/cli/upgrade_test.go @@ -0,0 +1,117 @@ +package cli + +import ( + "bytes" + "strings" + "testing" +) + +// TestUpgradeCmd_Metadata pins the command's shape without running it (RunE +// shells out to the installer — never invoked in a test). Passing an argument +// must be rejected by cobra's Args check BEFORE RunE, so this never triggers a +// real installer run. +func TestUpgradeCmd_Metadata(t *testing.T) { + c := newUpgradeCmd() + if c.Use != "upgrade" { + t.Errorf("Use = %q, want upgrade", c.Use) + } + if c.Short == "" { + t.Error("Short must be set") + } + + c.SetArgs([]string{"unexpected-arg"}) + c.SetOut(&bytes.Buffer{}) + c.SetErr(&bytes.Buffer{}) + if err := c.Execute(); err == nil { + t.Error("upgrade takes no args — an extra arg must error (before RunE)") + } +} + +// TestUpgradeCmd_HelpMentionsVerified: --help renders (no RunE) and states that +// the update is signature-verified, so the copy catalog + users see the safety +// property. +func TestUpgradeCmd_HelpMentionsVerified(t *testing.T) { + c := newUpgradeCmd() + var out bytes.Buffer + c.SetOut(&out) + c.SetErr(&out) + c.SetArgs([]string{"--help"}) + if err := c.Execute(); err != nil { + t.Fatalf("upgrade --help: %v", err) + } + got := out.String() + for _, want := range []string{"latest release", "signatures"} { + if !strings.Contains(got, want) { + t.Errorf("upgrade --help missing %q in:\n%s", want, got) + } + } +} + +// TestUpgradePlanFor_PerOS: Windows must NOT self-exec (a running .exe is locked +// and install.ps1 is CLI-only) — it only prints the manual command. Unix runs +// the verified installer via the shared download-then-execute script, never +// `curl | bash` (which would steal the installer's stdin), and reuses installCmd +// for the manual hint so the URL has one source (Bugbot #397). +func TestUpgradePlanFor_PerOS(t *testing.T) { + win := upgradePlanFor("windows") + if win.exec { + t.Error("windows upgrade must not self-exec (running .exe is locked)") + } + if !strings.Contains(win.manual, "install.ps1") { + t.Errorf("windows manual command must run install.ps1: %q", win.manual) + } + if strings.Contains(win.manual, "i.sh") || strings.Contains(win.manual, "bash") { + t.Errorf("windows must not point at the Unix installer: %q", win.manual) + } + + for _, goos := range []string{"linux", "darwin"} { + p := upgradePlanFor(goos) + if !p.exec || p.name != "bash" { + t.Errorf("%s upgrade should exec bash, got exec=%v name=%q", goos, p.exec, p.name) + } + joined := strings.Join(p.args, " ") + // Download-then-execute, not `curl | bash`: piping steals the installer's + // stdin so its interactive prompts can't read the TTY (Bugbot #397). + if strings.Contains(joined, "| bash") || strings.Contains(joined, "|bash") { + t.Errorf("%s upgrade must not pipe the installer into bash (steals its stdin): %q", goos, joined) + } + // `set -e` + `curl -o ` fails closed on a bad download instead of + // running an empty script and reporting a phantom success. + if !strings.Contains(joined, "set -e") || !strings.Contains(joined, "curl") || !strings.Contains(joined, "-o ") { + t.Errorf("%s upgrade must download the installer to a file (set -e + curl -o): %q", goos, joined) + } + if !strings.Contains(joined, "i.sh") { + t.Errorf("%s upgrade must run i.sh: %q", goos, joined) + } + // Manual hint reuses installCmd (single URL source), not a re-hardcoded URL. + if p.manual != installCmd { + t.Errorf("%s manual hint = %q, want installCmd %q", goos, p.manual, installCmd) + } + } +} + +// TestSkipUpdateNudge: the nudge must be suppressed right after `tracebloc +// upgrade` (the running process is stale-by-design once it swaps its own binary) +// AND after `tracebloc delete` (offboarding removed the CLI and wiped +// ~/.tracebloc — the nudge's cache write would resurrect the dir; Bugbot #397), +// but fire for any other command. +func TestSkipUpdateNudge(t *testing.T) { + if !SkipUpdateNudge(newUpgradeCmd()) { + t.Error("upgrade command must skip the update nudge") + } + if !SkipUpdateNudge(newDeleteCmd()) { + t.Error("delete command must skip the update nudge (its cache write would resurrect the wiped ~/.tracebloc)") + } + // `data delete` shares the leaf name "delete" but does NOT offboard/wipe — it + // must still get the nudge. Guards the name-collision fix (Bugbot #404): the + // skip is driven by an annotation, not by cmd.Name(). + if SkipUpdateNudge(newDataDeleteCmd()) { + t.Error("`data delete` must NOT skip the nudge — it doesn't wipe ~/.tracebloc or remove the CLI") + } + if SkipUpdateNudge(newDoctorCmd(false)) { + t.Error("an ordinary command (doctor) must not skip the nudge") + } + if SkipUpdateNudge(nil) { + t.Error("nil command must not skip the nudge") + } +} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index ddc9b455..83ffa661 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -30,6 +30,7 @@ import ( "k8s.io/client-go/kubernetes" "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/resources" ) // Status is a single check's severity. Ordered so the numerically-greatest @@ -596,12 +597,21 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // node as a whole — never OR cpu/mem and GPU across different nodes, which // would pass even when no single node can run the job (Bugbot on PR #91). var cpuMemFits, fullFits bool + // Largest Ready node for the drift nudge — CPU-major with memory as the + // tie-break, EXACTLY like resources.nodeLarger, so the advertised ceiling + // always matches what `resources set max` will actually apply (Bugbot). + var bestCPU, bestMem resource.Quantity for i := range nodes.Items { n := nodes.Items[i] if !nodeReady(n) { continue } alloc := n.Status.Allocatable + if alloc.Cpu().Cmp(bestCPU) > 0 || + (alloc.Cpu().Cmp(bestCPU) == 0 && alloc.Memory().Cmp(bestMem) > 0) { + bestCPU = *alloc.Cpu() + bestMem = *alloc.Memory() + } nodeCPUMem := alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 nodeGPU := !gpuRequested if gpuRequested { @@ -633,7 +643,19 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Remedy: "If GPU training is expected, ensure one node has both the compute and the GPU capacity, with its device plugin.", } default: - return Result{Name: name, Status: StatusOK, Detail: fmt.Sprintf("a Ready node can schedule a training job (%s)", req)} + detail := fmt.Sprintf("a Ready node can schedule a training job (%s)", req) + // Drift nudge (#400 / backend#1236): the install-time auto-size goes + // stale when a machine GROWS. When the configured budget uses no more + // than half of what this machine could give one run (largest node − + // platform overhead), say so. + m := resources.Machine{CPU: bestCPU, Mem: bestMem} + maxCores, maxGiB := resources.MaxRunCores(m), resources.MaxRunGiB(m) + if maxCores >= 1 && maxGiB >= 2 && + cpuReq.MilliValue()*2 <= int64(maxCores)*1000 && + memReq.Value()*2 <= int64(maxGiB)<<30 { + detail += fmt.Sprintf(" — this machine could give a run up to cpu=%d,memory=%dGi ('tracebloc resources set max')", maxCores, maxGiB) + } + return Result{Name: name, Status: StatusOK, Detail: detail} } } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index fff8b971..531ac8fe 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -657,6 +657,36 @@ func TestCheckNodeFit(t *testing.T) { t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) } }) + // Drift nudge (#400 / backend#1236): a budget using ≤ half of what the + // largest node could give one run gets a resources-set-max pointer; a snug + // fit stays quiet. + t.Run("big machine + small budget -> ok with the resize nudge", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "32", "64Gi")) + r := checkNodeFit(bg(), cs, cpuOnly) // 2/8Gi vs max 31/61 + if r.Status != StatusOK || !strings.Contains(r.Detail, "resources set max") { + t.Fatalf("=> %v (%q), want ok with nudge", r.Status, r.Detail) + } + }) + t.Run("snug fit -> ok without the nudge", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "12Gi")) + r := checkNodeFit(bg(), cs, cpuOnly) // 2/8Gi vs max 3/9: memory over half + if r.Status != StatusOK || strings.Contains(r.Detail, "resources set max") { + t.Fatalf("=> %v (%q), want ok without nudge", r.Status, r.Detail) + } + }) + t.Run("heterogeneous nodes: nudge quotes the CPU-major node, matching set max (Bugbot)", func(t *testing.T) { + // resources.LargestReadyNode (what `set max` applies) is CPU-major: + // it picks cpuBig (32/64Gi -> max 31/61Gi), not memBig (8/128Gi -> + // 7/125Gi). The advertised ceiling must be the SAME node's. + cs := fake.NewClientset( + node("cpuBig", "32", "64Gi"), + node("memBig", "8", "128Gi"), + ) + r := checkNodeFit(bg(), cs, cpuOnly) + if r.Status != StatusOK || !strings.Contains(r.Detail, "cpu=31,memory=61Gi") { + t.Fatalf("=> %v (%q), want the CPU-major node's ceiling cpu=31,memory=61Gi", r.Status, r.Detail) + } + }) } func dockerSecret(name string, data []byte) *corev1.Secret { diff --git a/internal/helm/seal.go b/internal/helm/seal.go new file mode 100644 index 00000000..9bca444c --- /dev/null +++ b/internal/helm/seal.go @@ -0,0 +1,190 @@ +// Seal-check plumbing — the helm side of `tracebloc client status --seal` +// (cli#393, RFC-0003 §8.2 D12). +// +// THE CONTRACT WITH THE CHART (client repo docs/SEAL-CHECK.md, backend#1184). +// The chart's conformance checks are ordinary `helm test` hook Jobs — +// egress-enforcement, backend-reachability, storage-assertions — each +// carrying the SealCheckLabel membership marker and the SealNameLabel +// per-check identifier; the machine contract is labels + Job exit status. +// Everything degrades gracefully against an older chart: no labelled hooks → +// the CLI falls back to ALL of the chart's runnable helm tests; no test hooks +// at all → the CLI reports the seal as unverifiable (never silently sealed — +// the chart's own design stance). +// +// Discovery reads `helm get hooks` (the release's stored hook manifests), so +// the names fed to `helm test --filter name=…` come from the same release +// store the test action reads — they can't drift apart. +// +// (The package comment lives in upgrade.go.) + +package helm + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "time" + + "gopkg.in/yaml.v3" +) + +// The label keys are the chart's enumeration contract (client repo +// docs/SEAL-CHECK.md): both are public API on every runnable check Job. The +// hint annotation is a CLI-side optional extension — labels can't carry +// free-form sentences — that the chart can adopt per check; absent, the CLI +// falls back to a kubectl-logs pointer. +const ( + // SealCheckLabel marks a helm-test hook as part of the chart's seal-check + // (conformance) suite: `tracebloc.io/seal-check: "true"`. + SealCheckLabel = "tracebloc.io/seal-check" + // SealNameLabel carries the stable per-check identifier + // (`tracebloc.io/seal-check-name`, e.g. "egress-enforcement"). + SealNameLabel = "tracebloc.io/seal-check-name" + // SealHintAnnotation optionally carries a one-line remediation hint the CLI + // shows when that check fails. + SealHintAnnotation = "tracebloc.io/seal-hint" +) + +// TestTarget identifies the release the helm-test commands act on. Kubeconfig +// and KubeContext pin helm to the SAME cluster the CLI resolved — never the +// ambient current-context — mirroring nodeboot.UninstallChart; both are +// appended only when non-empty. +type TestTarget struct { + Release string // helm release name (from cluster discovery) + Namespace string // release namespace + Kubeconfig string // kubeconfig path (empty = ambient $KUBECONFIG/~/.kube/config) + KubeContext string // kubeconfig context (empty = current-context) +} + +// kubeFlags renders the optional cluster-pinning flags helm accepts on every +// subcommand this package drives. +func (t TestTarget) kubeFlags() []string { + var f []string + if t.Kubeconfig != "" { + f = append(f, "--kubeconfig", t.Kubeconfig) + } + if t.KubeContext != "" { + f = append(f, "--kube-context", t.KubeContext) + } + return f +} + +// TestHook describes one helm-test hook rendered in the release's chart. +type TestHook struct { + Kind string // manifest kind (Job / Pod / a check's SA-RBAC plumbing) + Name string // hook resource name; what `helm test --filter name=…` matches + SealCheck bool // labelled tracebloc.io/seal-check="true" (the consolidated suite) + SealName string // tracebloc.io/seal-check-name label ("" when absent) + SealHint string // tracebloc.io/seal-hint annotation ("" when absent) +} + +// Runnable reports whether the hook is a check that executes and completes — +// a Job or a bare Pod, the kinds `helm test` waits on. Other test-event hooks +// (a check's ServiceAccount/Role plumbing, created at negative hook-weight) +// are applied but never "pass", so they are not checks; per the chart +// contract only runnable checks carry the seal labels. +func (h TestHook) Runnable() bool { + return strings.EqualFold(h.Kind, "Job") || strings.EqualFold(h.Kind, "Pod") +} + +// ListTestHooks enumerates the release's helm-test hooks from its stored hook +// manifests (`helm get hooks`). Only hooks whose `helm.sh/hook` annotation +// declares the test event are returned; install/upgrade/delete hooks are not +// conformance checks. A release with no hooks yields an empty slice, nil error. +func ListTestHooks(ctx context.Context, t TestTarget) ([]TestHook, error) { + args := append([]string{"get", "hooks", t.Release, "--namespace", t.Namespace}, t.kubeFlags()...) + out, err := Runner(ctx, "helm", args...) + if err != nil { + // Wrap with helm's own output (e.g. "release: not found", cluster + // unreachable) — that text is the actionable part, not the exit status. + return nil, fmt.Errorf("helm get hooks %s: %w\n%s", t.Release, err, strings.TrimSpace(out)) + } + return parseTestHooks(out) +} + +// hookManifest is the minimal slice of a rendered hook manifest the seal check +// reads. Everything else in the document is ignored. +type hookManifest struct { + Kind string `yaml:"kind"` + Metadata struct { + Name string `yaml:"name"` + Labels map[string]string `yaml:"labels"` + Annotations map[string]string `yaml:"annotations"` + } `yaml:"metadata"` +} + +// parseTestHooks decodes the multi-document YAML stream `helm get hooks` +// prints and keeps the test hooks. A document that fails to decode is a hard +// error, not a skip: a manifest we can't parse could be a conformance check, +// and silently dropping it from the verdict would overstate the seal. +func parseTestHooks(manifests string) ([]TestHook, error) { + dec := yaml.NewDecoder(strings.NewReader(manifests)) + var hooks []TestHook + for { + var m hookManifest + err := dec.Decode(&m) + if errors.Is(err, io.EOF) { + return hooks, nil + } + if err != nil { + return nil, fmt.Errorf("parsing the release's hook manifests: %w", err) + } + // Empty documents (comment-only separators) decode to a zero value. + if m.Metadata.Name == "" || !isTestHook(m.Metadata.Annotations["helm.sh/hook"]) { + continue + } + hooks = append(hooks, TestHook{ + Kind: m.Kind, + Name: m.Metadata.Name, + SealCheck: m.Metadata.Labels[SealCheckLabel] == "true", + SealName: m.Metadata.Labels[SealNameLabel], + SealHint: m.Metadata.Annotations[SealHintAnnotation], + }) + } +} + +// isTestHook reports whether a `helm.sh/hook` annotation value declares the +// test event. The value is a comma-separated event list; "test-success" is the +// legacy helm-2 spelling helm 3 still runs as a test. +func isTestHook(annotation string) bool { + for _, event := range strings.Split(annotation, ",") { + switch strings.TrimSpace(event) { + case "test", "test-success": + return true + } + } + return false +} + +// RunTest runs ONE of the release's checks (`helm test --filter +// name=a,name=b`) and returns helm's combined output plus its raw error. The +// caller derives the per-check verdict from the error (exit code is the +// pass/fail contract) and the failure detail from the output — so neither is +// wrapped or trimmed here. +// +// names is the check hook PLUS the release's non-runnable test hooks (the +// SA/RBAC plumbing a check may depend on, created at negative hook-weight). +// helm's --filter excludes every unlisted test hook from the run — plumbing +// included — so filtering to the check alone would strand e.g. the +// storage-assertions Job without its ServiceAccount and report a false +// failure. Non-runnable hooks are applied instantly (helm only waits on +// Jobs/Pods), so carrying them costs nothing. +// +// One invocation per check, not one `helm test` for the whole suite, is +// deliberate: helm stops a suite at the first failure, and the seal check must +// report EVERY check's state (the partially-degraded picture), not just the +// first break. +func RunTest(ctx context.Context, t TestTarget, names []string, timeout time.Duration) (string, error) { + filters := make([]string, len(names)) + for i, n := range names { + filters[i] = "name=" + n + } + args := []string{"test", t.Release, "--namespace", t.Namespace, "--filter", strings.Join(filters, ",")} + if timeout > 0 { + args = append(args, "--timeout", timeout.String()) + } + args = append(args, t.kubeFlags()...) + return Runner(ctx, "helm", args...) +} diff --git a/internal/helm/seal_test.go b/internal/helm/seal_test.go new file mode 100644 index 00000000..e65b09ca --- /dev/null +++ b/internal/helm/seal_test.go @@ -0,0 +1,210 @@ +package helm + +import ( + "context" + "errors" + "strings" + "testing" + "time" +) + +// sealRunner swaps Runner for a scripted fake and records every invocation, so +// the seal-check plumbing is exercised without helm or a cluster — the same +// double upgrade_test.go uses. +type sealRunner struct { + calls [][]string + out string + err error +} + +func (f *sealRunner) install(t *testing.T) { + t.Helper() + orig := Runner + Runner = func(_ context.Context, name string, args ...string) (string, error) { + f.calls = append(f.calls, append([]string{name}, args...)) + return f.out, f.err + } + t.Cleanup(func() { Runner = orig }) +} + +// hooksYAML mirrors what `helm get hooks` prints for the client chart +// (docs/SEAL-CHECK.md contract): two conformance Jobs carrying the seal +// labels (one also with the CLI's optional hint annotation), a non-runnable +// aux test hook (the storage check's ServiceAccount, negative hook-weight), a +// non-test hook that must be filtered out, and a legacy `test-success` Pod +// that must still count as a test. +const hooksYAML = `--- +# Source: client/templates/egress-enforcement-check.yaml +apiVersion: batch/v1 +kind: Job +metadata: + name: acme-egress-enforcement-check + namespace: acme + labels: + app.kubernetes.io/instance: acme + tracebloc.io/seal-check: "true" + tracebloc.io/seal-check-name: egress-enforcement + annotations: + "helm.sh/hook": test + "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded + tracebloc.io/seal-hint: ensure the CNI enforces egress NetworkPolicy, then re-run +spec: + backoffLimit: 0 +--- +# Source: client/templates/egress-reachability-check.yaml +apiVersion: batch/v1 +kind: Job +metadata: + name: acme-egress-reachability-check + namespace: acme + labels: + app.kubernetes.io/instance: acme + tracebloc.io/seal-check: "true" + tracebloc.io/seal-check-name: backend-reachability + annotations: + "helm.sh/hook": test +spec: + backoffLimit: 0 +--- +# Source: client/templates/storage-assertions-check.yaml +apiVersion: v1 +kind: ServiceAccount +metadata: + name: acme-storage-assertions-check + annotations: + "helm.sh/hook": test + "helm.sh/hook-weight": "-6" +--- +# Source: client/templates/some-migration.yaml +apiVersion: batch/v1 +kind: Job +metadata: + name: acme-pre-upgrade-migrate + annotations: + "helm.sh/hook": pre-upgrade +--- +apiVersion: v1 +kind: Pod +metadata: + name: acme-legacy-probe + annotations: + "helm.sh/hook": "test-success, something-else" +` + +func TestListTestHooks_ParsesAndFilters(t *testing.T) { + f := &sealRunner{out: hooksYAML} + f.install(t) + + hooks, err := ListTestHooks(context.Background(), TestTarget{ + Release: "acme", Namespace: "acme", + Kubeconfig: "/tmp/kc", KubeContext: "kind-acme", + }) + if err != nil { + t.Fatalf("ListTestHooks: %v", err) + } + + // argv: get hooks on the right release/namespace, pinned to the resolved + // cluster — never the ambient context. + want := []string{"helm", "get", "hooks", "acme", "--namespace", "acme", + "--kubeconfig", "/tmp/kc", "--kube-context", "kind-acme"} + if len(f.calls) != 1 || strings.Join(f.calls[0], " ") != strings.Join(want, " ") { + t.Fatalf("helm argv = %v, want %v", f.calls, want) + } + + if len(hooks) != 4 { + t.Fatalf("got %d hooks (%+v), want 4 (the pre-upgrade hook filtered out)", len(hooks), hooks) + } + enf := hooks[0] + if enf.Name != "acme-egress-enforcement-check" || enf.Kind != "Job" || !enf.SealCheck || !enf.Runnable() { + t.Errorf("enforcement hook parsed wrong: %+v", enf) + } + if enf.SealName != "egress-enforcement" || + enf.SealHint != "ensure the CNI enforces egress NetworkPolicy, then re-run" { + t.Errorf("seal name label / hint annotation parsed wrong: %+v", enf) + } + reach := hooks[1] + if reach.SealName != "backend-reachability" || !reach.SealCheck || reach.SealHint != "" { + t.Errorf("reachability hook parsed wrong: %+v", reach) + } + sa := hooks[2] + if sa.Kind != "ServiceAccount" || sa.SealCheck || sa.Runnable() { + t.Errorf("aux ServiceAccount hook parsed wrong (must be non-runnable, unlabelled): %+v", sa) + } + legacy := hooks[3] + if legacy.Name != "acme-legacy-probe" || legacy.Kind != "Pod" || !legacy.Runnable() { + t.Errorf("legacy test-success hook parsed wrong: %+v", legacy) + } +} + +// A release with no hooks prints nothing — that's an empty suite, not an error +// (the CLI turns it into the honest "unknown" verdict). +func TestListTestHooks_NoHooks(t *testing.T) { + f := &sealRunner{out: ""} + f.install(t) + hooks, err := ListTestHooks(context.Background(), TestTarget{Release: "acme", Namespace: "acme"}) + if err != nil || len(hooks) != 0 { + t.Fatalf("got %v, %v — want empty, nil", hooks, err) + } +} + +// A helm failure surfaces helm's own output (the actionable part), wrapped. +func TestListTestHooks_HelmError(t *testing.T) { + f := &sealRunner{out: "Error: release: not found\n", err: errors.New("exit status 1")} + f.install(t) + _, err := ListTestHooks(context.Background(), TestTarget{Release: "ghost", Namespace: "ghost"}) + if err == nil || !strings.Contains(err.Error(), "release: not found") { + t.Fatalf("want the helm output in the error, got: %v", err) + } +} + +// A manifest that doesn't parse must fail closed — dropping it could silently +// remove a conformance check from the verdict. +func TestListTestHooks_BadYAMLFailsClosed(t *testing.T) { + f := &sealRunner{out: "kind: Job\nmetadata: {name: x\n"} + f.install(t) + _, err := ListTestHooks(context.Background(), TestTarget{Release: "acme", Namespace: "acme"}) + if err == nil || !strings.Contains(err.Error(), "parsing the release's hook manifests") { + t.Fatalf("want a parse failure, got: %v", err) + } +} + +func TestRunTest_Argv(t *testing.T) { + f := &sealRunner{out: "NAME: acme\n"} + f.install(t) + // One check plus an aux plumbing hook: both must land in ONE --filter + // (comma-OR), so helm creates the check's dependencies but runs no other check. + out, err := RunTest(context.Background(), TestTarget{ + Release: "acme", Namespace: "acme", Kubeconfig: "/tmp/kc", KubeContext: "kind-acme", + }, []string{"acme-storage-assertions-check", "acme-storage-assertions-sa"}, 2*time.Minute) + if err != nil || out != "NAME: acme\n" { + t.Fatalf("RunTest = %q, %v", out, err) + } + want := []string{"helm", "test", "acme", "--namespace", "acme", + "--filter", "name=acme-storage-assertions-check,name=acme-storage-assertions-sa", + "--timeout", "2m0s", + "--kubeconfig", "/tmp/kc", "--kube-context", "kind-acme"} + if len(f.calls) != 1 || strings.Join(f.calls[0], " ") != strings.Join(want, " ") { + t.Fatalf("helm argv = %v, want %v", f.calls, want) + } +} + +// The raw error + output pass through unwrapped: the caller owns the per-check +// verdict (exit code) and the failure-detail extraction, and needs both intact. +func TestRunTest_PassesThroughFailure(t *testing.T) { + f := &sealRunner{ + out: "Error: 1 error occurred:\n\t* job failed: BackoffLimitExceeded\n", + err: errors.New("exit status 1"), + } + f.install(t) + out, err := RunTest(context.Background(), TestTarget{Release: "acme", Namespace: "acme"}, []string{"acme-x"}, 0) + if err == nil || err.Error() != "exit status 1" { + t.Fatalf("want the raw runner error, got: %v", err) + } + if !strings.Contains(out, "BackoffLimitExceeded") { + t.Fatalf("want the raw combined output, got: %q", out) + } + // timeout == 0 → no --timeout flag (helm's own default stands). + if got := strings.Join(f.calls[0], " "); strings.Contains(got, "--timeout") { + t.Fatalf("timeout 0 must not add --timeout: %v", got) + } +} diff --git a/internal/push/progress.go b/internal/push/progress.go index f5df7b3b..6f26a185 100644 --- a/internal/push/progress.go +++ b/internal/push/progress.go @@ -63,7 +63,21 @@ func NewProgress(out io.Writer, totalBytes int64, description string) Progress { progressbar.OptionShowBytes(true), progressbar.OptionShowCount(), progressbar.OptionThrottle(100), // ms — keep CPU low - progressbar.OptionSetRenderBlankState(true), + // Render nothing until the first byte flows. The caller + // builds this bar up front (data.go), but Stage then prints + // several setup lines ("Opened a secure channel…", + // "Preparing the copy…") and waits up to StagePodReadyTimeout + // for the Pod before a single byte moves. A blank-state 0% + // bar painted at construction would (a) sit frozen through + // that multi-minute wait, reading as "stuck", and (b) get + // clobbered mid-line by those setup lines — schollz redraws + // with \r while the setup lines are plain \n writes to the + // same terminal, so the two collide on one line + // ("…[0s:0s]Opened a secure channel…"). Deferring the first + // render to the first Add() — which happens inside + // StreamLayout, after every setup line has printed — lets the + // bar own its own line cleanly. + progressbar.OptionSetRenderBlankState(false), progressbar.OptionSetWidth(40), progressbar.OptionClearOnFinish(), ), diff --git a/internal/push/stage.go b/internal/push/stage.go index 2fa6615a..52d3f24b 100644 --- a/internal/push/stage.go +++ b/internal/push/stage.go @@ -136,7 +136,12 @@ func Stage(ctx context.Context, opts StageOptions) error { } // 5. Stream the tar. This is where actual bytes flow. The - // progress bar (if TTY) renders during this call. + // progress bar (if TTY) renders ONLY during this call — it's + // built with RenderBlankState(false) (see progress.go) precisely + // so the setup lines above print on their own clean lines instead + // of colliding with a prematurely-drawn 0% bar. Keep any new + // customer-facing status prints ahead of this call, not after the + // bar starts. _, _ = fmt.Fprintf(opts.Out, "Copying %d files (%s) into %q…\n", opts.Layout.FileCount(), HumanBytes(opts.Layout.TotalBytes), opts.Table) diff --git a/internal/resources/resources.go b/internal/resources/resources.go index b98bd021..3f4d8cb9 100644 --- a/internal/resources/resources.go +++ b/internal/resources/resources.go @@ -58,35 +58,18 @@ type Training struct { HasGPU bool } -// MachineCapacity sums the allocatable CPU/memory (and GPU) across every Ready -// node. A pod is scheduled onto ONE node, but the machine-capacity headline is a -// whole-machine figure the user recognizes ("this machine has 8 CPU"); the -// single-node fit question is `cluster doctor`'s job (checkNodeFit), which -// deliberately never ORs capacity across nodes. On the installer's single-node -// path the sum is just that node. +// MachineCapacity reports the machine headline ("equipped with …") as the +// LARGEST Ready node's allocatable — never a sum. The previous whole-machine +// sum assumed the installer path is single-node, but the default k3d topology +// is server-0 + agent-0: two kubernetes nodes on the SAME physical machine, +// so the sum double-counted it ("24 CPU · 13.4 GiB" on a 12-CPU / 6.7-GiB +// box, #399). A pod schedules onto ONE node anyway, so the largest node is +// both truthful and scheduling-relevant — and now every surface (this +// headline, the wizard, doctor's checkNodeFit) quotes the same machine. func MachineCapacity(nodes []corev1.Node) Machine { - m := Machine{GPU: map[corev1.ResourceName]resource.Quantity{}} - for i := range nodes { - n := nodes[i] - if !nodeReady(n) { - continue - } - for name, qty := range n.Status.Allocatable { - switch name { - case corev1.ResourceCPU: - addInto(&m.CPU, qty) - case corev1.ResourceMemory: - addInto(&m.Mem, qty) - default: - // Surface GPUs (and any other extended device) so the machine - // line can note "· 1 GPU" — but only non-zero quantities. - if isGPUResource(name) && !qty.IsZero() { - cur := m.GPU[name] - cur.Add(qty) - m.GPU[name] = cur - } - } - } + m, ok := LargestReadyNode(nodes) + if !ok { + return Machine{GPU: map[corev1.ResourceName]resource.Quantity{}} } return m } @@ -143,10 +126,6 @@ func FormatGPU(name corev1.ResourceName, q resource.Quantity) string { // --- internal helpers ------------------------------------------------------- -// addInto adds src into dst in place. resource.Quantity.Add is a pointer -// method; dst starts as a zero quantity (value 0), so the first add seeds it. -func addInto(dst *resource.Quantity, src resource.Quantity) { dst.Add(src) } - // nodeReady reports whether a node's Ready condition is True. Mirrors // doctor.nodeReady — kept local so this package stays leaf/pure and doesn't // import the doctor command package. diff --git a/internal/resources/resources_test.go b/internal/resources/resources_test.go index 8061c342..5ecc55c5 100644 --- a/internal/resources/resources_test.go +++ b/internal/resources/resources_test.go @@ -28,7 +28,10 @@ func readyNode(name, cpu, mem string, extra ...string) *corev1.Node { } } -func TestMachineCapacity_SumsReadyNodesAndGPU(t *testing.T) { +// The k3d regression (#399): server-0 + agent-0 are the SAME physical machine, +// so two identical Ready nodes must report 1× that machine, never 2×. The +// not-Ready node is bigger than both — it must not win either. +func TestMachineCapacity_LargestReadyNodeNotSum(t *testing.T) { notReady := readyNode("down", "16", "64Gi") notReady.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionFalse}} @@ -38,18 +41,11 @@ func TestMachineCapacity_SumsReadyNodesAndGPU(t *testing.T) { *notReady, // must be excluded entirely } m := MachineCapacity(nodes) - if got := FormatCPU(m.CPU); got != "8 CPU" { - t.Errorf("cpu sum = %q, want 8 CPU", got) + if got := FormatCPU(m.CPU); got != "4 CPU" { + t.Errorf("cpu = %q, want 4 CPU (largest node, not the 8-CPU sum)", got) } - if got := FormatMem(m.Mem); got != "32 GiB" { - t.Errorf("mem sum = %q, want 32 GiB", got) - } - gpu, ok := m.GPU["nvidia.com/gpu"] - if !ok || gpu.Value() != 1 { - t.Errorf("gpu = %v (ok=%v), want 1", gpu, ok) - } - if len(m.GPU) != 1 { - t.Errorf("unexpected extra GPU entries: %v", m.GPU) + if got := FormatMem(m.Mem); got != "16 GiB" { + t.Errorf("mem = %q, want 16 GiB (largest node, not the 32-GiB sum)", got) } } diff --git a/scripts/install.sh b/scripts/install.sh index ab6cdbe2..dcac1f5d 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -387,13 +387,34 @@ verify_cosign_signature PREFIX="$INSTALL_PREFIX" if ! mkdir -p "$PREFIX" 2>/dev/null || [ ! -w "$PREFIX" ]; then # The customer's chosen prefix isn't usable (no write perms on - # parent, /usr/local/bin without sudo, etc.). Fall back to a - # per-user dir; the PATH-advice block below tells them how to - # pick it up. - FALLBACK="$HOME/.local/bin" - echo "Note: $PREFIX isn't writable (couldn't mkdir or no -w); falling back to $FALLBACK" - mkdir -p "$FALLBACK" - PREFIX="$FALLBACK" + # parent, /usr/local/bin without sudo, etc.). Prefer ~/bin when it + # already exists on $PATH and is writable: the binary is then usable + # in THIS shell (and every new one) with NO rc edit and no new + # terminal — the least-privilege install's B2 goal (RFC 0001). We + # only consider the conventional general-purpose ~/bin, never a + # language-specific dir that merely happens to be on PATH (~/.cargo/bin, + # ~/go/bin, …). Otherwise fall back to ~/.local/bin, which the + # PATH-advice block below wires into the shell rc. + home_bin="${HOME%/}/bin" + home_bin_on_path=no + # Match both "$home_bin" and a trailing-slash "$home_bin/" PATH entry — some + # users have "$HOME/bin/" (with the slash) on PATH, which a bare + # ":$home_bin:" pattern would miss and wrongly fall back to ~/.local/bin + # (Bugbot #392). + case ":$PATH:" in *":$home_bin:"*|*":$home_bin/:"*) home_bin_on_path=yes ;; esac + # Guard HOME="" or "/": "${HOME%/}/bin" collapses to "/bin", so a root process + # (HOME=/) with /bin writable + on PATH would drop the CLI into /bin. Require a + # real, non-root $HOME before preferring ~/bin (Bugbot #392 r3). + if [ -n "${HOME:-}" ] && [ "$HOME" != "/" ] \ + && [ "$home_bin_on_path" = yes ] && [ -d "$home_bin" ] && [ -w "$home_bin" ]; then + echo "Note: $PREFIX isn't writable; installing to $home_bin (already on your PATH)." + PREFIX="$home_bin" + else + FALLBACK="$HOME/.local/bin" + echo "Note: $PREFIX isn't writable (couldn't mkdir or no -w); falling back to $FALLBACK" + mkdir -p "$FALLBACK" + PREFIX="$FALLBACK" + fi fi chmod +x "$TMP/$BINARY_FILE" @@ -442,9 +463,19 @@ home_dir="${HOME%/}" persist=no case "$PREFIX" in "$home_dir"/*) persist=yes ;; - *) case ":$PATH:" in *":$PREFIX:"*) ;; *) persist=yes ;; esac ;; + *) case ":$PATH:" in *":$PREFIX:"*|*":$PREFIX/:"*) ;; *) persist=yes ;; esac ;; esac +# Is $PREFIX on the CURRENT shell's PATH? If so the binary is usable right now +# (this covers the ~/bin-already-on-PATH case). We still persist a $HOME prefix +# to the rc (a current-$PATH hit may be session-only — a one-off export, direnv — +# that new terminals won't have; Bugbot #392 r2), but the message must say +# "ready now" instead of nagging "open a new terminal" for a dir that IS on PATH. +on_path=no +# Trailing-slash tolerant, as above (Bugbot #392): a "$PREFIX/" PATH entry still +# means the binary is usable now, so don't nag "open a new terminal". +case ":$PATH:" in *":$PREFIX:"*|*":$PREFIX/:"*) on_path=yes ;; esac + if [ "$persist" = "yes" ]; then shell_name="$(basename "${SHELL:-sh}")" case "$shell_name" in @@ -490,14 +521,28 @@ if [ "$persist" = "yes" ]; then echo "" case "$state" in added) - echo "Added $PREFIX to your PATH in $rc." - echo "Open a new terminal — or load it now: . \"$rc\"" + if [ "$on_path" = yes ]; then + # Usable in THIS shell already; the rc line is just so new + # terminals find it too (covers a session-only $PATH hit). + echo "tracebloc is ready to use now." + echo "Also added $PREFIX to $rc so new terminals find it too." + else + echo "Added $PREFIX to your PATH in $rc." + echo "Open a new terminal — or load it now: . \"$rc\"" + fi ;; present) - echo "$PREFIX is already in your PATH config ($rc) — nothing to add." - echo "If a new terminal can't find it yet, open one — or load it now: . \"$rc\"" + if [ "$on_path" = yes ]; then + echo "tracebloc is ready to use now ($PREFIX is on your PATH)." + else + echo "$PREFIX is already in your PATH config ($rc) — nothing to add." + echo "If a new terminal can't find it yet, open one — or load it now: . \"$rc\"" + fi ;; *) + # Usable in THIS shell already ($PREFIX on PATH) — say so even though + # we couldn't persist it for new terminals (Bugbot #392 r3). + [ "$on_path" = yes ] && echo "tracebloc is ready to use now ($PREFIX is on your PATH)." echo "Note: the installer couldn't update your shell config ($rc)." echo "Add this line to it (or your shell's startup file), then open a new terminal:" echo " $path_line"