Merge pull request #356 from huang195/fix/mutator-eager-keycloak-secret-mount

fix(webhook): pre-populate Keycloak client-credentials annotation at admission

Author: huang195

kagenti-operator/internal/clientreg/names.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2026.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+*/
+
+// Package clientreg holds the naming and eligibility logic shared by the AuthBridge mutating
+// webhook and the ClientRegistration controller. Both sides must agree on (a) the Secret name
+// and (b) whether a workload is eligible for operator-managed Keycloak client registration,
+// so the webhook can pre-populate the pod annotation at admission without waiting for the
+// controller to run. Centralizing these prevents the two sides from drifting.
+package clientreg
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
+)
+
+const (
+	// LabelClientRegistrationInject: when "true", the workload opts into the legacy
+	// client-registration sidecar and operator-managed registration is skipped.
+	LabelClientRegistrationInject = "kagenti.io/client-registration-inject"
+
+	// LabelAgentType distinguishes agents from tools; operator-managed registration runs for
+	// agents unconditionally and for tools only when the injectTools feature gate is on.
+	LabelAgentType  = "kagenti.io/type"
+	LabelValueAgent = "agent"
+	// LabelValueTool matches agentv1alpha1.RuntimeTypeTool — kept here as a string to avoid
+	// importing the API package from this leaf utility.
+	LabelValueTool = "tool"
+
+	// AnnotationKeycloakClientSecretName is set on workload pod templates (by the controller)
+	// and pre-populated on new pods (by the webhook) to signal the name of the Secret holding
+	// Keycloak client credentials. The webhook mounts that Secret into /shared/client-id.txt
+	// and /shared/client-secret.txt for any container that already mounts the shared-data volume.
+	AnnotationKeycloakClientSecretName = "kagenti.io/keycloak-client-credentials-secret-name"
+)
+
+// KeycloakClientCredentialsSecretName returns the deterministic name of the Secret the
+// ClientRegistration controller produces for (namespace, workload). It is a pure function of
+// those inputs only — the webhook can compute it at admission time without consulting the
+// API server, so a Secret volume can be declared before the controller has run. Kubelet will
+// retry the mount until the Secret appears.
+func KeycloakClientCredentialsSecretName(namespace, workload string) string {
+	sum := sha256.Sum256([]byte(namespace + "\000" + workload + "\000kagenti-keycloak-client-credentials"))
+	return "kagenti-keycloak-client-credentials-" + hex.EncodeToString(sum[:8])
+}
+
+// SkipReason returns a non-empty human-readable reason when operator-managed client registration
+// should not run for the workload. Empty string means "proceed". Both the controller's
+// reconcileOne and the webhook's admission handler use this to stay in lockstep.
+func SkipReason(labels map[string]string, injectTools bool) string {
+	if labels == nil {
+		return "pod template has no labels"
+	}
+	if labels[LabelClientRegistrationInject] == "true" {
+		return fmt.Sprintf("%s is \"true\" (legacy webhook client-registration sidecar; operator-managed registration disabled for this workload)", LabelClientRegistrationInject)
+	}
+	switch labels[LabelAgentType] {
+	case LabelValueAgent:
+		return ""
+	case LabelValueTool:
+		if !injectTools {
+			return "kagenti.io/type is tool but cluster injectTools feature gate is disabled"
+		}
+		return ""
+	default:
+		t := labels[LabelAgentType]
+		if t == "" {
+			return "kagenti.io/type label is missing or not agent/tool"
+		}
+		return fmt.Sprintf("kagenti.io/type=%q is not agent or tool", t)
+	}
+}
+
+// WorkloadWantsOperatorClientReg returns true when the workload's labels and the cluster
+// injectTools gate both permit operator-managed client registration.
+func WorkloadWantsOperatorClientReg(labels map[string]string, injectTools bool) bool {
+	return SkipReason(labels, injectTools) == ""
+}

kagenti-operator/internal/clientreg/names_test.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2026.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+*/
+
+package clientreg
+
+import "testing"
+
+func TestKeycloakClientCredentialsSecretName_Deterministic(t *testing.T) {
+	a := KeycloakClientCredentialsSecretName("team1", "weather-agent")
+	b := KeycloakClientCredentialsSecretName("team1", "weather-agent")
+	if a != b {
+		t.Fatalf("expected deterministic output, got %q and %q", a, b)
+	}
+	if a == "" {
+		t.Fatalf("expected non-empty secret name")
+	}
+}
+
+func TestKeycloakClientCredentialsSecretName_DistinctByInputs(t *testing.T) {
+	cases := []struct{ ns, w string }{
+		{"team1", "a"},
+		{"team1", "b"},
+		{"team2", "a"},
+	}
+	seen := map[string]string{}
+	for _, c := range cases {
+		got := KeycloakClientCredentialsSecretName(c.ns, c.w)
+		key := c.ns + "/" + c.w
+		if prev, ok := seen[got]; ok {
+			t.Fatalf("collision: %s and %s both produced %s", prev, key, got)
+		}
+		seen[got] = key
+	}
+}
+
+func TestSkipReason(t *testing.T) {
+	tests := []struct {
+		name        string
+		labels      map[string]string
+		injectTools bool
+		wantSkip    bool
+	}{
+		{"nil labels", nil, true, true},
+		{"empty labels", map[string]string{}, true, true},
+		{"legacy sidecar opt-in", map[string]string{LabelClientRegistrationInject: "true", LabelAgentType: LabelValueAgent}, true, true},
+		{"agent proceeds", map[string]string{LabelAgentType: LabelValueAgent}, false, false},
+		{"tool with gate on proceeds", map[string]string{LabelAgentType: LabelValueTool}, true, false},
+		{"tool with gate off skipped", map[string]string{LabelAgentType: LabelValueTool}, false, true},
+		{"unknown type skipped", map[string]string{LabelAgentType: "other"}, true, true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			reason := SkipReason(tc.labels, tc.injectTools)
+			gotSkip := reason != ""
+			if gotSkip != tc.wantSkip {
+				t.Fatalf("SkipReason(%v, %v) = %q; wantSkip=%v", tc.labels, tc.injectTools, reason, tc.wantSkip)
+			}
+			wants := WorkloadWantsOperatorClientReg(tc.labels, tc.injectTools)
+			if wants == tc.wantSkip {
+				t.Fatalf("WorkloadWantsOperatorClientReg disagrees with SkipReason for %v", tc)
+			}
+		})
+	}
+}

kagenti-operator/internal/controller/clientregistration_controller.go

@@ -9,8 +9,6 @@ package controller
 
 import (
 	"context"
-	"crypto/sha256"
-	"encoding/hex"
 	"fmt"
 	"strings"
 	"time"
@@ -32,7 +30,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/yaml"
 
-	agentv1alpha1 "github.com/kagenti/operator/api/v1alpha1"
+	"github.com/kagenti/operator/internal/clientreg"
 	"github.com/kagenti/operator/internal/keycloak"
 )
 
@@ -44,10 +42,12 @@ const (
 	// LabelClientRegistrationInject: when not "true", the operator registers the OAuth client and sets
 	// AnnotationKeycloakClientSecretName. Value "true" opts the workload into the legacy webhook
 	// client-registration sidecar; the operator skips registration for that workload.
-	LabelClientRegistrationInject = "kagenti.io/client-registration-inject"
+	// Re-exported from internal/clientreg so existing callers keep working.
+	LabelClientRegistrationInject = clientreg.LabelClientRegistrationInject
 
 	// AnnotationKeycloakClientSecretName must match kagenti-webhook injector.AnnotationKeycloakClientSecretName.
-	AnnotationKeycloakClientSecretName = "kagenti.io/keycloak-client-credentials-secret-name"
+	// Re-exported from internal/clientreg to give both the controller and the webhook one source of truth.
+	AnnotationKeycloakClientSecretName = clientreg.AnnotationKeycloakClientSecretName
 )
 
 // ClientRegistrationReconciler registers OAuth clients in Keycloak and patches agent/tool workloads that
@@ -303,34 +303,14 @@ func injectKeycloakClientCredentialsAnnotation(template *corev1.PodTemplateSpec,
 	return true
 }
 
-// keycloakClientCredentialsSkipReason returns a non-empty human-readable reason when this controller should
-// not process the workload; empty string means reconcile should continue.
+// keycloakClientCredentialsSkipReason and workloadWantsOperatorClientReg delegate to internal/clientreg
+// so the controller's reconcile decision and the webhook's admission decision stay in lockstep.
 func keycloakClientCredentialsSkipReason(labels map[string]string, injectTools bool) string {
-	if labels == nil {
-		return "pod template has no labels"
-	}
-	if labels[LabelClientRegistrationInject] == "true" {
-		return fmt.Sprintf("%s is \"true\" (legacy webhook client-registration sidecar; operator-managed registration disabled for this workload)", LabelClientRegistrationInject)
-	}
-	switch labels[LabelAgentType] {
-	case LabelValueAgent:
-		return ""
-	case string(agentv1alpha1.RuntimeTypeTool):
-		if !injectTools {
-			return "kagenti.io/type is tool but cluster injectTools feature gate is disabled"
-		}
-		return ""
-	default:
-		t := labels[LabelAgentType]
-		if t == "" {
-			return "kagenti.io/type label is missing or not agent/tool"
-		}
-		return fmt.Sprintf("kagenti.io/type=%q is not agent or tool", t)
-	}
+	return clientreg.SkipReason(labels, injectTools)
 }
 
 func workloadWantsOperatorClientReg(labels map[string]string, injectTools bool) bool {
-	return keycloakClientCredentialsSkipReason(labels, injectTools) == ""
+	return clientreg.WorkloadWantsOperatorClientReg(labels, injectTools)
 }
 
 type authbridgeConfig struct {
@@ -440,9 +420,11 @@ func resolveKeycloakClientID(namespace, workloadName, serviceAccount string, spi
 	return fmt.Sprintf("spiffe://%s/ns/%s/sa/%s", trustDomain, namespace, sa), nil
 }
 
+// keycloakClientCredentialsSecretName is a thin alias over the shared helper so call sites in this
+// package stay unchanged. The shared implementation lives in internal/clientreg so the AuthBridge
+// mutating webhook can compute the same name at admission time.
 func keycloakClientCredentialsSecretName(namespace, workload string) string {
-	sum := sha256.Sum256([]byte(namespace + "\000" + workload + "\000kagenti-keycloak-client-credentials"))
-	return "kagenti-keycloak-client-credentials-" + hex.EncodeToString(sum[:8])
+	return clientreg.KeycloakClientCredentialsSecretName(namespace, workload)
 }
 
 func (r *ClientRegistrationReconciler) ensureClientCredentialsSecret(ctx context.Context, owner client.Object, secretName, clientID, clientSecret string) error {

kagenti-operator/internal/webhook/injector/pod_mutator.go

@@ -413,6 +413,15 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp
 			}
 		}
 
+		// Mount operator-managed Keycloak client credentials for any container that uses
+		// shared-data (authbridge-proxy reads /shared/client-id.txt and /shared/client-secret.txt
+		// for its jwt-validation + token-exchange plugins). Without this, proxy-sidecar mode
+		// polls the credential files forever and rejects every inbound request with
+		// 503 "identity not yet configured (credentials pending)". Envoy-sidecar mode
+		// already calls this helper further down; the proxy-sidecar branch returns early,
+		// so it needs its own invocation.
+		ApplyKeycloakClientCredentialsSecretVolumes(podSpec, annotations)
+
 		mutatorLog.Info("proxy-sidecar mode injection complete",
 			"namespace", namespace, "crName", crName,
 			"image", builder.cfg.Images.AuthBridgeLight,

kagenti-operator/internal/webhook/injector/pod_mutator_test.go

@@ -746,6 +746,71 @@ func TestInjectAuthBridge_ProxySidecarMode_InjectsCorrectly(t *testing.T) {
 	}
 }
 
+func TestInjectAuthBridge_ProxySidecarMode_MountsKeycloakCredentials(t *testing.T) {
+	// Regression: the proxy-sidecar branch used to return before reaching
+	// ApplyKeycloakClientCredentialsSecretVolumes. That left authbridge-proxy polling
+	// /shared/client-id.txt forever and returning 503 "identity not yet configured".
+	m := newTestMutator()
+	ctx := context.Background()
+
+	podSpec := &corev1.PodSpec{
+		ServiceAccountName: "my-agent",
+		Containers: []corev1.Container{
+			{Name: "agent", Image: "my-agent:latest"},
+		},
+	}
+	labels := map[string]string{
+		KagentiTypeLabel: KagentiTypeAgent,
+	}
+	annotations := map[string]string{
+		AnnotationAuthBridgeMode:           ModeProxySidecar,
+		AnnotationKeycloakClientSecretName: "kagenti-keycloak-client-credentials-abc12345",
+	}
+
+	mutated, err := m.InjectAuthBridge(ctx, podSpec, "team1", "my-agent", labels, annotations)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !mutated {
+		t.Fatal("proxy-sidecar mode should mutate the pod")
+	}
+
+	// The Secret volume must be declared so kubelet can resolve it.
+	volFound := false
+	for _, v := range podSpec.Volumes {
+		if v.Secret != nil && v.Secret.SecretName == "kagenti-keycloak-client-credentials-abc12345" {
+			volFound = true
+			break
+		}
+	}
+	if !volFound {
+		t.Error("expected a Secret volume for operator-managed Keycloak client credentials; got none")
+	}
+
+	// The authbridge-proxy container must mount client-id.txt and client-secret.txt
+	// via subPath so its plugins can read them.
+	var proxyMounts []corev1.VolumeMount
+	for _, c := range podSpec.Containers {
+		if c.Name == AuthBridgeProxyContainerName {
+			proxyMounts = c.VolumeMounts
+			break
+		}
+	}
+	haveIDMount, haveSecretMount := false, false
+	for _, m := range proxyMounts {
+		if m.MountPath == "/shared/client-id.txt" && m.SubPath == "client-id.txt" {
+			haveIDMount = true
+		}
+		if m.MountPath == "/shared/client-secret.txt" && m.SubPath == "client-secret.txt" {
+			haveSecretMount = true
+		}
+	}
+	if !haveIDMount || !haveSecretMount {
+		t.Errorf("authbridge-proxy missing Keycloak credential subPath mounts: id=%v secret=%v",
+			haveIDMount, haveSecretMount)
+	}
+}
+
 func TestInjectHTTPProxyEnv_DoesNotDuplicate(t *testing.T) {
 	c := &corev1.Container{
 		Name: "agent",

kagenti-operator/internal/webhook/v1alpha1/authbridge_webhook.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/kagenti/operator/internal/clientreg"
 	"github.com/kagenti/operator/internal/webhook/injector"
 	corev1 "k8s.io/api/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -87,6 +88,26 @@ func (w *AuthBridgeWebhook) Handle(ctx context.Context, req admission.Request) a
 	// but GenerateName is set by the owning controller (e.g. "myapp-7d4f8b9c5-").
 	resourceName := deriveWorkloadName(&pod)
 
+	// Pre-populate the Keycloak client-credentials annotation for workloads eligible for
+	// operator-managed client registration. The ClientRegistration controller produces the
+	// Secret asynchronously (it calls out to Keycloak), so at first-deploy admission the
+	// Secret typically does not exist yet. By setting the annotation here and letting
+	// ApplyKeycloakClientCredentialsSecretVolumes declare the Secret volume with Optional=false,
+	// kubelet will wait for the Secret to appear and mount it — no pod restart required.
+	// Without this, the first pod comes up with an empty /shared/ and envoy returns 503
+	// "identity not yet configured (credentials pending)" until the user deletes the pod.
+	if pod.Annotations[injector.AnnotationKeycloakClientSecretName] == "" &&
+		clientreg.WorkloadWantsOperatorClientReg(pod.Labels, w.Mutator.GetFeatureGates().InjectTools) {
+		if pod.Annotations == nil {
+			pod.Annotations = map[string]string{}
+		}
+		pod.Annotations[injector.AnnotationKeycloakClientSecretName] =
+			clientreg.KeycloakClientCredentialsSecretName(req.Namespace, resourceName)
+		authbridgelog.Info("pre-populated Keycloak client credentials annotation",
+			"namespace", req.Namespace, "name", resourceName,
+			"secret", pod.Annotations[injector.AnnotationKeycloakClientSecretName])
+	}
+
 	// Check if already injected (idempotency / reinvocation)
 	if w.isAlreadyInjected(&pod.Spec) {
 		// Reinvocation: sidecars exist but Keycloak Secret mounts may still be