diff --git a/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go b/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go index 3b38d591a7..c020c7eadd 100644 --- a/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go +++ b/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" diff --git a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go index f224c48e73..b2978b7a9f 100644 --- a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go +++ b/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" diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index f3da34e04a..ebd4f7519a 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/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) } } diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go index c12788caa8..298207d825 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go +++ b/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{ diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go b/cmd/thv-operator/controllers/mcpserver_runconfig_test.go index 9aa7940d7c..1902cb3bb6 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go +++ b/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) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 5904753fc9..445c7cdeb5 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -606,6 +606,11 @@ 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, @@ -613,6 +618,9 @@ func (r *VirtualMCPServerReconciler) emitPrimaryUpstreamProviderDeprecatedEvent( 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; "+ diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index 1b49635ab9..8d69891965 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/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) diff --git a/cmd/thv-operator/pkg/controllerutil/authz.go b/cmd/thv-operator/pkg/controllerutil/authz.go index 51fb5d2782..fb829c3eb2 100644 --- a/cmd/thv-operator/pkg/controllerutil/authz.go +++ b/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") diff --git a/cmd/thv-operator/pkg/controllerutil/authz_test.go b/cmd/thv-operator/pkg/controllerutil/authz_test.go index 8effa521c2..25995f6c19 100644 --- a/cmd/thv-operator/pkg/controllerutil/authz_test.go +++ b/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);`}, diff --git a/cmd/thv-operator/test-integration/mcp-server/mcpserver_runconfig_integration_test.go b/cmd/thv-operator/test-integration/mcp-server/mcpserver_runconfig_integration_test.go index a2b42f18db..628145b8d4 100644 --- a/cmd/thv-operator/test-integration/mcp-server/mcpserver_runconfig_integration_test.go +++ b/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