Chore: sunset client-registration sidecar — remove legacy opt-in label#479
Conversation
The client-registration in-pod sidecar was removed in #1422. What remained was the kagenti.io/client-registration-inject=true label that workloads could set to opt OUT of operator-managed registration (and into the now-deleted sidecar). With the sidecar gone this label has no effect and only creates confusion. Removes: - LabelClientRegistrationInject constant from clientreg/names.go - The SkipReason case that checked for the label - Duplicate constant in webhook/injector/constants.go - Re-export alias in clientregistration_controller.go - Test case covering the legacy label behaviour After this change, operator-managed registration runs for all eligible agent/tool workloads unconditionally. The kagenti.io/client-registration-inject label is silently ignored if present on any existing workload. Closes #1913 Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
Missed in the previous commit — controller_test.go also referenced LabelClientRegistrationInject for the legacy sidecar opt-in cases. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
mrsabath
left a comment
There was a problem hiding this comment.
Clean pure-deletion sunsetting the dead kagenti.io/client-registration-inject label (Phase 5 of #1426). The code changes themselves are correct and match the stated behavior change (label now silently ignored; operator-managed registration runs unconditionally).
🔴 Must-fix — CI Unit Tests fail
TestAPIs fails at internal/webhook/v1alpha1/authbridge_webhook_test.go:327:
[FAIL] AuthBridge Pod Webhook operator-managed Keycloak credentials: annotation pre-population
[It] skips workloads opted into the legacy client-registration sidecar
The It("skips workloads opted into the legacy client-registration sidecar") block (around line 311) sets kagenti.io/client-registration-inject: "true" and asserts the operator skips annotation pre-population (Expect(pod.Annotations).NotTo(HaveKey(injector.AnnotationKeycloakClientSecretName))). This PR removed exactly that skip path, so the operator now pre-populates the annotation unconditionally for eligible agents — and the assertion fails.
(This file isn't in the diff, which is why I can't leave an inline comment.) It compiled only because it uses the hardcoded label string, not the now-removed LabelClientRegistrationInject constant — so nothing caught it until the runtime assertion. Same class of miss as commit b2050fe (the controller-test cleanup). This is the last client-registration-inject reference left on the branch (I grepped the whole tree). Fix: delete this It(...) block, consistent with how the other legacy opt-in cases were removed.
Scope note (non-blocking)
The PR carries Closes #1913, but two of that issue's four checklist items don't appear in this diff — "remove the sidecar container spec / image references from the injector code" and "update docs to reflect the sidecar is gone." If both were already handled in #1422, a one-line confirmation would help; otherwise consider leaving #1913 open (or narrowing what this PR closes), since the label removal is only one of its four items.
Areas reviewed: Go, Tests, Commit/PR conventions, epic/issue linkage
Commits: 2, both signed-off (DCO passes)
CI status: Unit Tests FAIL (must-fix); Build, Lint, Integration, CodeQL, Trivy pass; E2E pending
Used the hardcoded label string instead of the constant (missed by vet), caught by the runtime assertion. Same class of cleanup as b2050fe. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
|
Thanks for catching the webhook test — fixed in e526513. That was the last reference using the hardcoded string instead of the constant, so vet didn't catch it. On the scope of
Happy to narrow the |
mrsabath
left a comment
There was a problem hiding this comment.
Re-review after e526513. The must-fix from my prior review (CHANGES_REQUESTED on b2050fe) is resolved: the stale It("skips workloads opted into the legacy client-registration sidecar") block in authbridge_webhook_test.go is deleted cleanly (17 lines, nothing else touched), and all CI now passes — including the previously-failing Unit Tests and E2E. Verified zero client-registration-inject references remain anywhere on the branch.
The Closes #1913 scope note is resolved too: confirmed the sidecar image/chart refs and admin-cred mount were removed in #1422 (which touched charts/kagenti/values.yaml, Chart.yaml, agent-namespaces.yaml, identity-guide.md), and the 4 residual client-registration strings in the operator's values.yaml are all the operator-managed replacement (--enable-client-registration, ClientRegistrationReconciler, and a comment stating "no in-pod sidecar") — not the legacy path. All four #1913 items are genuinely covered.
Clean Phase-5 cleanup for epic #1426. LGTM.
Areas reviewed: Go, Tests, Commit/PR conventions, epic/issue linkage
Commits: 3, all signed-off (DCO passes)
CI status: All 16 checks pass
|
Thanks @Alan-Cha — Your #1913 scope breakdown checks out — I confirmed #1422 removed the sidecar chart/image refs and the admin-cred mount, and the remaining |
Summary
Closes rossoctl/rossoctl#1913. Removes the last remnant of the legacy in-pod client-registration sidecar: the
kagenti.io/client-registration-injectopt-in label.Background
The client-registration sidecar itself was removed in rossoctl/rossoctl#1422 (merged 2026-05-12). What remained was the
kagenti.io/client-registration-inject=truelabel that workloads could set to opt out of operator-managed registration — effectively saying "use the in-pod sidecar instead." With the sidecar gone, the label has no meaningful effect and only creates confusion for anyone who sets it expecting the old behavior.Changes
internal/clientreg/names.go: removeLabelClientRegistrationInjectconstant and theSkipReasoncase that checked for itinternal/webhook/injector/constants.go: remove duplicate constant definitioninternal/controller/clientregistration_controller.go: remove re-export aliasinternal/clientreg/names_test.go: remove test case covering the legacy labelBehavior change:
kagenti.io/client-registration-inject=trueis now silently ignored. Operator-managed registration runs for all eligible agent/tool workloads unconditionally.Checklist
internal/clientreg✅, all others ✅)Assisted-By: Claude Code