Skip to content

Commit 30b57ff

Browse files
committed
Soften PrimaryUpstreamProvider docs for non-vMCP CRDs
Tightens the docstrings on the two locations of `PrimaryUpstreamProvider` to match the actual code paths and addresses F4 (and the cascading F5) from the multi-agent review on PR #5290. `EmbeddedAuthServerConfig.PrimaryUpstreamProvider`: previously claimed that the field was validated against `UpstreamProviders` on MCPServer and MCPRemoteProxy as well. No code references the field on those paths: the proxyrunner pipeline does not read it, and the existing `AuthzPrimaryUpstreamProviderIgnored` advisory inspects only the legacy inline location. The new docstring says the field is meaningful only on `VirtualMCPServer`; on single-upstream consumers it is structurally present (the struct is shared) but silently ignored. `InlineAuthzConfig.PrimaryUpstreamProvider` (deprecated): the previous deprecation block named both `spec.authServerConfig.primaryUpstreamProvider` (canonical on vMCP) and `embeddedAuthServer.primaryUpstreamProvider` on a referenced `MCPExternalAuthConfig` as canonical replacements, then immediately said the field is structurally meaningless on MCPServer/MCPRemoteProxy. Two sentences pointing in opposite directions. The new block names only the vMCP canonical location and makes explicit that on MCPServer/MCPRemoteProxy the deprecated form was always a structural no-op and the deprecation does not change that behaviour. No code changes. Regenerated `deepcopy`, CRD YAML, and CRD reference docs to pick up the docstring updates.
1 parent c26d6ac commit 30b57ff

11 files changed

Lines changed: 244 additions & 168 deletions

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -235,10 +235,17 @@ type EmbeddedAuthServerConfig struct {
235235
// PrimaryUpstreamProvider names the upstream IDP whose access token Cedar
236236
// should read claims from when authorising a request. Must match the name
237237
// of one of the entries in UpstreamProviders. When empty, the controller
238-
// auto-selects the first entry of UpstreamProviders. Only meaningful when
239-
// at least one upstream is configured; on MCPServer and MCPRemoteProxy
240-
// (single-upstream consumers) the only validated values are empty or the
241-
// name of the sole upstream.
238+
// auto-selects the first entry of UpstreamProviders.
239+
//
240+
// Only meaningful on VirtualMCPServer, where multiple upstream providers
241+
// can be configured and Cedar needs to pick which token's claims to
242+
// evaluate. The VirtualMCPServer controller validates this field against
243+
// UpstreamProviders at admission and rejects unresolvable values.
244+
//
245+
// On MCPServer and MCPRemoteProxy this field is structurally present (the
246+
// EmbeddedAuthServerConfig struct is shared) but has no runtime effect:
247+
// those CRDs are restricted to a single upstream so there is no choice to
248+
// make. Setting it on those CRDs is silently ignored.
242249
// +optional
243250
// +kubebuilder:validation:MinLength=1
244251
// +kubebuilder:validation:MaxLength=63

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

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -788,16 +788,17 @@ type InlineAuthzConfig struct {
788788
// PrimaryUpstreamProvider names the upstream IDP whose access token's
789789
// claims Cedar should evaluate.
790790
//
791-
// Deprecated: this field has moved to
792-
// spec.authServerConfig.primaryUpstreamProvider on VirtualMCPServer (or to
793-
// the embeddedAuthServer.primaryUpstreamProvider field of a referenced
794-
// MCPExternalAuthConfig for MCPServer and MCPRemoteProxy). The old
795-
// location is read for one release for backward compatibility and a
796-
// Warning event is emitted whenever it is consumed; planned removal in
797-
// the release after the deprecation cycle. On MCPServer and MCPRemoteProxy
798-
// the field remains structurally meaningless (no embedded auth server
799-
// runtime) and continues to surface the AuthzPrimaryUpstreamProviderIgnored
800-
// advisory.
791+
// Deprecated: on VirtualMCPServer this field has moved to
792+
// spec.authServerConfig.primaryUpstreamProvider. The old location is
793+
// still read for one release for backward compatibility; the
794+
// VirtualMCPServer controller emits an AuthzPrimaryUpstreamProviderDeprecated
795+
// Warning event whenever it is consumed, and removal is planned for the
796+
// release after the deprecation cycle.
797+
//
798+
// On MCPServer and MCPRemoteProxy this field has always been a structural
799+
// no-op (those CRDs do not run an embedded auth server). Setting it
800+
// continues to surface the AuthzPrimaryUpstreamProviderIgnored advisory
801+
// condition; the deprecation does not change that behaviour.
801802
// +optional
802803
// +kubebuilder:validation:MinLength=1
803804
// +kubebuilder:validation:MaxLength=63

deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -272,10 +272,17 @@ spec:
272272
PrimaryUpstreamProvider names the upstream IDP whose access token Cedar
273273
should read claims from when authorising a request. Must match the name
274274
of one of the entries in UpstreamProviders. When empty, the controller
275-
auto-selects the first entry of UpstreamProviders. Only meaningful when
276-
at least one upstream is configured; on MCPServer and MCPRemoteProxy
277-
(single-upstream consumers) the only validated values are empty or the
278-
name of the sole upstream.
275+
auto-selects the first entry of UpstreamProviders.
276+
277+
Only meaningful on VirtualMCPServer, where multiple upstream providers
278+
can be configured and Cedar needs to pick which token's claims to
279+
evaluate. The VirtualMCPServer controller validates this field against
280+
UpstreamProviders at admission and rejects unresolvable values.
281+
282+
On MCPServer and MCPRemoteProxy this field is structurally present (the
283+
EmbeddedAuthServerConfig struct is shared) but has no runtime effect:
284+
those CRDs are restricted to a single upstream so there is no choice to
285+
make. Setting it on those CRDs is silently ignored.
279286
maxLength: 63
280287
minLength: 1
281288
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$
@@ -1527,10 +1534,17 @@ spec:
15271534
PrimaryUpstreamProvider names the upstream IDP whose access token Cedar
15281535
should read claims from when authorising a request. Must match the name
15291536
of one of the entries in UpstreamProviders. When empty, the controller
1530-
auto-selects the first entry of UpstreamProviders. Only meaningful when
1531-
at least one upstream is configured; on MCPServer and MCPRemoteProxy
1532-
(single-upstream consumers) the only validated values are empty or the
1533-
name of the sole upstream.
1537+
auto-selects the first entry of UpstreamProviders.
1538+
1539+
Only meaningful on VirtualMCPServer, where multiple upstream providers
1540+
can be configured and Cedar needs to pick which token's claims to
1541+
evaluate. The VirtualMCPServer controller validates this field against
1542+
UpstreamProviders at admission and rejects unresolvable values.
1543+
1544+
On MCPServer and MCPRemoteProxy this field is structurally present (the
1545+
EmbeddedAuthServerConfig struct is shared) but has no runtime effect:
1546+
those CRDs are restricted to a single upstream so there is no choice to
1547+
make. Setting it on those CRDs is silently ignored.
15341548
maxLength: 63
15351549
minLength: 1
15361550
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$

deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpremoteproxies.yaml

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -160,16 +160,17 @@ spec:
160160
PrimaryUpstreamProvider names the upstream IDP whose access token's
161161
claims Cedar should evaluate.
162162
163-
Deprecated: this field has moved to
164-
spec.authServerConfig.primaryUpstreamProvider on VirtualMCPServer (or to
165-
the embeddedAuthServer.primaryUpstreamProvider field of a referenced
166-
MCPExternalAuthConfig for MCPServer and MCPRemoteProxy). The old
167-
location is read for one release for backward compatibility and a
168-
Warning event is emitted whenever it is consumed; planned removal in
169-
the release after the deprecation cycle. On MCPServer and MCPRemoteProxy
170-
the field remains structurally meaningless (no embedded auth server
171-
runtime) and continues to surface the AuthzPrimaryUpstreamProviderIgnored
172-
advisory.
163+
Deprecated: on VirtualMCPServer this field has moved to
164+
spec.authServerConfig.primaryUpstreamProvider. The old location is
165+
still read for one release for backward compatibility; the
166+
VirtualMCPServer controller emits an AuthzPrimaryUpstreamProviderDeprecated
167+
Warning event whenever it is consumed, and removal is planned for the
168+
release after the deprecation cycle.
169+
170+
On MCPServer and MCPRemoteProxy this field has always been a structural
171+
no-op (those CRDs do not run an embedded auth server). Setting it
172+
continues to surface the AuthzPrimaryUpstreamProviderIgnored advisory
173+
condition; the deprecation does not change that behaviour.
173174
maxLength: 63
174175
minLength: 1
175176
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$
@@ -778,16 +779,17 @@ spec:
778779
PrimaryUpstreamProvider names the upstream IDP whose access token's
779780
claims Cedar should evaluate.
780781
781-
Deprecated: this field has moved to
782-
spec.authServerConfig.primaryUpstreamProvider on VirtualMCPServer (or to
783-
the embeddedAuthServer.primaryUpstreamProvider field of a referenced
784-
MCPExternalAuthConfig for MCPServer and MCPRemoteProxy). The old
785-
location is read for one release for backward compatibility and a
786-
Warning event is emitted whenever it is consumed; planned removal in
787-
the release after the deprecation cycle. On MCPServer and MCPRemoteProxy
788-
the field remains structurally meaningless (no embedded auth server
789-
runtime) and continues to surface the AuthzPrimaryUpstreamProviderIgnored
790-
advisory.
782+
Deprecated: on VirtualMCPServer this field has moved to
783+
spec.authServerConfig.primaryUpstreamProvider. The old location is
784+
still read for one release for backward compatibility; the
785+
VirtualMCPServer controller emits an AuthzPrimaryUpstreamProviderDeprecated
786+
Warning event whenever it is consumed, and removal is planned for the
787+
release after the deprecation cycle.
788+
789+
On MCPServer and MCPRemoteProxy this field has always been a structural
790+
no-op (those CRDs do not run an embedded auth server). Setting it
791+
continues to surface the AuthzPrimaryUpstreamProviderIgnored advisory
792+
condition; the deprecation does not change that behaviour.
791793
maxLength: 63
792794
minLength: 1
793795
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$

deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -167,16 +167,17 @@ spec:
167167
PrimaryUpstreamProvider names the upstream IDP whose access token's
168168
claims Cedar should evaluate.
169169
170-
Deprecated: this field has moved to
171-
spec.authServerConfig.primaryUpstreamProvider on VirtualMCPServer (or to
172-
the embeddedAuthServer.primaryUpstreamProvider field of a referenced
173-
MCPExternalAuthConfig for MCPServer and MCPRemoteProxy). The old
174-
location is read for one release for backward compatibility and a
175-
Warning event is emitted whenever it is consumed; planned removal in
176-
the release after the deprecation cycle. On MCPServer and MCPRemoteProxy
177-
the field remains structurally meaningless (no embedded auth server
178-
runtime) and continues to surface the AuthzPrimaryUpstreamProviderIgnored
179-
advisory.
170+
Deprecated: on VirtualMCPServer this field has moved to
171+
spec.authServerConfig.primaryUpstreamProvider. The old location is
172+
still read for one release for backward compatibility; the
173+
VirtualMCPServer controller emits an AuthzPrimaryUpstreamProviderDeprecated
174+
Warning event whenever it is consumed, and removal is planned for the
175+
release after the deprecation cycle.
176+
177+
On MCPServer and MCPRemoteProxy this field has always been a structural
178+
no-op (those CRDs do not run an embedded auth server). Setting it
179+
continues to surface the AuthzPrimaryUpstreamProviderIgnored advisory
180+
condition; the deprecation does not change that behaviour.
180181
maxLength: 63
181182
minLength: 1
182183
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$
@@ -1083,16 +1084,17 @@ spec:
10831084
PrimaryUpstreamProvider names the upstream IDP whose access token's
10841085
claims Cedar should evaluate.
10851086
1086-
Deprecated: this field has moved to
1087-
spec.authServerConfig.primaryUpstreamProvider on VirtualMCPServer (or to
1088-
the embeddedAuthServer.primaryUpstreamProvider field of a referenced
1089-
MCPExternalAuthConfig for MCPServer and MCPRemoteProxy). The old
1090-
location is read for one release for backward compatibility and a
1091-
Warning event is emitted whenever it is consumed; planned removal in
1092-
the release after the deprecation cycle. On MCPServer and MCPRemoteProxy
1093-
the field remains structurally meaningless (no embedded auth server
1094-
runtime) and continues to surface the AuthzPrimaryUpstreamProviderIgnored
1095-
advisory.
1087+
Deprecated: on VirtualMCPServer this field has moved to
1088+
spec.authServerConfig.primaryUpstreamProvider. The old location is
1089+
still read for one release for backward compatibility; the
1090+
VirtualMCPServer controller emits an AuthzPrimaryUpstreamProviderDeprecated
1091+
Warning event whenever it is consumed, and removal is planned for the
1092+
release after the deprecation cycle.
1093+
1094+
On MCPServer and MCPRemoteProxy this field has always been a structural
1095+
no-op (those CRDs do not run an embedded auth server). Setting it
1096+
continues to surface the AuthzPrimaryUpstreamProviderIgnored advisory
1097+
condition; the deprecation does not change that behaviour.
10961098
maxLength: 63
10971099
minLength: 1
10981100
pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$

0 commit comments

Comments
 (0)