diff --git a/VERSION b/VERSION index f00339d7..3971e7e2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.17 +0.10.18 diff --git a/internal/cli/client.go b/internal/cli/client.go index ea32d4e7..d69dbf55 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -918,6 +918,11 @@ func setActiveClient(p *config.Profile, c *api.ProvisionedClient) { p.ActiveClientID = strconv.Itoa(c.ID) p.ActiveClientNamespace = c.Namespace p.ActiveClientName = c.Name + // WHICH CLUSTER, not just which namespace (backend#2863). The namespace cache + // alone let a mutating command bind the right namespace on the wrong cluster; + // this is the anchor guardActiveClientCluster compares against. The backend + // record is authoritative — same value, so the two can never disagree. + p.ActiveClientClusterID = c.ClusterID } // renderClientReview shows the assembled inputs before the confirm prompt, so diff --git a/internal/cli/clusterguard.go b/internal/cli/clusterguard.go new file mode 100644 index 00000000..eb98937c --- /dev/null +++ b/internal/cli/clusterguard.go @@ -0,0 +1,97 @@ +package cli + +import ( + "context" + "fmt" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/ui" +) + +// clusterIDFromFn is a test seam over cluster.ClusterIDFrom. +var clusterIDFromFn = cluster.ClusterIDFrom + +// guardActiveClientCluster refuses a MUTATING operation when the cluster actually +// reached is not the one the active client lives on. +// +// WHY THIS EXISTS (backend#2863). Every command resolved its target cluster from the +// ambient kubeconfig + current-context, while binding only the NAMESPACE from the +// active client. So on a machine whose current-context pointed elsewhere — a laptop +// that also administers a managed cluster, which is the normal case for anyone who +// runs both — a mutating command acted on that other cluster: +// +// - `data ingest` staged a private dataset onto it +// - `data delete` dropped a table and removed files from its shared PVC +// - `resources set` rolled its jobs-manager with a new envelope +// - `tracebloc delete` uninstalled a release of the same name +// +// The namespace binding made this MORE likely, not less: it supplied a namespace that +// probably exists on the other cluster too, so discovery succeeded and nothing looked +// wrong. The hazard was already documented at internal/nodeboot/nodeboot.go, whose +// comment ends "preserving the default-context behavior" — that clause was the bug. +// +// The check is on IDENTITY, not on the context name. Pinning a context string would +// break bring-your-own-cluster installs (EKS/AKS/OpenShift have no k3d context) and +// would still pass if two kubeconfigs named the same context differently. The +// kube-system namespace UID is the same anchor the backend record uses, so local and +// remote agree by construction. +// +// FAILURE MODES, deliberately asymmetric: +// +// - mismatch -> REFUSE. Naming both ids and the way forward. +// - id unreadable -> REFUSE. We are about to write to a cluster we cannot +// identify. Every caller needs API access anyway, so this +// costs nothing legitimate. +// - no anchor recorded -> WARN and proceed. Configs written before this field +// exists must not be locked out of their own commands; +// `client create` records it and the warning names that. +// +// Read-only commands never call this: being wrong about which cluster you are +// READING is a confusing answer, not a destructive act, and the target is already +// printed by doctor / cluster info. +func guardActiveClientCluster(ctx context.Context, p *ui.Printer, t *clusterTarget) error { + if t == nil || t.Clientset == nil { + return nil // nothing resolved (a test seam, or a command that mutates nothing) + } + cfg, err := config.Load() + if err != nil { + // No readable config means no anchor to compare against — same case as an + // unrecorded anchor below, not a reason to block. + p.Warnf("Couldn't read the local config, so this machine's cluster couldn't be " + + "verified before changing anything. Proceeding.") + return nil + } + want := cfg.Current().ActiveClientClusterID + if want == "" { + p.Warnf("This machine hasn't recorded which cluster its secure environment runs on, "+ + "so the target couldn't be verified before changing anything. Run `tracebloc client "+ + "create` to record it. Proceeding against %s.", t.Resolved.ServerURL) + return nil + } + got, idErr := clusterIDFromFn(ctx, t.Clientset) + if idErr != nil { + return &exitError{code: exitLocalEnv, err: fmt.Errorf( + "couldn't confirm which cluster this is before changing anything (%w).\n"+ + " Refusing rather than writing to an unidentified cluster.\n"+ + " reached: %s", idErr, t.Resolved.ServerURL)} + } + if got != want { + return &exitError{code: exitLocalEnv, err: fmt.Errorf( + "this is not the cluster your secure environment runs on — refusing to change anything.\n"+ + " reached: %s (cluster %s, context %q)\n"+ + " expected: cluster %s\n"+ + " Your kubeconfig's current context points somewhere else. Either switch it, or pass\n"+ + " --context/--kubeconfig for the cluster your secure environment runs on.", + t.Resolved.ServerURL, short(got), t.Resolved.Context, short(want))} + } + return nil +} + +// short trims a UID for messages — enough to compare by eye, not a wall of hex. +func short(id string) string { + if len(id) <= 8 { + return id + } + return id[:8] + "…" +} diff --git a/internal/cli/clusterguard_test.go b/internal/cli/clusterguard_test.go new file mode 100644 index 00000000..5fa046fa --- /dev/null +++ b/internal/cli/clusterguard_test.go @@ -0,0 +1,494 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "go/ast" + "go/parser" + "go/token" + "net/http" + "path/filepath" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/ui" +) + +// --------------------------------------------------------------------------- +// The property under test (backend#2863): invoked with NO flags, a mutating +// command acts on the cluster the secure environment runs on — or refuses. The +// bug was that it acted on whatever the ambient current-context pointed at. +// +// Every test here constructs the WRONG-CLUSTER case explicitly. The pre-fix code +// had no notion of a recorded cluster at all, so each of these fails on it: not +// by a message change, but by the command proceeding. +// --------------------------------------------------------------------------- + +// kubeSystem returns a kube-system namespace object carrying uid, which is the +// anchor cluster.ClusterID reads (the same one the backend client record keys on). +func kubeSystem(uid string) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "kube-system", UID: types.UID(uid)}, + } +} + +// withAnchor records uid as this machine's cluster in the temp config dir. The +// caller must already have a config dir (withClientBackend / t.Setenv). +func withAnchor(t *testing.T, uid string) { + t.Helper() + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + // Current() hands back a THROWAWAY &Profile{} when no env is selected, so a + // write to it is silently discarded — select one first or the anchor never + // lands and every refusal test below passes for the wrong reason. + if cfg.CurrentEnv == "" { + cfg.CurrentEnv = "test" + } + cfg.Current().ActiveClientClusterID = uid + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + reloaded, err := config.Load() + if err != nil { + t.Fatal(err) + } + if got := reloaded.Current().ActiveClientClusterID; got != uid { + t.Fatalf("the anchor did not persist (got %q, want %q) — the tests using it"+ + " would pass by accident", got, uid) + } +} + +// withClusterID fakes the identity read for the resolveClusterTarget path. +func withClusterID(t *testing.T, id string, err error) { + t.Helper() + orig := clusterIDFromFn + t.Cleanup(func() { clusterIDFromFn = orig }) + clusterIDFromFn = func(context.Context, kubernetes.Interface) (string, error) { + return id, err + } +} + +// A mutating resolve against a cluster that is NOT the recorded one refuses, and +// refuses with the two ids named — a message that says only "wrong cluster" makes +// the user guess which of their contexts was right. +func TestResolveClusterTarget_Mutating_WrongCluster_Refuses(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "AAAAAAAA-1111-2222-3333-444444444444") + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("BBBBBBBB-9999-8888-7777-666666666666"))) + withClusterID(t, "BBBBBBBB-9999-8888-7777-666666666666", nil) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true) + if err == nil { + t.Fatal("a mutating command must refuse a cluster that is not the recorded one") + } + if got := ExitCodeFromError(err); got != exitLocalEnv { + t.Errorf("exit code = %d, want %d (a local-environment problem)", got, exitLocalEnv) + } + msg := err.Error() + for _, want := range []string{ + "not the cluster your secure environment runs on", + "BBBBBBBB", // reached — so the user can see WHERE it went + "AAAAAAAA", // expected — so they can see which context is right + "--context/--kubeconfig", + } { + if !strings.Contains(msg, want) { + t.Errorf("refusal must name %q; got:\n%s", want, msg) + } + } +} + +// The same resolve with mutates=false PROCEEDS. Being wrong about which cluster +// you are READING is a confusing answer, not a destructive act — and gating reads +// too would break `data list` on a machine whose anchor predates the field. +func TestResolveClusterTarget_ReadOnly_WrongCluster_Proceeds(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "AAAAAAAA-1111-2222-3333-444444444444") + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("BBBBBBBB"))) + withClusterID(t, "BBBBBBBB", nil) + + var out bytes.Buffer + if _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, false); err != nil { + t.Fatalf("a read-only command must not be gated on cluster identity: %v", err) + } +} + +// Matching cluster → proceeds. Without this the refusal above could be satisfied +// by a guard that refuses everything, which would pass the mismatch test and +// break every legitimate invocation. +func TestResolveClusterTarget_Mutating_RightCluster_Proceeds(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "SAME-CLUSTER-UID") + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"), kubeSystem("SAME-CLUSTER-UID"))) + withClusterID(t, "SAME-CLUSTER-UID", nil) + + var out bytes.Buffer + if _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true); err != nil { + t.Fatalf("the recorded cluster must be usable: %v", err) + } +} + +// An UNREADABLE identity refuses for a mutating command: we are about to write to +// a cluster we cannot name. Every caller already needs API access, so this costs +// nothing legitimate — and the alternative (proceed) is the bug with extra steps. +func TestResolveClusterTarget_Mutating_UnreadableID_Refuses(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withAnchor(t, "AAAAAAAA") + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"))) + withClusterID(t, "", errors.New("namespaces \"kube-system\" is forbidden")) + + var out bytes.Buffer + _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true) + if err == nil { + t.Fatal("an unidentifiable cluster must not be written to") + } + if !strings.Contains(err.Error(), "Refusing rather than writing to an unidentified cluster") { + t.Errorf("the refusal must say why it refused; got: %v", err) + } + if !strings.Contains(err.Error(), "forbidden") { + t.Errorf("the underlying cause must survive for diagnosis; got: %v", err) + } +} + +// NO anchor recorded → warn and proceed. Configs written before this field +// existed must not be locked out of their own commands, and the warning has to +// name the way to fix it or it is just noise. +func TestResolveClusterTarget_Mutating_NoAnchor_WarnsAndProceeds(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + withClusterSeams(t, fake.NewSimpleClientset(jmDep("gpu-box-01"))) + withClusterID(t, "WHATEVER", nil) + + var out bytes.Buffer + if _, err := resolveClusterTarget(context.Background(), ui.New(&out), + cluster.KubeconfigOptions{}, activeClientBinding{}, false, false, true); err != nil { + t.Fatalf("an unrecorded anchor must not block: %v", err) + } + got := out.String() + if !strings.Contains(got, "hasn't recorded which cluster") { + t.Errorf("an unverified mutation must say so; got:\n%s", got) + } + if !strings.Contains(got, "client create") { + t.Errorf("the warning must name how to record it; got:\n%s", got) + } +} + +// --------------------------------------------------------------------------- +// `delete` — the one command where the mistake is unrecoverable, and the one +// that reported it in the field: it hands the raw kubeconfig flags to +// `helm uninstall` and never resolved a target at all. +// --------------------------------------------------------------------------- + +// withDeleteClusterID fakes the delete path's own identity read. +func withDeleteClusterID(t *testing.T, id string, err error) { + t.Helper() + orig := clusterIDForDelete + t.Cleanup(func() { clusterIDForDelete = orig }) + clusterIDForDelete = func(context.Context, cluster.KubeconfigOptions) (string, error) { + return id, err + } +} + +// Offboarding against a cluster that is not this machine's refuses, and refuses +// BEFORE the credential is revoked and before any teardown step runs — so the +// machine is left exactly as it was and the command is re-runnable. +func TestDelete_WrongCluster_RefusesBeforeAnyChange(t *testing.T) { + 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(`[]`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + withAnchor(t, "MY-OWN-CLUSTER") + withDeleteClusterID(t, "SOMEONE-ELSES-CLUSTER", nil) + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}) + if err == nil { + t.Fatal("offboarding the wrong cluster must be refused") + } + if !strings.Contains(err.Error(), "not the cluster your secure environment runs on") { + t.Errorf("unexpected error: %v", err) + } + // The ways this command is destructive, none of which may have happened. + if revoked { + t.Error("the machine credential was revoked despite the refusal") + } + if len(fn.calls) != 0 { + t.Errorf("teardown steps ran against the wrong cluster — the field bug: %v", fn.calls) + } +} + +// A cluster we cannot reach must NOT block offboarding: a dead cluster is the +// main reason to offboard, and blocking would leave the machine unremovable. +// This is the deliberate asymmetry against the mutating-data commands above. +func TestDelete_UnreachableCluster_StillOffboards(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + withAnchor(t, "MY-OWN-CLUSTER") + withDeleteClusterID(t, "", errors.New("dial tcp 127.0.0.1:6443: connect: connection refused")) + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("an unreachable cluster must not block offboarding: %v", err) + } +} + +// A machine with no recorded anchor still offboards — same reason as the resolve +// path: a config written before this field existed must not be stuck. +func TestDelete_NoAnchor_StillOffboards(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + // deliberately no withAnchor + called := false + orig := clusterIDForDelete + t.Cleanup(func() { clusterIDForDelete = orig }) + clusterIDForDelete = func(context.Context, cluster.KubeconfigOptions) (string, error) { + called = true + return "ANY", nil + } + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("an unrecorded anchor must not block offboarding: %v", err) + } + if called { + t.Error("with nothing to compare against, the identity read is pointless work" + + " — and on an unreachable cluster it costs the read timeout") + } +} + +// Offboarding this machine clears the anchor along with the rest of the active +// client. A stale anchor would make the NEXT install's commands refuse against a +// cluster that is now legitimately theirs. +func TestDelete_ClearsTheAnchor(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + withAnchor(t, "MY-OWN-CLUSTER") + withDeleteClusterID(t, "MY-OWN-CLUSTER", nil) + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, + deleteOpts{yes: true, keepData: true}); err != nil { // keepData: config survives to be read + t.Fatalf("offboard failed: %v", err) + } + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + if got := cfg.Current().ActiveClientClusterID; got != "" { + t.Errorf("the cluster anchor survived the offboard as %q — the next install"+ + " on this machine would refuse its own cluster", got) + } +} + +// --------------------------------------------------------------------------- +// Anti-rot: the decision must be forced on every cluster-touching call site, +// including ones added later. This does NOT restate the list of commands — it +// DERIVES the call sites from the source and requires each to carry a recorded +// intent, so a new `resolveClusterTarget` call reddens until someone decides. +// --------------------------------------------------------------------------- + +// mutationIntent records, per production call site, whether that command mutates +// the cluster. Adding a call site without adding a row here fails the test below; +// so does removing one. The VALUES are the decision under review — a reviewer +// reads this map and asks "does this command write to the cluster?". +var mutationIntent = map[string]bool{ + "seal.go": true, // rolls the jobs-manager with a sealed envelope + "data_delete.go": true, // drops a table, removes files from the shared PVC + "resources_set.go": true, // rewrites the resource envelope and restarts + "data_ingest_cluster.go": true, // stages a private dataset onto the cluster + "data_list.go": false, // reads the catalog + "resources.go": false, // prints the current envelope +} + +func TestEveryClusterCallSiteDeclaresMutationIntent(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + found := map[string]bool{} + for _, f := range files { + if strings.HasSuffix(f, "_test.go") || f == "clustertarget.go" { + continue // the definition itself, and the tests, are not call sites + } + fset := token.NewFileSet() + af, perr := parser.ParseFile(fset, f, nil, 0) + if perr != nil { + // Fail closed: an unparseable file is "cannot tell", not "agrees". + t.Fatalf("parsing %s: %v", f, perr) + } + ast.Inspect(af, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + id, ok := call.Fun.(*ast.Ident) + if !ok || (id.Name != "resolveClusterTarget" && id.Name != "resolveClusterTargetFn") { + return true + } + if len(call.Args) != 7 { + t.Errorf("%s: resolveClusterTarget called with %d args, want 7 — the"+ + " mutates parameter is what forces the decision", f, len(call.Args)) + return true + } + lit, ok := call.Args[6].(*ast.Ident) + if !ok || (lit.Name != "true" && lit.Name != "false") { + t.Errorf("%s: mutates must be a literal true/false at the call site, so a"+ + " reader sees the decision without tracing a variable", f) + return true + } + want, declared := mutationIntent[f] + if !declared { + t.Errorf("%s calls resolveClusterTarget but has no row in mutationIntent."+ + " Decide whether it writes to the cluster and add one — a command that"+ + " mutates without the guard is backend#2863 all over again.", f) + return true + } + found[f] = true + if got := lit.Name == "true"; got != want { + t.Errorf("%s passes mutates=%v, but mutationIntent says %v", f, got, want) + } + return true + }) + } + for f := range mutationIntent { + if !found[f] { + t.Errorf("mutationIntent has a row for %s, which no longer calls"+ + " resolveClusterTarget — stale rows hide a removed guard", f) + } + } +} + +// `delete` does not go through resolveClusterTarget (it shells out to helm), so +// the sweep above cannot see it. Assert its guard by behavior, not by grepping: +// the wrong-cluster test above is the real check; this one only pins that the +// seam exists so the guard cannot be quietly dropped while its tests keep +// passing against a no-op. +func TestDeleteGuardUsesTheRealIdentityRead(t *testing.T) { + // A test seam that defaults to something OTHER than the production function + // would make every delete test above vacuous. + if clusterIDForDelete == nil { + t.Fatal("clusterIDForDelete is nil — delete's identity guard cannot fire") + } + got, err := clusterIDForDelete(context.Background(), cluster.KubeconfigOptions{ + Path: filepath.Join(t.TempDir(), "does-not-exist"), + }) + // The real cluster.ClusterID must FAIL on a nonexistent kubeconfig. A stub + // that returns a value would pass the tests above while guarding nothing. + if err == nil { + t.Errorf("the default clusterIDForDelete returned %q for a nonexistent"+ + " kubeconfig — it is not the real identity read", got) + } +} + +// The anchor is only as good as its recording. setActiveClient must capture the +// cluster alongside the namespace — an install that records the namespace but not +// the cluster leaves the guard permanently in warn-and-proceed, which looks +// identical to a working guard in every log. +func TestSetActiveClient_RecordsTheClusterAnchor(t *testing.T) { + var p config.Profile + setActiveClient(&p, &api.ProvisionedClient{ + ID: 7, Name: "gpu-box-01", Namespace: "gpu-box-01", + ClusterID: "KUBE-SYSTEM-UID-7", + }) + if p.ActiveClientClusterID != "KUBE-SYSTEM-UID-7" { + t.Errorf("ActiveClientClusterID = %q, want the client's ClusterID —"+ + " without it every mutating command degrades to unverified", p.ActiveClientClusterID) + } +} + +// A client provisioned WITHOUT an anchor (the degraded create path, when the +// cluster was unreachable at create time) records an empty anchor rather than a +// wrong one. Empty means "unverified, warn"; a fabricated value would mean +// "verified" and refuse the user's own cluster forever. +func TestSetActiveClient_NoBackendAnchor_StaysEmpty(t *testing.T) { + var p config.Profile + setActiveClient(&p, &api.ProvisionedClient{ID: 7, Name: "x", Namespace: "x"}) + if p.ActiveClientClusterID != "" { + t.Errorf("ActiveClientClusterID = %q, want empty for an unanchored client", + p.ActiveClientClusterID) + } +} + +// There must be exactly ONE place that makes a client active. The guard is silent +// when the anchor is missing (deliberately — see clusterguard.go), so a SECOND +// write path that set the id and namespace without the cluster would disable the +// guard for anyone who took that path, and nothing would say so. Derived from the +// source, so a new path reddens here rather than being noticed in the field. +func TestActiveClientHasOneWritePath(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + var writers []string + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + fset := token.NewFileSet() + af, perr := parser.ParseFile(fset, f, nil, 0) + if perr != nil { + t.Fatalf("parsing %s: %v", f, perr) // fail closed + } + ast.Inspect(af, func(n ast.Node) bool { + as, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + for _, lhs := range as.Lhs { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "ActiveClientID" { + continue + } + // delete.go's clear-on-offboard is a teardown, not an activation. + if f == "delete.go" { + continue + } + writers = append(writers, f+":"+fset.Position(as.Pos()).String()) + } + return true + }) + } + if len(writers) != 1 { + t.Errorf("ActiveClientID is assigned in %d places outside delete.go: %v\n"+ + "Only setActiveClient may activate a client, because it is what records the"+ + " cluster anchor. A second path silently disables the backend#2863 guard.", + len(writers), writers) + } + if len(writers) == 1 && !strings.HasPrefix(writers[0], "client.go:") { + t.Errorf("the single write path moved to %s — confirm it still records"+ + " ActiveClientClusterID", writers[0]) + } +} diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index 1ebcb68d..640525cd 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -94,7 +94,12 @@ type clusterTarget struct { // note then self-leads its one clean leading blank), false when the command has // already printed something before resolving (data ingest's "Connecting…", data // delete's warning — the note stays inline, no mid-output blank). See §380. -func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC, leadRedirect bool) (*clusterTarget, error) { +// mutates is NOT a convenience flag — it is the compiler making every caller decide. +// A mutating command that reaches the wrong cluster writes to it (backend#2863), so +// the choice must be impossible to omit rather than an opt-in helper a new command +// forgets to call. `true` runs guardActiveClientCluster before the target is handed +// back; `false` is a read. +func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC, leadRedirect, mutates bool) (*clusterTarget, error) { resolved, err := loadClusterFn(opts) if err != nil { return nil, &exitError{code: exitLocalEnv, err: fmt.Errorf("loading kubeconfig: %w", err)} @@ -126,6 +131,14 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // prints) keys on Resolved.Namespace, so it must follow. resolved.Namespace = nsUsed t := &clusterTarget{Resolved: resolved, Clientset: cs, Release: release} + // Identity check BEFORE the PVC discovery below and before the target is handed + // back, so a mutating command cannot have touched anything by the time it is told + // this is the wrong cluster. + if mutates { + if err := guardActiveClientCluster(ctx, p, t); err != nil { + return nil, err + } + } if needPVC { pvc, err := cluster.DiscoverSharedPVC(ctx, cs, resolved.Namespace) if err != nil { diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index 17724961..daf67d66 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -42,7 +42,7 @@ func withClusterSeams(t *testing.T, cs kubernetes.Interface) { func TestResolveClusterTarget_NoClient_InstallerMessageExit4(t *testing.T) { withClusterSeams(t, fake.NewSimpleClientset()) // empty cluster _, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{}, activeClientBinding{}, true, true) + cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false) if err == nil { t.Fatal("expected an error when the cluster hosts no client") } @@ -68,7 +68,7 @@ func TestResolveClusterTarget_NoClient_InstallerMessageExit4(t *testing.T) { func TestResolveClusterTarget_MultipleClients_PickOneExit4(t *testing.T) { withClusterSeams(t, fake.NewSimpleClientset(jmDep("alpha"), jmDep("beta"))) _, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{}, activeClientBinding{}, true, true) + cluster.KubeconfigOptions{}, activeClientBinding{}, true, true, false) if err == nil { t.Fatal("expected an error when multiple clients are present") } @@ -338,7 +338,7 @@ func TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting(t *testing.T) binding := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "stale-ns"} target, err := resolveClusterTarget(context.Background(), nil, - cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false) + cluster.KubeconfigOptions{Namespace: "stale-ns"}, binding, false, false, false) if err == nil { t.Fatal("a binding miss must still fail — this changes the message, not the target") } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 2e9aebc3..27ae2199 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -174,7 +174,7 @@ undone — re-ingesting the data is the only way back.`) // leadRedirect=false: the warning paragraph above is already this command's // opening output, so the multi-client redirect note stays inline — no // mid-output blank between the warning and the note (§380). - target, err := resolveClusterTargetFn(ctx, a.Printer, opts, binding, true, false) + target, err := resolveClusterTargetFn(ctx, a.Printer, opts, binding, true, false, true) if err != nil { return binding.explain(ctx, err) } diff --git a/internal/cli/data_delete_execute_test.go b/internal/cli/data_delete_execute_test.go index 645b11d6..2c8fa3d6 100644 --- a/internal/cli/data_delete_execute_test.go +++ b/internal/cli/data_delete_execute_test.go @@ -35,7 +35,7 @@ func TestRunDataDelete_Execute(t *testing.T) { resolveClusterTargetFn, listDatasetsFn, teardownFn = origRCT, origList, origTD }) - resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _ bool) (*clusterTarget, error) { + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _, _ bool) (*clusterTarget, error) { return &clusterTarget{ Resolved: &cluster.ResolvedConfig{Context: "ctx", Namespace: "tracebloc"}, Clientset: fake.NewSimpleClientset(), diff --git a/internal/cli/data_delete_json_test.go b/internal/cli/data_delete_json_test.go index 7bb00858..abdb4877 100644 --- a/internal/cli/data_delete_json_test.go +++ b/internal/cli/data_delete_json_test.go @@ -33,7 +33,7 @@ func TestRunDataDelete_OutputJSON(t *testing.T) { resolveClusterTargetFn, listDatasetsFn, teardownFn = origRCT, origList, origTD }) - resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _ bool) (*clusterTarget, error) { + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _, _ bool) (*clusterTarget, error) { return &clusterTarget{ Resolved: &cluster.ResolvedConfig{Context: "ctx", Namespace: "tracebloc"}, Clientset: fake.NewSimpleClientset(), diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index a750d800..51345612 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -56,7 +56,7 @@ func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clu // leadRedirect=false: the "Connecting…" line above is already this command's // opening output, so the multi-client redirect note stays inline — no // mid-output blank between "Connecting…" and the note (§380). - target, err = resolveClusterTarget(ctx, a.Printer, opts, binding, true, false) + target, err = resolveClusterTarget(ctx, a.Printer, opts, binding, true, false, true) if err != nil { return nil, "", false, binding.explain(ctx, err) } diff --git a/internal/cli/data_list.go b/internal/cli/data_list.go index 65776b80..367f406d 100644 --- a/internal/cli/data_list.go +++ b/internal/cli/data_list.go @@ -123,7 +123,7 @@ func runDataList(ctx context.Context, a runDataListArgs) (err error) { binding := bindActiveClientNamespace(&opts) // leadRedirect=true: data list prints nothing before resolving, so the // multi-client redirect note is the opening line and self-leads its blank. - target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) + target, err := resolveClusterTarget(ctx, p, opts, binding, false, true, false) if err != nil { return binding.explain(ctx, err) } diff --git a/internal/cli/delete.go b/internal/cli/delete.go index d0632199..093b3195 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -13,11 +13,16 @@ import ( "github.com/spf13/cobra" "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/nodeboot" "github.com/tracebloc/cli/internal/ui" ) +// clusterIDForDelete is a test seam over cluster.ClusterID for the identity guard +// in runDelete (backend#2863). +var clusterIDForDelete = cluster.ClusterID + // nodeboot teardown hooks — package vars so tests fake the k3d/helm/docker // shell-outs (a delete test never touches a real cluster or the docker daemon). var ( @@ -141,6 +146,38 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er ns = prof.ActiveClientNamespace } + // Cluster-identity guard (backend#2863). `delete` never resolves a cluster + // target — it hands the raw kubeconfig flags to `helm uninstall` — so on a + // machine whose current-context points elsewhere it uninstalled a release of + // the SAME NAME off that other cluster. This is the one command where the + // mistake is unrecoverable, so the check runs HERE: before the credential is + // revoked and before anything is torn down, so a refusal leaves the machine + // exactly as it was. + // + // Asymmetric on purpose, and the opposite way round from the data commands: + // a cluster we cannot reach must NOT block offboarding, because a dead + // cluster is the main reason to offboard. Only a cluster we positively + // identify as a DIFFERENT one refuses. + if want := prof.ActiveClientClusterID; want != "" { + if got, idErr := clusterIDForDelete(ctx, cluster.KubeconfigOptions{ + Path: o.kubeconfigPath, + Context: o.contextOverride, + // Namespace is irrelevant to the kube-system anchor read. + }); idErr == nil && got != want { + return &exitError{code: exitLocalEnv, err: fmt.Errorf( + "this is not the cluster your secure environment runs on — refusing to offboard.\n"+ + " reached: cluster %s\n"+ + " expected: cluster %s\n"+ + " Offboarding here would uninstall a release of the same name off the wrong\n"+ + " cluster. Your kubeconfig's current context points somewhere else: either\n"+ + " switch it, or pass --context/--kubeconfig for your own cluster.", + short(got), short(want))} + } + // idErr != nil is deliberately silent: unreachable is the expected state of + // a cluster being retired, and the Helm uninstall below is already + // best-effort and reports its own failure. + } + // Work-guard: refuse to offboard while training runs are ACTIVE (offboarding // would kill them), unless --force. Block on RUNNING experiments, NOT on // "online": a healthy environment is always online (it heartbeats), so @@ -276,6 +313,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // the --keep-data and killed-mid-teardown cases safe. Re-running `client create` // re-adopts by cluster_id. prof.ActiveClientID, prof.ActiveClientName, prof.ActiveClientNamespace = "", "", "" + prof.ActiveClientClusterID = "" // the cluster anchor goes with the rest (backend#2863) if serr := cfg.Save(); serr != nil { degraded = true p.Warnf("Couldn't clear the stored active-client pointer (%v) — the on-disk config "+ diff --git a/internal/cli/resources.go b/internal/cli/resources.go index e5252658..fc08e188 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -91,7 +91,7 @@ func runResourcesShow(ctx context.Context, p *ui.Printer, opts cluster.Kubeconfi binding := bindActiveClientNamespace(&opts) // leadRedirect=true: this resolve is the command's first output, so the // multi-client redirect note self-leads its one leading blank (§380). - target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) + target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true, false) if err != nil { return binding.explain(ctx, err) } diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 61b0fa74..29dfba22 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -144,7 +144,7 @@ func runResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, opts clust // single-client path there's no note and the confirm/dry-run self-lead // supplies the only leading blank — so `set` keeps NO pre-resolve Newline() // and never regresses the #375 double-blank (§380). - target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) + target, err := resolveClusterTarget(ctx, p, opts, binding, false, true, true) if err != nil { return binding.explain(ctx, err) } diff --git a/internal/cli/resources_test.go b/internal/cli/resources_test.go index 9eb6c8e5..599efe8a 100644 --- a/internal/cli/resources_test.go +++ b/internal/cli/resources_test.go @@ -98,7 +98,7 @@ func TestShow_OpensWithSingleBlank(t *testing.T) { orig := resolveClusterTargetFn t.Cleanup(func() { resolveClusterTargetFn = orig }) cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) - resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _ bool) (*clusterTarget, error) { + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _, _ bool) (*clusterTarget, error) { return resTarget(cs), nil } diff --git a/internal/cli/seal.go b/internal/cli/seal.go index d2dca1fd..40898a59 100644 --- a/internal/cli/seal.go +++ b/internal/cli/seal.go @@ -76,7 +76,7 @@ func runSealCheck(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOpt binding := bindActiveClientNamespace(&opts) // leadRedirect=true: seal prints nothing before resolving, so the // multi-client redirect note is the opening line and self-leads its blank. - target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) + target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true, true) if err != nil { return binding.explain(ctx, err) } diff --git a/internal/cli/seal_test.go b/internal/cli/seal_test.go index 32a26d7a..29c5fe9c 100644 --- a/internal/cli/seal_test.go +++ b/internal/cli/seal_test.go @@ -22,7 +22,7 @@ import ( func stubSealTarget(t *testing.T) { t.Helper() orig := resolveClusterTargetFn - resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _, _ bool) (*clusterTarget, error) { + 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"}, @@ -312,7 +312,7 @@ func TestSeal_HookListError_NoVerdict(t *testing.T) { // §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) { + 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 }) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index ee04e033..2e97ac52 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -163,6 +163,7 @@ screen. %s/%d are runtime placeholders. "Couldn't connect to your secure environment — check your kubeconfig/context." "Couldn't determine this client's namespace — skipped the Helm uninstall. If a release is still installed, re-run with --namespace ." "Couldn't locate the CLI binary to remove it (%v) — delete it by hand." +"Couldn't read the local config, so this machine's cluster couldn't be verified before changing anything. Proceeding." "Couldn't read the target cluster's identity — provisioning without a cluster anchor, so re-running won't be idempotent. Point --kubeconfig/--context at the reachable cluster to enable that." "Couldn't read your tracebloc config — run `%s login` to recreate it." "Couldn't reclaim the temporary copy (%v). It's harmless — the next re-ingest of %q or a `tracebloc data delete %s` will clear it." @@ -372,6 +373,7 @@ screen. %s/%d are runtime placeholders. "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)." "This is irreversible. Type the client name to confirm, or leave blank to cancel." +"This machine hasn't recorded which cluster its secure environment runs on, so the target couldn't be verified before changing anything. Run `tracebloc client create` to record it. Proceeding against %s." "This machine's credential — so tracebloc can no longer reach it" "This matches a previous run (same idempotency key) — attaching to the run already in progress." "This permanently removes a dataset you ingested earlier: it drops the table from\nthe cluster and deletes the dataset's files on the shared storage. It can't be\nundone — re-ingesting the data is the only way back." @@ -469,6 +471,7 @@ screen. %s/%d are runtime placeholders. "context" "could not check — cluster API unreachable (see 'Cluster reachable' above)" "couldn't check whether a tracebloc client is already running on this cluster (%w) — provisioning now could mint a duplicate that never deploys and locks the cluster to it. Re-run (if this was transient); if it persists, ensure your kubeconfig/context can list deployments and secrets across namespaces. Diagnose with `tracebloc doctor`" +"couldn't confirm which cluster this is before changing anything (%w).\n Refusing rather than writing to an unidentified cluster.\n reached: %s" "couldn't determine the installed client chart version (the release is missing its helm.sh/chart version label), so the upgrade can't be pinned to it. Refusing to change resources with an unpinned upgrade — it would pull the latest chart and could silently change your client. Re-run the tracebloc installer to repair the release, then try again" "couldn't reach the backend to choose a unique client name (%v) — retry, or pass --name explicitly" "couldn't reach the backend to finish signing in — %d attempts failed in a row (check your network / HTTPS_PROXY): %w" @@ -751,6 +754,8 @@ screen. %s/%d are runtime placeholders. "the time column %q has %d missing/invalid value(s) (first at data row(s) %v%s). Every timestep row needs a valid value to order it within its sequence — the cluster rejects this after the upload; fix the values and re-run." "this backend (%s) doesn't support browser login yet — the device-grant endpoints land in backend#835: %w" "this cluster is already registered to another tracebloc account (%s) — ask them to release it, or sign in as that account (cluster_conflict)" +"this is not the cluster your secure environment runs on — refusing to change anything.\n reached: %s (cluster %s, context %q)\n expected: cluster %s\n Your kubeconfig's current context points somewhere else. Either switch it, or pass\n --context/--kubeconfig for the cluster your secure environment runs on." +"this is not the cluster your secure environment runs on — refusing to offboard.\n reached: cluster %s\n expected: cluster %s\n Offboarding here would uninstall a release of the same name off the wrong\n cluster. Your kubeconfig's current context points somewhere else: either\n switch it, or pass --context/--kubeconfig for your own cluster." "this machine has %s, but you asked for %s." "this machine is too small to choose an amount — after tracebloc's ~1 core and 3 GiB overhead it can offer a training run at most %d core(s) and %d GiB. Free up resources or use a larger machine." "this task's data is sequence-grouped: the schema must declare %q (groups the timestep rows of one sequence — e.g. a patient/device/session id) and %q (orders the rows within each sequence). Missing: %s. The column names are fixed by the platform — rename your CSV columns to match and re-run." diff --git a/internal/cluster/identity.go b/internal/cluster/identity.go index b031491e..2f86e65d 100644 --- a/internal/cluster/identity.go +++ b/internal/cluster/identity.go @@ -53,6 +53,17 @@ func DiscoverInClusterClient(ctx context.Context, opts KubeconfigOptions) (*InCl return DiscoverInClusterClientID(ctx, cs) } +// ClusterIDFrom reads the kube-system UID from an ALREADY-BUILT clientset. +// +// Exported for the mutating-command identity guard (backend#2863): every mutating +// command has a clientset in hand by the time it would mutate, and re-entering +// ClusterID would load the kubeconfig a second time — a second chance to resolve a +// different cluster than the one about to be written to, which is the exact defect +// the guard exists to close. +func ClusterIDFrom(ctx context.Context, cs kubernetes.Interface) (string, error) { + return clusterIDFrom(ctx, cs) +} + // clusterIDFrom reads the kube-system UID from a clientset. Split out so it can be // exercised with a fake clientset without a real cluster. func clusterIDFrom(ctx context.Context, cs kubernetes.Interface) (string, error) { diff --git a/internal/config/config.go b/internal/config/config.go index e4e24102..2ffd4906 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -51,6 +51,21 @@ type Profile struct { // when no client is active or for pre-v2 configs that predate the cache. ActiveClientNamespace string `json:"active_client_namespace,omitempty"` ActiveClientName string `json:"active_client_name,omitempty"` + + // ActiveClientClusterID pins WHICH CLUSTER the active client lives on: the + // kube-system namespace UID, the same anchor the backend record uses + // (api.ProvisionedClient.ClusterID). Recorded at `client create` time. + // + // Why it exists (backend#2863): the namespace cache above let the data + // commands bind the right NAMESPACE while the CLUSTER stayed whatever + // kubectl's current-context happened to reach. So a mutating command run on a + // laptop whose context pointed at a remote cluster would act there — writing a + // dataset, dropping a table, rolling a release, or uninstalling a same-named + // release — with nothing on screen to reveal it. + // + // Empty for configs that predate this field, so the guard degrades to a warning + // rather than locking existing installs out of their own commands. + ActiveClientClusterID string `json:"active_client_cluster_id,omitempty"` } // Config is the on-disk CLI state: env-scoped profiles plus the current env. diff --git a/internal/resources/provenance_test.go b/internal/resources/provenance_test.go index 0e0ce497..0425cd89 100644 --- a/internal/resources/provenance_test.go +++ b/internal/resources/provenance_test.go @@ -29,7 +29,10 @@ func TestBuildEnvSpecStampsUserProvenance(t *testing.T) { t.Errorf("RESOURCE_LIMITS = %q, want cpu=6,memory=24Gi", env["RESOURCE_LIMITS"]) } if env["RESOURCE_REQUESTS"] != env["RESOURCE_LIMITS"] { - t.Error("requests and limits diverged — Guaranteed QoS is the chart contract") + t.Error("requests and limits diverged — BuildEnvSpec writes them equal so a" + + " CPU training pod is Guaranteed. (Not \"the chart contract\", as this" + + " message said until backend#2872: the chart's derive path writes no cpu" + + " limit at all since backend#2418.)") } } diff --git a/internal/resources/resources.go b/internal/resources/resources.go index b0f3e6bb..e54f9d68 100644 --- a/internal/resources/resources.go +++ b/internal/resources/resources.go @@ -8,8 +8,14 @@ // Grounding (verified against tracebloc/client + client-runtime, 2026-07): // - The machine's capacity is the sum of Ready nodes' Status.Allocatable // (the installer path is single-node, so this is normally one node). -// - A training run's ceiling is the jobs-manager env RESOURCE_LIMITS -// ("cpu=2,memory=8Gi", requests==limits for Guaranteed QoS). This is the +// - A training run's ceiling is the jobs-manager env RESOURCE_LIMITS. This +// package writes it equal to RESOURCE_REQUESTS on both dimensions, so a CPU +// training pod comes out Guaranteed. Two caveats this comment used to elide +// (backend#2872): the built-in fallback is the contract floor +// cpu=1,memory=2Gi since backend#2254, not the "cpu=2,memory=8Gi" named +// here before; and a GPU pod is BestEffort whatever this writes, because +// client-runtime's GPU path sets only nvidia.com/gpu and ephemeral-storage +// and neither counts toward QoS (backend#2871). This is the // exact value client-runtime's jobs_manager.py stamps on spawned jobs and // the same value `cluster doctor`'s checkNodeFit already parses — so the two // read it identically (di#358 lesson: a reader must mirror the writer). @@ -135,7 +141,9 @@ func MachineCapacity(nodes []corev1.Node) Machine { // ParseTraining reads the per-run ceiling from a jobs-manager env map. It // prefers RESOURCE_LIMITS (the true ceiling) and falls back to RESOURCE_REQUESTS -// (requests==limits by chart contract), then to the chart default so an older +// (which this package writes equal to the limits; NOT a chart-wide contract -- +// the chart's derive path writes no cpu limit at all since backend#2418, +// backend#2872), then to the chart default so an older // chart without the literal env still reports the effective size. GPU is read // from GPU_LIMITS, then GPU_REQUESTS. func ParseTraining(env map[string]string) Training { diff --git a/internal/resources/set.go b/internal/resources/set.go index bd63545e..9dd80959 100644 --- a/internal/resources/set.go +++ b/internal/resources/set.go @@ -194,8 +194,15 @@ func MemFloorText() string { return "2 GiB" } const NoGPUEnvValue = "" // BuildEnvSpec renders a per-run ceiling into the exact chart env the run is -// stamped with. RESOURCE_REQUESTS == RESOURCE_LIMITS (Guaranteed QoS, the chart -// contract), both "cpu=X,memory=Y". +// stamped with. RESOURCE_REQUESTS == RESOURCE_LIMITS, both "cpu=X,memory=Y" -- +// which yields a Guaranteed CPU training pod. +// +// NOT "the chart contract", as this said until backend#2872. The chart's own +// DERIVE path (DERIVE_JOB_ENVELOPE) writes no cpu limit at all since +// backend#2418, so requests == limits is this package's choice for the explicit +// envelope, not a chart-wide invariant -- and calling it a contract is what let +// the claim survive after #2418 falsified it elsewhere. A GPU pod is BestEffort +// regardless of what is written here (backend#2871). // // Every dimension is ALWAYS written — never omitted. The apply uses // `helm upgrade --reset-then-reuse-values`, which re-applies the release's