feat(operator): operator-managed Keycloak client registration by akram · Pull Request #247 · kagenti/kagenti-operator

akram · 2026-03-27T12:39:53Z

Summary

Registers Keycloak OAuth clients for agent/tool workloads that set kagenti.io/client-registration-inject: "false", creates a Secret with client-id.txt / client-secret.txt, and annotates the pod template so the kagenti-webhook mounts credentials into shared-data.

requires:

oc patch deployment weather-service  --type merge \
   -p '{"spec": { "template": { "metadata": { "labels": {"kagenti.io/client-registration-inject": "false" }}}}}'

requires: kagenti/kagenti-extensions#262

Changes

ClientRegistrationReconciler for Deployment/StatefulSet; Keycloak admin client (internal/keycloak).
Deterministic Secret name: kagenti-keycloak-client-credentials-<hash>; annotation kagenti.io/keycloak-client-credentials-secret-name.
Feature flag --enable-operator-client-registration; APIReader for authbridge-config / keycloak-admin-secret; RBAC for Secrets.
Docs: docs/operator-managed-client-registration.md.

Testing

Controller unit tests; deploy operator + webhook together on a cluster with Keycloak and namespace secrets.

fixes kagenti/kagenti#397

cwiklik

Review Summary

Well-designed Keycloak client registration controller with sound architecture: Secret-before-annotation ordering, owner references for GC, feature-gated, and uncached API reads for namespace config. Thorough documentation.

Key concerns: (1) indentation bug in the StatefulSet path, (2) Made-with: Cursor trailer violates commit conventions, (3) defaulting the feature to true is aggressive for a new controller. The Keycloak client is create-only (no update path).

Areas reviewed: Go, Helm/K8s RBAC, Makefile, Docs
Commits: 3 commits, all signed-off: yes
CI status: DCO passing

kagenti-operator/internal/controller/clientregistration_controller.go

kagenti-operator/cmd/main.go

cwiklik · 2026-03-27T13:22:41Z

kagenti-operator/internal/keycloak/admin.go

+// DefaultHTTPClient returns an HTTP client suitable for Keycloak admin calls.
+func DefaultHTTPClient() *http.Client {
+	return &http.Client{Timeout: 60 * time.Second}
+}


suggestion: DefaultHTTPClient() uses http.DefaultTransport which verifies TLS (good), but there's no way to inject a custom CA bundle for in-cluster Keycloak with a private CA. Consider accepting a transport option or *tls.Config for production clusters with internal CAs.

fixed: added *tls.Config

cwiklik · 2026-03-27T13:22:41Z

kagenti-operator/internal/controller/clientregistration_controller.go

+	}
+	_, clientSecret, err := kc.RegisterOrFetchClientWithToken(ctx, token, keycloak.ClientRegistrationParams{
+		Realm:               ab.KeycloakRealm,
+		ClientID:            clientID,


suggestion: The admin token is obtained fresh via PasswordGrantToken on every reconcile. For workloads reconciling frequently, this generates one Keycloak token request per reconcile. Consider caching the token (with expiry check) to reduce Keycloak load.

cwiklik · 2026-03-27T13:22:41Z

kagenti-operator/internal/controller/clientregistration_controller.go

+			continue
+		}
+		if v, ok := m["globalEnabled"].(bool); ok {
+			globalOn = v


nit: readClusterFeatureGates iterates cm.Data and returns after parsing the first non-empty YAML document. If the ConfigMap has multiple data keys, only the first is considered. A brief comment clarifying this assumption would help readability.

fixed: comment added

cwiklik · 2026-03-27T13:22:41Z

kagenti-operator/config/rbac/role.yaml

+  verbs:
+  - create
+  - get
+  - list


suggestion: This grants cluster-wide secrets read access (get/list/watch). The controller only needs secrets in agent namespaces. Consider whether a more restrictive RBAC binding is warranted (ClusterRole + per-namespace RoleBindings), or document why cluster-wide is needed.

cwiklik · 2026-03-27T13:22:41Z

kagenti-operator/internal/controller/clientregistration_controller.go

+			Name:      secretName,
+			Namespace: owner.GetNamespace(),
+			Labels: map[string]string{
+				LabelManagedBy: LabelManagedByValue,


👍 Good use of controllerutil.CreateOrUpdate with SetControllerReference — ensures GC when the parent Deployment/StatefulSet is deleted. The Secret-before-annotation ordering is also well done.

kagenti-operator/internal/keycloak/admin.go

kagenti-operator/internal/controller/clientregistration_controller_test.go

Introduce ClientRegistrationReconciler for Deployments and StatefulSets that use kagenti.io/client-registration-inject=false: register the workload as an OAuth client in Keycloak, create credentials secrets, and annotate the pod template so the webhook can mount them without injecting the registration sidecar. Add a small Keycloak admin API client and RBAC; gate the controller behind --enable-operator-client-registration. Signed-off-by: Akram <akram.benaissi@gmail.com>

Describe motivation, webhook/operator contract, requirements, rollout and rollback, and pointers to source files. Signed-off-by: Akram <akram.benaissi@gmail.com>

- Use kagenti.io/keycloak-client-credentials-secret-name and kagenti-keycloak-client-credentials-<uniq-id> Secrets - Rename inject/annotation helpers for Keycloak client credentials; update docs Assisted-by: Cursor Signed-off-by: Akram <akram.benaissi@gmail.com>

- Cache admin tokens via CachedAdminTokenProvider; wire from main when client registration is enabled. - Default --enable-operator-client-registration to false (opt-in). - Reconcile Keycloak client settings: GET client, PUT when desired state drifts from Keycloak. - Add HTTPClientConfig / NewHTTPClient for TLS and custom transport (e.g. private CA). - Document feature-gate ConfigMap iteration; fix StatefulSet branch indentation. - Add integration-style reconciler tests and expand admin tests (drift, HTTP client). Addresses review comments. Assisted-by: Cursor Signed-off-by: Akram <akram.benaissi@gmail.com>

Alan-Cha

Review Summary

This PR adds operator-managed Keycloak client registration as an alternative to sidecar-based registration. The implementation is well-designed with proper controller patterns, comprehensive tests (including envtest-style reconciler tests and mock Keycloak server), token caching, drift reconciliation, and thorough documentation.

The latest commit (7d9d86f) successfully addressed most of the previous review feedback: feature flag now defaults to false (opt-in), token caching is implemented, TLS config support added, indentation bug fixed, and test coverage expanded significantly.

One must-fix issue remains: Documentation inconsistency about the feature flag default value.

Areas reviewed: Go (controller, Keycloak admin client, tests), Makefile, RBAC, Documentation, Commit conventions

Commits: 4 commits, all DCO signed ✅

CI status: DCO passing ✅

Must-Fix Issues

Documentation default value mismatch (docs/operator-managed-client-registration.md, line ~77 in new file):

Line states the default is true, but cmd/main.go:112 sets the flag default to false. The docs should match the code:

-| Operator | `--enable-operator-client-registration` (default **true**) | Master switch for the ClientRegistration controller. |
+| Operator | `--enable-operator-client-registration` (default **false**) | Master switch for the ClientRegistration controller. |

Praise

Excellent reconciler patterns (clientregistration_controller.go):

✅ CreateOrUpdate with owner references ensures proper GC
✅ Secret-before-annotation ordering prevents pods from referencing missing Secrets
✅ Clean reconciler structure following controller-runtime best practices

Well-implemented drift reconciliation (internal/keycloak/admin.go):

✅ GET client, compare managed fields, merge desired state, PUT only when drifted
✅ Properly handles Keycloak's JSON shape quirks (string arrays in attributes)

Strong test coverage (clientregistration_controller_test.go):

✅ Reconciler integration tests with fake clients and mock Keycloak server
✅ Covers feature gates disabled, missing dependencies (requeue), and happy path
✅ Addresses previous review's testing concern

Suggestions

Token cache tuning (cmd/main.go): Token cache is instantiated with a hardcoded tokenCacheSkew. Consider exposing this as a flag if operators need to tune it for their Keycloak instance characteristics.

Commit 3 convention: Subject lacks emoji prefix: operator: Keycloak client credentials Secret naming and annotation. Should be feat(operator): or refactor(operator): per Kagenti conventions. Not blocking since commits can be squashed on merge.

Once the documentation is corrected, this PR is ready to merge. The implementation quality is high, test coverage is comprehensive, and all previous review concerns have been addressed.

akram requested a review from a team as a code owner March 27, 2026 12:39

akram force-pushed the feature/operator-managed-client-registration branch from d8863ed to 3d8c0b5 Compare March 27, 2026 12:41

akram mentioned this pull request Mar 27, 2026

feat(webhook): operator-managed Keycloak client credentials kagenti/kagenti-extensions#262

Open

cwiklik requested changes Mar 27, 2026

View reviewed changes

cwiklik mentioned this pull request Mar 27, 2026

fix(rbac): align Helm ClusterRole with controller-gen output #249

Open

akram added 4 commits March 27, 2026 16:45

docs: add operator-managed client registration design

b6b6c21

Describe motivation, webhook/operator contract, requirements, rollout and rollback, and pointers to source files. Signed-off-by: Akram <akram.benaissi@gmail.com>

akram force-pushed the feature/operator-managed-client-registration branch 2 times, most recently from 3d8c0b5 to 7d9d86f Compare March 27, 2026 15:46

Alan-Cha requested changes Mar 27, 2026

View reviewed changes

Alan-Cha mentioned this pull request Mar 27, 2026

fix(rbac): Add missing agentruntimes permissions to ClusterRole #253

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(operator): operator-managed Keycloak client registration#247

feat(operator): operator-managed Keycloak client registration#247
akram wants to merge 4 commits intokagenti:mainfrom
akram:feature/operator-managed-client-registration

akram commented Mar 27, 2026 •

edited

Loading

Uh oh!

cwiklik left a comment

Uh oh!

Uh oh!

Uh oh!

cwiklik Mar 27, 2026

Uh oh!

akram Mar 27, 2026

Uh oh!

cwiklik Mar 27, 2026

Uh oh!

cwiklik Mar 27, 2026

Uh oh!

akram Mar 27, 2026

Uh oh!

cwiklik Mar 27, 2026

Uh oh!

cwiklik Mar 27, 2026

Uh oh!

Uh oh!

Uh oh!

Alan-Cha left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

akram commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Related

Testing

Uh oh!

cwiklik left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

Uh oh!

Uh oh!

cwiklik Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

akram Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

cwiklik Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

cwiklik Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

akram Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

cwiklik Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

cwiklik Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Alan-Cha left a comment

Choose a reason for hiding this comment

Review Summary

Must-Fix Issues

Praise

Suggestions

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akram commented Mar 27, 2026 •

edited

Loading