Broker CR reconciler

[hidalgopl]

operator: Broker CRD and reconciler

Introduces the Broker custom resource and a fully functional BrokerReconciler: per-broker pod/PVC lifecycle as a replacement for StatefulSet-managed brokers, per the Broker CRD RFC. It supersedes the log-only stub merged in #1631.

This PR deliberately contains only the Broker controller side. The Cluster-controller side — roll-grant issuance, STS→Broker migration/rollback with safeguards, and central-config restart marking — is a follow-up PR, so this one can be reviewed standalone.

Everything stays opt-in: the CRD is only installed with crd --experimental, and the controller only runs with --enable-broker (default false). No behavior changes for existing installations.

What the reconciler does

Each Broker CR manages exactly one pod and its PVCs. The reconcile chain runs: PVC ensure → pod ensure/adoption → PVC adoption → PV-affinity remediation → pod rotation → registration verification → decommission, followed by an unconditional status sync.

Roll-grants (RFC Q4) — the Broker controller is a consumer only of the operator.redpanda.com/roll-grant annotation (<config-checksum>/<unix-deadline>). Disruptive actions (pod rotation, PV remediation) require a valid grant whose checksum matches the desired pod template; the TTL is a restart-safety valve. Grant issuance (single writer, serialized rolls) belongs to the cluster controller in the follow-up.

Pod ensure is not grant-gated (RFC Q5) — creation is non-disruptive, bootstrap needs all pods in parallel, and an expired grant must never strand a broker between rotation's delete and recreate.

Rotation — a pod is outdated when its config-checksum or restart-requiring cluster-config-version annotation drifts from the desired template (the kubectl rollout restart pattern; the controller never patches pods). With a grant, the broker is drained via maintenance mode (leadership drain polled at 10s so serialized fleet rolls don't crawl), then the pod is deleted and recreated from the template.

Identity continuity (RFC rolling step 3) — registration is re-verified via the admin API on every pass with a ready pod. A pod that re-registers under a different node_id (data dir lost) is refused: the conflict is surfaced as BrokerRegistered=False/IdentityChanged and the broker goes Stuck for an operator decision, instead of silently adopting the new identity.

Decommission (RFC Q2) — only ever runs on explicit spec.decommission: true, never on raw CR deletion. Includes a last-broker guard (blocks with Stuck), recommission when the intent is unset mid-flight, a sticky terminal Decommissioned phase, and pod/PVC cleanup on completion.

Deletion semantics (RFC Q2) — a finalizer distinguishes intents: deletion with spec.decommission decommissions first and blocks while guarded; raw deletion while the owning cluster is alive releases the pod and PVCs (ownerRefs stripped — the broker keeps serving, accidental deletion self-heals); during cluster teardown the propagated operator.redpanda.com/broker-deletion-policy annotation decides cascade (default) vs orphan. On any uncertainty the answer is orphan — releasing is recoverable, destroying data is not.

Dead-node PV remediation — when a pod is stuck Pending on volume node-affinity against a node that no longer exists (past --unbind-pvcs-after, and only with a grant), the affected PVs are patched to Retain, PVCs and pod deleted, and everything recreated on a live node. The controller waits for the old PVC to fully terminate before recreating — recreating early pins the Terminating PVC via pvc-protection and deadlocks permanently. ExistingClaims are never remediated (externally owned; admin must handle). The PV identification logic is exported from the pvcunbinder as DeadNodePVCs.

Adoption — orphaned pods (e.g. from an orphan-deleted StatefulSet) and spec.storage.existingClaims PVCs are adopted via controllerRef, enabling the STS→Broker migration path without restarting brokers. Pods owned by another controller put the broker in shadow mode (Pending).

Status — conditions are produced by the generated statuses.NewBroker() helpers from statuses.yaml: Ready, PodScheduled, StorageBound, BrokerRegistered, ConfigSynced, with Quiesced and the Stable roll-up derived by the generator. A pending rotation makes the broker not-Stable via ConfigSynced=False. Status writes are skipped when nothing changed (RFC Q11). status.phase provides the coarse view: Provisioning, Running, Stuck, Decommissioning, Decommissioned.

Pod naming — pods keep StatefulSet ordinal names: <cluster>-<ordinal> for the default pool, <cluster>-<pool>-<ordinal> for named pools. When clusterRef names a NodePool (which carries the pool name, not the cluster), the cluster half comes from the required cluster.redpanda.com/cluster-name label — every Broker creator must set it.

Commits

1. API — Broker CRD types and helpers (PodName, PodOutdated, BuildPod), Broker conditions in statuses.yaml + generated helpers, roll-grant/deletion-policy feature flags with parse/format round-trip tests, generated CRD/deepcopy/applyconfig/docs.
2. Reconciler — the controller described above, --enable-broker wiring, DeadNodePVCs extraction from pvcunbinder, RBAC.
3. Integration tests — 10-test BrokerControllerSuite on a real k3d cluster: bootstrap, adoption, grant-gated rotation (with and without grant), identity continuity, decommission + last-broker guard + revive-after-decommission, finalizer paths (decommission / raw-deletion release), PV-affinity remediation with a real node kill (runs serially — it deletes a shared k3d node — and always restores the node via cleanup, even on failure).
4. Acceptance — broker-crd-adoption.feature: adopt pods from an orphan-deleted STS on a live cluster, verify admin-API membership, grant-gated env rotation, decommission down to 2 brokers.

Verification

• Unit tests, golangci-lint (0 issues), and chart golden tests pass; generated files are in sync with task generate.
• The integration suite and the adoption acceptance scenario passed locally on fresh k3d clusters during development; the PV-remediation deadlock found by that run is fixed in this series (the Terminating-PVC wait above).

Out of scope / follow-up

The companion branch adapts the vectorized Cluster controller to issue roll-grants (single writer), orchestrate STS→Broker migration and rollback behind verification safeguards (rollout complete, cluster healthy, all pods ready), and mark Brokers for central-config restarts via the pod-template annotation. It lands as a separate PR once this one merges.

🤖 Generated with Claude Code

[secpanda]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[RafalKorepta]

Review panel — Broker CR feature stack (4 commits)

Reviewed b7827ed4 → 3426d2ae with a four-reviewer panel per commit (staff-eng correctness/design, docs, security, and a Codex adversarial pass). Findings are deduplicated across reviewers and sorted by severity.

Framing: this is an experimental feature gated behind --enable-broker (default false) with no owning cluster controller wired up yet, so none of this is reachable in a default deployment. Severities are "before this feature ships / gets enabled," not "live production regression."

Verdicts by commit

Commit	staff-eng	docs	security	Codex
b7827ed4 CRD API + flags	approve-with-nits	approve-with-nits	approve-with-nits	request-changes
98bf2813 reconciler	request-changes	approve-with-nits	approve-with-nits	request-changes (blocker)
68efddcb integration tests	approve-with-nits	approve-with-nits	approve	request-changes
3426d2ae acceptance tests	approve-with-nits	approve-with-nits	approve	request-changes

---

🔴 Blocker

B1. Admin-API calls hardcode LocalCluster while the controller reconciles provider clusters — operator/internal/controller/redpanda/broker_controller.go:672,701,928,1032,1041
The For(&Broker{}) watch engages provider clusters (WithEngageWithProviderClusters(true), lines 76–77). K8s reads/writes correctly use r.Manager.GetCluster(req.ClusterName) (line 142), but every admin-API call goes through RedpandaAdminClient(ctx, broker), which is hardcoded to mcmanager.LocalCluster (operator/pkg/client/factory.go:340). Failure: a Broker reconciled from a provider cluster manages Pods/PVCs in that provider cluster but issues register / decommission / maintenance-mode / node-id-resolution against the local cluster's Redpanda — cross-cluster mistargeting, potentially decommissioning/rolling the wrong cluster. Fix: thread req.ClusterName and call the already-existing RedpandaAdminClientForCluster(ctx, broker, req.ClusterName).

---

🟠 Major

M1. Decommission ordering / nil BrokerID → broker stalls in Decommissioning forever — broker_controller.go:396-403,463-471 (flagged independently by staff-eng and Codex)
If spec.decommission=true is set while Status.BrokerID is nil, reconcileBrokerRegistration early-returns (never resolves the ID) and reconcileDecommission only sets phase, never calling the admin API. The broker keeps running as a full cluster member while the CR reports Decommissioning indefinitely. The deletion path already resolves the ID live (line 775-782); the spec path should too.

M2. PV-affinity remediation & pod rotation run before decommission and don't skip decommissioning brokers — broker_controller.go:319-345,347-386 (staff-eng + Codex)
These gate only on outdated+grant, not Spec.Decommission. A decommissioning broker that goes outdated (or has a Pending PV-affinity pod) gets its pod deleted / storage remediated mid-drain — clearing Status.BrokerID and interrupting partition movement. The sibling reconcilers (reconcilePVCs, reconcilePod, reconcileBrokerRegistration) all early-return on decommission; these two should too.

M3. BrokerDeletionPolicy.Parse accepts any string → typo silently falls to destructive cascade (data loss) — operator/pkg/feature/flags.go:100 (Codex + re-run staff-eng, independently)
Parse is the identity function; the consumer checks == "orphan" exactly (broker_controller.go:870). A user setting orphaned/Orphan/retain to preserve data instead hits the cascade branch → finalizer removed without stripping PVC ownerRefs → GC deletes the data, opposite of intent, no error. Fix: validate against {cascade,orphan} so unknown values degrade to the documented default via the existing Get→Default error path.

M4. Raw Broker deletion with pod already gone removes finalizer without releasing PVC ownerRefs — broker_controller.go:756-760,805-819 (Codex)
Violates the raw-delete self-heal contract: on raw CR deletion when the pod is missing, PVCs keep their controller ownerRef, so CR deletion cascades and GC deletes them → data loss. Release PVCs even when the pod is absent.

M5. Orphan-pod adoption overwrites live checksum annotation with the desired value — broker_controller.go:270-277, operator/api/redpanda/v1alpha2/broker_helpers.go:43-49 (Codex)
Adoption stamps desired rotation annotations onto the existing pod; PodOutdated only compares annotations, so a stale pod adopted after raw deletion is marked current and never rotated, while status reports synced. Preserve live annotations during adoption.

M6. resolveBrokerID only handles the FQDN-prefix address form — broker_controller.go:1040-1056 (Codex)
It does strings.Split(InternalRPCAddress, ".")[0] == podName, missing host:port, bare-host, and pod-IP fallback that existing controller code handles. In bare-IP / non-FQDN advertise modes, a ready broker never gets a BrokerID, so registration stays unverified and decommission stalls. Reuse the existing brokerID-for-pod logic (net.SplitHostPort, IP fallback, ambiguity handling).

M7. PodName input-validation gaps produce collisions / malformed names — broker_helpers.go:27-36 (Codex + re-run staff-eng + docs)
Two related hazards, neither validated (no webhook/CEL): (a) networkIndex is optional and nil→0, so a Broker with networkIndex:0 and one omitting it both resolve to <cluster>-0 → pod-ownership collision; (b) on the NodePool clusterRef path, a missing cluster.redpanda.com/cluster-name label yields -<pool>-<index> (leading dash) → invalid DNS name, pod create rejected, error loop. Fix: make networkIndex required+Minimum=0, and validate/resolve the cluster name instead of reading a bare label.

M8. --unbind-pvcs-after silently gates the advertised dead-node PV remediation, defaults to 0, undocumented — operator/cmd/run/run.go:204 (docs)
The changelog advertises "dead-node PV remediation" as a Broker capability, but remediatePVAffinity no-ops when UnbindPVCsAfter <= 0 (the default). An operator who sets --enable-broker but leaves --unbind-pvcs-after at 0 gets a broker stuck Stuck/requeueing forever with no hint. Flag help & chart README still describe it as PVCUnbinder-only. Document the prerequisite.

M9. Grant-gated rotation is never negatively tested — the gate could be fully broken and every test still passes — acceptance/features/broker-crd-adoption.feature:14, broker_controller_test.go:471-492,526-529 (staff-eng + Codex, across both test commits)
The acceptance scenario only makes one broker outdated and grants it, so "outdated+granted→rotates" and "gate ignored→rotates" are observationally identical — hasValidRollGrant() could return true always and stay green. setupBrokerCluster also grants every broker a 30-min roll-grant, so TestPVAffinityRemediation's "once granted" and TestPodRotationWithoutGrant don't exercise refusal either. Add a negative case: outdated without grant, assert the pod does not rotate (stable UID/checksum via Consistently) before granting.

M10. TestPVAffinityRemediation deletes a node on the shared k3d cluster and may reschedule onto a node lacking the imported image — broker_controller_test.go:574,650 (staff-eng + Codex)
Node-deletion on SharedClusterName "testenv" can strand pods/PVCs of other integration packages (pkg/k3d/k3d.go explicitly warns against this; pvcunbinder_test.go uses a dedicated cluster). Separately, CreateNode doesn't re-import localhost/redpanda-operator:dev, and soft anti-affinity prefers the empty new node → sidecar ImagePullBackOff → waitForPhase(Running) times out. Use a dedicated cluster and/or re-import images onto the new node.

M11. Acceptance adoption steps copy the live pod spec (incl. nodeName) and don't wait for StatefulSet teardown — acceptance/steps/broker.go:33,79 (Codex)
(a) createBrokerCRsForCluster uses Spec: pod.Spec → replacement pods pinned to the original nodeName; if that node is gone in CI, scheduling times out. The integration helper clears NodeName (broker_controller_test.go:724); the acceptance path doesn't. (b) pauseReconciliation+orphanDeleteStatefulSet don't wait for the StatefulSet to stay NotFound, so the Redpanda controller can recreate it and re-adopt pods, failing the broker-count assertions. Port the integration guard over.

---

🟡 Minor

• DeadNodePVCs flips reclaim policy of PVs backing externally-managed ExistingClaims — operator/internal/controller/pvcunbinder/pvcunbinder.go:1218 (security). It patches PV reclaim to Retain on claims the controller then explicitly declines to delete; an admin's reclaimPolicy=Delete local PV silently becomes Retain, leaving Released volumes+data behind. Filter ExistingClaims before the Retain patch. (Reachable only with --enable-broker + --unbind-pvcs-after>0 + grant.)
• reconcilePVCAdoption re-adopts ExistingClaims during decommission — broker_controller.go:290 (staff-eng). Re-sets ownerRef right before the completion branch deletes them; wasteful, inconsistent with siblings. Early-return on decommission.
• BuildPod shares the Broker's maps/slices with the returned Pod (no deep copy) — broker_helpers.go:52 (re-run staff-eng). Latent: any caller mutating the returned pod corrupts broker.Spec.PodTemplate. Not triggered today (caller only sets ownerRef+Create). DeepCopy the spec/maps.
• Raw-deletion test asserts only on the pod, not PVC release — broker_controller_test.go:364 (docs). Comment + commit message claim "PVCs released (ownerRefs stripped)" but nothing Gets a PVC; a regression that stopped stripping PVC ownerRefs stays green. Add the PVC assertion or narrow the wording.
• Finalizer tests don't deterministically hit the branches they document — broker_controller_test.go:189,206 (staff-eng + Codex). TestFinalizerPodNotOwned (ownerless pod gets adopted → exercises release, not the foreign-pod rollback) and TestFinalizerPodGone (pod-ensure recreates a pod first). Give the pod a foreign controller ownerRef / ensure absence before delete.
• "Adopted without a restart" is not asserted — broker-crd-adoption.feature:11 (staff-eng). A delete-and-recreate regression would still report 3 brokers and pass. Capture pod UID/creationTimestamp before adoption and assert unchanged.
• Admin client leaked on every poll iteration — acceptance/steps/broker.go:237 (Codex). New RedpandaAdminClient inside Eventually, never closed, ~60 polls over 5 min. defer admin.Close() or hoist outside the loop.
• Heavy cloud/JWT dep chain pulled into the acceptance module for 3 trivial helpers — acceptance/steps/broker.go:28 (staff-eng). Importing operator/pkg/feature drags in lestrrat-go/jwx, secp256k1, dataplane/gatekeeper, etc. (~8 new indirect deps). Relocate RollGrant key/format/TTL to a leaf package or inline the formatter.
• Unrelated console v3.7.0→v3.8.0 golden bump bundled into the acceptance commit — charts/redpanda/testdata/template-cases.golden.txtar (staff-eng + re-run docs). Stale straggler from the console release; task generate swept it up. Harmless but muddies the diff — split out.

⚪ Nits

• PodOutdated/BuildPod (the "rotation contract") have zero direct test coverage — broker_helpers_test.go only tests PodName.
• Commit messages say condition ConfigOutdated; the actual condition is ConfigSynced (Outdated is only a reason) — appears in 3 of the 4 commit messages.
• SkipNameValidation: true broadened onto the pre-existing NodePool & Redpanda controllers — hides accidental double-registration; add a rationale comment.
• clusters RBAC grants get;list;watch but only get is used — broker_controller.go:47. Narrow to get.
• reconcileDelete/deletionPolicy doc comments describe self-heal & policy propagation that depend on the not-yet-implemented owning cluster controller, phrased as present-tense guarantees.
• allBrokerCRsRunning (and the cleanup closure) ignore the clusterName arg and operate on the whole namespace — fine for @serial, misleading for reuse.
• b7827ed4 commit message attributes the decommission field & networkIndex to itself, but they pre-date it (#1631).

---

Bottom line

The feature is carefully built and well-documented internally, and it's safely dark by default. But before it's wired to an owning controller and enabled, I'd treat B1 (wrong-cluster admin ops), M1–M4 (decommission stalls + two data-loss paths), and M9 (the headline safety gate has zero real coverage) as must-fix. The strongest signal is convergence: B1, M1, M3, M9, M10 were each flagged independently by two or more reviewers.

_{🤖 Generated with Claude Code — 4-reviewer panel (staff-eng, docs, security, Codex adversarial) per commit.}

[hidalgopl]

Thanks for the thorough panel review @RafalKorepta — every finding is addressed below, one fixup commit per finding (they'll be autosquashed into the 4-commit series before merge, preserving these bodies). Verified with the unit suites, a green acceptance run of the broker features, and the full integration suite (which caught two bugs in my first M6 fix — noted inline).

🔴 B1 — admin-API calls hardcoded LocalCluster → 39b63af1
req.ClusterName is now threaded through the reconciliation state and reconcileDelete into all five admin-API call sites via the existing RedpandaAdminClientForCluster. For local requests req.ClusterName == mcmanager.LocalCluster, so the local path is unchanged byte-for-byte.

🟠 M1 — decommission with nil BrokerID stalls forever → af5b5f2a
The spec path now resolves the ID live, mirroring the deletion path; until resolvable it stays Decommissioning with a short requeue instead of silently never starting.

🟠 M2 — remediation/rotation not gated on decommission → 8b0733a2
reconcilePVAffinity and reconcilePodRotation early-return on Spec.Decommission, matching the sibling reconcilers — the decommission owns the pod's fate from there. (reconcilePVCAdoption got the same treatment in the minors commit.)

🟠 M3 — BrokerDeletionPolicy.Parse accepts anything → b1e0bee2
Parse validates {cascade, orphan} case-insensitively; unknown values error and Get falls back to the documented default with a logged complaint, as suggested. Unit test included.

🟠 M4 — raw delete with pod gone skips PVC release → 84632da3
The pod-absent branch now makes the same deletion-policy decision as the pod-present path; releaseBrokerResources accepts a nil pod and still strips PVC ownerRefs.

🟠 M5 — adoption overwrites live checksum → 07d1170a
The desired checksum is stamped only when the pod carries none (the migration case, where preconditions verified the config); a live checksum is preserved so PodOutdated queues the rotation on self-heal re-adoption.

🟠 M6 — resolveBrokerID address-form gaps → 43b019f9, refined by 39b0788c and 805cc12a
Rewritten as resolveBroker with the v2 controller's matching (port-strip, first-label/raw-host/pod-IP, ambiguity refusal) returning the full membership entry; identity adoption additionally requires an active+alive entry — this also fixes a bug we hit separately where a replacement broker adopted its decommissioned predecessor's node_id (same reused pod name) and wedged Stuck on the continuity check. The integration suite then showed plain ambiguity-refusal wedges PV-remediation recovery (the dead entry lingers until ghost decommission), so among ambiguous matches a unique active+alive entry now wins. Third commit swaps the (nil, nil) return for an explicit (resolved, found, err) signature.

🟠 M7 — PodName validation gaps → aceb3ca9
networkIndex is now required with Minimum=0 (CRD regenerated), making the index-0 collision unrepresentable; a NodePool-referenced Broker missing the cluster-name label surfaces Stuck with a precise log instead of error-looping on invalid DNS names. Full clusterRef resolution stays with the owning-controller PR.

🟠 M8 — --unbind-pvcs-after silently gates remediation → b58dd5ca, then 46f4ad16
First documented the prerequisite (flag help, stuck-path log, changelog); we then went further — SetupBrokerController now honors the flag when set and falls back to its own 5m default otherwise, so remediation works out of the box (it's node-object-deleted + roll-grant gated, so default-on is safe). TODO left for a dedicated --broker-unbind-pvcs-after flag.

🟠 M9 — no negative roll-grant coverage → 1f5caaa3 (integration) + d408e720 (acceptance)
Fixture grants are now opt-in; the rotation test asserts the ungranted outdated pod keeps its UID (require.Never, 30s) before granting and asserts the granted rotation completes after; TestPVAffinityRemediation starts ungranted so its Stuck wait proves refusal; the acceptance scenario got the same negative step before its grant.

🟠 M10 — node deletion on the shared k3d cluster → 981c4824
TestPVAffinityRemediation now runs on a dedicated k3d cluster (env construction factored into newEnv, pvcunbinder pattern) — in-suite serialization wasn't enough since test packages run concurrently — and the mid-test replacement node gets the operator/Redpanda images re-imported.

🟠 M11 — acceptance adoption steps → 108e5d8f
NodeName is cleared before rendering the Broker pod template, and orphanDeleteStatefulSet fails fast if the STS doesn't stay gone (racing-reconcile guard, mirroring the integration helper).

🟡 Minors → ac2299b3 (DeadNodePVCs excludes ExistingClaims from the Retain patch up front; reconcilePVCAdoption decommission gate; RBAC narrowed to get; SkipNameValidation rationale; self-heal doc comments to future tense) · 039cceb0 (BuildPod deep-copies; direct PodOutdated/BuildPod unit tests) · 1907e961 + 7b22ebf1 (raw-deletion test asserts PVC release; finalizer tests hit their branches deterministically — foreign controller ownerRef on a real object, impossible pod for the pod-gone branch) · 547a028d (adoption asserts pod UIDs unchanged; admin client closed per poll; broker listings scoped to the cluster label)

Deferred, deliberately: the operator/pkg/feature dep-chain relocation (worth doing, but it churns the module layout mid-review — proposed as a follow-up); the console v3.7.0→v3.8.0 golden straggler and the two commit-message nits (ConfigSynced naming, #1631 attribution) get fixed during the autosquash rebase since they're message/hunk-only; chart README wording for --unbind-pvcs-after will ride the next chart-docs regeneration.

Also, once you approve, I'll squash these commits.