Skip to content

fix(migrate): up and run refuse a lab whose mirrored topics already exist on the target, and a run refused at its prerequisites removes nothing - #373

Merged
bmscomp merged 1 commit into
mainfrom
fix/audit-04-migrate-topics
Oct 17, 2026
Merged

bmscomp merged 1 commit into
mainfrom
fix/audit-04-migrate-topics

Conversation

@bmscomp

@bmscomp bmscomp commented Oct 17, 2026

Copy link
Copy Markdown
Owner

Why

Tearing down a migration lab can delete a real topic on the primary cluster, along with all its data. Checked in the code:

  • Nothing checks that a mirrored name is new on the target. migrate.NewLab (pkg/migrate/lab.go:347-356) checks only topic syntax and duplicates. refuseOverlappingLab (migrate_up.go:227-267) compares only against other labs' KafkaMirrorMaker2 CRs. The default policy is identity, so --topics orders mirrors into the primary's own orders.
  • The teardown deletes whatever it mirrored into. deleteLabTopics (migrate_lab.go:1001-1017) applies a KafkaTopic carrying the lab's labels for every replicated name. The Topic Operator adopts the existing topic, and the delete that follows removes it from the brokers.
  • Case. krafter has a topic orders, made by an app or by a Kates test through the AdminClient (TopicService.java:61, so it has no KafkaTopic). kates migrate run --from 2.8.2 --topics orders --yes first has MirrorMaker 2 append the 2.8.2 corpus to it. Then run's cleanup calls labDown, which adopts orders and deletes it from krafter, including the data it held before the lab. run's prompt doesn't list the topics, and --yes skips it anyway. kates migrate down does list them, but calls them lab topics.

The finding's fix, a check in the prerequisites, doesn't close this on its own:

  • run tears down after any failure. runMigrateRun (migrate_verify.go:1050-1062) calls labDown whenever --keep is off, including after a failure in the prerequisites. So a run the new check refuses would go straight on to delete the very topic it refused.
  • The existing overlap refusal has the same bug today. A run refused because another lab already mirrors one of its topics then deletes that other lab's topics.

Changes

cli/cmd/migrate_up.go

  • phasePrerequisites ends with a new refuseExistingTopics. It opens the kates-migrate-target client pod that kates migrate target topics uses (openTargetClient, as kates-mm2), lists the topics with tools.ListTopics, and deletes the pod afterwards. The pod has no lab label, so no teardown would remove it.
  • A lab is refused when any name in l.ReplicatedTopics(), that is, after the policy, is already listed. The error names each topic and the target, says that removing a lab deletes the topics it mirrored into, and gives the ways out: --topics with names the target does not have, or --policy default.
  • The check fails closed. If the pod can't be opened or the listing fails, the lab is refused with "cannot list the topics on …". refuseOverlappingLab stays best effort.
  • Re-runs. refuseOverlappingLab now also returns the target-side names that this lab's own KafkaMirrorMaker2 writes. It reads them, under that mirror's own policy, from its spec in the list it already fetches, through labDiscovery.readMirrorSpec and replicatedTopicOf. A re-run may keep only those names. New --topics or a new --policy under the same lab name are checked like a new lab, so --topics orders on a lab that mirrored kates.orders can't take over the primary's orders. When every name is the mirror's own, the pod isn't started.
  • The other-lab comparison reads the topics through the same readMirrorSpec, in place of its own topicsFromPattern loop.

cli/cmd/migrate_verify.go

  • runMigrateRun skips its whole cleanup when the run failed with a *migrate.PhaseError in prerequisites: no deleteClients, no keep block, no labDown. Nothing exists yet, and what the target holds isn't the lab's.

docs/book/10-cli-reference.md

  • Migration Commands, after the two fan-in refusals: one paragraph on the new refusal, the re-run rule, the client pod, the fail-closed listing, and a refused run that removes nothing.

Tests

  • New in cli/cmd/migrate_up_test.go:
    • TestMigrateUpRefusesATopicTheTargetHas: the target lists kates.orders. up is refused with an error that names kates.orders and --topics, creates no namespace, and deletes the kates-migrate-target pod;
    • TestMigrateUpReRunSkipsTheTopicCheck: the lab's own mirror writes kates.orders and the target has it. up passes, and the target's topics aren't listed;
    • TestMigrateUpReRunWithNewTopicsIsChecked: the same mirror, rerun with --topics orders, which the target has. It is refused, and no helm upgrade runs;
    • TestMigrateUpRefusesWhenTheTargetCannotBeListed, two cases: the listing fails, and the client pod is never Ready. Each is refused with "cannot list the topics", and no namespace is created;
    • TestMigrateUpChecksTheNameOnTheTarget: with --policy default and kates.orders on the target, up passes, because the lab writes source.kates.orders.
  • New in cli/cmd/migrate_run_test.go: TestMigrateRunRefusedAtPrerequisitesRemovesNothing, with --yes, two cases: the topic is on the target, and another lab mirrors it. Each is refused, with no delete kafkatopic, no helm uninstall, and no applied KafkaTopic.
  • Updated:
    • scriptPrerequisites in migrate_fake_test.go scripts the client pod and a listing without the lab's topics, since the fake fails on any command it doesn't know;
    • TestMigrateUpPhasesAndHelmArgv expects the listing after the kates-mm2 secret check;
    • TestMigrateUpSASLWritesTheCredentialOnStdin counts only the applies that carry a Secret, because the client pod is now applied in kafka too.
  • The backend is unchanged. The full backend suite ran on main and passed.
  • Run locally, all passing unless marked skipped:
    • cd kates && ./mvnw verify (Java backend suite): skipped. This PR doesn't change kates/, so the suite gives the same result as on main, where it already ran. CI's build-and-test job still starts, because its code filter includes cli/**.
    • cd cli && go test ./...: ran before and after the fix. The final run on a695734 passed in every package (exit 0). CI runs it with -p 1 -race -count=1.
    • cd cli && go vet ./...: rerun on the final tree, clean.
    • cd cli && golangci-lint run --new-from-rev=origin/main ./...: the first run failed with 6 SA1012 issues, all on this PR's new test lines (a nil Context). Fixed and amended; the rerun reports 0 issues.
    • cd cli && govulncheck ./...: no vulnerabilities found. It ran before the lint fix, which changed only test lines, and govulncheck doesn't scan those.
    • bash scripts/check-cli-style.sh: no hardcoded hex colors and no raw ANSI escapes, on the final tree.
    • bash scripts/check-cli-compat.sh: all terminal-compatibility checks pass (piped output, NO_COLOR, TERM=dumb, --plain, KATES_ASCII, no-TTY lab), on the final tree.
    • scripts/check-book-style.sh: the style checks and the structure ratchet pass (38 pages within the baseline).
    • scripts/gen-version-matrix.sh --check: the appendix-d version matrix is in sync.
    • scripts/gen-chart-table.sh --check: the README chart table is in sync.
    • python3 scripts/check-book-contrast.py: all 170 colour pairs meet WCAG AA.
    • scripts/check-mermaid.sh --quiet: 156 diagrams, 0 invalid.
    • A scratch-copy HTML render of the book (prepare-book-render.sh, then quarto render docs/book --to html): 39 chapters prepared, exit 0, no warnings or errors in the log. The worktree's docs/book was never touched.
    • python3 scripts/check-book-fragments.py on that render: all 7,458 fragment links in 38 pages land on an id.
    • lychee --offline --include-fragments (docs.yml's Markdown Link Check): skipped, since lychee isn't installed locally. It runs in CI, and check-book-fragments.py covers the rendered fragments.
    • helm lint charts/kates/, helm template kates charts/kates/ and bash scripts/check-versions.sh: charts/ is unchanged, but CI's Helm Lint job runs on cli/** changes, so I ran its first steps. All pass, as on main.
    • make check-help, actionlint, shellcheck, yamllint: not triggered, since this PR changes no Makefile, .github/**, scripts/** or YAML file.
  • Runs only in CI:
    • the book PDF render, which doesn't run on pull_request events at all;
    • lychee's Markdown Link Check (docs.yml), since lychee isn't installed locally;
    • Java build-and-test (mvnw verify, triggered by cli/** through the code filter), including the *IT tests, which need Docker;
    • the rest of the Helm Lint job, past check-versions.sh;
    • the native image build: not triggered, since kates/** is unchanged;
    • the Kind/integration tests and the Docker image builds: not triggered by these paths;
    • the github-advanced-security AI scan, which is red on every PR for a GitHub-side reason.

Decisions

  • Pod: reuse openTargetClient's kates-migrate-target pod and delete it after the check, rather than a lab-named pod. It's less code, and it keeps the prerequisites' fake apart from the lab's -tgt client pod.
  • MirrorMaker 2's internal topics are out of scope. deleteLabTopics also deletes mm2-<lab>-configs/offsets/status and <alias>.checkpoints.internal. Every single-source lab, and any migrate mirror deploy with alias source, writes the same source.checkpoints.internal, so checking these would forbid the concurrent single-source labs that refuseOverlappingLab is built to allow. Left as a follow-up.
  • run's confirmation prompt doesn't list the topics. With the refusal in place, the prompt is no longer the safeguard.
  • The test-migration label. The plan was to add it so Migration e2e runs kates migrate run on both legs, plus the default-policy variant, against a live target. The repo has no such label, so it isn't applied, and Migration e2e doesn't run on this PR. Metric contract (live scrape) still runs, because its live filter includes cli/cmd/migrate*.go. It runs kates migrate up on Kind, so it exercises the new prerequisite pod for real, but not run.

Deliberately not done

  • Refusals at resolve time aren't part of the run skip. The book paragraph says a run refused "by this check or because another lab already mirrors one of its topics" removes nothing. It doesn't cite the two fan-in refusals above it, because those come at resolve time, before any phase runs, so the new skip never sees them.
  • A lab whose MirrorMaker 2 CRs can't be listed is never taken for a re-run. refuseOverlappingLab returns no own names on its best-effort paths (a kubectl error, bad JSON). The topic check then still runs, and fails closed.
  • Labs created before this change are still unprotected. kates migrate down still deletes whatever they mirrored into. This is worth a release note.
  • The internal MM2 topics are not checked (see Decisions). This is a follow-up.
  • Known costs of the new pod:
    • Every up and run now starts a client pod (the Strimzi Kafka image) before anything else. That is slower on a cold node. A pod Ready timeout, a NetworkPolicy or a credential problem now fails up closed, where before it passed the prerequisites.
    • If openTargetClient fails after the pod is applied (a Ready timeout, say), it returns no client, and kates-migrate-target is left behind. runMigrateTarget has the same leak today. The leftover is a scratch pod, not data.
    • The check's delete removes the pod of a concurrent kates migrate target --keep-pod session.
  • A quick re-run can be refused. Kafka deletes topics asynchronously, so an identity-policy run right after a teardown may be refused while the old topic is still listed.

Landing

This PR shares no file with the earlier plan PRs (#370, #371, #372), so it can land at any time.

…xist on the target, and a run refused at its prerequisites removes nothing

Nothing checked that a lab's mirrored topic names were new on the
target. Under the default identity policy, --topics orders mirrors into
the primary's own orders. When krafter already had orders, made by a
Kates test through the AdminClient (so with no KafkaTopic) or by an
app, `kates migrate run --from 2.8.2 --topics orders --yes` had
MirrorMaker 2 append the 2.8.2 corpus to it. Then run's teardown
adopted orders as a lab topic and deleted it from krafter, with the
data it held before the lab.

The prerequisites of up and run now list the target's topics as
kates-mm2, from the kates-migrate-target client pod that
`kates migrate target topics` uses, and delete the pod afterwards. A
lab is refused when any of its replicated names, after the policy, is
already listed. The refusal names each topic and the ways out:
--topics with new names, or --policy default. The check fails closed:
a lab whose names cannot be listed is not created. A re-run, whose own
KafkaMirrorMaker2 already exists, may keep only the topics that mirror
already writes, read from its spec in the list the overlap check
already fetches; it skips the listing when it asks for nothing more.
New --topics or a new --policy under the same lab name are checked like
a new lab, so `--topics orders` on a lab that mirrored kates.orders
cannot take over the primary's orders.

run used to tear the lab down after any failure, including one in the
prerequisites, so this refusal would have been followed by the deletion
of the very topic it refused. The existing refusal of a lab whose
topics another lab already mirrors had that bug too: the teardown then
deleted the other lab's topics. A run that fails in its prerequisites
now removes nothing, since it has created nothing yet.

The MirrorMaker 2 internal topics a teardown also deletes are not
checked: every single-source lab writes the same
source.checkpoints.internal, so checking them would forbid concurrent
labs. Labs created before this change are not protected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant