Address review feedback on MCPAuthzConfig CRD

- Add CEL XValidation rules for authzConfig/authzConfigRef mutual
  exclusivity on MCPServerSpec, MCPRemoteProxySpec, and IncomingAuthConfig
- Add RBAC markers for watched workload resources (mcpservers,
  virtualmcpservers, mcpremoteproxies)
- Replace mustMarshalJSON panic with error-returning marshalJSON
- Add nil-check on spec.Config.Raw before use in BuildFullAuthzConfigJSON
- Fix reconcile logic: call findReferencingWorkloads before hash-changed
  check and return errors instead of swallowing them (matches
  MCPTelemetryConfig pattern)
- Move authorizer backend blank imports from controller to main.go
- Add comment explaining why validateAuthzConfigSpec is standalone
- Add listMapKey=kind to ReferencingWorkloads for composite uniqueness
- Add tests for findReferencingWorkloads covering all workload types,
  deletion blocking by VirtualMCPServer and MCPRemoteProxy, and empty
  Config.Raw validation
- Regenerate CRD manifests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: JAORMX

cmd/thv-operator/api/v1alpha1/mcpauthzconfig_types.go

@@ -58,6 +58,7 @@ type MCPAuthzConfigStatus struct {
 	// ReferencingWorkloads is a list of workload resources that reference this MCPAuthzConfig.
 	// Each entry identifies the workload by kind and name.
 	// +listType=map
+	// +listMapKey=kind
 	// +listMapKey=name
 	// +optional
 	ReferencingWorkloads []WorkloadReference `json:"referencingWorkloads,omitempty"`

cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go

@@ -38,6 +38,7 @@ type HeaderFromSecret struct {
 // MCPRemoteProxySpec defines the desired state of MCPRemoteProxy
 //
 // +kubebuilder:validation:XValidation:rule="!(has(self.oidcConfig) && has(self.oidcConfigRef))",message="oidcConfig and oidcConfigRef are mutually exclusive; use oidcConfigRef to reference a shared MCPOIDCConfig"
+// +kubebuilder:validation:XValidation:rule="!(has(self.authzConfig) && has(self.authzConfigRef))",message="authzConfig and authzConfigRef are mutually exclusive; use authzConfigRef to reference a shared MCPAuthzConfig"
 // +kubebuilder:validation:XValidation:rule="!(has(self.telemetry) && has(self.telemetryConfigRef))",message="telemetry and telemetryConfigRef are mutually exclusive; migrate to telemetryConfigRef"
 //
 //nolint:lll // CEL validation rules exceed line length limit

cmd/thv-operator/api/v1alpha1/mcpserver_types.go

@@ -176,6 +176,7 @@ const SessionStorageProviderRedis = "redis"
 // MCPServerSpec defines the desired state of MCPServer
 //
 // +kubebuilder:validation:XValidation:rule="!(has(self.oidcConfig) && has(self.oidcConfigRef))",message="oidcConfig and oidcConfigRef are mutually exclusive; use oidcConfigRef to reference a shared MCPOIDCConfig"
+// +kubebuilder:validation:XValidation:rule="!(has(self.authzConfig) && has(self.authzConfigRef))",message="authzConfig and authzConfigRef are mutually exclusive; use authzConfigRef to reference a shared MCPAuthzConfig"
 // +kubebuilder:validation:XValidation:rule="!(has(self.telemetry) && has(self.telemetryConfigRef))",message="telemetry and telemetryConfigRef are mutually exclusive; migrate to telemetryConfigRef"
 // +kubebuilder:validation:XValidation:rule="!has(self.rateLimiting) || (has(self.sessionStorage) && self.sessionStorage.provider == 'redis')",message="rateLimiting requires sessionStorage with provider 'redis'"
 // +kubebuilder:validation:XValidation:rule="!(has(self.rateLimiting) && has(self.rateLimiting.perUser)) || has(self.oidcConfig) || has(self.oidcConfigRef) || has(self.externalAuthConfigRef)",message="rateLimiting.perUser requires authentication (oidcConfig, oidcConfigRef, or externalAuthConfigRef)"

cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go

@@ -111,6 +111,7 @@ type EmbeddingServerRef struct {
 //
 // +kubebuilder:validation:XValidation:rule="self.type == 'oidc' ? (has(self.oidcConfig) || has(self.oidcConfigRef)) : true",message="spec.incomingAuth.oidcConfig or oidcConfigRef is required when type is oidc"
 // +kubebuilder:validation:XValidation:rule="!(has(self.oidcConfig) && has(self.oidcConfigRef))",message="oidcConfig and oidcConfigRef are mutually exclusive; use oidcConfigRef to reference a shared MCPOIDCConfig"
+// +kubebuilder:validation:XValidation:rule="!(has(self.authzConfig) && has(self.authzConfigRef))",message="authzConfig and authzConfigRef are mutually exclusive; use authzConfigRef to reference a shared MCPAuthzConfig"
 //
 //nolint:lll // CEL validation rules exceed line length limit
 type IncomingAuthConfig struct {

cmd/thv-operator/controllers/mcpauthzconfig_controller.go

@@ -24,9 +24,6 @@ import (
 	mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
 	ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
 	"github.com/stacklok/toolhive/pkg/authz/authorizers"
-	// Import authorizer backends so they register with the factory registry.
-	_ "github.com/stacklok/toolhive/pkg/authz/authorizers/cedar"
-	_ "github.com/stacklok/toolhive/pkg/authz/authorizers/http"
 )
 
 const (
@@ -53,6 +50,9 @@ type MCPAuthzConfigReconciler struct {
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpauthzconfigs,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpauthzconfigs/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpauthzconfigs/finalizers,verbs=update
+// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpservers,verbs=get;list;watch
+// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=virtualmcpservers,verbs=get;list;watch
+// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpremoteproxies,verbs=get;list;watch
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
@@ -114,36 +114,31 @@ func (r *MCPAuthzConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	// Calculate the hash of the current configuration
 	configHash := ctrlutil.CalculateConfigHash(authzConfig.Spec)
 
-	// Check if the hash has changed
+	// Track referencing workloads
+	referencingWorkloads, err := r.findReferencingWorkloads(ctx, authzConfig)
+	if err != nil {
+		logger.Error(err, "Failed to find referencing workloads")
+		return ctrl.Result{}, err
+	}
+
+	// Check what changed
 	hashChanged := authzConfig.Status.ConfigHash != configHash
+	refsChanged := !ctrlutil.WorkloadRefsEqual(authzConfig.Status.ReferencingWorkloads, referencingWorkloads)
+	needsUpdate := hashChanged || refsChanged || conditionChanged
+
 	if hashChanged {
 		logger.Info("MCPAuthzConfig configuration changed",
 			"oldHash", authzConfig.Status.ConfigHash,
 			"newHash", configHash)
+	}
 
+	if needsUpdate {
 		authzConfig.Status.ConfigHash = configHash
 		authzConfig.Status.ObservedGeneration = authzConfig.Generation
-
-		if err := r.Status().Update(ctx, authzConfig); err != nil {
-			logger.Error(err, "Failed to update MCPAuthzConfig status")
-			return ctrl.Result{}, err
-		}
-		return ctrl.Result{}, nil
-	}
-
-	// Refresh ReferencingWorkloads list
-	referencingWorkloads, err := r.findReferencingWorkloads(ctx, authzConfig)
-	if err != nil {
-		logger.Error(err, "Failed to find referencing workloads")
-	} else if !ctrlutil.WorkloadRefsEqual(authzConfig.Status.ReferencingWorkloads, referencingWorkloads) {
 		authzConfig.Status.ReferencingWorkloads = referencingWorkloads
-		conditionChanged = true
-	}
 
-	// Update condition if it changed (even without hash change)
-	if conditionChanged {
 		if err := r.Status().Update(ctx, authzConfig); err != nil {
-			logger.Error(err, "Failed to update MCPAuthzConfig status after condition change")
+			logger.Error(err, "Failed to update MCPAuthzConfig status")
 			return ctrl.Result{}, err
 		}
 	}
@@ -153,6 +148,9 @@ func (r *MCPAuthzConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 // validateAuthzConfigSpec validates the MCPAuthzConfig spec by reconstructing the
 // full authorizer config and delegating to the factory's ValidateConfig method.
+// Unlike other config CRDs (MCPOIDCConfig, MCPTelemetryConfig), validation lives here
+// rather than as a Validate() method on the type because it requires the authorizer
+// factory registry — an external dependency that the API types package should not import.
 func validateAuthzConfigSpec(spec mcpv1alpha1.MCPAuthzConfigSpec) error {
 	fullConfigJSON, err := BuildFullAuthzConfigJSON(spec)
 	if err != nil {
@@ -195,9 +193,22 @@ func BuildFullAuthzConfigJSON(spec mcpv1alpha1.MCPAuthzConfigSpec) ([]byte, erro
 
 	configKey := factory.ConfigKey()
 
+	if len(spec.Config.Raw) == 0 {
+		return nil, fmt.Errorf("config field is empty")
+	}
+
+	versionJSON, err := marshalJSON(authzConfigVersion)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal version: %w", err)
+	}
+	typeJSON, err := marshalJSON(spec.Type)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal type: %w", err)
+	}
+
 	fullConfig := map[string]json.RawMessage{
-		"version": mustMarshalJSON(authzConfigVersion),
-		"type":    mustMarshalJSON(spec.Type),
+		"version": versionJSON,
+		"type":    typeJSON,
 		configKey: spec.Config.Raw,
 	}
 
@@ -208,13 +219,13 @@ func BuildFullAuthzConfigJSON(spec mcpv1alpha1.MCPAuthzConfigSpec) ([]byte, erro
 	return result, nil
 }
 
-// mustMarshalJSON marshals a value to JSON or panics. Only for simple string values.
-func mustMarshalJSON(v string) json.RawMessage {
+// marshalJSON marshals a string value to JSON, returning an error instead of panicking.
+func marshalJSON(v string) (json.RawMessage, error) {
 	b, err := json.Marshal(v)
 	if err != nil {
-		panic(fmt.Sprintf("failed to marshal %q: %v", v, err))
+		return nil, fmt.Errorf("failed to marshal %q: %w", v, err)
 	}
-	return b
+	return b, nil
 }
 
 // handleDeletion handles the deletion of a MCPAuthzConfig.

cmd/thv-operator/controllers/mcpauthzconfig_controller_test.go

@@ -399,6 +399,75 @@ func TestMCPAuthzConfigReconciler_handleDeletion(t *testing.T) {
 						Namespace: "default",
 					},
 					Spec: mcpv1alpha1.MCPServerSpec{
+						Image: "example/mcp:latest",
+						AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+							Name: "test-config",
+						},
+					},
+				},
+			},
+			expectFinalizerRemoved: false,
+			expectRequeue:          true,
+		},
+		{
+			name: "referencing VirtualMCPServer blocks deletion",
+			authzConfig: &mcpv1alpha1.MCPAuthzConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-config",
+					Namespace:  "default",
+					Finalizers: []string{AuthzConfigFinalizerName},
+					DeletionTimestamp: &metav1.Time{
+						Time: time.Now(),
+					},
+				},
+				Spec: mcpv1alpha1.MCPAuthzConfigSpec{
+					Type:   "cedarv1",
+					Config: validCedarConfig(),
+				},
+			},
+			existingWorkloads: []client.Object{
+				&mcpv1alpha1.VirtualMCPServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "referencing-vmcp",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.VirtualMCPServerSpec{
+						IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{
+							Type: "anonymous",
+							AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+								Name: "test-config",
+							},
+						},
+					},
+				},
+			},
+			expectFinalizerRemoved: false,
+			expectRequeue:          true,
+		},
+		{
+			name: "referencing MCPRemoteProxy blocks deletion",
+			authzConfig: &mcpv1alpha1.MCPAuthzConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-config",
+					Namespace:  "default",
+					Finalizers: []string{AuthzConfigFinalizerName},
+					DeletionTimestamp: &metav1.Time{
+						Time: time.Now(),
+					},
+				},
+				Spec: mcpv1alpha1.MCPAuthzConfigSpec{
+					Type:   "cedarv1",
+					Config: validCedarConfig(),
+				},
+			},
+			existingWorkloads: []client.Object{
+				&mcpv1alpha1.MCPRemoteProxy{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "referencing-proxy",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.MCPRemoteProxySpec{
+						RemoteURL: "https://example.com",
 						AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
 							Name: "test-config",
 						},
@@ -614,3 +683,155 @@ func TestMCPAuthzConfigReconciler_ValidationRecovery(t *testing.T) {
 	assert.True(t, foundTrue, "Should have Valid=True condition after fix")
 	assert.NotEmpty(t, recoveredConfig.Status.ConfigHash, "Hash should be set after recovery")
 }
+
+func TestMCPAuthzConfigReconciler_findReferencingWorkloads(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name              string
+		authzConfigName   string
+		existingWorkloads []client.Object
+		expectedRefs      []mcpv1alpha1.WorkloadReference
+		expectEmpty       bool
+	}{
+		{
+			name:            "all three workload types referencing the same config",
+			authzConfigName: "shared-config",
+			existingWorkloads: []client.Object{
+				&mcpv1alpha1.MCPServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-server",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.MCPServerSpec{
+						Image: "example/mcp:latest",
+						AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+							Name: "shared-config",
+						},
+					},
+				},
+				&mcpv1alpha1.VirtualMCPServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-vmcp",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.VirtualMCPServerSpec{
+						IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{
+							Type: "anonymous",
+							AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+								Name: "shared-config",
+							},
+						},
+					},
+				},
+				&mcpv1alpha1.MCPRemoteProxy{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-proxy",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.MCPRemoteProxySpec{
+						RemoteURL: "https://example.com",
+						AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+							Name: "shared-config",
+						},
+					},
+				},
+			},
+			expectedRefs: []mcpv1alpha1.WorkloadReference{
+				{Kind: mcpv1alpha1.WorkloadKindMCPRemoteProxy, Name: "my-proxy"},
+				{Kind: mcpv1alpha1.WorkloadKindMCPServer, Name: "my-server"},
+				{Kind: mcpv1alpha1.WorkloadKindVirtualMCPServer, Name: "my-vmcp"},
+			},
+		},
+		{
+			name:            "no workloads reference the config",
+			authzConfigName: "unused-config",
+			existingWorkloads: []client.Object{
+				&mcpv1alpha1.MCPServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "unrelated-server",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.MCPServerSpec{
+						Image: "example/mcp:latest",
+						AuthzConfigRef: &mcpv1alpha1.MCPAuthzConfigReference{
+							Name: "other-config",
+						},
+					},
+				},
+				&mcpv1alpha1.VirtualMCPServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "unrelated-vmcp",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.VirtualMCPServerSpec{
+						IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{
+							Type: "anonymous",
+						},
+					},
+				},
+				&mcpv1alpha1.MCPRemoteProxy{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "unrelated-proxy",
+						Namespace: "default",
+					},
+					Spec: mcpv1alpha1.MCPRemoteProxySpec{
+						RemoteURL: "https://example.com",
+					},
+				},
+			},
+			expectEmpty: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx := t.Context()
+
+			scheme := runtime.NewScheme()
+			require.NoError(t, mcpv1alpha1.AddToScheme(scheme))
+
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(scheme).
+				WithObjects(tt.existingWorkloads...).
+				Build()
+
+			r := &MCPAuthzConfigReconciler{
+				Client: fakeClient,
+				Scheme: scheme,
+			}
+
+			authzConfig := &mcpv1alpha1.MCPAuthzConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      tt.authzConfigName,
+					Namespace: "default",
+				},
+			}
+
+			refs, err := r.findReferencingWorkloads(ctx, authzConfig)
+			require.NoError(t, err)
+
+			if tt.expectEmpty {
+				assert.Empty(t, refs)
+			} else {
+				assert.Equal(t, tt.expectedRefs, refs)
+			}
+		})
+	}
+}
+
+func TestBuildFullAuthzConfigJSON_EmptyConfigRaw(t *testing.T) {
+	t.Parallel()
+
+	spec := mcpv1alpha1.MCPAuthzConfigSpec{
+		Type:   "cedarv1",
+		Config: runtime.RawExtension{Raw: []byte{}},
+	}
+
+	result, err := BuildFullAuthzConfigJSON(spec)
+	require.Error(t, err)
+	assert.Nil(t, result)
+	assert.Contains(t, err.Error(), "config field is empty")
+}

cmd/thv-operator/main.go

@@ -32,6 +32,10 @@ import (
 	mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
 	"github.com/stacklok/toolhive/cmd/thv-operator/controllers"
 	ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
+	// Import authorizer backends so they register with the factory registry.
+	// Placed here (not in the controller) to keep the controller backend-agnostic.
+	_ "github.com/stacklok/toolhive/pkg/authz/authorizers/cedar"
+	_ "github.com/stacklok/toolhive/pkg/authz/authorizers/http"
 	"github.com/stacklok/toolhive/pkg/operator/telemetry"
 )