Add integration tests for authServerRef on MCPServer and MCPRemoteProxy#4708
Open
tgrunnagle wants to merge 6 commits intomainfrom
Open
Add integration tests for authServerRef on MCPServer and MCPRemoteProxy#4708tgrunnagle wants to merge 6 commits intomainfrom
tgrunnagle wants to merge 6 commits intomainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
github-actions
bot
added
size/XL
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4708 +/- ##
==========================================
+ Coverage 68.60% 68.68% +0.08%
==========================================
Files 516 517 +1
Lines 54490 54817 +327
==========================================
+ Hits 37381 37653 +272
- Misses 14219 14270 +51
- Partials 2890 2894 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/XL
tgrunnagle
requested review from
ChrisJBurns,
JAORMX,
jerm-dro,
jhrozek and
yrobla
as code owners
github-actions
bot
added
size/XL
github-actions
bot
dismissed
their stale review
Large PR justification has been provided. Thank you!
Contributor
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
github-actions
bot
added
size/XL
Implements E2E test suite for issue #4641 covering the authServerRef field on both MCPServer and MCPRemoteProxy resources: MCPServer tests: - Happy path: authServerRef -> Ready phase, ConfigMap has embedded_auth_server_config - Conflict: authServerRef + externalAuthConfigRef both embeddedAuthServer -> Failed - Type mismatch: authServerRef to unauthenticated -> Failed with condition message - Backward compatibility: externalAuthConfigRef only -> Ready phase MCPRemoteProxy tests: - Happy path: authServerRef -> Ready phase, ConfigMap has embedded_auth_server_config - Combined auth: authServerRef (embedded) + externalAuthConfigRef (awsSts) -> Ready with both embedded_auth_server_config and aws_sts_config in ConfigMap - Conflict: both refs to embeddedAuthServer -> Failed - Type mismatch: authServerRef to unauthenticated -> Failed with condition message - Backward compatibility: externalAuthConfigRef only -> Ready phase Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add MCPOIDCConfig resources and OIDCConfigRef to all happy path and backward compatibility tests (required for embedded auth server) - Add conflict message assertions to both MCPServer and MCPRemoteProxy conflict tests to distinguish correct failures - Add expectedStatus parameter to condition helper functions so tests verify both Status and Message fields - Fix empty message substring in AuthServerRefValidated assertion to check for "is valid" with ConditionTrue status - Add AuthServerRefValidated condition check to MCPRemoteProxy happy path - Add ConfigMap verification to backward compatibility tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The conflict E2E tests incorrectly asserted AuthServerRefValidated=False, but the controller sets it to True (individual ref validation passes in handleAuthServerRef) before the conflict is detected in runconfig generation. MCPServer conflict test: check Status.Message for the conflict error instead of the AuthServerRefValidated condition, since the error is surfaced via Phase=Failed + Status.Message. MCPRemoteProxy conflict test: the controller does not set Phase=Failed for ensureAllResources errors (it requeues instead). Assert the proxy never reaches Ready and that AuthServerRefValidated=True.
Embedded auth server tests cannot reach Ready/Running phase in the test cluster because the pods crash-loop with fake OIDC credentials. Instead, assert that the operator correctly sets AuthServerRefValidated conditions and generates the expected RunConfig ConfigMap entries, which verifies the full reconciliation path through config generation. Also add OIDCConfigRef to conflict tests so the reconciler reaches the conflict detection step instead of failing earlier on missing OIDC config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tgrunnagle
force-pushed
the
issue_4641_authServerRef-e2e
branch
from
d47b6e8 to
d040d02
Compare
github-actions
bot
added
size/XL
Collaborator
|
@tgrunnagle I'd probably opt for putting these in the integration test folder. They are a lot less flaky than the E2E tests and are faster due to not needing to wait for a Kind cluster. For reconciliation they are perfect. The E2E tests I believe are moreso for being able to test the whole reconcilition and request/response flow for MCP servers |
Move the authServerRef tests to envtest-based integration suites under cmd/thv-operator/test-integration/ where they belong. Integration tests exercise the full reconciliation path (conditions, ConfigMap generation, conflict detection) without needing a real cluster or running pods. - Add MCPServer tests to mcp-server/ integration suite - Add MCPRemoteProxy tests to mcp-remote-proxy/ integration suite - Register MCPOIDCConfigReconciler in both test suites - Add WithAuthServerRef/WithOIDCConfigRef builder methods - Remove E2E test/e2e/thv-operator/authserver/ directory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tgrunnagle
changed the title
github-actions
bot
added
size/L
jhrozek
reviewed
Apr 9, 2026
Contributor
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Overall this is solid test code with good scenario coverage. One suggestion below.
cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.go
Outdated
Show resolved
Hide resolved
The Eventually on a negative condition (phase != Failed) passed on the first poll because Phase starts as "" before reconciliation. Since the test is Ordered and the prior It already waits for the ConfigMap, a simple Expect suffices. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
size/L
jhrozek
approved these changes
Apr 9, 2026
Contributor
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
I meant to ack, sorry, the inline comments are nits
ChrisJBurns
approved these changes
Apr 9, 2026
jerm-dro
approved these changes
Apr 10, 2026
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
4 participants
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 authServerRef CRD and controller logic (#4640) lacks verification that reconciliation works correctly end-to-end -- from resource creation through condition updates to ConfigMap generation. This PR adds envtest-based integration tests that exercise the full operator reconciliation path for both MCPServer and MCPRemoteProxy resources using the new
authServerReffield.Integration tests (envtest) are the right fit here because the tests verify reconciliation-level behavior (conditions, ConfigMap content, phase transitions) rather than runtime behavior requiring real pods.
Closes #4641
Type of change
Test plan
task operator-test-integration)task lint-fix)Changes
cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.gocmd/thv-operator/test-integration/mcp-server/suite_test.gocmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_authserverref_integration_test.gocmd/thv-operator/test-integration/mcp-remote-proxy/suite_test.gocmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.goWithAuthServerRefandWithOIDCConfigRefbuilder methodsSpecial notes for reviewers
Consistentlyto verify the proxy never reaches Ready, rather than waiting forFailedphase. This matches actual controller behavior: the MCPRemoteProxy controller returns the conflict error to controller-runtime for requeue rather than setting a terminalFailedphase.embedded_auth_server_configandaws_sts_configkeys exist in the RunConfig ConfigMap.WithOIDCConfigRefclears the default inlineOIDCConfigsince the CRD enforces mutual exclusivity betweenoidcConfigandoidcConfigRef.Generated with Claude Code