From 8a6f7211af24ab7afb3157006945fbe2d6fe902b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 8 Jul 2026 15:55:51 +0200 Subject: [PATCH] =?UTF-8?q?fix(client=20create):=20never=20mint=20over=20a?= =?UTF-8?q?=20live=20cluster=20=E2=80=94=20fail=20closed=20on=20unreadable?= =?UTF-8?q?=20discovery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tracebloc client create` could mint a NEW backend client and stamp the cluster's cluster_id anchor onto it, even when a healthy DIFFERENT client was already running on the cluster. The installer then refused to deploy the new client (one-client-per-machine), leaving an orphaned "phantom" that owns the anchor — so every later re-provision 409s ("cluster_conflict", mislabeled as cross-account) and the real client can never reclaim the anchor. Confirmed in the field: edge_device 1060 minted + anchored, never deployed, wedging a cluster that actually runs 1044. Root of the reproducible class: DiscoverInClusterClientID swallowed List/RBAC errors into (nil, nil) — "nothing installed" — which is indistinguishable from a genuinely fresh cluster, so runClientCreate fell through to a mint. Fix (two edits): - cluster/discover.go: DiscoverInClusterClientID is now three-valued. It returns (nil, err) when it CANNOT determine — a deployments List error, a secrets List error, or a release present whose CLIENT_ID is unreadable. (nil, nil) now means only "reachable and genuinely no client". An empty cluster still reports emptiness via an empty list, not an error, so fresh installs are unaffected. - cli/client.go: adoptLiveInClusterClient fails closed on a discovery error when the cluster is REACHABLE (clusterID != "", i.e. the kube-system UID read succeeded over the same kubeconfig) — refusing to mint a duplicate that could strand the anchor. Only a genuinely unreachable cluster (clusterID == "", where the UID read failed too) keeps the old fall-through to a non-anchored mint (which stamps no anchor, so it can't orphan one) — the deliberate headless/no-cluster path. Tests (verified to FAIL against the pre-fix code): - discover_test.go: DeploymentsListError / SecretsListError / ReleaseButNoSecret now expect (nil, error). - client_test.go: DiscoveryErrorReachableFailsClosed (reachable + discovery error -> exitError, no mint) and DiscoveryErrorUnreachableMintsNonAnchored (unreachable -> still mints, cluster_id empty) — the latter guards against over-tightening into the legitimate headless path. Full repo suite green; adversarially reviewed. This is fix #1 of the phantom-client investigation (never mint over a live cluster). Follow-ups (separate): backend same-account re-anchor + honest 409 message; orphan reaper. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/client.go | 23 ++++++++++-- internal/cli/client_test.go | 61 +++++++++++++++++++++++++++++++ internal/cluster/discover.go | 27 ++++++++++---- internal/cluster/discover_test.go | 38 ++++++++++++++++++- 4 files changed, 136 insertions(+), 13 deletions(-) diff --git a/internal/cli/client.go b/internal/cli/client.go index 4277bb0..95a4e1f 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -465,13 +465,28 @@ func adoptLiveInClusterClient( ) (*api.ProvisionedClient, bool, error) { live, err := readInClusterClient(ctx, cluster.KubeconfigOptions{Path: opts.kubeconfigPath, Context: opts.contextOverride}) if err != nil { - // Best-effort: couldn't inspect the cluster for a live client. Fall through - // to a plain create (the backend's cluster_id get-or-create still applies). - ilog.Logf("in-cluster client discovery failed (non-fatal): %v", err) + // We couldn't inspect the cluster for an existing client. Whether that's + // safe to ignore depends on reachability: clusterID != "" means we DID read + // the cluster's kube-system UID over the same kubeconfig, so the cluster is + // reachable and this is an RBAC/transient read failure — NOT proof it's + // empty. Minting here could create a duplicate over a live client that then + // never deploys and permanently strands the cluster anchor (the phantom-1060 + // class). Fail closed. Only a genuinely unreachable cluster (clusterID == "", + // where the UID read failed too) falls through to a plain, non-anchored + // create — that mint stamps no anchor, so it can't orphan one. + if clusterID != "" { + ilog.Logf("in-cluster client discovery failed on a reachable cluster (failing closed): %v", err) + return nil, false, &exitError{code: 1, err: fmt.Errorf( + "couldn't check whether a tracebloc client is already running on this cluster (%w) — "+ + "provisioning now could mint a duplicate that never deploys and locks the cluster to it. "+ + "Re-run (if this was transient); if it persists, ensure your kubeconfig/context can list "+ + "deployments and secrets across namespaces. Diagnose with `tracebloc cluster doctor`", err)} + } + ilog.Logf("in-cluster client discovery skipped — cluster unreachable (non-fatal): %v", err) return nil, false, nil } if live == nil { - return nil, false, nil // fresh cluster — nothing installed to adopt + return nil, false, nil // reachable, nothing installed to adopt — a genuine fresh cluster } ilog.Logf("live in-cluster client: id=%s namespace=%s", live.ClientID, live.Namespace) diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index a721d84..0528f68 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -288,6 +288,67 @@ func TestClientCreate_R7_CrossAccountRefuse(t *testing.T) { } } +// TestClientCreate_R7_DiscoveryErrorReachableFailsClosed: the cluster is REACHABLE +// (its kube-system UID read cleanly, clusterID != "") but in-cluster client discovery +// ERRORS (RBAC/transient List failure). We can't tell whether a client is already +// running, so minting would risk a duplicate that never deploys and strands the +// cluster anchor (the phantom-1060 class). Must fail closed — no mint, no backfill. +func TestClientCreate_R7_DiscoveryErrorReachableFailsClosed(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(`[]`)) // account list (fetched before adopt) is allowed + default: + t.Errorf("unexpected %s %s — must fail closed before any mint/backfill", r.Method, r.URL.Path) + } + }) + stubClusterID(t, "uid-9", nil) // cluster reachable + stubInClusterClient(t, nil, errors.New("forbidden: cannot list deployments")) + + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{name: "box", location: "DE", yes: true}) + if err == nil || !strings.Contains(err.Error(), "couldn't check whether a tracebloc client is already running") { + t.Errorf("want fail-closed error, got %v", err) + } +} + +// TestClientCreate_DiscoveryErrorUnreachableMintsNonAnchored: when the cluster is +// genuinely UNREACHABLE (the UID read failed too → clusterID == ""), a discovery +// error is not proof a client is running, and a non-anchored mint stamps no anchor +// so it can't orphan one. Provisioning must still proceed (the deliberate no-cluster +// fallback), minting with an empty cluster_id. Guards against over-tightening the +// fail-closed gate into the legitimate headless path. +func TestClientCreate_DiscoveryErrorUnreachableMintsNonAnchored(t *testing.T) { + var body api.CreateClientRequest + postCalled := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + postCalled = true + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":8,"first_name":"box","username":"u-8","namespace":"box","location":"DE"}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + }) + stubClusterID(t, "", errors.New("no cluster reachable")) + stubInClusterClient(t, nil, errors.New("no cluster reachable")) + + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{name: "box", location: "DE", yes: true}); err != nil { + t.Fatalf("unreachable-cluster provisioning must still mint (non-anchored): %v", err) + } + if !postCalled { + t.Fatal("expected a non-anchored mint when the cluster is unreachable") + } + if body.ClusterID != "" { + t.Errorf("cluster_id = %q, want empty (non-anchored mint)", body.ClusterID) + } +} + func TestClientCreate_RequiresLogin(t *testing.T) { t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) // no config → not signed in err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "x", location: "DE", yes: true}) diff --git a/internal/cluster/discover.go b/internal/cluster/discover.go index 00a7b7f..f853e6d 100644 --- a/internal/cluster/discover.go +++ b/internal/cluster/discover.go @@ -297,15 +297,25 @@ const clientChartSelector = "app.kubernetes.io/name=client,app.kubernetes.io/man // carries the same CLIENT_ID under the same labels. // // This anchors R7 adopt-backfill: a live client whose backend cluster_id is null -// must be adopted (and its anchor backfilled), never re-minted. Best-effort — it -// returns (nil, nil) when nothing is installed or the cluster can't be read -// (unreachable / restricted RBAC), so callers fall back to a plain create. +// must be adopted (and its anchor backfilled), never re-minted. +// +// Return contract (deliberately three-valued, so callers can tell "empty" from +// "couldn't tell" — collapsing the two is what let `client create` mint a +// duplicate over a live client and orphan it, the phantom-1060 class): +// - (client, nil) — a live client was found; +// - (nil, nil) — the cluster is READABLE and genuinely has no client release; +// - (nil, err) — a read/RBAC error meant we could NOT determine either way. +// +// Callers must fail closed on the error case, never treat it as "nothing installed". func DiscoverInClusterClientID(ctx context.Context, cs kubernetes.Interface) (*InClusterClient, error) { deps, err := cs.AppsV1().Deployments(metav1.NamespaceAll).List(ctx, metav1.ListOptions{ LabelSelector: clientChartSelector, }) if err != nil { - return nil, nil // best-effort: treat an unreadable cluster as "nothing installed" + // A reachable-but-unreadable cluster must NOT be reported as "nothing + // installed" — that ambiguity is exactly what let a duplicate be minted + // over a live client. Surface it so the caller fails closed. + return nil, fmt.Errorf("listing client deployments to check for an existing client: %w", err) } ns := "" for _, d := range deps.Items { @@ -315,20 +325,23 @@ func DiscoverInClusterClientID(ctx context.Context, cs kubernetes.Interface) (*I } } if ns == "" { - return nil, nil // no client release on this cluster + return nil, nil // readable, no client release installed — a genuine fresh cluster } secrets, err := cs.CoreV1().Secrets(ns).List(ctx, metav1.ListOptions{ LabelSelector: clientChartSelector, }) if err != nil { - return nil, nil + return nil, fmt.Errorf("reading the existing client's identity in namespace %q: %w", ns, err) } for _, s := range secrets.Items { if v, ok := s.Data["CLIENT_ID"]; ok && len(v) > 0 { return &InClusterClient{ClientID: string(v), Namespace: ns}, nil } } - return nil, nil + // A client release IS installed here (its jobs-manager Deployment exists) but its + // CLIENT_ID secret wasn't readable — we know a client is present, so this is not a + // fresh cluster. Fail closed rather than let the caller mint over it. + return nil, fmt.Errorf("a tracebloc client is installed in namespace %q but its CLIENT_ID could not be read", ns) } // pickJobsManagerService probes for the chart's jobs-manager diff --git a/internal/cluster/discover_test.go b/internal/cluster/discover_test.go index 10466c5..d316eb3 100644 --- a/internal/cluster/discover_test.go +++ b/internal/cluster/discover_test.go @@ -9,7 +9,9 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" ) // jobsManagerDeployment builds the minimal Deployment the chart @@ -135,10 +137,42 @@ func TestDiscoverInClusterClientID_NoRelease(t *testing.T) { } func TestDiscoverInClusterClientID_ReleaseButNoSecret(t *testing.T) { + // A release IS installed (jobs-manager present) but its CLIENT_ID secret is + // absent/unreadable: we KNOW a client is here, so this must NOT read as "nothing + // installed". It returns an error so the caller fails closed rather than mint a + // duplicate over the live client (phantom-1060 class). cs := fake.NewClientset(jobsManagerDeployment("tracebloc", "tracebloc", "client-1.3.5", "1.3.5", "d")) got, err := DiscoverInClusterClientID(context.Background(), cs) - if err != nil || got != nil { - t.Errorf("release but no secret: want (nil,nil), got (%+v,%v)", got, err) + if err == nil || got != nil { + t.Errorf("release but unreadable CLIENT_ID: want (nil, error), got (%+v, %v)", got, err) + } +} + +func TestDiscoverInClusterClientID_DeploymentsListError_FailsClosed(t *testing.T) { + // A reachable-but-unreadable cluster (RBAC/transient List failure) must NOT be + // reported as (nil,nil) "nothing installed" — that ambiguity is what let a + // duplicate be minted over a live client. Surface an error so the caller fails + // closed. Regression guard for the phantom-1060 root cause. + cs := fake.NewClientset() + cs.PrependReactor("list", "deployments", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("forbidden: cannot list deployments") + }) + got, err := DiscoverInClusterClientID(context.Background(), cs) + if err == nil || got != nil { + t.Errorf("deployments list error: want (nil, error), got (%+v, %v)", got, err) + } +} + +func TestDiscoverInClusterClientID_SecretsListError_FailsClosed(t *testing.T) { + // A release is present but the secret read fails — still "couldn't determine", + // so return an error (fail closed), never (nil,nil). + cs := fake.NewClientset(jobsManagerDeployment("tracebloc", "tracebloc", "client-1.3.5", "1.3.5", "d")) + cs.PrependReactor("list", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("forbidden: cannot list secrets") + }) + got, err := DiscoverInClusterClientID(context.Background(), cs) + if err == nil || got != nil { + t.Errorf("secrets list error: want (nil, error), got (%+v, %v)", got, err) } }