fix(data delete): resolve the table name case-insensitively; fail closed (#1027)

internal/cli/data_delete.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
+	"k8s.io/client-go/kubernetes"
 
 	"github.com/tracebloc/cli/internal/cluster"
 	"github.com/tracebloc/cli/internal/push"
@@ -58,7 +60,9 @@ Exit codes:
   0  artifacts removed (or --dry-run, or the user declined)
   2  invalid table name
   3  kubeconfig error, or refused (no confirmation off a terminal)
-  4  cluster reachable but no tracebloc client / shared storage missing
+  4  cluster reachable but no tracebloc client / shared storage missing,
+     or the client's dataset list couldn't be read (can't confirm the target)
+  5  no dataset by that name on this client (nothing to delete)
   7  teardown failed mid-flight (table drop or PVC rm errored)`,
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -123,9 +127,23 @@ undone — re-ingesting the data is the only way back.`)
 	}
 	resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC
 
+	// 3. Resolve the requested name against the datasets actually on the
+	//    client — case-INSENSITIVELY, the same EqualFold match `data ingest`'s
+	//    destination guard uses (destTableExists / listDatasetsFn). The
+	//    teardown itself (DROP TABLE / rm) is case-SENSITIVE on the cluster, so
+	//    `data delete Churn` for a table named `churn` used to drop nothing and
+	//    still exit 0 — a silent no-op that read as a successful delete
+	//    (backend#1027). We tear down the REAL spelling from here on, and fail
+	//    CLOSED (never exit 0) if the name isn't there or the list can't be
+	//    read: a destructive, unrecoverable delete must confirm its target.
+	matched, err := resolveDeleteTarget(ctx, cs, resolved, a.Table)
+	if err != nil {
+		return err
+	}
+
 	// 4. Show exactly what will be deleted — the customer's last look
 	//    before destructive, unrecoverable work.
-	plan := push.PlanTeardown(a.Table)
+	plan := push.PlanTeardown(matched)
 
 	p.Section("Target")
 	p.Field("context", resolved.Context)
@@ -155,7 +173,7 @@ undone — re-ingesting the data is the only way back.`)
 				"refusing to delete without confirmation: pass --yes or run on a terminal")}
 		}
 		p.PromptHint("This drops the table and removes the files listed above — there's no undo. Pass --yes next time to skip this prompt.")
-		ok, err := a.Prompter.Confirm(fmt.Sprintf("Delete %q and its files?", a.Table), false)
+		ok, err := a.Prompter.Confirm(fmt.Sprintf("Delete %q and its files?", matched), false)
 		if err != nil {
 			if errors.Is(err, errInteractiveCancelled) {
 				p.Infof("Cancelled — nothing was deleted.")
@@ -179,7 +197,7 @@ undone — re-ingesting the data is the only way back.`)
 		Namespace:          resolved.Namespace,
 		PVCClaimName:       pvc.ClaimName,
 		PVCMountPath:       pvc.MountPath,
-		Table:              a.Table,
+		Table:              matched,
 		ServiceAccountName: release.IngestorSAName,
 		// Image left empty → push.DefaultStagePodImage (alpine; has rm).
 	})
@@ -192,7 +210,7 @@ undone — re-ingesting the data is the only way back.`)
 			return &exitError{code: 7, err: fmt.Errorf(
 				"teardown incomplete — the table %s.%s was dropped, but removing its files failed: %w; "+
 					"re-run `tracebloc data delete %s`, or delete the leftover staging dirs on the node",
-				plan.Database, plan.Table, err, a.Table)}
+				plan.Database, plan.Table, err, matched)}
 		}
 		return &exitError{code: 7, err: fmt.Errorf("teardown failed: %w", err)}
 	}
@@ -202,3 +220,45 @@ undone — re-ingesting the data is the only way back.`)
 	p.Infof("The dataset's catalog metadata is kept as a record on tracebloc, marked unavailable — never removed.")
 	return nil
 }
+
+// resolveDeleteTarget maps the user-supplied dataset name onto the REAL
+// spelling of a dataset that actually exists on the client, matching
+// case-INSENSITIVELY exactly as `data ingest`'s destination guard does
+// (see destTableExists — same listDatasetsFn seam, same strings.EqualFold).
+// It exists because the teardown is case-SENSITIVE on the cluster: DROP TABLE
+// / rm against a mis-cased name removes nothing yet still succeeds, so
+// `data delete Churn` for a table named `churn` used to be a silent no-op that
+// exited 0 (backend#1027). The caller tears down the returned name, never the
+// raw flag.
+//
+// Unlike the ingest guard — which fails OPEN, because the in-cluster duplicate
+// check still backstops it — a destructive, unrecoverable delete fails CLOSED:
+//   - listing the datasets errored  → exit 4 (cluster reachable but we can't
+//     confirm what's there; refuse rather than delete blind).
+//   - no dataset matches (any case) → exit 5 (nothing to delete), naming what
+//     IS on the client so the caller can fix the spelling.
+func resolveDeleteTarget(ctx context.Context, cs kubernetes.Interface, resolved *cluster.ResolvedConfig, requested string) (string, error) {
+	names, err := listDatasetsFn(ctx, cs, resolved.RestConfig, resolved.Namespace)
+	if err != nil {
+		return "", &exitError{code: 4, err: fmt.Errorf(
+			"can't confirm %q exists on this client — refusing to delete without "+
+				"confirming the target first: %w", requested, err)}
+	}
+	for _, n := range names {
+		if strings.EqualFold(n, requested) {
+			return n, nil
+		}
+	}
+	return "", &exitError{code: 5, err: fmt.Errorf(
+		"no dataset named %q on this client%s", requested, availableHint(names))}
+}
+
+// availableHint renders the "here's what IS on the client" tail of a
+// not-found delete error, so a mis-typed name (case now resolves on its own)
+// is easy to correct.
+func availableHint(names []string) string {
+	if len(names) == 0 {
+		return " — this client has no ingested datasets"
+	}
+	return fmt.Sprintf(" (datasets on this client: %s)", strings.Join(names, ", "))
+}

internal/cli/data_delete_test.go

@@ -0,0 +1,97 @@
+package cli
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
+
+	"github.com/tracebloc/cli/internal/cluster"
+	"github.com/tracebloc/cli/internal/push"
+)
+
+// resolveDeleteTarget is the backend#1027 fix: `data delete` must match the
+// requested name against the client's datasets case-INSENSITIVELY (the
+// teardown is case-SENSITIVE, so a mis-cased name used to DROP nothing and
+// still exit 0), tear down the REAL spelling, and — because a delete is
+// destructive and unrecoverable — fail CLOSED when it can't confirm the
+// target. Mirrors TestDestTableExists (same listDatasetsFn seam).
+func TestResolveDeleteTarget(t *testing.T) {
+	resolved := &cluster.ResolvedConfig{Namespace: "ns"}
+
+	restore := listDatasetsFn
+	defer func() { listDatasetsFn = restore }()
+
+	withDatasets := func(names []string, err error) {
+		listDatasetsFn = func(_ context.Context, _ kubernetes.Interface, _ *rest.Config, _ string) ([]string, error) {
+			return names, err
+		}
+	}
+
+	t.Run("mis-cased name resolves to the REAL spelling and tears THAT down", func(t *testing.T) {
+		withDatasets([]string{"other", "Churn"}, nil)
+		matched, err := resolveDeleteTarget(context.Background(), nil, resolved, "churn")
+		if err != nil {
+			t.Fatalf("a mis-cased name must resolve, not fail: %v", err)
+		}
+		if matched != "Churn" {
+			t.Errorf("matched = %q, want the EXISTING spelling %q", matched, "Churn")
+		}
+		// The teardown keys on the returned name — a DROP/rm against the raw
+		// "churn" flag would silently no-op on a case-sensitive cluster.
+		if plan := push.PlanTeardown(matched); plan.Table != "Churn" {
+			t.Errorf("teardown plan targets %q, want the real name %q", plan.Table, "Churn")
+		}
+	})
+
+	t.Run("exact match resolves unchanged", func(t *testing.T) {
+		withDatasets([]string{"other", "Churn"}, nil)
+		matched, err := resolveDeleteTarget(context.Background(), nil, resolved, "Churn")
+		if err != nil || matched != "Churn" {
+			t.Errorf("exact match: matched=%q err=%v, want Churn/nil", matched, err)
+		}
+	})
+
+	t.Run("nonexistent name fails closed with the not-found exit code", func(t *testing.T) {
+		withDatasets([]string{"alpha", "beta"}, nil)
+		matched, err := resolveDeleteTarget(context.Background(), nil, resolved, "ghost")
+		if matched != "" {
+			t.Errorf("a no-match must not resolve a target: matched=%q", matched)
+		}
+		if ExitCodeFromError(err) != 5 {
+			t.Fatalf("exit code = %d, want 5 (no such dataset)", ExitCodeFromError(err))
+		}
+		msg := err.Error()
+		if !strings.Contains(msg, "ghost") || !strings.Contains(msg, "alpha") || !strings.Contains(msg, "beta") {
+			t.Errorf("not-found error = %q, want it to name the request and the available datasets", msg)
+		}
+	})
+
+	t.Run("nonexistent name on an empty client says so", func(t *testing.T) {
+		withDatasets(nil, nil)
+		_, err := resolveDeleteTarget(context.Background(), nil, resolved, "ghost")
+		if ExitCodeFromError(err) != 5 {
+			t.Fatalf("exit code = %d, want 5", ExitCodeFromError(err))
+		}
+		if !strings.Contains(err.Error(), "no ingested datasets") {
+			t.Errorf("empty-client error = %q, want it to say there are no datasets", err.Error())
+		}
+	})
+
+	t.Run("listing failure fails CLOSED — refuse, never delete blind", func(t *testing.T) {
+		withDatasets(nil, errors.New("mysql pod not found"))
+		matched, err := resolveDeleteTarget(context.Background(), nil, resolved, "churn")
+		if matched != "" {
+			t.Errorf("a broken list must not resolve a target: matched=%q", matched)
+		}
+		if ExitCodeFromError(err) != 4 {
+			t.Fatalf("exit code = %d, want 4 (can't confirm the target)", ExitCodeFromError(err))
+		}
+		if !strings.Contains(err.Error(), "refusing to delete") || !strings.Contains(err.Error(), "mysql pod not found") {
+			t.Errorf("fail-closed error = %q, want it to refuse and surface why", err.Error())
+		}
+	})
+}