DRAFT: Refactor/typed backend auth strategy #2797
Open
+2,146
−1,368
Conversation
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
Add a new leaf package pkg/vmcp/auth/types containing: - BackendAuthStrategy struct with typed fields (HeaderInjection, TokenExchange) - HeaderInjectionConfig for header injection auth strategy - TokenExchangeConfig for OAuth 2.0 token exchange auth strategy - Strategy type constants (StrategyTypeUnauthenticated, etc.) This package has no dependencies on other vmcp packages, breaking the import cycle between config, strategies, and vmcp packages. The typed fields replace the previous map[string]any Metadata field, providing compile-time type safety and clean YAML serialization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update core domain types to use the new typed BackendAuthStrategy: - pkg/vmcp/types.go: Replace AuthStrategy string and AuthMetadata map[string]any with AuthConfig *authtypes.BackendAuthStrategy in Backend and BackendTarget structs - pkg/vmcp/registry.go: Update BackendToTarget to copy AuthConfig - pkg/vmcp/config/config.go: Update OutgoingAuthConfig to use *authtypes.BackendAuthStrategy, update ResolveForBackend to return the typed strategy directly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update Strategy interface and implementations to use typed *authtypes.BackendAuthStrategy instead of map[string]any: - pkg/vmcp/auth/auth.go: Update Strategy interface methods Authenticate() and Validate() to accept typed strategy - pkg/vmcp/auth/strategies/header_injection.go: Access typed HeaderInjection config fields directly instead of map lookups - pkg/vmcp/auth/strategies/tokenexchange.go: Access typed TokenExchange config fields directly instead of map lookups - pkg/vmcp/auth/strategies/unauthenticated.go: Update to accept typed strategy parameter - pkg/vmcp/auth/strategies/constants.go: Re-export strategy type constants from auth/types for backward compatibility This eliminates runtime type assertions and provides compile-time type safety for auth strategy configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update auth converters to return *authtypes.BackendAuthStrategy instead of map[string]any: - Rename ConvertToMetadata to Convert returning typed strategy - Update ResolveSecrets to accept and return typed strategy - Rename ConvertToStrategyMetadata to ConvertToBackendAuthStrategy - Update DiscoverAndResolveAuth to return typed strategy directly Converters now build typed HeaderInjectionConfig or TokenExchangeConfig structs directly, eliminating the need for runtime type assertions when consuming the converted auth configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update YAML loader and validator to use typed auth configuration: - pkg/vmcp/config/yaml_loader.go: Update transformBackendAuthStrategy() to populate typed HeaderInjection or TokenExchange fields instead of the Metadata map. Environment variable resolution (header_value_env) now stores resolved value directly in HeaderValue field. - pkg/vmcp/config/validator.go: Use authtypes.StrategyType* constants for validation instead of importing from strategies package. This simplifies the config loading pipeline by working with typed structs throughout, eliminating intermediate map representations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update all remaining code that consumes auth configuration: - pkg/vmcp/workloads/k8s.go: Use typed BackendAuthStrategy from converters, simplified since converters return typed values - pkg/vmcp/client/client.go: Access AuthConfig.Type directly - pkg/vmcp/aggregator/discoverer.go: Use typed auth config - pkg/vmcp/auth/factory/outgoing.go: Pass typed strategy to strategy.Authenticate() and Validate() - cmd/thv-operator/pkg/vmcpconfig/converter.go: Build typed BackendAuthStrategy directly - pkg/vmcp/auth/mocks/mock_strategy.go: Regenerated mock with updated Strategy interface signatures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update all test files to use typed BackendAuthStrategy: - Strategy tests: Use typed config in Authenticate/Validate calls - Converter tests: Verify typed output instead of map[string]any - Config tests: Build typed strategy structs in test cases - Integration tests: Use typed WithAuth helper - Operator tests: Remove Metadata field assertions All tests updated to construct typed HeaderInjectionConfig or TokenExchangeConfig structs instead of map[string]any metadata. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add integration tests that verify vMCP configuration serialized from Kubernetes CRDs can be properly deserialized and used by the CLI. Tests cover: - HeaderInjection config roundtrip (literal values, env vars) - TokenExchange config roundtrip (all fields, minimal, secrets) - Full OutgoingAuthConfig with default and per-backend strategies - Unauthenticated strategy roundtrip - YAML field naming consistency between operator and CLI types These tests prove that the typed BackendAuthStrategy fields serialize correctly to YAML and can be parsed by the CLI's yaml_loader.go code, validating the fix for the CRD-to-CLI serialization issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
- Coverage 56.52% 56.42% -0.10%
==========================================
Files 319 319
Lines 30886 30926 +40
==========================================
- Hits 17458 17451 -7
- Misses 11924 11974 +50
+ Partials 1504 1501 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
This is a draft, because this is completely clauded. Normally I don't open PRs like this even as a draft, but I wanted to see if this solves the issue @amirejaz was having. This PR will be checked, refactored and split before normal submission.
We've had issues serializing the CRDs for use by the vMCP CLI. Let's use types instead of working around the serialization.
Contains a serialization/deserialization unit test.