Add authServerRef CRD types, controller logic, and unit tests#4644
Draft
tgrunnagle wants to merge 4 commits intomainfrom
Draft
Add authServerRef CRD types, controller logic, and unit tests#4644tgrunnagle wants to merge 4 commits intomainfrom
tgrunnagle wants to merge 4 commits intomainfrom
Conversation
Implements changes for issue #4640 (Phase 1 of RFC-0050): - Add AuthServerRef (TypedLocalObjectReference) to MCPServerSpec and MCPRemoteProxySpec - Add AuthServerConfigHash to MCPServerStatus and MCPRemoteProxyStatus - Add AddAuthServerRefOptions and ValidateAndAddAuthServerRefOptions utility functions - Wire conflict validation (authServerRef + externalAuthConfigRef both embedded = error) - Extend watch handlers to reconcile on authServerRef config changes - Extend MCPExternalAuthConfig controller to find resources via authServerRef - Add handleAuthServerRef config hash tracking in both reconcile loops - Add authServerRef volume/env generation for deployments - Regenerate CRD YAML and deepcopy functions - Add unit tests for AddAuthServerRefOptions and authServerRef referencing logic
Fixed issues from code review: - HIGH: Return non-NotFound errors in conflict validation instead of silently ignoring - HIGH: Add table-driven tests for ValidateAndAddAuthServerRefOptions - HIGH: Add table-driven tests for GenerateAuthServerConfigFromRef - MEDIUM: Return error for unsupported Kind in GenerateAuthServerConfigFromRef - MEDIUM: Add comments explaining reconcile-time validation in deployment builders - MEDIUM: Remove redundant name-matching in refExtractor closures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The findReferencingMCPServers and findReferencingWorkloads closures used an early-return pattern that only returned the externalAuthConfigRef name, causing authServerRef changes to be missed when both fields were set on the same server pointing to different MCPExternalAuthConfig resources. Split each closure into two separate FindReferencingMCPServers calls (one per ref field) and merge with deduplication. Also rewire findReferencingWorkloads to delegate to findReferencingMCPServers to avoid duplicating merge logic. Add test cases covering: - Server with both refs pointing to different configs (both trigger reconciliation) - Server with both refs pointing to the same config (deduplicated to one entry) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4644 +/- ##
==========================================
- Coverage 68.88% 68.82% -0.07%
==========================================
Files 505 508 +3
Lines 52437 52762 +325
==========================================
+ Hits 36121 36312 +191
- Misses 13525 13641 +116
- Partials 2791 2809 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ce887cc to
30def11
Compare
30def11 to
0234528
Compare
0234528 to
5c69e4e
Compare
…rces The MCPExternalAuthConfig controller only tracked MCPServer references, missing MCPRemoteProxy resources that also have externalAuthConfigRef and authServerRef fields. This adds: - FindReferencingMCPRemoteProxies utility in controllerutil/config.go - findReferencingMCPRemoteProxies and mapMCPRemoteProxyToExternalAuthConfig in the MCPExternalAuthConfig controller - MCPRemoteProxy listing in findReferencingWorkloads - Watches() registration for MCPRemoteProxy in SetupWithManager - Kind check on AuthServerRef before treating it as MCPExternalAuthConfig - Extracted mapMCPServerToExternalAuthConfig from anonymous closure - Consolidated GenerateAuthServerConfig/FromRef into GenerateAuthServerConfigByName - EmbeddedAuthServerConfigName helper for single auth config selection - Fixed data race in parallel tests by using factory functions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5c69e4e to
9f7ba26
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The embedded auth server currently competes with outgoing auth types (like AWS STS) for the single
externalAuthConfigRefslot on MCPServer and MCPRemoteProxy CRDs. BecauseMCPExternalAuthConfigenforces mutually exclusive types, users cannot configure both an embedded auth server for incoming client authentication and an outgoing token exchange on the same resource. This PR adds a dedicatedauthServerReffield to both CRDs, separating the embedded auth server fromexternalAuthConfigRefso both can coexist. This is Phase 1 of RFC-0050.Closes #4640
Type of change
Test plan
task test)task lint-fix)Changes
api/v1alpha1/mcpserver_types.goAuthServerReffield toMCPServerSpecandAuthServerConfigHashtoMCPServerStatusapi/v1alpha1/mcpremoteproxy_types.goAuthServerReffield toMCPRemoteProxySpecandAuthServerConfigHashtoMCPRemoteProxyStatusapi/v1alpha1/zz_generated.deepcopy.gopkg/controllerutil/authserver.goAddAuthServerRefOptions,ValidateAndAddAuthServerRefOptions, andGenerateAuthServerConfigFromRefutility functionscontrollers/mcpserver_runconfig.gocontrollers/mcpremoteproxy_runconfig.gocontrollers/mcpserver_controller.gohandleAuthServerReffor config hash tracking, extend watch handler and deployment builder for authServerRefcontrollers/mcpremoteproxy_controller.gohandleAuthServerReffor config hash tracking, extend watch handler for authServerRefcontrollers/mcpremoteproxy_deployment.gocontrollers/mcpexternalauthconfig_controller.gofindReferencingMCPServersandfindReferencingWorkloadsto check authServerRef with deduplication; add reverse watch for authServerRef inSetupWithManagerpkg/controllerutil/authserver_test.goAddAuthServerRefOptionsandValidateAndAddAuthServerRefOptionscovering nil ref, invalid kind, wrong type, valid ref, conflict detection, and combined auth pathscontrollers/mcpexternalauthconfig_controller_test.goDoes this introduce a user-facing change?
Yes. MCPServer and MCPRemoteProxy resources now accept an optional
authServerReffield that references anMCPExternalAuthConfigof typeembeddedAuthServer. This allows configuring the embedded auth server independently fromexternalAuthConfigRef, enabling use cases like embedded auth server + AWS STS token exchange on the same resource.Special notes for reviewers
authServerReffield usescorev1.TypedLocalObjectReference(not the existingExternalAuthConfigReftype) to future-proof for a dedicated auth server CRD.authServerRefandexternalAuthConfigRefcannot point to anembeddedAuthServertype simultaneously.Generated with Claude Code