feat(kafka): add topic-data backup/restore example and e2e roundtrip - #3580

Actionable comments posted: 7

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@examples/backups/kafka/01-create-strategy.sh`:
- Around line 193-202: Update the Fidelity bullet in the Kafka backup README to
explicitly state that null-key records are replayed with the literal
four-character key “null” rather than a null key, in addition to the existing
partition-change caveat.
- Around line 120-124: Update the s3() wrapper to stop passing AWS credentials
via curl’s --user command-line argument; provide them through a curl
configuration file on stdin while preserving the existing SigV4, TLS, and
forwarded-argument behavior.

In `@examples/backups/kafka/05-create-backupjob.sh`:
- Around line 36-38: Persist the resolved backupRef name after retrieving it in
the backup job script, using the established variable or state mechanism
consumed by later steps. Update downstream steps 06 and 07 to reference the
cached backupRef instead of BACKUPJOB_NAME, while preserving the existing
empty-value validation and success logging.

In `@examples/backups/kafka/06-restore-in-place.sh`:
- Around line 37-38: Update the backupRef.name value in the restore
configuration to use the resolved Backup name from
BackupJob.status.backupRef.name rather than BACKUPJOB_NAME, preserving the
reference to the created Backup resource.

In `@examples/backups/kafka/07-restore-to-copy.sh`:
- Around line 50-51: Update the backupRef.name value in the restore manifest to
reference the resolved Backup resource name rather than BACKUPJOB_NAME, matching
the correction made in step 06.

In `@examples/backups/kafka/cleanup.sh`:
- Line 14: Update the cleanup command to delete the Backup using its resolved
Backup resource name rather than assuming it matches BACKUPJOB_NAME; reuse the
variable or lookup established by the script’s resolution flow, while preserving
the namespace and ignore-not-found options.

In `@hack/e2e-chainsaw/kafka/chainsaw-test.yaml`:
- Around line 200-205: Update the KAFKA_IMAGE extraction in the backup setup to
parse the multi-line STRIMZI_KAFKA_IMAGES version-to-image map by selecting one
explicit mapping and extracting its image value. Validate that the result is
exactly one non-empty image value, and retain the existing failure path when
parsing does not produce a valid image.

---

Nitpick comments:
In `@examples/backups/kafka/00-helpers.sh`:
- Around line 55-82: Update wait_for_field to accept an optional
terminal-failure value in addition to desired, and return failure immediately
when the current field matches it. Preserve existing success and timeout
behavior, and keep the argument optional so current callers remain compatible
while Kafka BackupJob/RestoreJob steps can pass Failed.

In `@examples/backups/kafka/03-create-bucket.sh`:
- Around line 62-68: Update the .bucket-info.env cache generation in the heredoc
to emit shell-safe quoted literals for every S3 value, using printf %q before
writing S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT, S3_REGION, and S3_BUCKET.
Keep create_s3_secret’s sourcing behavior unchanged while ensuring values
containing spaces, quotes, dollar signs, or semicolons remain data.

Verdict

LGTM with non-blocking notes.

This is example scripts plus one appended Chainsaw Test. Nothing here touches a chart, package, controller, CRD, or values contract, so the upgrade / SSA / CRD-transition / vendored-rename / config-matrix regression classes don't apply, and I checked that against the envelope: 0 charts, 0 Go packages, 0 packages/ files, and the "make generate produces no diff" claim holds up. The scripts fail closed on the paths that matter, cleanup.sh deletes the right Backup object (Backup.Name == BackupJob.Name, internal/backupcontroller/jobstrategy_controller.go:348), and the e2e actually asserts on record count rather than passing vacuously.

The notes below are accuracy and coverage refinements. None of them block merge. Two are worth calling out first, though: the ^_ over-match and the unvalidated explicit-topic list both let a backup example silently produce an incomplete backup that still reports success, which is the one failure a backup example must not teach. Both are one-line fixes.

Findings

[MINOR] 01-create-strategy.sh:167, grep -v '^_' silently drops legal user topics in the default "all topics" mode

Kafka's internal topics are __consumer_offsets and __transaction_state, both double-underscore. This filter also drops every single-underscore name, so a user topic like _schemas (Confluent Schema Registry) or _events holds user data yet is silently excluded from the documented "every non-internal topic" default. The comment at line 155 ("bookkeeping topics start with _") carries the same imprecision. Use grep -v '^__'.

[MINOR] 01-create-strategy.sh:156-157, an explicit topics: value naming a missing topic yields a successful empty backup

The explicit-TOPICS branch takes the list verbatim with no existence check, unlike the empty branch right below it, which is deliberately fail-closed. kafka-get-offsets.sh on a nonexistent topic prints nothing and exits 0, so the loop writes no manifest entry, uploads a near-empty tarball, and the BackupJob reports Succeeded. A typo in the BackupClass topics parameter produces a "backup" that restores nothing, and you find out at restore time. (I reasoned the exit-0-on-missing-topic behaviour from the CLI, I did not run it here.) A pre-flight check that each requested topic exists, failing loudly otherwise, closes this.

[MINOR] 01-create-strategy.sh:139-140, one S3 key per app: a later backup overwrites the earlier one, but old Backup CRs stay Ready

The object key is ${SRC}/kafka-topics.tar, scoped only by app name. Take backup A, produce more data, take backup B, then run a RestoreJob referencing A: the restore succeeds and hands back B's data, because the key is the same. Nothing marks the older Backup CR as superseded. This matches the nats sibling's single-key pattern and may be forced by the engine (the template gets no per-backup identity in backup mode, .Backup is nil there), so I read it as a documentation gap rather than a code defect. The README should say that only the latest backup of an app is actually restorable through this strategy.

[MINOR] 06-restore-in-place.sh:22, the topic-deletion wait can false-positive on a transient broker error and hollow out what the e2e proves

Inside the Pod snippet (set -eu, no pipefail) the check is ! kafka-topics.sh ... --list | grep -qx "$TOPIC". If --list itself fails, its output is empty, grep exits 1, and the negation declares the topic deleted while it's still there with all its records. Restore then replays into the live topic, the count reaches 60, and because the verification is >= (next finding) it still passes. So the round-trip test can go green without ever exercising topic recreation. Add set -o pipefail to that snippet, or check --list's own exit status before trusting its output.

[MINOR] 06-restore-in-place.sh:49, 07-restore-to-copy.sh:65, the round-trip check is >= and total-only

Verification passes when count >= MESSAGE_COUNT over a partition-summed offset delta. That can't catch over-restore (duplicate records: a restore Pod retried mid-replay re-runs the whole producer, and --create --if-not-exists skips the topic, so records double) and it doesn't assert per-partition distribution. The PR's headline claim is partition-preserving replay ("8/11/11 distribution preserved"), but nothing in the scripts or the test asserts per-partition counts, so the murmur2 repartitioning this PR is about is never exercised by CI. Consider exact equality where the flow uses a fresh/empty topic, plus a per-partition kafka-get-offsets assertion.

[MINOR] 01-create-strategy.sh:189-194, the drain assumes end - begin equals the consumable-record count

--max-messages $((end - begin)) holds only for a non-compacted, non-transactional topic. On a compacted topic (sparse offsets) or one written by transactional producers (control markers occupy offsets but are never delivered), fewer records than end-begin are consumable, so kafka-console-consumer idles until --timeout-ms 120000 and exits non-zero, killing the Pod under set -e and failing the BackupJob. That's fail-closed, no silent partial tarball, which is the right direction. But the topic_message_count comment at 00-helpers.sh:190 ("exact ... even for a compacted or truncated topic") is about the offset-math helper, not the drain, and could mislead. A one-line README limitation that compacted/transactional topics are out of scope for this logical path settles it.

[MINOR] README.md:21 / 01-create-strategy.sh:227-232, null-key fidelity is understated

The README says null-key records "are not guaranteed to land in their original partition." It's worse than that. print.key=true renders a null key as the literal token null, and replay with parse.key=true turns it into the 4-byte string key "null". So a null-key record doesn't just move partition, it gains a non-null key, and every originally-null-key record collapses onto one partition; null values (tombstones) similarly become ordinary payloads. The demo uses keyed records only, so this isn't exercised, but the limitation text should say null-key/null-value records are not round-tripped faithfully. The pinned 3.8 image supports the null.marker property (KIP-810) if faithful round-tripping is wanted later.

[NIT] 01-create-strategy.sh:128-129, hand-rolled endpoint parse mishandles IPv6 / path-carrying endpoints

HOST="${HOSTPORT%%:*}" mis-splits an IPv6 literal like [::1]:8333, and any endpoint carrying a path prefix loses it, since OBJ_URL is rebuilt from the bare host. The BucketInfo host:port shape works; other legal endpoint forms don't. Demo-scope, worth a comment.

Claim mismatches

[PARTIAL] "every partition is captured as of the same instant" / "a coherent point-in-time view" (README.md:12, PR body). The --time -1/--time -2 reads sit inside the for t in $TLIST loop (01-create-strategy.sh:172-178), so each topic is frozen when its iteration starts, after the previous topics have drained. The cut is atomic within a topic, not across topics. Kafka gives no cross-topic guarantee anyway, but the prose generalizes to "every non-internal topic." Narrow it to per-topic consistency.

[PARTIAL] "recreates each topic with only its original partition count and replication factor" (README.md:18). The partition count is original (from the manifest). The replication factor is not: restore uses the static REPLICATION_FACTOR parameter, default 1 (01-create-strategy.sh:53-54, applied at :222), and the source RF is never captured. Restoring into a multi-broker cluster without setting the parameter recreates topics at RF=1, a silent durability downgrade. Say "the configured replication factor (default 1)", not "its original replication factor."

[PARTIAL] "partition distribution preserved" (PR body). Correct in the code path (topic recreated with original partition count, deterministic murmur2), but not asserted by the shipped e2e, which checks total record count only.

Caveats

• Mechanical fail-open sweep ran on all 10 shell files. Every || true / 2>/dev/null hit was verified non-gating: 01-create-strategy.sh:167 handles grep's no-match exit only (the deciding --list is a separate command under set -e); 00-helpers.sh:77,110,116,126 are poll-loop reads; 00-helpers.sh:195-196 || exit 0 returns empty and every caller rejects a non-numeric count (06:48, 07:64); 06:20 --delete ... || true is backstopped by the disappearance wait-loop. Clean, with the one exception that the wait-loop's own --list pipe is unguarded (finding above).
• shellcheck SC2016 (00-helpers.sh:175,194; 06:19) and SC1091 (dynamic source) are false positives: the single-quoted bodies are Pod snippets whose $VAR must stay unexpanded until they reach the in-Pod bash, and the sourced path resolves at runtime. No action.
• job-service-label-overlap: no overlap. The throwaway CLI pods use kubectl run (run=kafka-cli-<rand>), which doesn't match the Strimzi bootstrap Service selector; the strategy Job pod labels are stamped by the controller, out of this PR.
• Sound on independent check: .bucket-info.env is gitignored (examples/backups/*/.bucket-info.env); empty partitions are handled (no data file, manifest still records the partition count); a tab inside a value round-trips because parse.key splits at the first separator only; the to-copy source-keyed S3 read via .Backup.ApplicationRef.Name is correct; the curl 7.76.1 --aws-sigv4 port-stripping workaround is internally consistent.

Recommended follow-ups

• grep -v '^__' and a topic-existence pre-check are the two cheapest, highest-value fixes: they stop the example from silently teaching a lossy backup.
• Add a per-partition offset assertion (and exact equality where the flow uses a fresh topic) to the e2e, so the partition-preserving-replay claim is guarded and duplicate-on-retry is caught.
• Note the latest-only-restorable and compacted/transactional-out-of-scope limitations in the README.

Thanks for this — the value here is bigger than the title suggests. The generic Job strategy has had unit tests and exactly one worked example (examples/backups/nats/) but no end-to-end coverage at all, and no Chainsaw suite drives it. kafka-2-backup-roundtrip is therefore the first e2e that exercises the generic driver for real: template rendering including .Mode/.Parameters/.Backup.ApplicationRef.Name, the parameter round-trip through the Backup's driverMetadata, and the to-copy retarget. The run bears it out — 452s, and the log shows 30 records seeded, backed up, the topic deleted, restored in place, then restored into a second cluster.

The design call also looks right to me. A logical frozen-end-offset drain is not the highest-fidelity Kafka backup available, but the README says so plainly and up front rather than burying it: no topic configs, no ACLs, no SCRAM users, no consumer-group offsets, offsets reassigned on restore, no headers or timestamps, and an explicit "for a faithful, offset-preserving Kafka backup, prefer a volume-snapshot strategy over this logical one". An example gets copied, so being told where it bites is most of what makes it safe to ship. The comment density throughout is well above average and the reasoning held up everywhere I cross-checked it against the controller.

Two things I'd like changed before merge, both small. The rest below are comments rather than blockers.

Blocking

1. Restore is not idempotent, and the assertion cannot see the failure mode. 01-create-strategy.sh:221-233 recreates topics with --create --if-not-exists (existing topics are left alone) and then replays every partition file unconditionally, so restoring into a topic that still holds data appends rather than replaces. That matters more than it looks, because buildJobStrategyBatchJob in internal/backupcontroller/jobstrategy_controller.go hard-codes backoffLimit: 2 — a restore Pod that dies partway through replay is retried, and the retry replays from the beginning. Both verifications then assert if (( count < MESSAGE_COUNT )), strictly less-than, so a restore that produced 45 or 60 records instead of 30 prints "In-place restore verified" and passes (06-restore-in-place.sh:49, 07-restore-to-copy.sh:65). Could we make both comparisons -eq, and add a line to the README's limitations saying restore appends into a non-empty topic and is safe only against an absent or empty one?

2. The roundtrip proves record counts, never record content. topic_message_count (00-helpers.sh:192) sums end - begin offsets per partition, and nothing ever consumes the records back. Meanwhile the README makes three fidelity claims the test does not check: keys survive, keyed records land in their original partition via the murmur2 partitioner, and per-partition order is preserved. A restore that replayed 30 records of garbage, or scrambled every key, passes today. The seeded sentinels are already perfect for this (k-<n> / order-<n>) — consuming the topic once and diffing a sorted list would close the gap in a few lines, and given that data fidelity is the entire subject of the example it seems worth having.

Non-blocking, roughly by weight

Helper Pods will time out on a first run outside CI. kafka_run (00-helpers.sh:157) uses kubectl run --restart=Never --rm -i, and kubectl run with attach waits for Running under --pod-running-timeout, whose default is 1m, with no override and no retry. CI never hits it because the Chainsaw test resolves KAFKA_IMAGE from the running strimzi-cluster-operator, so the image is already cached on the nodes. A user following the README gets the default quay.io/strimzi/kafka:0.45.0-kafka-3.8.0 pulled cold, which routinely takes longer than 60s, and step 04 dies with timed out waiting for the container to run before seeding anything. --pod-running-timeout=5m on the four call sites would cover it.

Helper Pods carry no securityContext at all. The passing CI log contains the evidence three times: Warning: would violate PodSecurity "restricted:latest": allowPrivilegeEscalation != false ..., unrestricted capabilities ..., runAsNonRoot != true ..., seccompProfile ... for each kafka-cli-* Pod. Warn-only in the e2e cluster, so the run stays green, but in a tenant that enforces restricted the demo cannot run at all. The strategy Pod is in better shape and its comment already acknowledges the runAsNonRoot gap; the helper Pods get no mention. Either --overrides with a securityContext, or a README line naming the requirement.

Null-key records come back with the literal key null. The dump uses --property print.key=true and Kafka's default formatter renders a null key as the string null; the replay reads that back through parse.key=true as a four-byte key (01-create-strategy.sh:190-192, 231-232). So such a record does not merely land in an unpredictable partition, which is what the README currently says — its key changes from null to "null", which also changes compaction semantics downstream. Worth restating that bullet in those terms.

No bound on the drain's local disk usage. WORK=/tmp/kafka-dump plus /tmp/kafka-topics.tar materialises the whole topic twice on the Pod's writable layer, and resources (01-create-strategy.sh:236-242) declares cpu and memory but no ephemeral-storage request or limit. A tenant copying this for a large topic fills the node's ephemeral storage and evicts unrelated Pods before the backup fails. A sizing caveat in the limitations section would earn its place.

Every backup of an app overwrites the previous one. The S3 key is ${SRC}/kafka-topics.tar with no run identifier (01-create-strategy.sh:139), so N Backup objects all resolve to the same tarball and restoring an older Backup silently restores current data. This is largely forced by the driver — at backup-render time the context carries only Application/Release/Mode/Parameters, and the Backup artifact is created after the Job completes, so the Pod has no unique handle — and the NATS example has the same shape. Still belongs in the limitations rather than being discovered later.

set -e fragility in the partition-count loop. [ "${mp}" -ge "${parts}" ] && parts=$((mp + 1)) as the last statement of a while read body (01-create-strategy.sh:217-220): if the final iteration's test is false, the loop returns non-zero and set -eu kills the restore Pod with no message. It cannot fire today because kafka-get-offsets emits partitions in ascending order, but if ... then ... fi is the same length and closes it.

The e2e never exercises the all-topics path. The BackupClass pins topics: "${TOPIC}" (02-create-backupclass.sh:33), so the --list-and-filter branch — exactly what "fix(kafka): fail closed when listing topics for an all-topics backup" addresses — has no coverage from the test that ships with it.

Smaller things: the hack/e2e-chainsaw/.chainsaw.yaml header enumerates the Tests that override the namespace to tenant-root as "postgres-2-, mariadb-2-, clickhouse-2-" and could pick up kafka-2-; every other script-based backup example ships the 90/91/92 scenario docs (clickhouse, nats, etcd, foundationdb) and this one has only the README, which may well be deliberate; and the default KAFKA_IMAGE pins 0.45.0 while the vendored operator chart is strimzi-kafka-operator 0.45.1-rc1, where the Chainsaw test already shows the drift-free way to resolve it.

On CI cost, for the record, since a backup roundtrip is the kind of thing that quietly grows the suite: hack/select-e2e.sh maps both examples/backups/kafka/* and hack/e2e-chainsaw/kafka/* to the single kafka suite, Chainsaw runs parallel: 1, and the measured 452s sits well inside the declared 40m budget, which is justified in-line as convention 6 asks. That reads as appropriately gated to me. Test hygiene follows the mariadb precedent closely and correctly — pre-clean before the run so a leftover Succeeded BackupJob in the shared namespace cannot instantly satisfy the wait, cleanup in an always-reached finally, wait_hr_ready failing fast on Stalled=True, wait_for_field failing fast on the terminal Failed phase, and a catch block selecting the strategy Pods by the real job.strategy.backups.cozystack.io/mode constant. The note in 05-create-backupjob.sh explaining why there is no intermediate "wait Running" gate is exactly the right instinct. I did not find anything I would expect to go red intermittently in CI.