diff --git a/internal/cli/delete.go b/internal/cli/delete.go index 4d85e85b..754b7b11 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -193,21 +193,57 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // 1. Revoke the machine credential server-side (POST /edge-device//revoke, // §7.10 / C.6). This kills the credential without deleting the row — the - // retained history in scope 2 stays intact. A 403 → ask-an-admin. + // retained history in scope 2 stays intact. + revoked := true if rerr := client.RevokeClient(ctx, id); rerr != nil { + revoked = false + // A 426 (CLI too old) won't recover by continuing — the whole offboard talks + // to the same backend — so fail fast with the upgrade message rather than + // tear the machine down against a backend that can't process the revoke. + // Mirrors the pre-offboard guard above (which treats 426 as terminal). + var ue *api.UpgradeRequiredError + if errors.As(rerr, &ue) { + return &exitError{code: 1, err: rerr} + } var ae *api.APIError - if errors.As(rerr, &ae) && ae.StatusCode == http.StatusForbidden { - return askAnAdmin(ctx, p, client, "offboard this machine", "offboarding") + if errors.As(rerr, &ae) { + switch ae.StatusCode { + case http.StatusForbidden: + // A 403 is a genuine authorization decision — you can't revoke a + // client you don't manage; route to ask-an-admin (unchanged). + return askAnAdmin(ctx, p, client, "offboard this machine", "offboarding") + case http.StatusUnauthorized: + // A 401 means the signed-in session is expired/revoked. With --force + // the online-guard above is skipped, so DON'T silently tear the machine + // down while a live credential remains — fail fast and point at sign-in, + // mirroring the pre-offboard guard's 401 handling. + return &exitError{code: 1, err: errors.New( + "tracebloc rejected your credentials — run `tracebloc login`, then retry `tracebloc delete`")} + } } - return &exitError{code: 1, err: fmt.Errorf("revoking the machine credential: %w", rerr)} + // Any OTHER revoke failure must NOT block the local teardown. Removing + // tracebloc from THIS machine (helm uninstall, cluster teardown, on-host + // data + config wipe, self-remove) is offline-capable and is the command's + // primary job — it can't be held hostage to a best-effort remote call. This + // hits on a 404 (a stale/wrong-account active-client pointer, or a backend + // predating the /revoke route), a transient network/5xx error, etc. Warn and + // continue, mirroring the online-guard above. The credential may remain live + // server-side (revoked stays false), so the closing summary says so: the user + // can revoke it from the dashboard, and the orphan reaper (backend#970) sweeps + // a never-torn-down record later. + p.Hintf("Couldn't revoke the credential server-side (%v) — continuing with local teardown. "+ + "The credential may still be live on tracebloc; revoke it from the dashboard if needed.", rerr) + } else { + p.Successf("Revoked this machine's credential (client %q kept on tracebloc as a record).", name) } - p.Successf("Revoked this machine's credential (client %q kept on tracebloc as a record).", name) - // The teardown steps below are best-effort (the credential is already revoked), - // but a step that leaves real state behind — a live release, the local cluster, - // or on-host data — must NOT be papered over by the final success line. Track it - // so the closing message tells the truth (image reclaim is pure disk cleanup, so - // it's intentionally excluded — its own warning already surfaces it). + // The teardown steps below are best-effort. (The credential is revoked when the + // server-side revoke above succeeded; on a best-effort revoke failure it may + // still be live — the closing summary reports which.) A step that leaves real + // state behind — a live release, the local cluster, or on-host data — must NOT be + // papered over by the final success line. Track it so the closing message tells + // the truth (image reclaim is pure disk cleanup, so it's intentionally excluded — + // its own warning already surfaces it). degraded := false // Clear the local enrollment pointer and persist it IMMEDIATELY — before the @@ -283,14 +319,23 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er } p.Newline() - // The credential is revoked either way, so the machine can no longer connect — - // but only claim a clean offboard when the teardown actually completed. If a - // step left real state behind, say so instead of printing an unqualified success. - if degraded { + // Be honest on BOTH axes: whether the server-side revoke succeeded (revoked) and + // whether the local teardown completed (!degraded). Neither is guaranteed — the + // revoke is best-effort on a non-terminal failure, and the teardown steps are + // best-effort — so only claim "revoked / no longer connected" when it's true. + switch { + case revoked && !degraded: + p.Successf("Offboarded %q. This machine is no longer connected to tracebloc.", name) + case revoked && degraded: p.Warnf("Offboarded %q: the machine credential is revoked, so it can no longer connect to tracebloc — "+ "but some cleanup above didn't complete. Finish the flagged steps by hand.", name) - } else { - p.Successf("Offboarded %q. This machine is no longer connected to tracebloc.", name) + case !revoked && !degraded: + p.Warnf("Tore down %q on this machine. The server-side revoke didn't complete, so the credential may "+ + "still be live on tracebloc — revoke it from the dashboard if needed (the orphan reaper sweeps it otherwise).", name) + default: // !revoked && degraded + p.Warnf("Tore down %q on this machine, but some cleanup above didn't complete and the server-side revoke "+ + "didn't complete — the credential may still be live on tracebloc (revoke it from the dashboard). "+ + "Finish the flagged steps by hand.", name) } return nil } diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index fbfab450..f70bb56c 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -193,6 +193,138 @@ func TestDelete_Yes_FullSequence(t *testing.T) { assertRemoved(t, fn, filepath.Join(filepath.Dir(exe), "tb")) } +// (b') A non-403 revoke failure (a 404 from a stale/wrong-account pointer or a +// backend predating /revoke, a transient network/5xx error, …) must NOT brick the +// offboard: local teardown is the command's real job and runs anyway. Only a 403 +// (a genuine authz denial) aborts to ask-an-admin (covered separately). +func TestDelete_RevokeNon403_ContinuesTeardown(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): + w.WriteHeader(http.StatusNotFound) // 404: stale pointer / backend predates /revoke + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + exe := writeBinaryWithTBAlias(t) + fn := &fakeNodeboot{executable: exe} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("a non-403 revoke error must not abort the offboard, got: %v", err) + } + // Teardown still ran despite the failed revoke. + for _, want := range []string{"uninstall:gpu-box-01", "teardown:" + nodeboot.ClusterName, "prune"} { + found := false + for _, c := range fn.calls { + if c == want { + found = true + break + } + } + if !found { + t.Errorf("teardown step %q must run even when revoke fails; calls: %v", want, fn.calls) + } + } + // Honest messaging: warn about the failed revoke, and do NOT claim success. + s := out.String() + if !strings.Contains(s, "Couldn't revoke the credential server-side") { + t.Errorf("want a warning about the failed revoke, got:\n%s", s) + } + if strings.Contains(s, "Revoked this machine's credential") { + t.Errorf("must NOT claim the credential was revoked when it wasn't:\n%s", s) + } +} + +// (b”) A non-403 revoke failure that ALSO hits a degraded teardown step (here: no +// namespace → the uninstall is skipped) must tell the truth on BOTH axes — it must +// NOT claim the credential was revoked or the machine disconnected. Regression for +// the overclaim: the old closing hardcoded "the credential is revoked, so it can no +// longer connect" even when the revoke had failed. +func TestDelete_RevokeNon403_Degraded_HonestClosing(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): + w.WriteHeader(http.StatusNotFound) // 404: best-effort revoke fails + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "") // no namespace → uninstall skipped → degraded + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("a best-effort revoke failure must not abort the offboard, got: %v", err) + } + s := out.String() + if strings.Contains(s, "no longer connect") || strings.Contains(s, "credential is revoked") { + t.Errorf("closing must NOT claim the credential was revoked / the machine disconnected when revoke failed:\n%s", s) + } + if !strings.Contains(s, "revoke didn't complete") { + t.Errorf("closing should say the server-side revoke didn't complete:\n%s", s) + } + if !strings.Contains(s, "still be live") { + t.Errorf("closing should point at the possibly-live credential:\n%s", s) + } +} + +// A 426 from the REVOKE call (reached under --force, which skips the online guard) +// must fail fast with the upgrade message rather than warn-and-continue into a +// teardown against a backend that can't process the revoke — matching the guard. +func TestDelete_RevokeUpgradeRequired_FailsFast(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { + w.WriteHeader(http.StatusUpgradeRequired) // 426 + _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"1.2.3"}`)) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true, force: true}) + if err == nil || !strings.Contains(err.Error(), "too old") { + t.Fatalf("a 426 revoke must fail fast with the upgrade message, got: %v", err) + } + if len(fn.calls) != 0 { + t.Errorf("no teardown after a 426 revoke, got: %v", fn.calls) + } +} + +// A 401 from the REVOKE call (reached under --force) means the session is expired/ +// revoked; fail fast and point at re-login rather than tear the machine down while a +// live credential remains. Without this, --force + an expired token silently wipes +// the machine and leaves the credential alive. +func TestDelete_RevokeUnauthorized_FailsFast(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { + w.WriteHeader(http.StatusUnauthorized) // 401 + _, _ = w.Write([]byte(`{"detail":"invalid token"}`)) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true, force: true}) + if err == nil || !strings.Contains(err.Error(), "tracebloc login") { + t.Fatalf("a 401 revoke must fail fast with a sign-in hint, got: %v", err) + } + if len(fn.calls) != 0 { + t.Errorf("no teardown after a 401 revoke, got: %v", fn.calls) + } +} + // (c) --keep-data spares ~/.tracebloc but still uninstalls + removes the binary. func TestDelete_KeepData_SparesDataDir(t *testing.T) { withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {