Skip to content

Commit 3fb59cf

Browse files
ChrisJBurnsclaude
andauthored
Wire MCPAuthzConfig references into the MCPServer controller (#5563)
* Wire MCPAuthzConfig references into the MCPServer controller An MCPServer that sets spec.authzConfigRef now resolves and enforces the referenced MCPAuthzConfig at runtime, mirroring the OIDCConfigRef pattern and building on the Stage-1 controllerutil helpers. Backend-agnostic: the proxy runner's authz factory handles cedarv1 and httpv1 alike. - handleAuthzConfig: fetch + validate-ready, track AuthzConfigHash, set the AuthzConfigRefValidated condition; on nil-ref it clears the hash AND removes the condition so a stale "valid" signal does not linger (unlike the OIDC version). Wired into Reconcile after handleOIDCConfig. - mapAuthzConfigToServers watch (extracted as a named method) + Watches on MCPAuthzConfig so a config change re-reconciles referencing servers. - Runtime resolution via AddAuthzConfigRefOptions in the runconfig builder; ConfigMap materialization via EnsureAuthzConfigMapFromRef and a mounted volume via GenerateAuthzVolumeConfigFromRef. Inline spec.authzConfig is untouched and remains mutually exclusive (CRD XValidation). - Add the mcpauthzconfigs get;list;watch RBAC marker. - Unit tests for handleAuthzConfig and an envtest integration suite proving both backends, the watch re-reconcile, ConfigMap materialization, and the invalid case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard ref authz volume against duplicate volume name Belt-and-suspenders for review feedback: only add the spec.authzConfigRef volume when the inline authz volume was not added. Inline and ref share the "authz-config" volume name and are mutually exclusive via CRD XValidation, so a hypothetical CEL regression now degrades to "inline wins" rather than producing an invalid pod spec with a duplicate volume name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review feedback on MCPServer authz wiring - Document fail-stale-on-revocation semantics on handleAuthzConfig: a previously-valid ref that later becomes invalid/missing leaves an already- running workload enforcing its last-applied policy (fail-stale, not fail-open) while the MCPServer is marked Failed/Ready=False. (#2) - Note on GenerateAuthzVolumeConfigFromRef that the mounted ConfigMap is not the active enforcement path in operator deployments (the proxy reads authz from the RunConfig-embedded config); the mount is for inline parity / future file-based consumption. (#3) - Reword the status-write error messages to name the MCPServer (the object being written), not the MCPAuthzConfig. (#4) - Add a transition unit test (valid -> invalid -> valid) covering the needsUpdate bookkeeping, plus an integration It asserting the valid -> invalid flip propagates to the MCPServer via the watch. (#1) - Soften the watch test's "via the watch" phrasing to claim only the observable outcome. (#5) - Tighten the unit not-found/not-valid assertions with ErrorContains. (#6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 55de829 commit 3fb59cf

5 files changed

Lines changed: 630 additions & 3 deletions

File tree

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package controllers
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
corev1 "k8s.io/api/core/v1"
12+
"k8s.io/apimachinery/pkg/api/meta"
13+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/apimachinery/pkg/runtime"
15+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
16+
17+
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
18+
"github.com/stacklok/toolhive/pkg/container/kubernetes"
19+
)
20+
21+
// authzConfigForTest builds an MCPAuthzConfig with the given validity and hash.
22+
func authzConfigForTest(name string, valid bool, hash string) *mcpv1beta1.MCPAuthzConfig {
23+
status := metav1.ConditionFalse
24+
if valid {
25+
status = metav1.ConditionTrue
26+
}
27+
return &mcpv1beta1.MCPAuthzConfig{
28+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"},
29+
Spec: mcpv1beta1.MCPAuthzConfigSpec{
30+
Type: "cedarv1",
31+
Config: runtime.RawExtension{Raw: []byte(`{"policies":["permit(principal, action, resource);"],"entities_json":"[]"}`)},
32+
},
33+
Status: mcpv1beta1.MCPAuthzConfigStatus{
34+
ConfigHash: hash,
35+
Conditions: []metav1.Condition{{
36+
Type: mcpv1beta1.ConditionTypeAuthzConfigValid,
37+
Status: status,
38+
Reason: "Test",
39+
}},
40+
},
41+
}
42+
}
43+
44+
func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) {
45+
t.Parallel()
46+
47+
tests := []struct {
48+
name string
49+
mcpServer *mcpv1beta1.MCPServer
50+
authzConfig *mcpv1beta1.MCPAuthzConfig
51+
expectError bool
52+
expectErrContains string
53+
expectHash string
54+
expectHashCleared bool
55+
expectConditionStatus *metav1.ConditionStatus
56+
expectConditionReason string
57+
expectConditionGone bool
58+
}{
59+
{
60+
name: "no ref clears stored hash and condition",
61+
mcpServer: &mcpv1beta1.MCPServer{
62+
ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"},
63+
Spec: mcpv1beta1.MCPServerSpec{Image: "img"},
64+
Status: mcpv1beta1.MCPServerStatus{
65+
AuthzConfigHash: "old",
66+
Conditions: []metav1.Condition{{
67+
Type: mcpv1beta1.ConditionAuthzConfigRefValidated,
68+
Status: metav1.ConditionTrue,
69+
Reason: mcpv1beta1.ConditionReasonAuthzConfigRefValid,
70+
}},
71+
},
72+
},
73+
expectHashCleared: true,
74+
expectConditionGone: true,
75+
},
76+
{
77+
name: "referenced config not found sets NotFound condition",
78+
mcpServer: &mcpv1beta1.MCPServer{
79+
ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"},
80+
Spec: mcpv1beta1.MCPServerSpec{
81+
Image: "img",
82+
AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "missing"},
83+
},
84+
},
85+
expectError: true,
86+
expectErrContains: "not found",
87+
expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse),
88+
expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotFound,
89+
},
90+
{
91+
name: "referenced config not valid sets NotValid condition",
92+
mcpServer: &mcpv1beta1.MCPServer{
93+
ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"},
94+
Spec: mcpv1beta1.MCPServerSpec{
95+
Image: "img",
96+
AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "bad"},
97+
},
98+
},
99+
authzConfig: authzConfigForTest("bad", false, ""),
100+
expectError: true,
101+
expectErrContains: "not valid",
102+
expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse),
103+
expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotValid,
104+
},
105+
{
106+
name: "valid ref sets condition True and tracks hash",
107+
mcpServer: &mcpv1beta1.MCPServer{
108+
ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"},
109+
Spec: mcpv1beta1.MCPServerSpec{
110+
Image: "img",
111+
AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "ok"},
112+
},
113+
},
114+
authzConfig: authzConfigForTest("ok", true, "hash-123"),
115+
expectHash: "hash-123",
116+
expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue),
117+
expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefValid,
118+
},
119+
}
120+
121+
for _, tt := range tests {
122+
t.Run(tt.name, func(t *testing.T) {
123+
t.Parallel()
124+
ctx := t.Context()
125+
126+
scheme := runtime.NewScheme()
127+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
128+
require.NoError(t, corev1.AddToScheme(scheme))
129+
130+
objs := []runtime.Object{tt.mcpServer}
131+
if tt.authzConfig != nil {
132+
objs = append(objs, tt.authzConfig)
133+
}
134+
fakeClient := fake.NewClientBuilder().
135+
WithScheme(scheme).
136+
WithRuntimeObjects(objs...).
137+
WithStatusSubresource(&mcpv1beta1.MCPServer{}, &mcpv1beta1.MCPAuthzConfig{}).
138+
Build()
139+
140+
reconciler := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes)
141+
142+
err := reconciler.handleAuthzConfig(ctx, tt.mcpServer)
143+
if tt.expectError {
144+
assert.Error(t, err)
145+
if tt.expectErrContains != "" {
146+
assert.ErrorContains(t, err, tt.expectErrContains)
147+
}
148+
} else {
149+
assert.NoError(t, err)
150+
}
151+
152+
if tt.expectHash != "" {
153+
assert.Equal(t, tt.expectHash, tt.mcpServer.Status.AuthzConfigHash)
154+
}
155+
if tt.expectHashCleared {
156+
assert.Empty(t, tt.mcpServer.Status.AuthzConfigHash)
157+
}
158+
159+
cond := meta.FindStatusCondition(tt.mcpServer.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated)
160+
if tt.expectConditionGone {
161+
assert.Nil(t, cond, "AuthzConfigRefValidated condition must be cleared when the ref is removed")
162+
}
163+
if tt.expectConditionStatus != nil {
164+
require.NotNil(t, cond, "expected AuthzConfigRefValidated condition")
165+
assert.Equal(t, *tt.expectConditionStatus, cond.Status)
166+
assert.Equal(t, tt.expectConditionReason, cond.Reason)
167+
}
168+
})
169+
}
170+
}
171+
172+
// TestMCPServerReconciler_handleAuthzConfig_Transitions exercises the stateful
173+
// bookkeeping (needsUpdate from the prior condition, and recovery) that the
174+
// static single-state cases above do not: valid -> invalid -> valid.
175+
func TestMCPServerReconciler_handleAuthzConfig_Transitions(t *testing.T) {
176+
t.Parallel()
177+
ctx := t.Context()
178+
179+
scheme := runtime.NewScheme()
180+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
181+
require.NoError(t, corev1.AddToScheme(scheme))
182+
183+
authzConfig := authzConfigForTest("cfg", true, "h1")
184+
server := &mcpv1beta1.MCPServer{
185+
ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"},
186+
Spec: mcpv1beta1.MCPServerSpec{
187+
Image: "img",
188+
AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "cfg"},
189+
},
190+
}
191+
fakeClient := fake.NewClientBuilder().
192+
WithScheme(scheme).
193+
WithObjects(server, authzConfig).
194+
WithStatusSubresource(&mcpv1beta1.MCPServer{}, &mcpv1beta1.MCPAuthzConfig{}).
195+
Build()
196+
r := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes)
197+
198+
condStatus := func() *metav1.Condition {
199+
return meta.FindStatusCondition(server.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated)
200+
}
201+
202+
// valid -> condition True + hash tracked
203+
require.NoError(t, r.handleAuthzConfig(ctx, server))
204+
require.NotNil(t, condStatus())
205+
assert.Equal(t, metav1.ConditionTrue, condStatus().Status)
206+
assert.Equal(t, "h1", server.Status.AuthzConfigHash)
207+
208+
// flip the referenced config to invalid -> condition transitions to False/NotValid
209+
authzConfig.Status.Conditions = []metav1.Condition{{
210+
Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionFalse, Reason: "Invalidated",
211+
}}
212+
require.NoError(t, fakeClient.Status().Update(ctx, authzConfig))
213+
214+
assert.Error(t, r.handleAuthzConfig(ctx, server))
215+
require.NotNil(t, condStatus())
216+
assert.Equal(t, metav1.ConditionFalse, condStatus().Status)
217+
assert.Equal(t, mcpv1beta1.ConditionReasonAuthzConfigRefNotValid, condStatus().Reason)
218+
219+
// recover the config -> condition transitions back to True
220+
authzConfig.Status.Conditions = []metav1.Condition{{
221+
Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionTrue, Reason: "Test",
222+
}}
223+
require.NoError(t, fakeClient.Status().Update(ctx, authzConfig))
224+
225+
require.NoError(t, r.handleAuthzConfig(ctx, server))
226+
require.NotNil(t, condStatus())
227+
assert.Equal(t, metav1.ConditionTrue, condStatus().Status)
228+
}

0 commit comments

Comments
 (0)