Repository navigation
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
Conversation
…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.
This was referenced Oct 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Tearing down a migration lab can delete a real topic on the primary cluster, along with all its data. Checked in the code:
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 ordersmirrors into the primary's ownorders.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.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 --yesfirst has MirrorMaker 2 append the 2.8.2 corpus to it. Then run's cleanup callslabDown, which adoptsordersand deletes it from krafter, including the data it held before the lab. run's prompt doesn't list the topics, and--yesskips it anyway.kates migrate downdoes list them, but calls them lab topics.The finding's fix, a check in the prerequisites, doesn't close this on its own:
runMigrateRun(migrate_verify.go:1050-1062) callslabDownwhenever--keepis 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.Changes
cli/cmd/migrate_up.gophasePrerequisitesends with a newrefuseExistingTopics. It opens thekates-migrate-targetclient pod thatkates migrate target topicsuses (openTargetClient, askates-mm2), lists the topics withtools.ListTopics, and deletes the pod afterwards. The pod has no lab label, so no teardown would remove it.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:--topicswith names the target does not have, or--policy default.refuseOverlappingLabstays best effort.refuseOverlappingLabnow 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, throughlabDiscovery.readMirrorSpecandreplicatedTopicOf. A re-run may keep only those names. New--topicsor a new--policyunder the same lab name are checked like a new lab, so--topics orderson a lab that mirroredkates.orderscan't take over the primary'sorders. When every name is the mirror's own, the pod isn't started.readMirrorSpec, in place of its owntopicsFromPatternloop.cli/cmd/migrate_verify.gorunMigrateRunskips its whole cleanup when the run failed with a*migrate.PhaseErrorinprerequisites: nodeleteClients, no keep block, nolabDown. Nothing exists yet, and what the target holds isn't the lab's.docs/book/10-cli-reference.mdrunthat removes nothing.Tests
cli/cmd/migrate_up_test.go:TestMigrateUpRefusesATopicTheTargetHas: the target listskates.orders.upis refused with an error that nameskates.ordersand--topics, creates no namespace, and deletes thekates-migrate-targetpod;TestMigrateUpReRunSkipsTheTopicCheck: the lab's own mirror writeskates.ordersand the target has it.uppasses, and the target's topics aren't listed;TestMigrateUpReRunWithNewTopicsIsChecked: the same mirror, rerun with--topics orders, which the target has. It is refused, and nohelm upgraderuns;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 defaultandkates.orderson the target,uppasses, because the lab writessource.kates.orders.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 nodelete kafkatopic, nohelm uninstall, and no applied KafkaTopic.scriptPrerequisitesinmigrate_fake_test.goscripts the client pod and a listing without the lab's topics, since the fake fails on any command it doesn't know;TestMigrateUpPhasesAndHelmArgvexpects the listing after thekates-mm2secret check;TestMigrateUpSASLWritesTheCredentialOnStdincounts only the applies that carry a Secret, because the client pod is now applied inkafkatoo.cd kates && ./mvnw verify(Java backend suite): skipped. This PR doesn't changekates/, so the suite gives the same result as on main, where it already ran. CI's build-and-test job still starts, because itscodefilter includescli/**.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.prepare-book-render.sh, thenquarto render docs/book --to html): 39 chapters prepared, exit 0, no warnings or errors in the log. The worktree'sdocs/bookwas never touched.python3 scripts/check-book-fragments.pyon that render: all 7,458 fragment links in 38 pages land on an id.--offline --include-fragments(docs.yml's Markdown Link Check): skipped, since lychee isn't installed locally. It runs in CI, andcheck-book-fragments.pycovers the rendered fragments.helm lint charts/kates/,helm template kates charts/kates/andbash scripts/check-versions.sh:charts/is unchanged, but CI's Helm Lint job runs oncli/**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.pull_requestevents at all;docs.yml), since lychee isn't installed locally;mvnw verify, triggered bycli/**through thecodefilter), including the*ITtests, which need Docker;check-versions.sh;kates/**is unchanged;github-advanced-securityAI scan, which is red on every PR for a GitHub-side reason.Decisions
openTargetClient'skates-migrate-targetpod 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-tgtclient pod.deleteLabTopicsalso deletesmm2-<lab>-configs/offsets/statusand<alias>.checkpoints.internal. Every single-source lab, and anymigrate mirror deploywith aliassource, writes the samesource.checkpoints.internal, so checking these would forbid the concurrent single-source labs thatrefuseOverlappingLabis built to allow. Left as a follow-up.test-migrationlabel. The plan was to add it so Migration e2e runskates migrate runon 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 itslivefilter includescli/cmd/migrate*.go. It runskates migrate upon Kind, so it exercises the new prerequisite pod for real, but notrun.Deliberately not done
runrefused "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.refuseOverlappingLabreturns no own names on its best-effort paths (a kubectl error, bad JSON). The topic check then still runs, and fails closed.kates migrate downstill deletes whatever they mirrored into. This is worth a release note.upandrunnow 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 failsupclosed, where before it passed the prerequisites.openTargetClientfails after the pod is applied (a Ready timeout, say), it returns no client, andkates-migrate-targetis left behind.runMigrateTargethas the same leak today. The leftover is a scratch pod, not data.kates migrate target --keep-podsession.Landing
This PR shares no file with the earlier plan PRs (#370, #371, #372), so it can land at any time.