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) } }