Address post-approval review feedback on VirtualMCPServer authz

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

@@ -332,9 +332,17 @@ const (
 	// ConditionReasonAuthzPolicySyntaxInvalid indicates an inline Cedar policy has a syntax error
 	ConditionReasonAuthzPolicySyntaxInvalid = "AuthzPolicySyntaxInvalid"
 
-	// ConditionReasonAuthzConfigMapNotFound indicates the referenced authz ConfigMap was not found
+	// ConditionReasonAuthzConfigMapNotFound indicates the referenced authz ConfigMap was not found.
+	// Shared with VirtualMCPServer (see virtualmcpserver_types.go); both reconcilers use this
+	// reason when the ConfigMap itself is absent.
 	ConditionReasonAuthzConfigMapNotFound = "AuthzConfigMapNotFound"
 
+	// ConditionReasonAuthzConfigMapInvalid indicates the referenced authz ConfigMap was found
+	// but its payload is missing/empty/malformed, fails validation, or does not contain a
+	// Cedar-flavoured config. Shared with VirtualMCPServer; both reconcilers use this reason
+	// to distinguish a malformed payload from a missing ConfigMap.
+	ConditionReasonAuthzConfigMapInvalid = "AuthzConfigMapInvalid"
+
 	// ConditionReasonHeaderSecretNotFound indicates a referenced header Secret was not found
 	ConditionReasonHeaderSecretNotFound = "HeaderSecretNotFound"
 

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

@@ -338,12 +338,8 @@ const (
 	// ConditionReasonIncomingAuthInvalid indicates incoming auth is invalid
 	ConditionReasonIncomingAuthInvalid = "IncomingAuthInvalid"
 
-	// Note: ConditionReasonAuthzConfigMapNotFound is shared with MCPRemoteProxy and is
-	// declared in mcpremoteproxy_types.go.
-
-	// ConditionReasonAuthzConfigMapInvalid indicates the referenced authz ConfigMap was
-	// found but its payload is missing/empty/malformed or fails Cedar validation.
-	ConditionReasonAuthzConfigMapInvalid = "AuthzConfigMapInvalid"
+	// Note: ConditionReasonAuthzConfigMapNotFound and ConditionReasonAuthzConfigMapInvalid
+	// are shared with MCPRemoteProxy and are declared in mcpremoteproxy_types.go.
 
 	// ConditionReasonGroupRefValid indicates the GroupRef is valid
 	ConditionReasonVirtualMCPServerGroupRefValid = "GroupRefValid"

cmd/thv-operator/controllers/mcpremoteproxy_controller.go

@@ -460,42 +460,34 @@ func (*MCPRemoteProxyReconciler) validateAuthzPolicySyntax(
 }
 
 // validateK8sRefs validates that referenced ConfigMaps and Secrets exist.
+// Authz ConfigMap references are validated through the shared loader so a malformed
+// payload surfaces as AuthzConfigMapInvalid here instead of crashing the runner pod.
 func (r *MCPRemoteProxyReconciler) validateK8sRefs(
 	ctx context.Context, proxy *mcpv1beta1.MCPRemoteProxy,
 ) error {
-	// Check authz ConfigMap reference
+	// Check authz ConfigMap reference.
 	if proxy.Spec.AuthzConfig != nil &&
 		proxy.Spec.AuthzConfig.Type == mcpv1beta1.AuthzConfigTypeConfigMap &&
 		proxy.Spec.AuthzConfig.ConfigMap != nil {
-		cm := &corev1.ConfigMap{}
 		cmName := proxy.Spec.AuthzConfig.ConfigMap.Name
-		err := r.Get(ctx, types.NamespacedName{
-			Name: cmName, Namespace: proxy.Namespace,
-		}, cm)
+		cfg, err := ctrlutil.LoadAuthzConfigFromConfigMap(ctx, r.Client, proxy.Namespace, proxy.Spec.AuthzConfig)
+		if err == nil {
+			if _, cerr := ctrlutil.ExtractCedarAuthzOptions(cfg); cerr != nil {
+				err = fmt.Errorf("authz ConfigMap %q is not a Cedar config: %w", cmName, cerr)
+			}
+		}
 		if err != nil {
+			reason := mcpv1beta1.ConditionReasonAuthzConfigMapInvalid
+			msg := fmt.Sprintf("authorization ConfigMap %q is invalid: %v", cmName, err)
 			if errors.IsNotFound(err) {
-				msg := fmt.Sprintf(
-					"authorization ConfigMap %q not found in namespace %q",
-					cmName, proxy.Namespace,
-				)
-				r.recordValidationEvent(
-					proxy,
-					mcpv1beta1.ConditionReasonAuthzConfigMapNotFound,
-					msg,
-				)
-				setConfigurationInvalidCondition(
-					proxy,
-					mcpv1beta1.ConditionReasonAuthzConfigMapNotFound,
-					msg,
-				)
-				return stderrors.New(msg)
+				reason = mcpv1beta1.ConditionReasonAuthzConfigMapNotFound
+				msg = fmt.Sprintf("authorization ConfigMap %q not found in namespace %q", cmName, proxy.Namespace)
+			} else {
+				log.FromContext(ctx).Error(err, "Authz ConfigMap validation failed", "name", cmName, "namespace", proxy.Namespace)
 			}
-			ctxLogger := log.FromContext(ctx)
-			ctxLogger.Error(err, "Failed to fetch authorization ConfigMap", "name", cmName, "namespace", proxy.Namespace)
-			genericMsg := fmt.Sprintf("failed to fetch authorization ConfigMap %q", cmName)
-			r.recordValidationEvent(proxy, mcpv1beta1.ConditionReasonAuthzConfigMapNotFound, genericMsg)
-			setConfigurationInvalidCondition(proxy, mcpv1beta1.ConditionReasonAuthzConfigMapNotFound, genericMsg)
-			return stderrors.New(genericMsg)
+			r.recordValidationEvent(proxy, reason, msg)
+			setConfigurationInvalidCondition(proxy, reason, msg)
+			return stderrors.New(msg)
 		}
 	}
 

cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go

@@ -809,6 +809,58 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
 			expectCondition: mcpv1beta1.ConditionReasonHeaderSecretNotFound,
 			conditionStatus: metav1.ConditionFalse,
 		},
+		{
+			name: "referenced authz ConfigMap with malformed payload is rejected",
+			proxy: &mcpv1beta1.MCPRemoteProxy{
+				ObjectMeta: metav1.ObjectMeta{Name: "malformed-configmap-proxy", Namespace: "default"},
+				Spec: mcpv1beta1.MCPRemoteProxySpec{
+					RemoteURL: "https://mcp.example.com",
+					AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+						Type: mcpv1beta1.AuthzConfigTypeConfigMap,
+						ConfigMap: &mcpv1beta1.ConfigMapAuthzRef{
+							Name: "malformed-authz",
+						},
+					},
+				},
+			},
+			existingObjects: []runtime.Object{
+				&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{Name: "malformed-authz", Namespace: "default"},
+					Data:       map[string]string{ctrlutil.DefaultAuthzKey: "this is not valid yaml or json: {"},
+				},
+			},
+			expectError:     true,
+			errContains:     "malformed-authz",
+			expectCondition: mcpv1beta1.ConditionReasonAuthzConfigMapInvalid,
+			conditionStatus: metav1.ConditionFalse,
+		},
+		{
+			name: "referenced authz ConfigMap with non-Cedar payload is rejected",
+			proxy: &mcpv1beta1.MCPRemoteProxy{
+				ObjectMeta: metav1.ObjectMeta{Name: "non-cedar-configmap-proxy", Namespace: "default"},
+				Spec: mcpv1beta1.MCPRemoteProxySpec{
+					RemoteURL: "https://mcp.example.com",
+					AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+						Type: mcpv1beta1.AuthzConfigTypeConfigMap,
+						ConfigMap: &mcpv1beta1.ConfigMapAuthzRef{
+							Name: "non-cedar-authz",
+						},
+					},
+				},
+			},
+			existingObjects: []runtime.Object{
+				&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{Name: "non-cedar-authz", Namespace: "default"},
+					Data: map[string]string{
+						ctrlutil.DefaultAuthzKey: `{"version":"1.0","type":"httpv1","pdp":{"http":{"url":"http://pdp.example.com"},"claim_mapping":"standard"}}`,
+					},
+				},
+			},
+			expectError:     true,
+			errContains:     "not a Cedar config",
+			expectCondition: mcpv1beta1.ConditionReasonAuthzConfigMapInvalid,
+			conditionStatus: metav1.ConditionFalse,
+		},
 		{
 			name: "malformed remote URL is rejected",
 			proxy: &mcpv1beta1.MCPRemoteProxy{

cmd/thv-operator/controllers/mcpserver_runconfig_test.go

@@ -417,7 +417,7 @@ func TestCreateRunConfigFromMCPServer(t *testing.T) {
 
 				// Verify authorization config is set
 				assert.NotNil(t, config.AuthzConfig)
-				assert.Equal(t, "v1", config.AuthzConfig.Version)
+				assert.Equal(t, ctrlutil.AuthzConfigVersion, config.AuthzConfig.Version)
 				assert.Equal(t, authz.ConfigType(cedar.ConfigType), config.AuthzConfig.Type)
 
 				// Check Cedar-specific configuration
@@ -455,7 +455,7 @@ func TestCreateRunConfigFromMCPServer(t *testing.T) {
 
 				// For ConfigMap type, with new feature, authorization config is embedded in RunConfig
 				require.NotNil(t, config.AuthzConfig)
-				assert.Equal(t, "v1", config.AuthzConfig.Version)
+				assert.Equal(t, ctrlutil.AuthzConfigVersion, config.AuthzConfig.Version)
 				assert.Equal(t, authz.ConfigType(cedar.ConfigType), config.AuthzConfig.Type)
 
 				cedarCfg, err := cedar.ExtractConfig(config.AuthzConfig)
@@ -493,7 +493,7 @@ func TestCreateRunConfigFromMCPServer(t *testing.T) {
 							}
 							return ctrlutil.DefaultAuthzKey
 						}(): `{
-							"version": "v1",
+							"version": "1.0",
 							"type": "cedarv1",
 							"cedar": {
 								"policies": [
@@ -799,7 +799,7 @@ func TestEnsureRunConfigConfigMap(t *testing.T) {
 
 				// Verify authorization configuration is properly serialized
 				assert.NotNil(t, runConfig.AuthzConfig, "AuthzConfig should be present in runconfig.json")
-				assert.Equal(t, "v1", runConfig.AuthzConfig.Version)
+				assert.Equal(t, ctrlutil.AuthzConfigVersion, runConfig.AuthzConfig.Version)
 				assert.Equal(t, authz.ConfigType(cedar.ConfigType), runConfig.AuthzConfig.Type)
 
 				// Check Cedar-specific configuration
@@ -927,7 +927,7 @@ func TestEnsureRunConfigConfigMap(t *testing.T) {
 			},
 			Data: map[string]string{
 				"authz.json": `{
-					"version": "v1",
+					"version": "1.0",
 					"type": "cedarv1",
 					"cedar": {
 						"policies": [
@@ -965,7 +965,7 @@ func TestEnsureRunConfigConfigMap(t *testing.T) {
 		require.NoError(t, err)
 
 		require.NotNil(t, runConfig.AuthzConfig)
-		assert.Equal(t, "v1", runConfig.AuthzConfig.Version)
+		assert.Equal(t, ctrlutil.AuthzConfigVersion, runConfig.AuthzConfig.Version)
 		assert.Equal(t, authz.ConfigType(cedar.ConfigType), runConfig.AuthzConfig.Type)
 
 		cedarCfg, err := cedar.ExtractConfig(runConfig.AuthzConfig)

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -606,13 +606,21 @@ func rejectAuthzAdmission(
 // spec.incomingAuth.authzConfig.inline.primaryUpstreamProvider location.
 // Called from every validation branch that observes the explicit provider so
 // the kubectl-visible hint is consistent regardless of the validation outcome.
+//
+// Only emits when the spec has changed since the last observed generation, so
+// the event fires once per spec change instead of on every reconcile. K8s
+// event aggregation would dedupe within a 10-minute window anyway, but spec
+// changes are the load-bearing signal users care about.
 func (r *VirtualMCPServerReconciler) emitPrimaryUpstreamProviderDeprecatedEvent(
 	vmcp *mcpv1beta1.VirtualMCPServer,
 	fromDeprecated bool,
 ) {
 	if !fromDeprecated || r.Recorder == nil {
 		return
 	}
+	if vmcp.Generation == vmcp.Status.ObservedGeneration {
+		return
+	}
 	r.Recorder.Eventf(vmcp, nil, corev1.EventTypeWarning,
 		"AuthzPrimaryUpstreamProviderDeprecated", "ResolvePrimaryUpstreamProvider",
 		"spec.incomingAuth.authzConfig.inline.primaryUpstreamProvider is deprecated; "+

cmd/thv-operator/controllers/virtualmcpserver_controller_test.go

@@ -3832,11 +3832,12 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable_DeprecationEvent(t *test
 	}
 
 	tests := []struct {
-		name             string
-		incomingAuth     *mcpv1beta1.IncomingAuthConfig
-		authServerConfig *mcpv1beta1.EmbeddedAuthServerConfig
-		wantEvent        bool
-		wantError        bool
+		name               string
+		incomingAuth       *mcpv1beta1.IncomingAuthConfig
+		authServerConfig   *mcpv1beta1.EmbeddedAuthServerConfig
+		observedGeneration int64
+		wantEvent          bool
+		wantError          bool
 	}{
 		{
 			name: "deprecated inline primary emits the deprecation event",
@@ -3852,6 +3853,25 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable_DeprecationEvent(t *test
 			},
 			wantEvent: true,
 		},
+		{
+			// Steady-state reconcile (e.g. watch resync, dependent resource
+			// change) on a VMCP whose spec has already been observed. The
+			// deprecation hint must not fire again until the user changes the
+			// spec, so logs are not flooded during the deprecation window.
+			name: "deprecated inline primary suppresses event when generation already observed",
+			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type:        "oidc",
+				AuthzConfig: inlineAuthzRefWithDeprecatedPrimary,
+			},
+			authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{
+				Issuer: "https://authserver.example.com",
+				UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{
+					{Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC},
+				},
+			},
+			observedGeneration: 1,
+			wantEvent:          false,
+		},
 		{
 			name: "canonical authServerConfig primary does not emit the event",
 			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
@@ -3913,6 +3933,9 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable_DeprecationEvent(t *test
 					IncomingAuth:     tt.incomingAuth,
 					AuthServerConfig: tt.authServerConfig,
 				},
+				Status: mcpv1beta1.VirtualMCPServerStatus{
+					ObservedGeneration: tt.observedGeneration,
+				},
 			}
 
 			recorder := events.NewFakeRecorder(10)

cmd/thv-operator/pkg/controllerutil/authz.go

@@ -27,6 +27,13 @@ import (
 const (
 	// DefaultAuthzKey is the default key for authorization policies in ConfigMaps
 	DefaultAuthzKey = "authz.json"
+
+	// AuthzConfigVersion is the version stamped onto authz configs written or
+	// constructed by the operator. Kept consistent across `EnsureAuthzConfigMap`
+	// and `BuildInlineCedarAuthzConfig` so a future version enum on
+	// `authz.Config.Validate()` does not see two divergent values from the same
+	// pipeline. The rest of `pkg/authz/` uses the same literal.
+	AuthzConfigVersion = "1.0"
 )
 
 // GenerateAuthzVolumeConfig generates volume mount and volume for authorization policies
@@ -128,7 +135,7 @@ func EnsureAuthzConfigMap(
 	configMapName := fmt.Sprintf("%s-authz-inline", resourceName)
 
 	authzConfigData := map[string]interface{}{
-		"version": "1.0",
+		"version": AuthzConfigVersion,
 		"type":    "cedarv1",
 		"cedar": map[string]interface{}{
 			"policies": authzConfig.Inline.Policies,
@@ -179,7 +186,7 @@ func BuildInlineCedarAuthzConfig(authzRef *mcpv1beta1.AuthzConfigRef) (*authz.Co
 		return nil, fmt.Errorf("inline authz config type specified but inline config is nil")
 	}
 	authzCfg, err := authz.NewConfig(cedar.Config{
-		Version: "v1",
+		Version: AuthzConfigVersion,
 		Type:    cedar.ConfigType,
 		Options: &cedar.ConfigOptions{
 			Policies:        authzRef.Inline.Policies,
@@ -281,9 +288,10 @@ func LoadAuthzConfigFromConfigMap(
 // (e.g. a future HTTP authorizer); callers that need to handle non-Cedar
 // configs gracefully should treat the error as "not Cedar, pass through".
 //
-// This wrapper exists so callers outside pkg/authz can avoid importing
-// pkg/authz/authorizers/cedar directly, keeping the Cedar dependency localised
-// to the resolver layer.
+// The return type is *cedar.ConfigOptions, so any caller that reads fields off
+// the result still imports pkg/authz/authorizers/cedar. Packages like
+// vmcpconfig avoid the import by copying the fields they need into a local
+// struct, which this wrapper does not do for them.
 func ExtractCedarAuthzOptions(cfg *authz.Config) (*cedar.ConfigOptions, error) {
 	if cfg == nil {
 		return nil, fmt.Errorf("authz config is nil")

cmd/thv-operator/pkg/controllerutil/authz_test.go

@@ -782,7 +782,7 @@ func TestApplyClaimMappingOverrides(t *testing.T) {
 	baseCfg := func(t *testing.T, opts cedar.ConfigOptions) *authz.Config {
 		t.Helper()
 		cfg, err := authz.NewConfig(cedar.Config{
-			Version: "v1",
+			Version: AuthzConfigVersion,
 			Type:    cedar.ConfigType,
 			Options: &opts,
 		})
@@ -913,7 +913,7 @@ func TestExtractCedarAuthzOptions(t *testing.T) {
 	t.Run("happy path returns the embedded cedar options", func(t *testing.T) {
 		t.Parallel()
 		cfg, err := authz.NewConfig(cedar.Config{
-			Version: "v1",
+			Version: AuthzConfigVersion,
 			Type:    cedar.ConfigType,
 			Options: &cedar.ConfigOptions{
 				Policies:     []string{`permit(principal, action, resource);`},

cmd/thv-operator/test-integration/mcp-server/mcpserver_runconfig_integration_test.go

@@ -17,6 +17,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
+	ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
 	"github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum"
 	"github.com/stacklok/toolhive/pkg/authz"
 	"github.com/stacklok/toolhive/pkg/authz/authorizers/cedar"
@@ -666,7 +667,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
 
 			// Verify authorization configuration
 			Expect(runConfig.AuthzConfig).NotTo(BeNil())
-			Expect(runConfig.AuthzConfig.Version).To(Equal("v1"))
+			Expect(runConfig.AuthzConfig.Version).To(Equal(ctrlutil.AuthzConfigVersion))
 			Expect(runConfig.AuthzConfig.Type).To(Equal(authz.ConfigType(cedar.ConfigType)))
 
 			cedarCfg, err := cedar.ExtractConfig(runConfig.AuthzConfig)
@@ -807,7 +808,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
 				},
 				Data: map[string]string{
 					"authz.json": `{
-						"version": "v1",
+						"version": "1.0",
 						"type": "cedarv1",
 						"cedar": {
 							"policies": [
@@ -870,7 +871,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
 
 			// Verify authorization configuration was embedded from external ConfigMap
 			Expect(runConfig.AuthzConfig).NotTo(BeNil())
-			Expect(runConfig.AuthzConfig.Version).To(Equal("v1"))
+			Expect(runConfig.AuthzConfig.Version).To(Equal(ctrlutil.AuthzConfigVersion))
 			Expect(runConfig.AuthzConfig.Type).To(Equal(authz.ConfigType(cedar.ConfigType)))
 
 			// Verify Cedar configuration