Wire MCPAuthzConfig references into workload controllers#4778
Wire MCPAuthzConfig references into workload controllers#4778JAORMX wants to merge 2 commits intojaosorior/mcpauthzconfig-crdfrom
Conversation
The existing AuthzConfigRef inline variant is Cedar-specific, with fields that only make sense for the Cedar authorizer. This adds a new MCPAuthzConfig CRD that accepts any registered authorizer backend configuration via a type discriminator and an opaque config blob. The MCPAuthzConfig CRD follows the established shared-config pattern (MCPOIDCConfig, MCPExternalAuthConfig) with status conditions, hash-based change detection, referencing workload tracking, and deletion protection. A new ConfigKey() method on the AuthorizerFactory interface enables the controller to reconstruct the full runtime config JSON from the CRD spec. The AuthzConfigRef field is added to MCPServer, MCPRemoteProxy, and VirtualMCPServer as a new option alongside the existing authzConfig field, enabling incremental migration with no breaking changes. Refs: #3157 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MCPAuthzConfig resources created in PR 1 need to be consumed by the workload controllers (MCPServer, MCPRemoteProxy, VirtualMCPServer) so that shared authorization configs actually take effect at runtime. - Add CEL mutual exclusion for authzConfig/authzConfigRef on all specs - Add AuthzConfigHash to all workload status types for change detection - Add condition constants (AuthzConfigRefValidated) and reasons - Move BuildFullAuthzConfigJSON to controllerutil for shared use - Add controllerutil helpers: GetAuthzConfigForWorkload, ValidateAuthzConfigReady, GenerateAuthzVolumeConfigFromRef, EnsureAuthzConfigMapFromRef, AddAuthzConfigRefOptions - Add handleAuthzConfig to MCPServer, MCPRemoteProxy, VirtualMCPServer controllers following the OIDC config ref pattern - Add MCPAuthzConfig watches to all three workload controllers - Update runconfig builders to resolve AuthzConfigRef - Update vmcpconfig converter to extract Cedar policies from MCPAuthzConfig for VirtualMCPServer runtime config - Add SetAuthzConfigHash to VirtualMCPServer status manager Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JAORMX
requested review from
ChrisJBurns,
jerm-dro,
jhrozek and
yrobla
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## jaosorior/mcpauthzconfig-crd #4778 +/- ##
================================================================
- Coverage 68.85% 68.44% -0.41%
================================================================
Files 520 519 -1
Lines 55137 55199 +62
================================================================
- Hits 37964 37783 -181
- Misses 14242 14499 +257
+ Partials 2931 2917 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) | ||
| } | ||
|
|
||
| // handleAuthzConfig validates and tracks the hash of the referenced MCPAuthzConfig. | ||
| // It updates the MCPServer status when the authorization configuration changes and sets | ||
| // the AuthzConfigRefValidated condition. | ||
| func (r *MCPServerReconciler) handleAuthzConfig(ctx context.Context, m *mcpv1alpha1.MCPServer) error { | ||
| ctxLogger := log.FromContext(ctx) | ||
|
|
||
| if m.Spec.AuthzConfigRef == nil { | ||
| // No MCPAuthzConfig referenced, clear any stored hash | ||
| if m.Status.AuthzConfigHash != "" { | ||
| m.Status.AuthzConfigHash = "" | ||
| if err := r.Status().Update(ctx, m); err != nil { | ||
| return fmt.Errorf("failed to clear MCPAuthzConfig hash from status: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔴 When AuthzConfigRef is removed from an MCPServer or VirtualMCPServer spec, the ConditionAuthzConfigRefValidated condition is left stale in status instead of being cleared. This causes the condition to remain True indefinitely, misleading operators and any tooling that inspects status conditions about whether authorization is still active.
Extended reasoning...
Root cause — MCPServer
In MCPServer.handleAuthzConfig() (mcpserver_controller.go), the nil-ref branch clears AuthzConfigHash but never calls meta.RemoveStatusCondition:
if m.Spec.AuthzConfigRef == nil {
if m.Status.AuthzConfigHash != "" {
m.Status.AuthzConfigHash = ""
if err := r.Status().Update(ctx, m); err != nil { ... }
}
return nil // ← ConditionAuthzConfigRefValidated is never removed
}Root cause — VirtualMCPServer
In VirtualMCPServer.handleAuthzConfig() (virtualmcpserver_controller.go), the nil-ref branch has the same omission through the StatusManager abstraction:
if vmcp.Spec.IncomingAuth == nil || vmcp.Spec.IncomingAuth.AuthzConfigRef == nil {
if vmcp.Status.AuthzConfigHash != "" {
statusManager.SetAuthzConfigHash("")
}
return nil // ← no SetCondition/removal call for ConditionAuthzConfigRefValidated
}The reference implementation shows the correct pattern
MCPRemoteProxy.handleAuthzConfig() (introduced in the same PR) does it correctly:
if proxy.Spec.AuthzConfigRef == nil {
meta.RemoveStatusCondition(&proxy.Status.Conditions, mcpv1alpha1.ConditionAuthzConfigRefValidated)
if proxy.Status.AuthzConfigHash != "" { ... }
return nil
}Step-by-step proof (MCPServer)
- User creates an
MCPServerwithspec.authzConfigRef.name: my-authz. - Controller reconciles:
handleAuthzConfigfetches the config, validates it, setsConditionAuthzConfigRefValidated=Trueand stores the hash. - User removes
spec.authzConfigReffrom the MCPServer (sets to nil). - Controller reconciles:
handleAuthzConfigenters the nil branch, clearsAuthzConfigHash, and returns — no call tometa.RemoveStatusCondition. - Result:
kubectl get mcpserver <name> -o yamlstill showsConditionAuthzConfigRefValidated=Trueindefinitely, even though no authorization config is referenced.
The same sequence applies to VirtualMCPServer via the StatusManager path.
Why existing code doesn't prevent it
The StatusCollector for VirtualMCPServer is instantiated fresh each reconciliation with an empty conditions map. Conditions not explicitly set during a reconcile pass are left untouched in the live status (only conditions stored in s.conditions are applied in UpdateStatus). So the omission of a SetCondition call with removal intent means the live stale condition persists.
Fix
- MCPServer: Add
meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1alpha1.ConditionAuthzConfigRefValidated)at the start of the nil branch, matching MCPRemoteProxy. - VirtualMCPServer: Call
statusManager.SetCondition(mcpv1alpha1.ConditionAuthzConfigRefValidated, "", "", "")or use the existing empty-Status marker trick inRemoveConditionsWithPrefixso the collector removes the condition inUpdateStatus.
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Looks good -- solid pattern consistency with MCPOIDCConfig. Two minor nits inline.
| // set the type. Full support for other backends is a follow-up. | ||
| return &vmcpconfig.AuthzConfig{ | ||
| Type: authzConfig.Spec.Type, | ||
| }, nil |
There was a problem hiding this comment.
nit: For non-Cedar types this returns an AuthzConfig with only Type set and no actual config. At runtime the vMCP would fail with an unclear error. Would it be better to fail fast here?
| }, nil | |
| return nil, fmt.Errorf("MCPAuthzConfig type %%q is not yet supported for VirtualMCPServer; only cedarv1 is supported", authzConfig.Spec.Type) |
|
nit: // +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 |
JAORMX
force-pushed
the
jaosorior/mcpauthzconfig-crd
branch
from
734e8db to
d571e89
Compare
2 participants
Summary
Type of change
Test plan
task test)task lint-fix)Changes
api/v1alpha1/mcpserver_types.goapi/v1alpha1/mcpremoteproxy_types.goapi/v1alpha1/virtualmcpserver_types.gopkg/controllerutil/authz.gocontrollers/mcpauthzconfig_controller.gocontrollers/mcpserver_controller.gocontrollers/mcpserver_runconfig.gocontrollers/mcpremoteproxy_controller.gocontrollers/mcpremoteproxy_runconfig.gocontrollers/virtualmcpserver_controller.gopkg/virtualmcpserverstatus/types.gopkg/virtualmcpserverstatus/collector.gopkg/vmcpconfig/converter.goSpecial notes for reviewers
BuildFullAuthzConfigJSONwas moved from the MCPAuthzConfig controller tocontrollerutilso both the config controller and workload controllers can use it without circular imports.Generated with Claude Code