From b86e1b2144ce2f834f9b2e39f27943b1433adb22 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Fri, 10 Jul 2026 22:13:55 +0200 Subject: [PATCH] fix(data delete): resolve the table name case-insensitively; fail closed (#1027) A mis-cased name was a silent no-op. Resolve via listDatasetsFn + EqualFold to the stored spelling (fed into teardown), exit 5 not-found, exit 4 on listing failure. Refs #1008. --- internal/cli/data_delete.go | 70 +++++++++++++++++++++-- internal/cli/data_delete_test.go | 97 ++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 internal/cli/data_delete_test.go diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 5be9f37..923cafb 100644 --- a/internal/cli/data_delete.go +++ b/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, ", ")) +} diff --git a/internal/cli/data_delete_test.go b/internal/cli/data_delete_test.go new file mode 100644 index 0000000..2d0a148 --- /dev/null +++ b/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()) + } + }) +}