diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 6b3306c..9e6d70c 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -73,6 +73,27 @@ Two things make this repo unusual and should shape every finding: (`internal/api/client.go`, `nextPath`). Where "empty" and "unknown" are different answers, prefer a three-valued return (`internal/cluster/discover.go:302`). +- **"Couldn't confirm" used as "confirmed" — in a BRANCH, not just a return type.** The rule + above is about the value a function hands back; this is about the `if` that consumes it, and + it is the defect this repo produces most often. cli#515 shipped it three times in one PR, + each in a different file, each after the previous one was fixed: a failed cluster scan + reported as "no client is running here"; an empty `cluster_id` on a legacy client read as + "runs elsewhere" (`ProvisionedClient.ClusterID` is documented empty on not-yet-backfilled + records); and `reachStateOf(x) != ReachNoEnv` used to mean "an environment is here", when + `ReachState` also has `ReachUnreachable` and `ReachError`. Two concrete shapes to flag: + - **A negated comparison against ONE member of a multi-valued enum.** `!= ReachNoEnv`, + `!= StatusFail` and friends silently include every member added later. Compare against the + member you actually require (`== ReachOK`), and derive the test's input domain from the + enum's declared surface — mutation coverage cannot see a vocabulary gap. + - **A lenient "not found" default reused where the question is "may I believe this?"** + `reachStateOf` returns `ReachOK` for an ABSENT check, which is right for a verdict roll-up + and wrong for authorising a claim — hence the separate `reachConfirmedOK` + (`internal/cli/doctor.go`). The same default is rarely correct for both. + + The customer-visible cost is never a wrong log line: on #515 each instance ended in advice to + run `client create` on a cluster nothing was confirmed on, where it MINTS rather than adopts — + i.e. the guidance manufactured the orphaned phantom of `backend#970`. + - **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 diff --git a/VERSION b/VERSION index 1a46c7f..f314d02 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.8 +0.10.9 diff --git a/docs/cli-navigation.md b/docs/cli-navigation.md index f8ce400..cba1423 100644 --- a/docs/cli-navigation.md +++ b/docs/cli-navigation.md @@ -22,7 +22,7 @@ flowchart TD ACCT --> logout["logout"] ACCT --> authst["auth status"] ACCT --> clis["client status"] - ACCT -.-> clcreate["client create"]:::hidden + ACCT --> clcreate["client create — point this machine at its client"] ACCT -.-> cllist["client list"]:::hidden ENVC --> di["data ingest"] @@ -172,7 +172,7 @@ flowchart TD - **not signed in / token 401·403** → `login` - **426 upgrade-required** → upgrade the CLI - **kubeconfig (exit 3)** → fix `--kubeconfig`/`--context`, then `doctor` -- **no client / environment (exit 4)** → run the installer (or `--namespace`); triage with `doctor` +- **no client / environment (exit 4)** → the error now says what IS on the reached cluster before advising (cli#515): one client on a local cluster → `client create` repoints this machine (it adopts, no new credential); a client on a remote/shared cluster → `--namespace ` only; nothing there → run the installer. Triage with `doctor` - **no token (exit 5)** → grant RBAC; diagnose with `cluster info` / `doctor` - **destination exists (exit 6)** → `--overwrite`, a different `--name`, or `data delete` first - **staging partial (exit 7)** → `data delete` then re-ingest diff --git a/internal/cli/client.go b/internal/cli/client.go index cd662a4..d7246ee 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -39,8 +39,9 @@ var readInClusterClient = cluster.DiscoverInClusterClient // The single-machine CLI (RFC-0001 §7.10) owns exactly one client, so there is // nothing to *select*: `client use` is withdrawn, and `client list` is hidden // (kept callable for the installer's one-client-per-machine pre-flight, off the -// user-facing surface). `create` provisions this machine's client; offboarding -// is the top-level `tracebloc delete`. +// user-facing surface). `create` points this machine at its client — adopting +// the one already on the cluster, which is the supported repoint (#515) — +// and offboarding is the top-level `tracebloc delete`. func newClientCmd() *cobra.Command { cmd := &cobra.Command{ Use: "client", @@ -63,15 +64,31 @@ func newClientCreateCmd() *cobra.Command { var yes bool cmd := &cobra.Command{ Use: "create", - Short: "Provision a tracebloc client for this machine (auto-named; no flags required)", - // HIDDEN: provisioning is the installer's job — provision.sh calls this with - // zero flags (cli#137). It stays fully callable (including `--help`, so the - // installer's capability probe still works), but is kept off the user-facing - // surface: a human running `client create` STANDALONE mints a client the - // installer never deploys — an orphaned "phantom" (backend#970). Mirrors the - // hidden `list`; leaves `tracebloc client` showing only the user-useful `status`. - Hidden: true, - Args: cobra.NoArgs, + Short: "Point this machine at its tracebloc client — adopts the one already on this cluster", + Long: `Point this machine at its tracebloc client. + +Keyed on the cluster your kubeconfig reaches: if a tracebloc client already runs +there, this ADOPTS it — no prompt, no new credential, no duplicate — which is how +you repoint a machine whose active client went stale. On a cluster that runs no +client yet it provisions a new one, and asks first. + +Provisioning a brand-new machine is normally the installer's job — it calls this +for you, with no flags.`, + // WAS HIDDEN (backend#970), and the reason still stands: a human running + // this STANDALONE on a cluster with no client mints one the installer never + // deploys — an orphaned "phantom". Hiding it was never what prevented that, + // though; the mint-path guards below are, and they are untouched: + // • on a TTY, the review + `Provision this client?` confirm (which a + // re-run on an already-registered cluster never reaches — it adopts + // before the prompt, so the repoint stays zero-friction); + // • off a TTY, a hard refusal without --yes/--credential-file, so a pipe + // or CI can never mint silently. + // What hiding DID cost is #515: the §7.3 "your active client runs on another + // machine" error had no supported way back, because the one command that + // repoints a machine was unlisted. Advice pointing at a hidden command is + // not advice, so it is listed now — described by what it does for a user + // (adopt/repoint) rather than by the installer's use of it. + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runClientCreate(cmd.Context(), printerFor(cmd), clientPrompter(), clientCreateOpts{name: name, location: location, kubeconfigPath: kubeconfigPath, contextOverride: contextOverride, credentialFile: credentialFile, yes: yes}) @@ -747,21 +764,106 @@ func runClientList(ctx context.Context, p *ui.Printer) error { } p.Section("Clients in your account") active := cfg.Current().ActiveClientID + // #515: "active" is a LOCAL POINTER, and this listing used to render it as + // "(active — this machine)" — a claim about location it never checked. When + // the pointer is stale that label sits next to a client provably not on the + // cluster this machine reaches, which is the state the whole ticket is about. + // Read the cluster anchor (kube-system UID, the §7.2 identity `client create` + // keys on) and mark residency separately from selection. A failed read is + // three-valued on purpose: unknown is not "elsewhere", so the marker then + // claims nothing about where anything runs. + clusterID, cidErr := readClusterID(ctx, cluster.KubeconfigOptions{}) + hereKnown := cidErr == nil && clusterID != "" + activeElsewhere, anyHere := false, false for _, c := range clients { - marker := "" - if strconv.Itoa(c.ID) == active { - marker = " (active — this machine)" + isActive := strconv.Itoa(c.ID) == active + res := residencyOf(hereKnown, clusterID, c.ClusterID) + if isActive && res == resElsewhere { + activeElsewhere = true } - p.Field(strconv.Itoa(c.ID)+marker, + if res == resHere { + anyHere = true + } + p.Field(strconv.Itoa(c.ID)+clientListMarker(isActive, res), fmt.Sprintf("%s state=%s namespace=%s location=%s", c.Name, clientStateLabel(c.Status), c.Namespace, c.Location)) } // §7.3: separate "selected" (this machine's local pointer) from "connected" // (the backend's last-heartbeat state) so a stale pointer is visible. p.Hintf("\"active\" is this machine's selected client; state is its last reported status to tracebloc.") + switch { + case activeElsewhere && anyHere: + // The exact state #515 describes, and the one supported way out of it: + // re-running create on a cluster that already hosts a client adopts it — + // no prompt, no new credential (§7.2). anyHere is what earns the phrase + // "the client that IS there": without a row we KNOW is on this cluster, + // `client create` would fall through to the mint path and produce the + // phantom backend#970 is about (Bugbot). + p.Hintf("Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create", launcher()) + case activeElsewhere: + // The pointer is provably wrong, but no listed client is provably here — + // either none is, or the ones that might be carry no anchor to prove it + // (resUnknown). Both are "we can't name a target", so name none: send + // them to the command whose whole job is to say what's on this cluster + // rather than advertise a repoint that may have nothing to adopt. + p.Hintf("Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor", launcher()) + } return nil } +// residency answers "does this client run on the cluster the kubeconfig +// reaches" in THREE values, because two of them are absences and an absence is +// never a "no" (Bugbot, #515). +type residency int + +const ( + // resUnknown: we cannot tell. Either the local cluster anchor was + // unreadable (no kubeconfig, unreachable API server, RBAC on kube-system), + // or the CLIENT carries no anchor — `ProvisionedClient.ClusterID` is empty + // on legacy / not-yet-backfilled records (api/client.go), and a record that + // never learned where it lives is not a record that lives elsewhere. + resUnknown residency = iota + resHere + resElsewhere +) + +// residencyOf compares the local cluster anchor with a client's, keeping both +// missing-anchor cases at resUnknown. Collapsing either into "elsewhere" would +// print "NOT on the cluster your kubeconfig reaches" — and the repoint hint — +// next to a legacy client that may be running on this very machine. +func residencyOf(hereKnown bool, localAnchor, clientAnchor string) residency { + if !hereKnown || clientAnchor == "" { + return resUnknown + } + if clientAnchor == localAnchor { + return resHere + } + return resElsewhere +} + +// clientListMarker renders one row's suffix in `client list`, keeping SELECTION +// (this machine's local pointer) and RESIDENCY (where the client actually runs) +// as two separate facts (#515). Under resUnknown the marker degrades to bare +// "(active)" — which says only what the local config actually knows — and +// claims nothing about location in either direction. +func clientListMarker(isActive bool, res residency) string { + switch { + case res == resUnknown: + if isActive { + return " (active)" + } + return "" + case isActive && res == resHere: + return " (active — on this cluster)" + case isActive: + return " (active — NOT on the cluster your kubeconfig reaches)" + case res == resHere: + return " (on this cluster)" + default: + return "" + } +} + // 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. diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index fab9bd4..d771124 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -1470,19 +1470,18 @@ func TestClientStatus_WaitCtrlCIsSilent(t *testing.T) { } func TestClientSubcommandVisibility(t *testing.T) { - // `create` and `list` are installer-internal — Hidden so a user isn't invited to - // run them (a standalone `tracebloc client create` mints a client the installer - // never deploys, i.e. an orphaned phantom, backend#970). `status` stays - // user-visible. Hidden != disabled: all remain runnable (the installer still - // invokes create/list). + // `create` is VISIBLE since #515: it is the supported way to repoint a machine + // whose active client went stale, and the §7.3 error now names it — advice + // pointing at a hidden command is not advice. `list` stays installer-internal. + // `status` stays user-visible. Hidden != disabled: all remain runnable. hidden := map[string]bool{} runnable := map[string]bool{} for _, c := range newClientCmd().Commands() { hidden[c.Name()] = c.Hidden runnable[c.Name()] = c.RunE != nil } - if !hidden["create"] { - t.Error("client create must be Hidden (installer-internal; standalone mints a phantom)") + if hidden["create"] { + t.Error("client create must be visible — the #515 repoint advice names it") } if !hidden["list"] { t.Error("client list must stay Hidden") @@ -1491,7 +1490,248 @@ func TestClientSubcommandVisibility(t *testing.T) { t.Error("client status must stay user-visible") } if !runnable["create"] { - t.Error("hidden create must still be runnable (the installer invokes it)") + t.Error("create must still be runnable (the installer invokes it)") + } +} + +// Unhiding `create` must not reopen backend#970: hiding it was never what +// stopped a standalone run from minting a phantom — these two guards are, and +// they have to survive the visibility change. Off a TTY (pr == nil) with no +// --yes and no --credential-file, a fresh mint is REFUSED; nothing is posted. +func TestClientCreate_UnhiddenStillRefusesSilentMint(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) // no clients on the account → a mint, not an adopt + }) + signInAs(t, "Lab", "lab@example.com") + var out bytes.Buffer + err := runClientCreate(context.Background(), ui.New(&out), nil, clientCreateOpts{}) + if err == nil { + t.Fatal("a non-interactive bare `client create` must refuse to mint") + } + if !strings.Contains(err.Error(), "refusing to provision non-interactively") { + t.Errorf("want the pipe refusal, got: %v", err) + } + if posted { + t.Error("nothing may be provisioned by a refused run") + } +} + +// The TTY half of the same guard: on a terminal a fresh mint asks first, and a +// "no" provisions nothing. (The repoint itself never reaches this prompt — an +// already-registered cluster adopts before it, covered by the adopt tests.) +func TestClientCreate_UnhiddenStillPromptsBeforeMinting(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) + }) + signInAs(t, "Lab", "lab@example.com") + no := false + var out bytes.Buffer + if err := runClientCreate(context.Background(), ui.New(&out), &fakePrompter{confirm: &no}, clientCreateOpts{}); err != nil { + t.Fatalf("declining is a clean exit, got: %v", err) + } + if posted { + t.Error("a declined confirm must provision nothing") + } +} + +// #515: `client list` used to label the active pointer "(active — this machine)" +// without ever checking where that client runs, so a stale pointer read as +// confirmation. Selection and residency are now two separate facts, keyed on the +// cluster anchor (§7.2). +func TestClientListMarker(t *testing.T) { + cases := []struct { + name string + isActive bool + res residency + want string + }{ + {"residency unknown, active → claims only selection", true, resUnknown, " (active)"}, + {"residency unknown, other → no claim", false, resUnknown, ""}, + {"active and here", true, resHere, " (active — on this cluster)"}, + {"active but elsewhere", true, resElsewhere, " (active — NOT on the cluster your kubeconfig reaches)"}, + {"here but not selected", false, resHere, " (on this cluster)"}, + {"elsewhere and not selected", false, resElsewhere, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := clientListMarker(c.isActive, c.res); got != c.want { + t.Errorf("clientListMarker(%v,%v) = %q, want %q", c.isActive, c.res, got, c.want) + } + }) + } + // The specific claim #515 calls out: unknown residency must never let a row + // assert — or deny — that it is here. + for _, isActive := range []bool{true, false} { + if strings.Contains(clientListMarker(isActive, resUnknown), "this cluster") { + t.Errorf("isActive=%v: unknown residency must claim nothing about location", isActive) + } + } +} + +// Bugbot (#515): residency has to stay THREE-valued on both sides of the +// comparison. An unreadable LOCAL anchor was already handled; a client whose OWN +// anchor is empty — legacy / not-yet-backfilled, per api.ProvisionedClient — +// was being forced to "elsewhere", which told the owner of a perfectly local +// legacy client that it is not on this cluster. +func TestResidencyOf(t *testing.T) { + cases := []struct { + name string + hereKnown bool + local, clnt string + want residency + }{ + {"local anchor unreadable", false, "", "uid-A", resUnknown}, + {"client anchor empty (legacy record)", true, "uid-A", "", resUnknown}, + {"both unknown", false, "", "", resUnknown}, + {"anchors match", true, "uid-A", "uid-A", resHere}, + {"anchors differ", true, "uid-A", "uid-B", resElsewhere}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := residencyOf(c.hereKnown, c.local, c.clnt); got != c.want { + t.Errorf("residencyOf(%v,%q,%q) = %v, want %v", c.hereKnown, c.local, c.clnt, got, c.want) + } + }) + } +} + +// End to end: a legacy ACTIVE client with no anchor, on a machine whose cluster +// anchor reads fine, must not be accused of running elsewhere — and must not +// trigger the repoint hint, which would be advice to fix a non-problem. +func TestClientList_LegacyClientWithNoAnchorIsNotAccused(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"legacy","namespace":"legacy-ns"}]`)) // no cluster_id + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "NOT on the cluster") { + t.Errorf("a client with no anchor has UNKNOWN residency, not elsewhere:\n%s", got) + } + if strings.Contains(got, "client create") { + t.Errorf("no mismatch is known, so the repoint must not be advised:\n%s", got) + } + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker:\n%s", got) + } +} + +// End-to-end: with the anchor readable, the row that matches the LOCAL cluster +// is marked as such — even when the pointer names a different one — and the +// listing names the repoint. +func TestClientList_MarksTheClientOnThisCluster(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"here","namespace":"lukas-02","cluster_id":"uid-HERE"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" // the pointer names the client that is NOT here + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active — NOT on the cluster your kubeconfig reaches)") { + t.Errorf("the stale active pointer must be marked as not here:\n%s", got) + } + if !strings.Contains(got, "2 (on this cluster)") { + t.Errorf("the client that IS here must be marked:\n%s", got) + } + if !strings.Contains(got, "client create") { + t.Errorf("a mismatch must name the repoint:\n%s", got) + } +} + +// Bugbot (#515): the repoint hint says "the client that IS there", which is only +// true if some row is provably here. With the active client elsewhere and NOTHING +// confirmed on this cluster, `client create` would fall through to the MINT path +// and produce exactly the phantom backend#970 exists to prevent — so the advice +// must not be given. +func TestClientList_MismatchWithNoLocalClient_DoesNotPushCreate(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"},` + + `{"id":2,"first_name":"third","namespace":"third-ns","cluster_id":"uid-THIRD"}]`)) + }) + stubClusterID(t, "uid-HERE", nil) // this cluster hosts NEITHER + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if strings.Contains(got, "client create") { + t.Errorf("no client is confirmed here — advising the repoint would push a MINT:\n%s", got) + } + if !strings.Contains(got, "no client here is confirmed") { + t.Errorf("the mismatch is still real and must be reported, just without a target:\n%s", got) + } + // The mismatch itself must still be visible on the row. + if !strings.Contains(got, "NOT on the cluster your kubeconfig reaches") { + t.Errorf("the stale active pointer must still be marked:\n%s", got) + } +} + +// The unreadable-anchor path end to end: no cluster reachable ⇒ no row claims a +// location, and the mismatch hint stays silent (we cannot know there is one). +func TestClientList_UnreadableAnchorClaimsNoLocation(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, _ *http.Request) { // stubs readClusterID to an error + _, _ = w.Write([]byte(`[{"id":1,"first_name":"stale","namespace":"stale-ns","cluster_id":"uid-OTHER"}]`)) + }) + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + cfg.Current().ActiveClientID = "1" + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := runClientList(context.Background(), ui.New(&out, ui.WithColor(false))); err != nil { + t.Fatal(err) + } + got := out.String() + if !strings.Contains(got, "1 (active)") { + t.Errorf("want the bare selection marker when the anchor is unreadable:\n%s", got) + } + if strings.Contains(got, "this cluster") || strings.Contains(got, "kubeconfig reaches") { + t.Errorf("an unreadable anchor must claim nothing about location:\n%s", got) } } diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index 6d331a9..b75b8e2 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -164,7 +164,10 @@ func runClusterInfo( // installed on this cluster". A binding miss gets the §7.3 // "runs elsewhere" explanation, same as the data commands. if errors.Is(err, cluster.ErrNoParentRelease) { - return binding.explain(&exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}}) + return binding.explain(ctx, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }}) } return &exitError{code: exitNoWorkspace, err: err} } diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index b760867..1ebcb68 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -5,11 +5,13 @@ import ( "errors" "fmt" "strings" + "time" "k8s.io/client-go/kubernetes" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -19,11 +21,36 @@ import ( // release, an API/RBAC list failure, or an ambiguous multiple-release match. // §7.3 uses it to turn an active-client binding miss into a clear "runs on // another machine" message; the other failures keep their own diagnostics. -type noParentReleaseError struct{ err error } +// +// probe carries the read-only handles explain needs to say what IS on the +// reached cluster before advising (#515). It is attached wherever the error is +// built — both sites already hold a clientset and the resolved server URL — and +// travels ON the error rather than through the call signature so a caller can +// never hand explain a clientset for a DIFFERENT cluster than the one that +// missed. A nil probe (a synthesised error, a resolveClusterTargetFn test +// double) means "we could not look", and explain then claims nothing. +type noParentReleaseError struct { + err error + probe *clusterProbe +} func (e *noParentReleaseError) Error() string { return e.err.Error() } func (e *noParentReleaseError) Unwrap() error { return e.err } +// clusterProbe is the pair explain needs to diagnose before advising: a +// clientset for the cluster the kubeconfig actually reached, and that cluster's +// server URL (which isLocalServerURL judges). +type clusterProbe struct { + cs kubernetes.Interface + serverURL string +} + +// explainScanTimeout bounds the naming-only cluster scan explain runs on the +// §7.3 error path. The scan only makes the message better, so it must never +// make the failure slower than the failure itself: past this, explain falls +// back to the message it would have printed without looking. +const explainScanTimeout = 5 * time.Second + // loadClusterFn / newClientsetFn are the kubeconfig-load + clientset-build // seams every command that reaches a cluster goes through — resolveClusterTarget // (data ingest/list/delete), runClusterInfo, and runClusterDoctor. Production @@ -87,7 +114,10 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // "runs elsewhere" rewrite; an API/RBAC list failure or an // ambiguous multiple-release match keeps its own message. if errors.Is(err, cluster.ErrNoParentRelease) { - return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}} + return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{ + err: err, + probe: &clusterProbe{cs: cs, serverURL: resolved.ServerURL}, + }} } return nil, &exitError{code: exitNoWorkspace, err: err} } @@ -214,7 +244,18 @@ func (b activeClientBinding) allowScan() bool { return !b.applied && !b.explicit // came from the active-client binding: the cluster the kubeconfig reaches // doesn't host that client. Non-binding errors (and PVC-missing, where the // release *was* found) pass through unchanged. -func (b activeClientBinding) explain(err error) error { +// +// DIAGNOSE BEFORE ADVISING (#515). The shipped §7.3 sentence named no way back: +// it offered --namespace without ever saying WHICH namespace, so a user on a +// healthy local install had no supported recovery. explain now spends one +// naming-only cluster scan — the same read discoverRelease already spends +// purely to write a better message — and says what is actually here. +// +// This changes what the CLI SAYS, never what it TARGETS: allowScan() stays +// false, so a binding miss still never silently retargets to some other +// machine's client (§7.5). The scan's result reaches the user as text they must +// act on, which is the whole difference. +func (b activeClientBinding) explain(ctx context.Context, err error) error { if !b.applied { return err } @@ -226,8 +267,88 @@ func (b activeClientBinding) explain(err error) error { if handle == "" { handle = b.namespace } - return &exitError{code: exitNoWorkspace, err: fmt.Errorf( - "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; "+ - "run this command there, or override with --namespace/--context", - handle, b.namespace)} + // errors.New, not %w: the rewrite deliberately REPLACES the discovery error + // rather than wrapping it, so the §7.3 guidance is the whole message and the + // raw "no release in namespace X" doesn't trail it. That was already true of + // the fmt.Errorf this replaced — the exit code (exitNoWorkspace, on the + // *exitError above) is the machine-readable contract here, not the chain. + // Wrapping would also make the result re-match errors.As(*noParentReleaseError) + // and so re-explainable, which nothing wants. + return &exitError{code: exitNoWorkspace, + err: errors.New(repointMessage(handle, b.namespace, surveyCluster(ctx, npr.probe)))} +} + +// clientSurvey is what explain managed to learn about the reached cluster +// before advising. +// +// looked distinguishes "we scanned and the cluster hosts none" from "we could +// not scan at all" (no probe on the error, or the cluster-wide list failed — +// RBAC, a timeout, an unreachable API server). Collapsing the two would let an +// absence of evidence print as evidence of absence: the CLI would tell a user +// with a perfectly healthy client that nothing is running here. When looked is +// false the message says nothing about the cluster's contents at all. +type clientSurvey struct { + looked bool + namespaces []string + local bool // the kubeconfig's server is THIS machine (isLocalServerURL) +} + +// surveyCluster runs cluster.FindClientNamespaces FOR NAMING ONLY — nothing in +// this path changes the namespace anything targets. A nil probe (synthesised +// error / test double) or a failed scan both return a survey that looked at +// nothing, so explain falls back to the message it printed before #515. +func surveyCluster(ctx context.Context, probe *clusterProbe) clientSurvey { + if probe == nil || probe.cs == nil { + return clientSurvey{} + } + ctx, cancel := context.WithTimeout(ctx, explainScanTimeout) + defer cancel() + found, err := cluster.FindClientNamespaces(ctx, probe.cs) + if err != nil { + return clientSurvey{} + } + return clientSurvey{looked: true, namespaces: found, local: isLocalServerURL(probe.serverURL)} +} + +// repointMessage is the §7.3 error text, branched on what surveyCluster found. +// Pure (no I/O) so every branch is unit-testable as text. +// +// - exactly one client on a LOCAL cluster — a cluster that IS this machine — +// name it and offer the repoint. `client create` re-run on a cluster that +// already hosts a client adopts it: no prompt, no new credential (§7.2). +// - any client on a remote/shared cluster (or several anywhere) — name the +// namespaces and offer ONLY --namespace. Never `client create` here: that +// is the §7.5 boundary, and on a shared cluster the client we found may well +// be a colleague's. +// - none, scan clean — say so, and point at the installer, which is then the +// correct advice rather than a guess. +// - could not look — the pre-#515 sentence, unchanged. We make no claim. +// +// Each branch is ONE format literal rather than a concatenation, so the whole +// sentence lands in the copy catalog (zz-all-strings harvests literal arguments; +// a `+`-joined message is only ever half-visible there) and can be reviewed as +// the user reads it. +func repointMessage(handle, boundNS string, s clientSurvey) string { + switch { + case !s.looked: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context", + handle, boundNS) + case len(s.namespaces) == 0: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.\n\nNo tracebloc client is running on this cluster either — if this machine should have one, set one up: %s", + handle, boundNS, installer.Cmd) + case len(s.namespaces) == 1 && s.local: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client IS running on this machine, in namespace %q.\n Point this machine at it: %s client create\n (this cluster already runs a client, so it adopts it — no new credential)\n Or target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], launcher(), s.namespaces[0]) + case len(s.namespaces) == 1: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client is running on this cluster, in namespace %q.\n Target it just this once: --namespace %s", + handle, boundNS, s.namespaces[0], s.namespaces[0]) + default: + return fmt.Sprintf( + "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\ntracebloc clients are running on this cluster, in namespaces: %s.\n Target one just this once: --namespace %s", + handle, boundNS, strings.Join(s.namespaces, ", "), s.namespaces[0]) + } } diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index e9e394d..1772496 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -18,6 +18,7 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) @@ -162,12 +163,13 @@ func TestBindActiveClientNamespace_NoActiveClient(t *testing.T) { } func TestActiveClientBinding_Explain(t *testing.T) { - noRelease := &exitError{code: 4, err: &noParentReleaseError{errors.New("no release")}} + noRelease := &exitError{code: 4, err: &noParentReleaseError{err: errors.New("no release")}} pvcMissing := &exitError{code: 4, err: errors.New("shared PVC not bound")} + ctx := context.Background() // Applied + "no release here" → rewritten to the §7.3 guidance. bound := activeClientBinding{applied: true, name: "gpu-box-01", namespace: "gpu-box-01"} - got := bound.explain(noRelease) + got := bound.explain(ctx, noRelease) if got == noRelease { t.Fatal("expected a rewritten error") } @@ -180,16 +182,181 @@ func TestActiveClientBinding_Explain(t *testing.T) { } // Applied but a PVC failure (release WAS found) → pass through untouched. - if bound.explain(pvcMissing) != pvcMissing { + if bound.explain(ctx, pvcMissing) != pvcMissing { t.Error("PVC-missing error should not be rewritten") } // Not applied → always pass through. - if (activeClientBinding{}).explain(noRelease) != noRelease { + if (activeClientBinding{}).explain(ctx, noRelease) != noRelease { t.Error("unbound explain should pass the error through") } } +// #515 — the three branches of the §7.3 message, as text. A binding miss used to +// name --namespace without ever saying WHICH namespace; each branch below is the +// answer explain now derives from what is actually on the reached cluster. +// +// repointMessage is pure, so this pins the exact wording; the surveyCluster +// tests below pin that the survey fed to it is honest. +func TestRepointMessage_Branches(t *testing.T) { + const handle, boundNS = "gpu-box-01", "gpu-box-01" + lead := `active client "gpu-box-01" runs on another machine — namespace "gpu-box-01" isn't on the cluster your kubeconfig points at` + + t.Run("one client on a local cluster offers the repoint", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"lukas-02"}, local: true}) + for _, want := range []string{ + lead, + `A tracebloc client IS running on this machine, in namespace "lukas-02".`, + "client create", + "no new credential", + "--namespace lukas-02", + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("remote cluster never suggests client create", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"colleague-07"}, local: false}) + if !strings.Contains(got, "colleague-07") || !strings.Contains(got, "--namespace colleague-07") { + t.Errorf("remote branch must name the namespace and offer --namespace:\n%s", got) + } + // The §7.5 boundary: on a shared cluster the client we found may be + // someone else's, so the repoint must NOT be advertised. + if strings.Contains(got, "client create") { + t.Errorf("remote/shared cluster must never suggest `client create`:\n%s", got) + } + }) + + t.Run("several clients name them all and offer only --namespace", func(t *testing.T) { + // Local or not: with more than one client here, "point this machine at + // it" has no unambiguous "it" — so this stays the --namespace branch even + // on a local cluster. + for _, local := range []bool{true, false} { + got := repointMessage(handle, boundNS, clientSurvey{looked: true, namespaces: []string{"alpha", "beta"}, local: local}) + if !strings.Contains(got, "alpha, beta") || !strings.Contains(got, "--namespace alpha") { + t.Errorf("local=%v: multi branch should list both and offer --namespace:\n%s", local, got) + } + if strings.Contains(got, "client create") { + t.Errorf("local=%v: ambiguous multi-client must not suggest `client create`:\n%s", local, got) + } + } + }) + + t.Run("clean scan finding nothing points at the installer", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{looked: true}) + for _, want := range []string{ + lead, + "--namespace/--context", + "No tracebloc client is running on this cluster either", + installer.Cmd, + } { + if !strings.Contains(got, want) { + t.Errorf("missing %q:\n%s", want, got) + } + } + }) + + t.Run("could not look claims nothing", func(t *testing.T) { + got := repointMessage(handle, boundNS, clientSurvey{}) + want := lead + "; run this command there, or override with --namespace/--context" + if got != want { + t.Errorf("unlooked message must stay the pre-#515 sentence exactly\n got: %q\nwant: %q", got, want) + } + // An absence of evidence must never print as evidence of absence. + if strings.Contains(got, "No tracebloc client is running") { + t.Errorf("a failed/absent scan must not claim the cluster is empty:\n%s", got) + } + }) +} + +// surveyCluster is the only thing standing between the message and a false +// claim, so each way of "we could not look" has to come back as looked=false. +func TestSurveyCluster_FailsClosed(t *testing.T) { + t.Run("nil probe", func(t *testing.T) { + if s := surveyCluster(context.Background(), nil); s.looked { + t.Errorf("a nil probe must not report as looked: %+v", s) + } + }) + + t.Run("nil clientset", func(t *testing.T) { + if s := surveyCluster(context.Background(), &clusterProbe{serverURL: "https://127.0.0.1:6550"}); s.looked { + t.Errorf("a probe with no clientset must not report as looked: %+v", s) + } + }) + + t.Run("scan forbidden", func(t *testing.T) { + cs := fake.NewSimpleClientset() + cs.PrependReactor("list", "deployments", func(ktesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("forbidden: cannot list deployments at the cluster scope") + }) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://127.0.0.1:6550"}) + if s.looked { + t.Errorf("an RBAC-refused scan must not report as looked: %+v", s) + } + }) + + t.Run("clean empty scan looked and found nothing", func(t *testing.T) { + s := surveyCluster(context.Background(), &clusterProbe{cs: fake.NewSimpleClientset(), serverURL: "https://127.0.0.1:6550"}) + if !s.looked || len(s.namespaces) != 0 { + t.Errorf("a clean empty scan is looked-with-nothing: %+v", s) + } + if !s.local { + t.Error("a loopback server URL must survey as local") + } + }) + + t.Run("finds the client and judges locality", func(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + s := surveyCluster(context.Background(), &clusterProbe{cs: cs, serverURL: "https://k8s.corp.example:6443"}) + if !s.looked || len(s.namespaces) != 1 || s.namespaces[0] != "lukas-02" { + t.Errorf("survey = %+v, want the one namespace", s) + } + if s.local { + t.Error("a corporate API server must not survey as local") + } + }) +} + +// End-to-end through the real resolve path: a binding miss on a LOCAL cluster +// that hosts the client elsewhere must NAME it — and must still not target it. +// This is the pairing that matters (§7.5): the namespace appears in the message +// and nowhere else. +func TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting(t *testing.T) { + cs := fake.NewSimpleClientset(jmDep("lukas-02")) + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return &cluster.ResolvedConfig{ + Namespace: o.Namespace, + ServerURL: "https://127.0.0.1:6550", + RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + 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) + if err == nil { + t.Fatal("a binding miss must still fail — this changes the message, not the target") + } + if target != nil { + t.Fatalf("no target may be resolved from a binding miss, got %+v", target) + } + got := binding.explain(context.Background(), err) + if !strings.Contains(got.Error(), "lukas-02") { + t.Errorf("the message must name the client that IS here:\n%s", got) + } + if !strings.Contains(got.Error(), "client create") { + t.Errorf("a single client on a local cluster must be offered the repoint:\n%s", got) + } + if ExitCodeFromError(got) != 4 { + t.Errorf("exit code = %d, want 4", ExitCodeFromError(got)) + } +} + // jmDep builds a chart-labeled jobs-manager Deployment in the given namespace, // for the fallback-scan tests (mirrors the cluster package's fixture). func jmDep(namespace string) *appsv1.Deployment { diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index e90c9fc..98c84d2 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -175,7 +175,7 @@ undone — re-ingesting the data is the only way back.`) // mid-output blank between the warning and the note (§380). target, err := resolveClusterTargetFn(ctx, a.Printer, opts, binding, true, false) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index ffd17b4..a750d80 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -58,7 +58,7 @@ func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clu // mid-output blank between "Connecting…" and the note (§380). target, err = resolveClusterTarget(ctx, a.Printer, opts, binding, true, false) if err != nil { - return nil, "", false, binding.explain(err) + return nil, "", false, binding.explain(ctx, err) } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by diff --git a/internal/cli/data_list.go b/internal/cli/data_list.go index 0196273..f89cbab 100644 --- a/internal/cli/data_list.go +++ b/internal/cli/data_list.go @@ -124,7 +124,7 @@ func runDataList(ctx context.Context, a runDataListArgs) (err error) { // multi-client redirect note is the opening line and self-leads its blank. target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } resolved, cs, release := target.Resolved, target.Clientset, target.Release diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 3e22d3e..cd3341a 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -156,7 +156,7 @@ func runClusterDoctor( // problem goes unexplained. (A 401/426 is a hard stop earlier; only the // soft tokenUnreachable/tokenServerErr states reach here.) opts := cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride, Namespace: nsOverride} - bindActiveClientNamespace(&opts) + binding := bindActiveClientNamespace(&opts) resolved, err = loadClusterFn(opts) if err != nil { p.Newline() @@ -176,6 +176,39 @@ func runClusterDoctor( // 4. Probe the cluster. results = doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace, ServerURL: resolved.ServerURL}) + // #515: a namespace we CHOSE for the user can be wrong, and a miss on it is + // not evidence that this machine has no environment — yet doctor's only + // reading of "no chart here" is "no secure environment on this machine yet", + // which then recommends reinstalling over a healthy install. Extend #401's + // local fallback to the wrong-pointer case: re-probe the namespace the + // KUBECONFIG selects, but only when the binding (not the user) picked the + // namespace that missed, and only on a LOCAL cluster — on a remote/shared one + // the ownership gate stands and we would risk naming a colleague's client. + // The retry is adopted only if it actually finds an environment, so a genuine + // no-environment machine keeps the original results (and the --diagnose + // bundle keeps describing the namespace the user is configured for). + // pointerStale records that the re-probe SUCCEEDED — i.e. this machine has a + // healthy environment AND its active-client pointer is wrong. Both halves + // have to be said; see the verdict block below for why finding the + // environment is not on its own good news. + pointerStale := false + if binding.applied && reachStateOf(results) == doctor.ReachNoEnv { + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{Path: kubeconfigPath, Context: contextOverride}); ok && ns != resolved.Namespace { + // Adopt the re-probe ONLY on a positive confirmation that a client is + // there. `!= ReachNoEnv` was wrong (Bugbot): ReachUnreachable and + // ReachError also satisfy it, and both mean "we could not tell" — so a + // stale pointer plus RBAC or a transient read on the context namespace + // would have named an unconfirmed namespace as a secure environment and + // told the user this cluster already runs a client, pushing `client + // create` into a MINT. Same absence-as-presence collapse surveyCluster + // and residencyOf exist to avoid; an unconfirmed re-probe keeps the + // original results and the honest no-environment path. + if retry := doctorRunFn(ctx, cs, doctor.Options{Namespace: ns, ServerURL: resolved.ServerURL}); reachConfirmedOK(retry) { + resolved.Namespace, results, pointerStale = ns, retry, true + } + } + } + // A reachable cluster with no tracebloc chart installed is the same "no secure // environment here" state as a missing kubeconfig — route it through the same // message (which also surfaces any session fault) rather than naming an @@ -194,6 +227,23 @@ func runClusterDoctor( // "Signed in" above, so the two context lines read as a pair), then roll up. p.Para(fmt.Sprintf("Secure environment %q", envDisplayName(resolved))) connected, ready = summarizeDoctor(results, tok) + if pointerStale { + // The environment above is healthy — but `data ingest`, `data list`, + // `resources` and `seal` all bind the active-client POINTER, and that + // still misses, so they keep failing with exit 4. "Ready to run training" + // is therefore false no matter how green the cluster checks are. Replace + // the readiness line rather than printing a green tick with a warning + // beside it that contradicts it — and the replacement carries the remedy, + // so the finding and the fix read as one thing. The support bundle + // records this line too, which is what triage needs to see. + ready = healthLine{ + status: doctor.StatusFail, + text: fmt.Sprintf("Not ready — your active client points at namespace %q, which isn't on this cluster, so data commands will keep failing until you repoint.", + binding.namespace), + remedy: fmt.Sprintf("Point this machine at the environment above: %s client create (this cluster already runs a client, so it adopts it — no new credential)", + launcher()), + } + } p.Newline() renderHealth(p, connected) @@ -206,6 +256,14 @@ func runClusterDoctor( p.Newline() fail, allGood := doctorVerdict(connected.status, ready.status) switch { + case pointerStale: + // A problem WAS found — it just isn't in the cluster. Exit 2 (the code + // doctor already uses for every actionable finding), never 0 with + // "you're ready to run training": the very next `data ingest` exits 4, + // and a doctor that greens that is reporting success it hasn't earned + // — the class BUGBOT.md flags first. The remedy is already printed + // above, so this doesn't also send them to write a support bundle. + return &exitError{code: exitChecksFailed, err: nil} case fail: if !diagnose { // they just wrote a bundle — don't send them to write it again p.Hintf("Still stuck? Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher()) @@ -531,6 +589,26 @@ func reachStateOf(results []doctor.Result) doctor.ReachState { return doctor.ReachOK } +// reachConfirmedOK reports whether the "Cluster reachable" check RAN and came +// back ReachOK — a positive confirmation that a tracebloc client is in the probed +// namespace. +// +// Deliberately not `reachStateOf(results) == ReachOK`: reachStateOf defaults to +// ReachOK when the check is ABSENT, which is the right lenient default for the +// main path (an older probe set shouldn't block a verdict) and precisely the +// wrong one for #515's re-probe, where the whole question is whether we may +// believe an unproven namespace. Absent, unreachable and errored all answer +// "we could not tell", and none of them may authorize naming a secure +// environment or advising `client create`. +func reachConfirmedOK(results []doctor.Result) bool { + for _, r := range results { + if r.Name == "Cluster reachable" { + return r.Reach == doctor.ReachOK + } + } + return false +} + // worseStatus returns the more severe of two doctor statuses (Fail > Warn > OK). // StatusUnknown carries no signal, so it never worsens the verdict. func worseStatus(a, b doctor.Status) doctor.Status { diff --git a/internal/cli/doctor_pointer_test.go b/internal/cli/doctor_pointer_test.go new file mode 100644 index 0000000..d7a03b3 --- /dev/null +++ b/internal/cli/doctor_pointer_test.go @@ -0,0 +1,331 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "net/http" + "strings" + "testing" + + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/doctor" + "github.com/tracebloc/cli/internal/ui" +) + +// #515 — a WRONG active-client pointer must not read as "no environment". +// Split from doctor_test.go, which is already at its file budget. + +// stubDoctorForNamespace makes doctor's cluster I/O deterministic: the kubeconfig +// resolves to a caller-chosen server URL with the namespace opts asked for (or +// kubeconfigNS when nothing was pinned — exactly how cluster.Load layers an +// explicit namespace over the context's own), and the probe reports a healthy +// environment in envNS and ReachNoEnv anywhere else. It returns the list of +// namespaces the probe ran against, so a test can assert what was and wasn't +// re-probed rather than inferring it from the rendered text. +func stubDoctorForNamespace(t *testing.T, serverURL, kubeconfigNS, envNS string) *[]string { + t.Helper() + origLoad, origCS, origRun := loadClusterFn, newClientsetFn, doctorRunFn + t.Cleanup(func() { loadClusterFn, newClientsetFn, doctorRunFn = origLoad, origCS, origRun }) + + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = kubeconfigNS + } + return &cluster.ResolvedConfig{ + Namespace: ns, Context: "test-ctx", ServerURL: serverURL, RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + return fake.NewSimpleClientset(), nil + } + var probed []string + doctorRunFn = func(_ context.Context, _ kubernetes.Interface, o doctor.Options) []doctor.Result { + probed = append(probed, o.Namespace) + if o.Namespace != envNS { + return []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}} + } + return []doctor.Result{ + {Name: "Cluster reachable", Status: doctor.StatusOK, Reach: doctor.ReachOK}, + {Name: "Pod health", Status: doctor.StatusOK}, + } + } + return &probed +} + +// okWhoAmI stubs the session probe so these tests reach the cluster stage. +func okWhoAmI(t *testing.T) { + t.Helper() + stubBackend(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"email":"a@b.io","account":"Acme"}`)) + }) +} + +// The field symptom: a healthy local k3d install whose active-client pointer +// names a namespace that isn't on this cluster. doctor bound the wrong pointer, +// probed only it, and told the user to reinstall over a working environment +// (#401 fixed only the EMPTY-pointer case). +func TestDoctor_WrongPointerOnLocalCluster_FindsTheEnvironment(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + if strings.Contains(out.String(), "No secure environment") { + t.Errorf("must not recommend a reinstall over a healthy install:\n%s", out.String()) + } + if !strings.Contains(out.String(), "lukas-02") { + t.Errorf("doctor should name the environment that IS here:\n%s", out.String()) + } + if len(*probed) != 2 || (*probed)[0] != "stale-ns" || (*probed)[1] != "lukas-02" { + t.Errorf("probe namespaces = %v, want [stale-ns lukas-02] (bound pointer first, then the kubeconfig's own)", *probed) + } + + // Bugbot (#515): finding the environment is only HALF the story. The pointer + // is still stale, so `data ingest`/`resources`/`seal` keep exiting 4 — doctor + // must say so and must not green the machine. + if strings.Contains(out.String(), "Everything looks good") { + t.Errorf("a stale pointer means data commands still fail — this is not 'ready to run training':\n%s", out.String()) + } + // …and the readiness LINE must carry it too: a green "✔ Ready to run + // training" beside a warning that contradicts it is the same unearned + // success, just moved up the screen. + if strings.Contains(out.String(), "✔ Ready to run training") { + t.Errorf("the readiness line must not tick green while the pointer is stale:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Not ready") { + t.Errorf("want the readiness line to carry the finding:\n%s", out.String()) + } + if !strings.Contains(out.String(), "stale-ns") { + t.Errorf("doctor must name the stale pointer, not just the environment it found:\n%s", out.String()) + } + if !strings.Contains(out.String(), "client create") { + t.Errorf("doctor must name the repoint:\n%s", out.String()) + } + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("a stale pointer is a problem doctor found → want exit 2, got %v", err) + } +} + +// The other side of the same coin: with NO stale pointer, a healthy machine +// still gets its green line and exit 0. Without this, the assertion above could +// be satisfied by doctor never saying "Everything looks good" at all. +func TestDoctor_HealthyPointer_StillGreen(t *testing.T) { + writeActiveClientConfig(t, "lukas-02", "Lukas") // pointer matches reality + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + if err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false); err != nil { + t.Fatalf("a healthy machine with a correct pointer must exit 0, got %v", err) + } + if !strings.Contains(out.String(), "Everything looks good") { + t.Errorf("want the green verdict when nothing is stale:\n%s", out.String()) + } + if !strings.Contains(out.String(), "✔ Ready to run training") { + t.Errorf("control: the readiness line must still tick green when nothing is stale:\n%s", out.String()) + } + if strings.Contains(out.String(), "client create") { + t.Errorf("no repoint advice when the pointer is correct:\n%s", out.String()) + } + if len(*probed) != 1 { + t.Errorf("probe namespaces = %v, want one (nothing to re-probe)", *probed) + } +} + +// The ownership gate is what keeps the fallback honest, so it gets its own test: +// on a REMOTE/shared cluster a pointer miss stays "no secure environment" and the +// re-probe never runs — the client sitting in another namespace there may well be +// a colleague's (§7.5). +func TestDoctor_WrongPointerOnRemoteCluster_StaysGated(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://k8s.corp.example:6443", "colleague-07", "colleague-07") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + if err == nil { + t.Fatal("a pointer miss on a remote cluster is still a problem — want a non-zero exit") + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("remote cluster must keep the honest no-environment message:\n%s", out.String()) + } + if strings.Contains(out.String(), "colleague-07") { + t.Errorf("a remote cluster's other namespace must never be named as yours:\n%s", out.String()) + } + if len(*probed) != 1 { + t.Errorf("probe namespaces = %v, want exactly one (no re-probe off a remote cluster)", *probed) + } +} + +// A user who pinned --namespace themselves is never second-guessed: no binding +// was applied, so nothing re-probes and the miss stands as they asked for it. +func TestDoctor_ExplicitNamespaceMiss_IsNotReprobed(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "lukas-02", "lukas-02") + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "chosen-ns", false) + if err == nil { + t.Fatal("an explicit --namespace miss must still fail") + } + if len(*probed) != 1 || (*probed)[0] != "chosen-ns" { + t.Errorf("probe namespaces = %v, want only the namespace the user pinned", *probed) + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the no-environment message for an explicitly-pinned miss:\n%s", out.String()) + } +} + +// A machine that genuinely has nothing must keep the installer advice: the +// re-probe runs, finds no environment either, and the original results stand. +func TestDoctor_LocalClusterWithNothing_KeepsInstallerAdvice(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + probed := stubDoctorForNamespace(t, "https://127.0.0.1:6550", "default", "nowhere") + + var out bytes.Buffer + if err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false); err == nil { + t.Fatal("a machine with no environment must still exit non-zero") + } + if !strings.Contains(out.String(), "No secure environment") { + t.Errorf("want the no-environment message when the re-probe finds nothing too:\n%s", out.String()) + } + if len(*probed) != 2 { + t.Errorf("probe namespaces = %v, want the bound namespace and the kubeconfig's own", *probed) + } +} + +// Bugbot (#515): the re-probe used to adopt on anything that wasn't ReachNoEnv, +// which swept in ReachUnreachable and ReachError — both of which mean "we could +// not tell". Adopting either would name an unconfirmed namespace as a secure +// environment and tell the user this cluster already runs a client, pushing +// `client create` into a MINT on a cluster that may host nothing. +// +// The input domain is derived from doctor.ReachState's declared surface rather +// than hand-picked, plus the ABSENT case (reachStateOf's lenient default is +// exactly what must not apply here). Only ReachOK may adopt. +func TestDoctor_ReProbeAdoptsOnlyOnConfirmedReach(t *testing.T) { + // Every non-OK member of the enum, and the missing-check case. + cases := []struct { + name string + results []doctor.Result + }{ + {"unreachable", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachUnreachable}}}, + {"error (RBAC/NotFound)", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachError}}}, + {"no env", []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}}}, + {"check absent entirely", []doctor.Result{{Name: "Pod health", Status: doctor.StatusOK}}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + okWhoAmI(t) + + origLoad, origCS, origRun := loadClusterFn, newClientsetFn, doctorRunFn + t.Cleanup(func() { loadClusterFn, newClientsetFn, doctorRunFn = origLoad, origCS, origRun }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "unproven-ns" + } + return &cluster.ResolvedConfig{Namespace: ns, ServerURL: "https://127.0.0.1:6550", RestConfig: &rest.Config{}}, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { + return fake.NewSimpleClientset(), nil + } + doctorRunFn = func(_ context.Context, _ kubernetes.Interface, o doctor.Options) []doctor.Result { + if o.Namespace == "stale-ns" { // the bound pointer always misses + return []doctor.Result{{Name: "Cluster reachable", Status: doctor.StatusFail, Reach: doctor.ReachNoEnv}} + } + return c.results // the re-probe's inconclusive answer + } + + var out bytes.Buffer + err := runClusterDoctor(context.Background(), ui.New(&out, ui.WithColor(false)), "", "", "", false) + got := out.String() + if strings.Contains(got, "unproven-ns") { + t.Errorf("an unconfirmed namespace must never be named as a secure environment:\n%s", got) + } + if strings.Contains(got, "client create") { + t.Errorf("advising the repoint here can push a MINT — the cluster was never confirmed to run a client:\n%s", got) + } + if !strings.Contains(got, "No secure environment") { + t.Errorf("an unconfirmed re-probe must keep the honest no-environment path:\n%s", got) + } + if err == nil { + t.Error("want a non-zero exit when nothing was confirmed") + } + }) + } +} + +// reachConfirmedOK is the guard above, tested directly against the whole +// declared enum so a future ReachState member can't quietly slip through the +// "could not tell" side. Mutation coverage cannot see a vocabulary gap. +func TestReachConfirmedOK(t *testing.T) { + res := func(r doctor.ReachState) []doctor.Result { + return []doctor.Result{{Name: "Cluster reachable", Reach: r}} + } + if !reachConfirmedOK(res(doctor.ReachOK)) { + t.Error("ReachOK is the one positive confirmation") + } + for _, r := range []doctor.ReachState{doctor.ReachUnreachable, doctor.ReachNoEnv, doctor.ReachError} { + if reachConfirmedOK(res(r)) { + t.Errorf("Reach %v must not count as confirmed", r) + } + } + if reachConfirmedOK([]doctor.Result{{Name: "Pod health"}}) { + t.Error("an ABSENT reachability check is 'could not tell', not OK — reachStateOf's lenient default must not leak in here") + } + if reachConfirmedOK(nil) { + t.Error("no results at all is not a confirmation") + } +} + +// localEnvNamespace is the doctor-side half of the #401 carve-out; its three +// refusals are what stop the re-probe from ever naming someone else's client. +func TestLocalEnvNamespace(t *testing.T) { + set := func(rc *cluster.ResolvedConfig, err error) { + t.Helper() + orig := loadClusterFn + t.Cleanup(func() { loadClusterFn = orig }) + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { return rc, err } + } + + t.Run("local server with a namespace", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "lukas-02", ServerURL: "https://127.0.0.1:6550"}, nil) + ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}) + if !ok || ns != "lukas-02" { + t.Errorf("= %q,%v; want lukas-02,true", ns, ok) + } + }) + + t.Run("remote server is refused", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "colleague-07", ServerURL: "https://k8s.corp.example:6443"}, nil) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; a remote cluster must be refused", ns, ok) + } + }) + + t.Run("empty namespace is refused", func(t *testing.T) { + set(&cluster.ResolvedConfig{Namespace: "", ServerURL: "https://127.0.0.1:6550"}, nil) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; an empty namespace is not a reading", ns, ok) + } + }) + + t.Run("load failure is refused", func(t *testing.T) { + set(nil, context.DeadlineExceeded) + if ns, ok := localEnvNamespace(cluster.KubeconfigOptions{}); ok { + t.Errorf("= %q,%v; an unreadable kubeconfig is not a reading", ns, ok) + } + }) +} diff --git a/internal/cli/home.go b/internal/cli/home.go index c438ff8..7855db3 100644 --- a/internal/cli/home.go +++ b/internal/cli/home.go @@ -30,7 +30,6 @@ package cli import ( "context" - "errors" "fmt" "math" "os" @@ -155,6 +154,13 @@ type envProbe struct { name string compute computeInfo hasCompute bool + // pointerStale: the environment above was found by the #515 local fallback + // AFTER the active-client pointer missed — so the release we can see running + // here is NOT the client the pointer names. The heartbeat is looked up by + // that pointer's client id, so it describes a different machine's client and + // must never be allowed to green this one (Bugbot: local liveness from one + // client + a heartbeat from another is an Online nobody earned). + pointerStale bool } // homeDeps are the detection seams. defaultHomeDeps wires the real @@ -270,7 +276,14 @@ func resolveHomeModel(ctx context.Context, d homeDeps) homeModel { // a release present on a machine that never cached a client. This also keeps // the "provisioned ⇒ named offline" fallback (a degraded probe returns no // name) living in exactly one place. - if remembered != "" { + // + // EXCEPT when the pointer is stale (#515): the remembered name is the handle + // of the client the pointer names, and that client is NOT what the fallback + // found running here. Applying it would print another machine's handle as the + // environment on this one — a wrong label, and one that contradicts doctor's + // namespace-based name for the very same state (Bugbot). Fall through to the + // probe's own name, which at least describes what is actually running. + if remembered != "" && !env.pointerStale { env.name = remembered } @@ -291,11 +304,16 @@ func resolveHomeModel(ctx context.Context, d homeDeps) homeModel { // honest "· running" state, never a green Online — but the model records // WHICH kind of not-Online it is, so the running line can word a // backend-confirmed "not online" differently from a mere couldn't-confirm. - if beat == beatOnline { + // #515: with a stale pointer the heartbeat is about a DIFFERENT client + // than the release running here, so it carries no signal about this one + // in either direction — it can neither green it (a colleague's machine + // being online is not this one being online) nor red it. Drop to the + // honest "running, couldn't confirm" line, which is exactly true. + if beat == beatOnline && !env.pointerStale { m.state = homeOnline } else { m.state = homeRunning - m.confirmedNotOnline = beat == beatNotOnline + m.confirmedNotOnline = beat == beatNotOnline && !env.pointerStale } m.fullMenu = true case localDegraded: @@ -463,78 +481,6 @@ func realRememberedClient() (provisioned bool, name string) { return p.ActiveClientNamespace != "", name } -// realProbeEnv is the bounded cluster probe. It reuses the exact namespace -// binding + discovery the data/cluster commands use, so the home screen reports -// the very environment those commands would target. Best-effort throughout: any -// failure degrades to unreachable/no-release, never an error. -func realProbeEnv(ctx context.Context) envProbe { - ctx, cancel := context.WithTimeout(ctx, homeProbeTimeout) - defer cancel() - - // The name for a discovered release is set below; the unreachable / no-release - // returns leave it empty and let resolveHomeModel fill the remembered name, so - // the "provisioned ⇒ named offline" fallback lives in exactly one place. - opts := cluster.KubeconfigOptions{} - binding := bindActiveClientNamespace(&opts) - // OWNERSHIP GATE: no active-client binding ⇒ nothing was ever provisioned - // for this profile, so no release the kubeconfig can reach is honestly - // YOURS. Without the binding, discovery would fall back to the kubeconfig's - // default namespace and then the cluster-wide scan — either can surface an - // UNRELATED client (a shared cluster, a colleague's install), which this - // screen would then greet as "your secure environment". The data commands - // run that scan behind a visible retarget note and an explicit user action; - // a status screen has neither, and §7.5's rule (a miss must never silently - // retarget to some other client) applies doubly here. Report no-release — - // resolveHomeModel renders the honest no-env screen (or a named offline via - // the remembered-name fallback) — and skip the cluster I/O entirely, which - // also keeps the common unprovisioned re-entry instant. - if !binding.applied { - // #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 { - return envProbe{local: localUnreachable} - } - // Bound every API call so an unreachable API server can't hang the home - // screen (mirrors cluster.ClusterID's time-boxed best-effort read). - resolved.RestConfig.Timeout = homeProbeTimeout - cs, err := newClientsetFn(resolved) - if err != nil { - return envProbe{local: localUnreachable} - } - - release, nsUsed, err := discoverRelease(ctx, nil, cs, resolved.Namespace, binding.allowScan(), false) - if err != nil { - if errors.Is(err, cluster.ErrNoParentRelease) { - // Cluster reachable, but this release isn't in the resolved context. - // Provisioned ⇒ resolveHomeModel turns this into a named "offline". - return envProbe{local: localNoRelease} - } - // A list/RBAC/connect failure: we couldn't confirm what's here. Treat it - // as unreachable (→ offline if provisioned, else no-env). - return envProbe{local: localUnreachable} - } - - ep := envProbe{name: release.ReleaseName} - if jobsManagerReady(ctx, cs, nsUsed, release) { - ep.local = localLive - } else { - ep.local = localDegraded - } - // Compute is only surfaced on the Online line, and only worth reading when the - // environment is actually up. - if ep.local == localLive { - if c, ok := machineCapacity(ctx, cs); ok { - ep.compute, ep.hasCompute = c, true - } - } - return ep -} - // realHeartbeat reports tracebloc's view of this machine's client — the honest // "is it heartbeating" signal. // diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go index 8900154..abbd580 100644 --- a/internal/cli/home_local_fallback.go +++ b/internal/cli/home_local_fallback.go @@ -1,10 +1,15 @@ package cli -// Home-screen fallbacks for machines the provisioning pointer never reached -// (#401). Split from home.go to respect its file budget. +// The home screen's environment probe, and the fallbacks it leans on when the +// provisioning pointer can't be trusted — never reached this machine at all +// (#401) or names a namespace that isn't on the reached cluster (#515). Split +// from home.go to respect its file budget; realProbeEnv moved here under #515 +// because it is now mostly a decision about WHICH fallback to take, and reads +// better next to them than next to the renderer. import ( "context" + "errors" "net" "net/url" "os" @@ -15,10 +20,23 @@ import ( ) // 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". +// when the active-client pointer cannot be trusted: +// +// - it is EMPTY (#401) — 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". +// - it is SET BUT WRONG (#515) — it names a namespace that isn't on the +// cluster this kubeconfig reaches (an orphaned record left by a cluster +// recreation, a pointer written on another machine). #401 covered only the +// empty case, so a wrong pointer went on recommending a reinstall over a +// healthy install. +// +// Both are the same question, and the answer must not come from the pointer: +// this reloads the kubeconfig with NO binding applied, so it probes the +// namespace the kubeconfig itself selects — which is the client's own namespace +// on any installer-provisioned machine (install-client-helm.sh runs +// `kubectl config set-context --current --namespace `). // // 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 @@ -62,6 +80,31 @@ func localEnvFallback(ctx context.Context) envProbe { return ep } +// localEnvNamespace reports the namespace the KUBECONFIG itself selects, and +// whether that reading is usable — i.e. the kubeconfig loads and the cluster it +// reaches is LOCAL (the #401 carve-out: a cluster that is this machine by +// definition, so whatever tracebloc release runs there is this machine's). +// +// It is the doctor-side half of localEnvFallback (#515), for the one caller that +// already holds a clientset and only needs the namespace to re-probe. Like the +// fallback it applies NO active-client binding — that pointer is precisely the +// thing under suspicion — and it never scans: the installer points the +// kubeconfig context at the client's namespace +// (install-client-helm.sh: `kubectl config set-context --current --namespace`), +// so reading it is enough and no cluster-wide list is spent. On a remote or +// shared cluster it returns false, so the ownership gate holds exactly as it +// does on the home screen. +func localEnvNamespace(opts cluster.KubeconfigOptions) (string, bool) { + resolved, err := loadClusterFn(opts) + if err != nil || resolved == nil { + return "", false + } + if !isLocalServerURL(resolved.ServerURL) || resolved.Namespace == "" { + return "", false + } + return resolved.Namespace, true +} + // 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 @@ -116,3 +159,88 @@ func tbCmdAliasOurs(dir, exe string) bool { } return strings.Contains(strings.ToLower(string(b)), strings.ToLower(filepath.Clean(exe))) } + +// realProbeEnv is the bounded cluster probe. It reuses the exact namespace +// binding + discovery the data/cluster commands use, so the home screen reports +// the very environment those commands would target. Best-effort throughout: any +// failure degrades to unreachable/no-release, never an error. +func realProbeEnv(ctx context.Context) envProbe { + ctx, cancel := context.WithTimeout(ctx, homeProbeTimeout) + defer cancel() + + // The name for a discovered release is set below; the unreachable / no-release + // returns leave it empty and let resolveHomeModel fill the remembered name, so + // the "provisioned ⇒ named offline" fallback lives in exactly one place. + opts := cluster.KubeconfigOptions{} + binding := bindActiveClientNamespace(&opts) + // OWNERSHIP GATE: no active-client binding ⇒ nothing was ever provisioned + // for this profile, so no release the kubeconfig can reach is honestly + // YOURS. Without the binding, discovery would fall back to the kubeconfig's + // default namespace and then the cluster-wide scan — either can surface an + // UNRELATED client (a shared cluster, a colleague's install), which this + // screen would then greet as "your secure environment". The data commands + // run that scan behind a visible retarget note and an explicit user action; + // a status screen has neither, and §7.5's rule (a miss must never silently + // retarget to some other client) applies doubly here. Report no-release — + // resolveHomeModel renders the honest no-env screen (or a named offline via + // the remembered-name fallback) — and skip the cluster I/O entirely, which + // also keeps the common unprovisioned re-entry instant. + if !binding.applied { + // #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 { + return envProbe{local: localUnreachable} + } + // Bound every API call so an unreachable API server can't hang the home + // screen (mirrors cluster.ClusterID's time-boxed best-effort read). + resolved.RestConfig.Timeout = homeProbeTimeout + cs, err := newClientsetFn(resolved) + if err != nil { + return envProbe{local: localUnreachable} + } + + release, nsUsed, err := discoverRelease(ctx, nil, cs, resolved.Namespace, binding.allowScan(), false) + if err != nil { + if errors.Is(err, cluster.ErrNoParentRelease) { + // Cluster reachable, but this release isn't in the resolved context. + // #515: a WRONG pointer is no more proof of "no environment" than the + // empty one #401 covered — the binding above overrode the kubeconfig's + // own namespace with a stale/foreign one, so this miss says nothing + // about what runs here. Re-ask through the same local-only fallback: + // it adopts a release ONLY when the kubeconfig's server is this + // machine, so the shared-cluster guarantee is untouched, and every + // other outcome is localNoRelease — exactly what this branch returned + // before. Provisioned ⇒ resolveHomeModel turns that into a named + // "offline". + ep := localEnvFallback(ctx) + // Mark it: the release the fallback found is not the client the + // pointer names, so the heartbeat keyed on that pointer describes + // someone else. resolveHomeModel refuses to render Online off this. + ep.pointerStale = ep.local != localNoRelease + return ep + } + // A list/RBAC/connect failure: we couldn't confirm what's here. Treat it + // as unreachable (→ offline if provisioned, else no-env). + return envProbe{local: localUnreachable} + } + + ep := envProbe{name: release.ReleaseName} + if jobsManagerReady(ctx, cs, nsUsed, release) { + ep.local = localLive + } else { + ep.local = localDegraded + } + // Compute is only surfaced on the Online line, and only worth reading when the + // environment is actually up. + if ep.local == localLive { + if c, ok := machineCapacity(ctx, cs); ok { + ep.compute, ep.hasCompute = c, true + } + } + return ep +} diff --git a/internal/cli/home_local_fallback_test.go b/internal/cli/home_local_fallback_test.go index b8e3894..a194067 100644 --- a/internal/cli/home_local_fallback_test.go +++ b/internal/cli/home_local_fallback_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "testing" + "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -91,6 +92,140 @@ func TestLocalEnvFallback_NoKubeconfigIsNoRelease(t *testing.T) { } } +// #515: the home screen's hole was the mirror of doctor's. Its local-env +// fallback was reached only when the pointer was EMPTY (`if !binding.applied`), +// so a pointer that was set but WRONG skipped the #401 fix entirely and the +// screen said "No secure environment on this machine yet" over a live install. +func TestRealProbeEnv_WrongPointerOnLocalCluster_FallsBack(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") // binding APPLIED, and wrong + o := fallbackRelease("lukas-02") + cs := fake.NewClientset(o[0].(*appsv1.Deployment), o[1].(*corev1.Service)) + + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + // Models cluster.Load: an explicit opts.Namespace wins, otherwise the + // context's own namespace — which the installer points at the client's + // namespace (install-client-helm.sh `kubectl config set-context --current + // --namespace`). So the binding sends the first probe to "stale-ns" and the + // unbound fallback reload lands on "lukas-02". + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "lukas-02" + } + return &cluster.ResolvedConfig{ + Namespace: ns, ServerURL: "https://127.0.0.1:6550", RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + ep := realProbeEnv(context.Background()) + if ep.local != localLive || ep.name != "tracebloc" { + t.Fatalf("=> %+v, want the live local environment despite the stale pointer", ep) + } + // Bugbot (#515): the release we found is NOT the client the pointer names, so + // the heartbeat (looked up by that pointer's id) is about a different machine + // and must be barred from greening this one. + if !ep.pointerStale { + t.Error("a fallback that fired after a pointer MISS must mark the pointer stale") + } +} + +// The stale mark must actually change the verdict: local liveness from one +// client plus a beatOnline from another is an Online nobody earned. +func TestResolveHomeModel_StalePointerNeverRendersOnline(t *testing.T) { + base := func(stale bool) homeModel { + return resolveHomeModel(context.Background(), homeDeps{ + budget: 2 * time.Second, + invoked: func() string { return binTB }, + tbAvailable: func() bool { return true }, + hasResources: func() bool { return true }, + signIn: func() (bool, string, string) { return true, "a@b.io", "Lukas" }, + rememberedClient: func() (bool, string) { return true, "stale-01" }, + probeBeat: func(context.Context) heartbeatState { return beatOnline }, + probeEnv: func(context.Context) envProbe { + return envProbe{local: localLive, name: "tracebloc", pointerStale: stale} + }, + }) + } + + if m := base(false); m.state != homeOnline { + t.Fatalf("control: live + beatOnline + fresh pointer must be Online, got %v", m.state) + } + m := base(true) + if m.state == homeOnline { + t.Error("a stale pointer must never render Online — the heartbeat is another client's") + } + if m.state != homeRunning { + t.Errorf("want the honest running state, got %v", m.state) + } + if m.confirmedNotOnline { + t.Error("nor may another client's heartbeat be reported as THIS one being not-online") + } + // Bugbot (#515): the LABEL is a claim too. "stale-01" is the handle of the + // client the pointer names — not what the fallback found running here — so + // presenting it as this machine's environment is a wrong name, and one that + // contradicts doctor's namespace-based label for the same state. + if m.envName == "stale-01" { + t.Errorf("a stale pointer's client handle must not label the environment running here, got %q", m.envName) + } + if m.envName != "tracebloc" { + t.Errorf("want the probe's own name for the release that IS here, got %q", m.envName) + } + + // Control: with a fresh pointer the remembered handle is still preferred — + // otherwise "don't use the remembered name" could pass by never using it. + if fresh := base(false); fresh.envName != "stale-01" { + t.Errorf("a non-stale pointer must still prefer the remembered client name, got %q", fresh.envName) + } +} + +// …and a beatNotOnline off a stale pointer is equally uninformative: it must not +// harden into "backend reports not online" for a client it isn't about. +func TestResolveHomeModel_StalePointerNotOnlineIsNotConfirmed(t *testing.T) { + m := resolveHomeModel(context.Background(), homeDeps{ + budget: 2 * time.Second, + invoked: func() string { return binTB }, + tbAvailable: func() bool { return true }, + hasResources: func() bool { return true }, + signIn: func() (bool, string, string) { return true, "a@b.io", "Lukas" }, + rememberedClient: func() (bool, string) { return true, "stale-01" }, + probeBeat: func(context.Context) heartbeatState { return beatNotOnline }, + probeEnv: func(context.Context) envProbe { + return envProbe{local: localLive, name: "tracebloc", pointerStale: true} + }, + }) + if m.confirmedNotOnline { + t.Error("a stale pointer's heartbeat carries no signal in either direction") + } +} + +// …and the ownership gate survives it: the same wrong pointer on a REMOTE +// cluster stays no-release, so a shared cluster's unrelated client is never +// greeted as yours (§7.5). +func TestRealProbeEnv_WrongPointerOnRemoteCluster_StaysGated(t *testing.T) { + writeActiveClientConfig(t, "stale-ns", "Stale") + o := fallbackRelease("colleague-07") + cs := fake.NewClientset(o[0].(*appsv1.Deployment), o[1].(*corev1.Service)) + + origLoad, origCS := loadClusterFn, newClientsetFn + t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS }) + loadClusterFn = func(o cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + ns := o.Namespace + if ns == "" { + ns = "colleague-07" + } + return &cluster.ResolvedConfig{ + Namespace: ns, ServerURL: "https://k8s.corp.example:6443", RestConfig: &rest.Config{}, + }, nil + } + newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil } + + if ep := realProbeEnv(context.Background()); ep.local != localNoRelease || ep.name != "" { + t.Fatalf("=> %+v, want localNoRelease (a shared cluster's client is not yours)", ep) + } +} + func TestIsLocalServerURL(t *testing.T) { local := []string{ "https://127.0.0.1:6550", diff --git a/internal/cli/resources.go b/internal/cli/resources.go index b2ac85c..c636297 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -92,7 +92,7 @@ func runResourcesShow(ctx context.Context, p *ui.Printer, opts cluster.Kubeconfi // multi-client redirect note self-leads its one leading blank (§380). target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } return renderResources(ctx, p, target) } diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index b9f5048..c9f5c27 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -145,7 +145,7 @@ func runResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, opts clust // and never regresses the #375 double-blank (§380). target, err := resolveClusterTarget(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } return applyResourcesSet(ctx, p, pr, target, opts, req) } diff --git a/internal/cli/seal.go b/internal/cli/seal.go index 04ec661..d2dca1f 100644 --- a/internal/cli/seal.go +++ b/internal/cli/seal.go @@ -78,7 +78,7 @@ func runSealCheck(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOpt // multi-client redirect note is the opening line and self-leads its blank. target, err := resolveClusterTargetFn(ctx, p, opts, binding, false, true) if err != nil { - return binding.explain(err) + return binding.explain(ctx, err) } tt := helm.TestTarget{ Release: target.Release.ReleaseName, diff --git a/internal/cli/testdata/golden/08-client.golden b/internal/cli/testdata/golden/08-client.golden index 8a94809..f8ff84e 100644 --- a/internal/cli/testdata/golden/08-client.golden +++ b/internal/cli/testdata/golden/08-client.golden @@ -57,6 +57,7 @@ Usage: tracebloc client [command] Available Commands: + create Point this machine at its tracebloc client — adopts the one already on this cluster status Show whether tracebloc can see this machine's client (online) Flags: @@ -69,7 +70,15 @@ Global Flags: Use "tracebloc client [command] --help" for more information about a command. $ tracebloc client create --help -Provision a tracebloc client for this machine (auto-named; no flags required) +Point this machine at its tracebloc client. + +Keyed on the cluster your kubeconfig reaches: if a tracebloc client already runs +there, this ADOPTS it — no prompt, no new credential, no duplicate — which is how +you repoint a machine whose active client went stale. On a cluster that runs no +client yet it provisions a new one, and asks first. + +Provisioning a brand-new machine is normally the installer's job — it calls this +for you, with no flags. Usage: tracebloc client create [flags] diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 3dec5c0..dbb06f0 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -225,6 +225,7 @@ screen. %s/%d are runtime placeholders. "Not ready — part of your secure environment can't start yet." "Not ready — part of your secure environment isn't running." "Not ready — the training images can't be pulled." +"Not ready — your active client points at namespace %q, which isn't on this cluster, so data commands will keep failing until you repoint." "Not signed in yet." "Not signed in — run `%s login`." "Not signed in. Run `tracebloc login`." @@ -238,6 +239,7 @@ screen. %s/%d are runtime placeholders. "Pending > %s: %v" "Pick this dataset when you set it up." "Please name the dataset." +"Point this machine at the environment above: %s client create (this cluster already runs a client, so it adopts it — no new credential)" "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." @@ -348,6 +350,8 @@ screen. %s/%d are runtime placeholders. "Wrote client id + namespace to %s (no new credential — the existing one stands)." "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." +"Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor" +"Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" "Your dataset records (marked unavailable, not deleted)" "Your files are copied securely into your secure environment's storage — set up and cleaned up for you." @@ -363,6 +367,11 @@ screen. %s/%d are runtime placeholders. "a training run needs at least %s — %s is too little." "account" "active client" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client IS running on this machine, in namespace %q.\n Point this machine at it: %s client create\n (this cluster already runs a client, so it adopts it — no new credential)\n Or target it just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\nA tracebloc client is running on this cluster, in namespace %q.\n Target it just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at.\n\ntracebloc clients are running on this cluster, in namespaces: %s.\n Target one just this once: --namespace %s" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context" +"active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; run this command there, or override with --namespace/--context.\n\nNo tracebloc client is running on this cluster either — if this machine should have one, set one up: %s" "annotations" "app version" "authorized — confirming the token with the backend …"