From 7b845115df1d094d84051dbb652109768302dfc0 Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 5 Dec 2025 12:28:11 +0100 Subject: [PATCH 1/5] Add unauthenticated auth strategy to MCPExternalAuthConfig Implements support for the 'unauthenticated' authentication strategy to align MCPExternalAuthConfig CRD with vMCP's supported auth strategies. This change adds the third and final auth strategy type: - tokenExchange (CRD) -> token_exchange (vMCP) - headerInjection (CRD) -> header_injection (vMCP) - unauthenticated (CRD) -> unauthenticated (vMCP) --- .../v1alpha1/mcpexternalauthconfig_types.go | 7 +- .../v1alpha1/mcpexternalauthconfig_webhook.go | 75 ++ .../mcpexternalauthconfig_webhook_test.go | 262 ++++++ .../virtualmcpserver_deployment.go | 4 + .../pkg/controllerutil/tokenexchange.go | 3 + config/webhook/manifests.yaml | 20 + ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 1 + docs/operator/crd-api.md | 3 +- pkg/vmcp/auth/converters/interface.go | 1 + pkg/vmcp/auth/converters/unauthenticated.go | 42 + .../auth/converters/unauthenticated_test.go | 138 +++ .../virtualmcp_external_auth_test.go | 816 ++++++++++++++++++ 12 files changed, 1370 insertions(+), 2 deletions(-) create mode 100644 cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go create mode 100644 cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go create mode 100644 pkg/vmcp/auth/converters/unauthenticated.go create mode 100644 pkg/vmcp/auth/converters/unauthenticated_test.go create mode 100644 test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go index a742f083d..1f42d5b05 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go @@ -11,6 +11,11 @@ const ( // ExternalAuthTypeHeaderInjection is the type for custom header injection ExternalAuthTypeHeaderInjection ExternalAuthType = "headerInjection" + + // ExternalAuthTypeUnauthenticated is the type for no authentication + // This should only be used for backends on trusted networks (e.g., localhost, VPC) + // or when authentication is handled by network-level security + ExternalAuthTypeUnauthenticated ExternalAuthType = "unauthenticated" ) // ExternalAuthType represents the type of external authentication @@ -21,7 +26,7 @@ type ExternalAuthType string // MCPServer resources in the same namespace. type MCPExternalAuthConfigSpec struct { // Type is the type of external authentication to configure - // +kubebuilder:validation:Enum=tokenExchange;headerInjection + // +kubebuilder:validation:Enum=tokenExchange;headerInjection;unauthenticated // +kubebuilder:validation:Required Type ExternalAuthType `json:"type"` diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go new file mode 100644 index 000000000..ecae8b226 --- /dev/null +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go @@ -0,0 +1,75 @@ +package v1alpha1 + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// SetupWebhookWithManager sets up the webhook with the Manager +func (r *MCPExternalAuthConfig) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(r). + Complete() +} + +//nolint:lll // kubebuilder webhook marker cannot be split +// +kubebuilder:webhook:path=/validate-toolhive-stacklok-com-v1alpha1-mcpexternalauthconfig,mutating=false,failurePolicy=fail,sideEffects=None,groups=toolhive.stacklok.com,resources=mcpexternalauthconfigs,verbs=create;update,versions=v1alpha1,name=vmcpexternalauthconfig.kb.io,admissionReviewVersions=v1 + +var _ webhook.CustomValidator = &MCPExternalAuthConfig{} + +// ValidateCreate implements webhook.CustomValidator +func (r *MCPExternalAuthConfig) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, r.validate() +} + +// ValidateUpdate implements webhook.CustomValidator +func (r *MCPExternalAuthConfig) ValidateUpdate( + _ context.Context, _ runtime.Object, _ runtime.Object, +) (admission.Warnings, error) { + return nil, r.validate() +} + +// ValidateDelete implements webhook.CustomValidator +func (*MCPExternalAuthConfig) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + // No validation needed for deletion + return nil, nil +} + +// validate performs validation on the MCPExternalAuthConfig spec +func (r *MCPExternalAuthConfig) validate() error { + switch r.Spec.Type { + case ExternalAuthTypeTokenExchange: + if r.Spec.TokenExchange == nil { + return fmt.Errorf("tokenExchange configuration is required when type is 'tokenExchange'") + } + if r.Spec.HeaderInjection != nil { + return fmt.Errorf("headerInjection must not be set when type is 'tokenExchange'") + } + + case ExternalAuthTypeHeaderInjection: + if r.Spec.HeaderInjection == nil { + return fmt.Errorf("headerInjection configuration is required when type is 'headerInjection'") + } + if r.Spec.TokenExchange != nil { + return fmt.Errorf("tokenExchange must not be set when type is 'headerInjection'") + } + + case ExternalAuthTypeUnauthenticated: + if r.Spec.TokenExchange != nil { + return fmt.Errorf("tokenExchange must not be set when type is 'unauthenticated'") + } + if r.Spec.HeaderInjection != nil { + return fmt.Errorf("headerInjection must not be set when type is 'unauthenticated'") + } + + default: + return fmt.Errorf("unsupported auth type: %s", r.Spec.Type) + } + + return nil +} diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go new file mode 100644 index 000000000..d2837217d --- /dev/null +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go @@ -0,0 +1,262 @@ +package v1alpha1 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config *MCPExternalAuthConfig + expectError bool + errorMsg string + }{ + { + name: "valid unauthenticated", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + }, + }, + expectError: false, + }, + { + name: "unauthenticated with tokenExchange should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + TokenExchange: &TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + }, + }, + }, + expectError: true, + errorMsg: "tokenExchange must not be set when type is 'unauthenticated'", + }, + { + name: "unauthenticated with headerInjection should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + HeaderInjection: &HeaderInjectionConfig{ + HeaderName: "Authorization", + ValueSecretRef: &SecretKeyRef{ + Name: "secret", + Key: "key", + }, + }, + }, + }, + expectError: true, + errorMsg: "headerInjection must not be set when type is 'unauthenticated'", + }, + { + name: "valid tokenExchange", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-tokenexchange", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeTokenExchange, + TokenExchange: &TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + Audience: "backend-service", + }, + }, + }, + expectError: false, + }, + { + name: "tokenExchange without config should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeTokenExchange, + }, + }, + expectError: true, + errorMsg: "tokenExchange configuration is required when type is 'tokenExchange'", + }, + { + name: "tokenExchange with headerInjection should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeTokenExchange, + TokenExchange: &TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + Audience: "backend-service", + }, + HeaderInjection: &HeaderInjectionConfig{ + HeaderName: "Authorization", + ValueSecretRef: &SecretKeyRef{ + Name: "secret", + Key: "key", + }, + }, + }, + }, + expectError: true, + errorMsg: "headerInjection must not be set when type is 'tokenExchange'", + }, + { + name: "valid headerInjection", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-headerinjection", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeHeaderInjection, + HeaderInjection: &HeaderInjectionConfig{ + HeaderName: "X-API-Key", + ValueSecretRef: &SecretKeyRef{ + Name: "api-key-secret", + Key: "api-key", + }, + }, + }, + }, + expectError: false, + }, + { + name: "headerInjection without config should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeHeaderInjection, + }, + }, + expectError: true, + errorMsg: "headerInjection configuration is required when type is 'headerInjection'", + }, + { + name: "headerInjection with tokenExchange should fail", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeHeaderInjection, + HeaderInjection: &HeaderInjectionConfig{ + HeaderName: "X-API-Key", + ValueSecretRef: &SecretKeyRef{ + Name: "secret", + Key: "key", + }, + }, + TokenExchange: &TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + }, + }, + }, + expectError: true, + errorMsg: "tokenExchange must not be set when type is 'headerInjection'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + warnings, err := tt.config.ValidateCreate(context.Background(), tt.config) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMsg) + } else { + require.NoError(t, err) + } + + // Warnings should always be nil for now + assert.Nil(t, warnings) + }) + } +} + +func TestMCPExternalAuthConfig_ValidateUpdate(t *testing.T) { + t.Parallel() + + config := &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + }, + } + + // ValidateUpdate should use the same logic as ValidateCreate + warnings, err := config.ValidateUpdate(context.Background(), nil, config) + require.NoError(t, err) + assert.Nil(t, warnings) + + // Test invalid update + invalidConfig := &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + TokenExchange: &TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + }, + }, + } + + warnings, err = invalidConfig.ValidateUpdate(context.Background(), nil, invalidConfig) + require.Error(t, err) + assert.Contains(t, err.Error(), "tokenExchange must not be set when type is 'unauthenticated'") + assert.Nil(t, warnings) +} + +func TestMCPExternalAuthConfig_ValidateDelete(t *testing.T) { + t.Parallel() + + config := &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeUnauthenticated, + }, + } + + // ValidateDelete should always succeed + warnings, err := config.ValidateDelete(context.Background(), config) + require.NoError(t, err) + assert.Nil(t, warnings) +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index ced251d6c..664a49476 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -446,6 +446,10 @@ func (r *VirtualMCPServerReconciler) getExternalAuthConfigSecretEnvVar( envVarName = generateUniqueHeaderInjectionEnvVarName(externalAuthConfigName) secretRef = externalAuthConfig.Spec.HeaderInjection.ValueSecretRef + case mcpv1alpha1.ExternalAuthTypeUnauthenticated: + // No secrets to mount for unauthenticated + return nil, nil + default: return nil, nil // Not applicable } diff --git a/cmd/thv-operator/pkg/controllerutil/tokenexchange.go b/cmd/thv-operator/pkg/controllerutil/tokenexchange.go index 7ae396da1..f64fe4d77 100644 --- a/cmd/thv-operator/pkg/controllerutil/tokenexchange.go +++ b/cmd/thv-operator/pkg/controllerutil/tokenexchange.go @@ -116,6 +116,9 @@ func AddExternalAuthConfigOptions( return addTokenExchangeConfig(ctx, c, namespace, externalAuthConfig, options) case mcpv1alpha1.ExternalAuthTypeHeaderInjection: return addHeaderInjectionConfig(ctx, c, namespace, externalAuthConfig, options) + case mcpv1alpha1.ExternalAuthTypeUnauthenticated: + // No config to add for unauthenticated + return nil default: return fmt.Errorf("unsupported external auth type: %s", externalAuthConfig.Spec.Type) } diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 5e040977a..dce4598d8 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -4,6 +4,26 @@ kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-toolhive-stacklok-com-v1alpha1-mcpexternalauthconfig + failurePolicy: Fail + name: vmcpexternalauthconfig.kb.io + rules: + - apiGroups: + - toolhive.stacklok.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - mcpexternalauthconfigs + sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index d8f4ace8f..cecd3d6a3 100644 --- a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -147,6 +147,7 @@ spec: enum: - tokenExchange - headerInjection + - unauthenticated type: string required: - type diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index e9d5af6b8..6fded8c2c 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -360,6 +360,7 @@ _Appears in:_ | --- | --- | | `tokenExchange` | ExternalAuthTypeTokenExchange is the type for RFC-8693 token exchange
| | `headerInjection` | ExternalAuthTypeHeaderInjection is the type for custom header injection
| +| `unauthenticated` | ExternalAuthTypeUnauthenticated is the type for no authentication
This should only be used for backends on trusted networks (e.g., localhost, VPC)
or when authentication is handled by network-level security
| #### FailureHandlingConfig @@ -580,7 +581,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `type` _[ExternalAuthType](#externalauthtype)_ | Type is the type of external authentication to configure | | Enum: [tokenExchange headerInjection]
Required: \{\}
| +| `type` _[ExternalAuthType](#externalauthtype)_ | Type is the type of external authentication to configure | | Enum: [tokenExchange headerInjection unauthenticated]
Required: \{\}
| | `tokenExchange` _[TokenExchangeConfig](#tokenexchangeconfig)_ | TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange
Only used when Type is "tokenExchange" | | | | `headerInjection` _[HeaderInjectionConfig](#headerinjectionconfig)_ | HeaderInjection configures custom HTTP header injection
Only used when Type is "headerInjection" | | | diff --git a/pkg/vmcp/auth/converters/interface.go b/pkg/vmcp/auth/converters/interface.go index 8a911a19b..163b7b63e 100644 --- a/pkg/vmcp/auth/converters/interface.go +++ b/pkg/vmcp/auth/converters/interface.go @@ -69,6 +69,7 @@ func NewRegistry() *Registry { // Register built-in converters r.Register(mcpv1alpha1.ExternalAuthTypeTokenExchange, &TokenExchangeConverter{}) r.Register(mcpv1alpha1.ExternalAuthTypeHeaderInjection, &HeaderInjectionConverter{}) + r.Register(mcpv1alpha1.ExternalAuthTypeUnauthenticated, &UnauthenticatedConverter{}) return r } diff --git a/pkg/vmcp/auth/converters/unauthenticated.go b/pkg/vmcp/auth/converters/unauthenticated.go new file mode 100644 index 000000000..899947f57 --- /dev/null +++ b/pkg/vmcp/auth/converters/unauthenticated.go @@ -0,0 +1,42 @@ +package converters + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" +) + +// UnauthenticatedConverter converts unauthenticated external auth configs to BackendAuthStrategy. +// This converter handles the case where no authentication is required for a backend. +type UnauthenticatedConverter struct{} + +// StrategyType returns the vMCP strategy type identifier for unauthenticated auth. +func (*UnauthenticatedConverter) StrategyType() string { + return authtypes.StrategyTypeUnauthenticated +} + +// ConvertToStrategy converts an MCPExternalAuthConfig with type "unauthenticated" to a BackendAuthStrategy. +// Since unauthenticated requires no configuration, this simply returns a strategy with the correct type. +func (*UnauthenticatedConverter) ConvertToStrategy( + _ *mcpv1alpha1.MCPExternalAuthConfig, +) (*authtypes.BackendAuthStrategy, error) { + return &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + // No additional fields needed for unauthenticated + }, nil +} + +// ResolveSecrets is a no-op for unauthenticated strategy since there are no secrets to resolve. +func (*UnauthenticatedConverter) ResolveSecrets( + _ context.Context, + _ *mcpv1alpha1.MCPExternalAuthConfig, + _ client.Client, + _ string, + strategy *authtypes.BackendAuthStrategy, +) (*authtypes.BackendAuthStrategy, error) { + // No secrets to resolve for unauthenticated strategy + return strategy, nil +} diff --git a/pkg/vmcp/auth/converters/unauthenticated_test.go b/pkg/vmcp/auth/converters/unauthenticated_test.go new file mode 100644 index 000000000..dc0613e13 --- /dev/null +++ b/pkg/vmcp/auth/converters/unauthenticated_test.go @@ -0,0 +1,138 @@ +package converters + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" +) + +func TestUnauthenticatedConverter_StrategyType(t *testing.T) { + t.Parallel() + + converter := &UnauthenticatedConverter{} + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, converter.StrategyType()) +} + +func TestUnauthenticatedConverter_ConvertToStrategy(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + externalAuth *mcpv1alpha1.MCPExternalAuthConfig + expectedType string + expectedError bool + }{ + { + name: "valid unauthenticated config", + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + }, + expectedType: authtypes.StrategyTypeUnauthenticated, + expectedError: false, + }, + { + name: "unauthenticated with no extra fields", + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated-minimal", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + // No TokenExchange or HeaderInjection + }, + }, + expectedType: authtypes.StrategyTypeUnauthenticated, + expectedError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + converter := &UnauthenticatedConverter{} + strategy, err := converter.ConvertToStrategy(tt.externalAuth) + + if tt.expectedError { + require.Error(t, err) + assert.Nil(t, strategy) + } else { + require.NoError(t, err) + require.NotNil(t, strategy) + assert.Equal(t, tt.expectedType, strategy.Type) + // Verify no auth-specific fields are set + assert.Nil(t, strategy.TokenExchange) + assert.Nil(t, strategy.HeaderInjection) + } + }) + } +} + +func TestUnauthenticatedConverter_ResolveSecrets(t *testing.T) { + t.Parallel() + + converter := &UnauthenticatedConverter{} + externalAuth := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + } + + strategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + } + + // ResolveSecrets should be a no-op for unauthenticated + resolvedStrategy, err := converter.ResolveSecrets(context.Background(), externalAuth, nil, "default", strategy) + + require.NoError(t, err) + require.NotNil(t, resolvedStrategy) + assert.Equal(t, strategy, resolvedStrategy, "Strategy should be unchanged") + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, resolvedStrategy.Type) +} + +func TestUnauthenticatedConverter_Integration(t *testing.T) { + t.Parallel() + + // Test that unauthenticated converter is registered in default registry + registry := DefaultRegistry() + converter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeUnauthenticated) + require.NoError(t, err) + require.NotNil(t, converter) + assert.IsType(t, &UnauthenticatedConverter{}, converter) + + // Test end-to-end conversion using ConvertToStrategy convenience function + externalAuth := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unauthenticated", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + } + + strategy, err := ConvertToStrategy(externalAuth) + require.NoError(t, err) + require.NotNil(t, strategy) + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, strategy.Type) + assert.Nil(t, strategy.TokenExchange) + assert.Nil(t, strategy.HeaderInjection) +} diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go new file mode 100644 index 000000000..d7d1ad8ac --- /dev/null +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go @@ -0,0 +1,816 @@ +package virtualmcp + +import ( + "context" + "fmt" + "time" + + "github.com/mark3labs/mcp-go/client" + "github.com/mark3labs/mcp-go/mcp" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/test/e2e/images" +) + +var _ = Describe("VirtualMCPServer Unauthenticated Backend Auth", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "test-unauthenticated-auth-group" + vmcpServerName = "test-vmcp-unauthenticated" + backendName = "backend-fetch-unauthenticated" + externalAuthConfigName = "test-unauthenticated-auth-config" + timeout = 5 * time.Minute + pollingInterval = 5 * time.Second + vmcpNodePort int32 + ) + + BeforeAll(func() { + By("Creating MCPExternalAuthConfig with unauthenticated type") + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + // No TokenExchange or HeaderInjection fields needed + }, + } + Expect(k8sClient.Create(ctx, externalAuthConfig)).To(Succeed()) + + By("Creating MCPGroup") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for VirtualMCP unauthenticated auth", timeout, pollingInterval) + + By("Creating backend MCPServer without auth (localhost, trusted)") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + // Reference the unauthenticated external auth config + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: externalAuthConfigName, + }, + }, + } + Expect(k8sClient.Create(ctx, backend)).To(Succeed()) + + By("Waiting for backend MCPServer to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backendName, + Namespace: testNamespace, + }, server) + if err != nil { + return fmt.Errorf("failed to get server: %w", err) + } + if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { + return nil + } + return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) + }, timeout, pollingInterval).Should(Succeed()) + + By("Creating VirtualMCPServer with discovered auth mode (should use unauthenticated)") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + GroupRef: mcpv1alpha1.GroupRef{ + Name: mcpGroupName, + }, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Will discover unauthenticated from backend + }, + ServiceType: "NodePort", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout) + + By("Getting NodePort for VirtualMCPServer") + vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + }) + + AfterAll(func() { + By("Cleaning up test resources") + _ = k8sClient.Delete(ctx, &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: vmcpServerName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: backendName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: mcpGroupName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: externalAuthConfigName, Namespace: testNamespace}, + }) + }) + + Context("when using unauthenticated backend auth", func() { + It("should discover unauthenticated auth from backend MCPServer", func() { + By("Verifying VirtualMCPServer discovered unauthenticated auth") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + Expect(err).ToNot(HaveOccurred()) + Expect(vmcpServer.Spec.OutgoingAuth.Source).To(Equal("discovered")) + + // Check that backend was discovered with auth config + Expect(vmcpServer.Status.DiscoveredBackends).ToNot(BeEmpty()) + found := false + for _, backend := range vmcpServer.Status.DiscoveredBackends { + if backend.Name == backendName { + found = true + Expect(backend.AuthConfigRef).To(Equal(externalAuthConfigName)) + break + } + } + Expect(found).To(BeTrue(), "Backend should be discovered with auth config reference") + }) + + It("should successfully connect and call tools with unauthenticated backend", func() { + By("Creating MCP client") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + By("Starting MCP client") + err = mcpClient.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + + By("Initializing MCP session") + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-test", + Version: "1.0.0", + } + _, err = mcpClient.Initialize(ctx, initRequest) + Expect(err).ToNot(HaveOccurred()) + + By("Listing tools from unauthenticated backend") + listRequest := mcp.ListToolsRequest{} + tools, err := mcpClient.ListTools(ctx, listRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(tools.Tools).ToNot(BeEmpty()) + + By("Calling a tool from unauthenticated backend") + // Find the fetch tool + var fetchTool *mcp.Tool + for _, tool := range tools.Tools { + if tool.Name == fetchToolName || tool.Name == "backend-fetch-unauthenticated_fetch" { + t := tool + fetchTool = &t + break + } + } + Expect(fetchTool).ToNot(BeNil(), "fetch tool should be available") + + // Call the fetch tool + callRequest := mcp.CallToolRequest{} + callRequest.Params.Name = fetchTool.Name + callRequest.Params.Arguments = map[string]interface{}{ + "url": "https://example.com", + } + + result, err := mcpClient.CallTool(ctx, callRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(result.Content).ToNot(BeEmpty()) + }) + + It("should validate MCPExternalAuthConfig with unauthenticated type", func() { + By("Verifying MCPExternalAuthConfig exists and is valid") + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, authConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(authConfig.Spec.Type).To(Equal(mcpv1alpha1.ExternalAuthTypeUnauthenticated)) + Expect(authConfig.Spec.TokenExchange).To(BeNil()) + Expect(authConfig.Spec.HeaderInjection).To(BeNil()) + }) + }) +}) + +var _ = Describe("VirtualMCPServer Inline Unauthenticated Backend Auth", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "test-inline-unauth-group" + vmcpServerName = "test-vmcp-inline-unauth" + backendName = "backend-inline-unauth" + externalAuthConfigName = "test-inline-unauth-config" + timeout = 5 * time.Minute + pollingInterval = 5 * time.Second + vmcpNodePort int32 + ) + + BeforeAll(func() { + By("Creating MCPExternalAuthConfig with unauthenticated type for inline mode") + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + } + Expect(k8sClient.Create(ctx, externalAuthConfig)).To(Succeed()) + + By("Creating MCPGroup") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for inline unauthenticated auth", timeout, pollingInterval) + + By("Creating backend MCPServer") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + }, + } + Expect(k8sClient.Create(ctx, backend)).To(Succeed()) + + By("Waiting for backend MCPServer to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backendName, + Namespace: testNamespace, + }, server) + if err != nil { + return fmt.Errorf("failed to get server: %w", err) + } + if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { + return nil + } + return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) + }, timeout, pollingInterval).Should(Succeed()) + + By("Creating VirtualMCPServer with inline unauthenticated auth") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + GroupRef: mcpv1alpha1.GroupRef{ + Name: mcpGroupName, + }, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "inline", + // Explicitly configure unauthenticated for specific backend + Backends: map[string]mcpv1alpha1.BackendAuthConfig{ + backendName: { + Type: "external_auth_config_ref", + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: externalAuthConfigName, + }, + }, + }, + }, + ServiceType: "NodePort", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout) + + By("Getting NodePort for VirtualMCPServer") + vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + }) + + AfterAll(func() { + By("Cleaning up test resources") + _ = k8sClient.Delete(ctx, &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: vmcpServerName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: backendName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: mcpGroupName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: externalAuthConfigName, Namespace: testNamespace}, + }) + }) + + Context("when using inline unauthenticated backend auth", func() { + It("should configure inline unauthenticated auth for specific backend", func() { + By("Verifying VirtualMCPServer has inline auth configured") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + Expect(err).ToNot(HaveOccurred()) + Expect(vmcpServer.Spec.OutgoingAuth.Source).To(Equal("inline")) + Expect(vmcpServer.Spec.OutgoingAuth.Backends).To(HaveKey(backendName)) + Expect(vmcpServer.Spec.OutgoingAuth.Backends[backendName].Type).To(Equal("external_auth_config_ref")) + Expect(vmcpServer.Spec.OutgoingAuth.Backends[backendName].ExternalAuthConfigRef.Name).To(Equal(externalAuthConfigName)) + }) + + It("should successfully proxy tool calls with inline unauthenticated auth", func() { + By("Creating MCP client") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + By("Starting and initializing MCP client") + err = mcpClient.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-test", + Version: "1.0.0", + } + _, err = mcpClient.Initialize(ctx, initRequest) + Expect(err).ToNot(HaveOccurred()) + + By("Listing and calling tools through inline unauthenticated proxy") + listRequest := mcp.ListToolsRequest{} + tools, err := mcpClient.ListTools(ctx, listRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(tools.Tools).ToNot(BeEmpty()) + + // Verify tools are accessible + var fetchTool *mcp.Tool + for _, tool := range tools.Tools { + if tool.Name == fetchToolName || tool.Name == "backend-inline-unauth_fetch" { + t := tool + fetchTool = &t + break + } + } + Expect(fetchTool).ToNot(BeNil(), "fetch tool should be available") + }) + }) +}) + +var _ = Describe("VirtualMCPServer HeaderInjection Backend Auth", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "test-headerinjection-auth-group" + vmcpServerName = "test-vmcp-headerinjection" + backendName = "backend-fetch-headerinjection" + externalAuthConfigName = "test-headerinjection-auth-config" + secretName = "test-headerinjection-secret" + timeout = 5 * time.Minute + pollingInterval = 5 * time.Second + vmcpNodePort int32 + ) + + BeforeAll(func() { + By("Creating Secret for header injection") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: testNamespace, + }, + StringData: map[string]string{ + "api-key": "test-api-key-value", + }, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + + By("Creating MCPExternalAuthConfig with headerInjection type") + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeHeaderInjection, + HeaderInjection: &mcpv1alpha1.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: secretName, + Key: "api-key", + }, + }, + }, + } + Expect(k8sClient.Create(ctx, externalAuthConfig)).To(Succeed()) + + By("Creating MCPGroup") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for VirtualMCP headerInjection auth", timeout, pollingInterval) + + By("Creating backend MCPServer with headerInjection auth") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + // Reference the headerInjection external auth config + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: externalAuthConfigName, + }, + }, + } + Expect(k8sClient.Create(ctx, backend)).To(Succeed()) + + By("Waiting for backend MCPServer to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backendName, + Namespace: testNamespace, + }, server) + if err != nil { + return fmt.Errorf("failed to get server: %w", err) + } + if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { + return nil + } + return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) + }, timeout, pollingInterval).Should(Succeed()) + + By("Creating VirtualMCPServer with discovered auth mode (should use headerInjection)") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + GroupRef: mcpv1alpha1.GroupRef{ + Name: mcpGroupName, + }, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Will discover headerInjection from backend + }, + ServiceType: "NodePort", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout) + + By("Getting NodePort for VirtualMCPServer") + vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + }) + + AfterAll(func() { + By("Cleaning up test resources") + _ = k8sClient.Delete(ctx, &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: vmcpServerName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: backendName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: mcpGroupName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: externalAuthConfigName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: testNamespace}, + }) + }) + + Context("when using headerInjection backend auth", func() { + It("should discover headerInjection auth from backend MCPServer", func() { + By("Verifying VirtualMCPServer discovered headerInjection auth") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + Expect(err).ToNot(HaveOccurred()) + Expect(vmcpServer.Spec.OutgoingAuth.Source).To(Equal("discovered")) + + // Check that backend was discovered with auth config + Expect(vmcpServer.Status.DiscoveredBackends).ToNot(BeEmpty()) + found := false + for _, backend := range vmcpServer.Status.DiscoveredBackends { + if backend.Name == backendName { + found = true + Expect(backend.AuthConfigRef).To(Equal(externalAuthConfigName)) + break + } + } + Expect(found).To(BeTrue(), "Backend should be discovered with auth config reference") + }) + + It("should successfully connect and call tools with headerInjection backend", func() { + By("Creating MCP client") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + By("Starting MCP client") + err = mcpClient.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + + By("Initializing MCP session") + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-test", + Version: "1.0.0", + } + _, err = mcpClient.Initialize(ctx, initRequest) + Expect(err).ToNot(HaveOccurred()) + + By("Listing tools from headerInjection backend") + listRequest := mcp.ListToolsRequest{} + tools, err := mcpClient.ListTools(ctx, listRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(tools.Tools).ToNot(BeEmpty()) + + By("Calling a tool from headerInjection backend") + // Find the fetch tool + var fetchTool *mcp.Tool + for _, tool := range tools.Tools { + if tool.Name == fetchToolName || tool.Name == "backend-fetch-headerinjection_fetch" { + t := tool + fetchTool = &t + break + } + } + Expect(fetchTool).ToNot(BeNil(), "fetch tool should be available") + + // Call the fetch tool + callRequest := mcp.CallToolRequest{} + callRequest.Params.Name = fetchTool.Name + callRequest.Params.Arguments = map[string]interface{}{ + "url": "https://example.com", + } + + result, err := mcpClient.CallTool(ctx, callRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(result.Content).ToNot(BeEmpty()) + }) + + It("should validate MCPExternalAuthConfig with headerInjection type", func() { + By("Verifying MCPExternalAuthConfig exists and is valid") + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, authConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(authConfig.Spec.Type).To(Equal(mcpv1alpha1.ExternalAuthTypeHeaderInjection)) + Expect(authConfig.Spec.TokenExchange).To(BeNil()) + Expect(authConfig.Spec.HeaderInjection).ToNot(BeNil()) + Expect(authConfig.Spec.HeaderInjection.HeaderName).To(Equal("X-API-Key")) + Expect(authConfig.Spec.HeaderInjection.ValueSecretRef.Name).To(Equal(secretName)) + Expect(authConfig.Spec.HeaderInjection.ValueSecretRef.Key).To(Equal("api-key")) + }) + }) +}) + +var _ = Describe("VirtualMCPServer Inline HeaderInjection Backend Auth", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "test-inline-headerinjection-group" + vmcpServerName = "test-vmcp-inline-headerinjection" + backendName = "backend-inline-headerinjection" + externalAuthConfigName = "test-inline-headerinjection-config" + secretName = "test-inline-headerinjection-secret" + timeout = 5 * time.Minute + pollingInterval = 5 * time.Second + vmcpNodePort int32 + ) + + BeforeAll(func() { + By("Creating Secret for inline header injection") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: testNamespace, + }, + StringData: map[string]string{ + "api-key": "test-inline-api-key-value", + }, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + + By("Creating MCPExternalAuthConfig with headerInjection type for inline mode") + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: externalAuthConfigName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeHeaderInjection, + HeaderInjection: &mcpv1alpha1.HeaderInjectionConfig{ + HeaderName: "X-Custom-Auth", + ValueSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: secretName, + Key: "api-key", + }, + }, + }, + } + Expect(k8sClient.Create(ctx, externalAuthConfig)).To(Succeed()) + + By("Creating MCPGroup") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for inline headerInjection auth", timeout, pollingInterval) + + By("Creating backend MCPServer") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + }, + } + Expect(k8sClient.Create(ctx, backend)).To(Succeed()) + + By("Waiting for backend MCPServer to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backendName, + Namespace: testNamespace, + }, server) + if err != nil { + return fmt.Errorf("failed to get server: %w", err) + } + if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { + return nil + } + return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) + }, timeout, pollingInterval).Should(Succeed()) + + By("Creating VirtualMCPServer with inline headerInjection auth") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + GroupRef: mcpv1alpha1.GroupRef{ + Name: mcpGroupName, + }, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "inline", + // Explicitly configure headerInjection for specific backend + Backends: map[string]mcpv1alpha1.BackendAuthConfig{ + backendName: { + Type: "external_auth_config_ref", + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: externalAuthConfigName, + }, + }, + }, + }, + ServiceType: "NodePort", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout) + + By("Getting NodePort for VirtualMCPServer") + vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + }) + + AfterAll(func() { + By("Cleaning up test resources") + _ = k8sClient.Delete(ctx, &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: vmcpServerName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: backendName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: mcpGroupName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: externalAuthConfigName, Namespace: testNamespace}, + }) + _ = k8sClient.Delete(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: testNamespace}, + }) + }) + + Context("when using inline headerInjection backend auth", func() { + It("should configure inline headerInjection auth for specific backend", func() { + By("Verifying VirtualMCPServer has inline auth configured") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + Expect(err).ToNot(HaveOccurred()) + Expect(vmcpServer.Spec.OutgoingAuth.Source).To(Equal("inline")) + Expect(vmcpServer.Spec.OutgoingAuth.Backends).To(HaveKey(backendName)) + Expect(vmcpServer.Spec.OutgoingAuth.Backends[backendName].Type).To(Equal("external_auth_config_ref")) + Expect(vmcpServer.Spec.OutgoingAuth.Backends[backendName].ExternalAuthConfigRef.Name).To(Equal(externalAuthConfigName)) + }) + + It("should successfully proxy tool calls with inline headerInjection auth", func() { + By("Creating MCP client") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + By("Starting and initializing MCP client") + err = mcpClient.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-test", + Version: "1.0.0", + } + _, err = mcpClient.Initialize(ctx, initRequest) + Expect(err).ToNot(HaveOccurred()) + + By("Listing and calling tools through inline headerInjection proxy") + listRequest := mcp.ListToolsRequest{} + tools, err := mcpClient.ListTools(ctx, listRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(tools.Tools).ToNot(BeEmpty()) + + // Verify tools are accessible + var fetchTool *mcp.Tool + for _, tool := range tools.Tools { + if tool.Name == fetchToolName || tool.Name == "backend-inline-headerinjection_fetch" { + t := tool + fetchTool = &t + break + } + } + Expect(fetchTool).ToNot(BeNil(), "fetch tool should be available") + }) + }) +}) From 59134f44a38fc1a2a894da42f1f55ddb7c85a4ef Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 5 Dec 2025 13:00:47 +0100 Subject: [PATCH 2/5] changes from review --- cmd/thv-operator/main.go | 5 + cmd/thv-operator/pkg/vmcpconfig/converter.go | 314 ++++++++++++++++--- 2 files changed, 279 insertions(+), 40 deletions(-) diff --git a/cmd/thv-operator/main.go b/cmd/thv-operator/main.go index 831d64436..1f2ac566a 100644 --- a/cmd/thv-operator/main.go +++ b/cmd/thv-operator/main.go @@ -199,6 +199,11 @@ func setupControllersAndWebhooks(mgr ctrl.Manager) error { if err := (&mcpv1alpha1.VirtualMCPCompositeToolDefinition{}).SetupWebhookWithManager(mgr); err != nil { return fmt.Errorf("unable to create webhook VirtualMCPCompositeToolDefinition: %w", err) } + + // Set up MCPExternalAuthConfig webhook + if err := (&mcpv1alpha1.MCPExternalAuthConfig{}).SetupWebhookWithManager(mgr); err != nil { + return fmt.Errorf("unable to create webhook MCPExternalAuthConfig: %w", err) + } //+kubebuilder:scaffold:builder return nil diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index 792631b26..c15dac904 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -5,8 +5,11 @@ import ( "context" "encoding/json" "fmt" + "regexp" + "strings" "time" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -40,7 +43,8 @@ type Converter struct { // NewConverter creates a new Converter instance. // oidcResolver is required and used to resolve OIDC configuration from various sources // (kubernetes, configMap, inline). Use a mock resolver in tests. -// k8sClient is required and used to fetch referenced VirtualMCPCompositeToolDefinition resources. +// k8sClient is required for resolving MCPToolConfig references and fetching referenced +// VirtualMCPCompositeToolDefinition resources. // Returns an error if oidcResolver or k8sClient is nil. func NewConverter(oidcResolver oidc.Resolver, k8sClient client.Client) (*Converter, error) { if oidcResolver == nil { @@ -76,7 +80,11 @@ func (c *Converter) Convert( // Convert OutgoingAuth - always set with defaults if not specified if vmcp.Spec.OutgoingAuth != nil { - config.OutgoingAuth = c.convertOutgoingAuth(ctx, vmcp) + outgoingAuth, err := c.convertOutgoingAuth(ctx, vmcp) + if err != nil { + return nil, fmt.Errorf("failed to convert outgoing auth: %w", err) + } + config.OutgoingAuth = outgoingAuth } else { // Provide default outgoing auth config config.OutgoingAuth = &vmcpconfig.OutgoingAuthConfig{ @@ -86,7 +94,11 @@ func (c *Converter) Convert( // Convert Aggregation - always set with defaults if not specified if vmcp.Spec.Aggregation != nil { - config.Aggregation = c.convertAggregation(ctx, vmcp) + agg, err := c.convertAggregation(ctx, vmcp) + if err != nil { + return nil, fmt.Errorf("failed to convert aggregation config: %w", err) + } + config.Aggregation = agg } else { // Provide default aggregation config with prefix conflict resolution config.Aggregation = &vmcpconfig.AggregationConfig{ @@ -221,9 +233,9 @@ func mapResolvedOIDCToVmcpConfig( // convertOutgoingAuth converts OutgoingAuthConfig from CRD to vmcp config func (c *Converter) convertOutgoingAuth( - _ context.Context, + ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, -) *vmcpconfig.OutgoingAuthConfig { +) (*vmcpconfig.OutgoingAuthConfig, error) { outgoing := &vmcpconfig.OutgoingAuthConfig{ Source: vmcp.Spec.OutgoingAuth.Source, Backends: make(map[string]*authtypes.BackendAuthStrategy), @@ -231,41 +243,136 @@ func (c *Converter) convertOutgoingAuth( // Convert Default if vmcp.Spec.OutgoingAuth.Default != nil { - outgoing.Default = c.convertBackendAuthConfig(vmcp.Spec.OutgoingAuth.Default) + defaultStrategy, err := c.convertBackendAuthConfig(ctx, vmcp, "default", vmcp.Spec.OutgoingAuth.Default) + if err != nil { + return nil, fmt.Errorf("failed to convert default backend auth: %w", err) + } + outgoing.Default = defaultStrategy } // Convert per-backend overrides for backendName, backendAuth := range vmcp.Spec.OutgoingAuth.Backends { - outgoing.Backends[backendName] = c.convertBackendAuthConfig(&backendAuth) + strategy, err := c.convertBackendAuthConfig(ctx, vmcp, backendName, &backendAuth) + if err != nil { + return nil, fmt.Errorf("failed to convert backend auth for %s: %w", backendName, err) + } + outgoing.Backends[backendName] = strategy } - return outgoing + return outgoing, nil } // convertBackendAuthConfig converts BackendAuthConfig from CRD to vmcp config -func (*Converter) convertBackendAuthConfig( +func (c *Converter) convertBackendAuthConfig( + ctx context.Context, + vmcp *mcpv1alpha1.VirtualMCPServer, + backendName string, crdConfig *mcpv1alpha1.BackendAuthConfig, -) *authtypes.BackendAuthStrategy { - strategy := &authtypes.BackendAuthStrategy{ - Type: crdConfig.Type, +) (*authtypes.BackendAuthStrategy, error) { + // If type is "discovered", return unauthenticated strategy + if crdConfig.Type == "discovered" { + return &authtypes.BackendAuthStrategy{ + Type: "unauthenticated", + }, nil + } + + // If type is "external_auth_config_ref", resolve the MCPExternalAuthConfig + if crdConfig.Type == "external_auth_config_ref" { + if crdConfig.ExternalAuthConfigRef == nil { + return nil, fmt.Errorf("backend %s: external_auth_config_ref type requires externalAuthConfigRef field", backendName) + } + + // Fetch the MCPExternalAuthConfig resource + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{} + err := c.k8sClient.Get(ctx, types.NamespacedName{ + Name: crdConfig.ExternalAuthConfigRef.Name, + Namespace: vmcp.Namespace, + }, externalAuthConfig) + if err != nil { + return nil, fmt.Errorf("failed to get MCPExternalAuthConfig %s/%s: %w", + vmcp.Namespace, crdConfig.ExternalAuthConfigRef.Name, err) + } + + // Convert the external auth config to backend auth strategy + return c.convertExternalAuthConfigToStrategy(ctx, externalAuthConfig) } - // Note: When Type is "external_auth_config_ref", the actual MCPExternalAuthConfig - // resource should be resolved at runtime and its configuration (TokenExchange or - // HeaderInjection) should be populated into the corresponding typed fields. - // This conversion happens during server initialization when the referenced - // MCPExternalAuthConfig can be looked up. + // Unknown type + return nil, fmt.Errorf("backend %s: unknown auth type %q", backendName, crdConfig.Type) +} + +// convertExternalAuthConfigToStrategy converts MCPExternalAuthConfig to BackendAuthStrategy +func (*Converter) convertExternalAuthConfigToStrategy( + _ context.Context, + externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig, +) (*authtypes.BackendAuthStrategy, error) { + strategy := &authtypes.BackendAuthStrategy{} + + switch externalAuthConfig.Spec.Type { + case mcpv1alpha1.ExternalAuthTypeUnauthenticated: + strategy.Type = "unauthenticated" + + case mcpv1alpha1.ExternalAuthTypeHeaderInjection: + if externalAuthConfig.Spec.HeaderInjection == nil { + return nil, fmt.Errorf("headerInjection config is required when type is headerInjection") + } - return strategy + strategy.Type = "header_injection" + strategy.HeaderInjection = &authtypes.HeaderInjectionConfig{ + HeaderName: externalAuthConfig.Spec.HeaderInjection.HeaderName, + // The secret value will be mounted as an environment variable by the deployment controller + // Use the same env var naming convention as the deployment controller + HeaderValueEnv: generateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name), + } + + case mcpv1alpha1.ExternalAuthTypeTokenExchange: + if externalAuthConfig.Spec.TokenExchange == nil { + return nil, fmt.Errorf("tokenExchange config is required when type is tokenExchange") + } + + strategy.Type = "token_exchange" + strategy.TokenExchange = &authtypes.TokenExchangeConfig{ + TokenURL: externalAuthConfig.Spec.TokenExchange.TokenURL, + ClientID: externalAuthConfig.Spec.TokenExchange.ClientID, + Audience: externalAuthConfig.Spec.TokenExchange.Audience, + Scopes: externalAuthConfig.Spec.TokenExchange.Scopes, + SubjectTokenType: externalAuthConfig.Spec.TokenExchange.SubjectTokenType, + } + + // If client secret ref is set, use an environment variable + if externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { + // The secret value will be mounted as an environment variable by the deployment controller + // Use the same env var naming convention as the deployment controller + strategy.TokenExchange.ClientSecretEnv = generateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) + } + + default: + return nil, fmt.Errorf("unknown external auth type: %s", externalAuthConfig.Spec.Type) + } + + return strategy, nil } // convertAggregation converts AggregationConfig from CRD to vmcp config -func (*Converter) convertAggregation( - _ context.Context, +func (c *Converter) convertAggregation( + ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, -) *vmcpconfig.AggregationConfig { +) (*vmcpconfig.AggregationConfig, error) { agg := &vmcpconfig.AggregationConfig{} + c.convertConflictResolution(vmcp, agg) + if err := c.convertToolConfigs(ctx, vmcp, agg); err != nil { + return nil, err + } + + return agg, nil +} + +// convertConflictResolution converts conflict resolution strategy and config +func (*Converter) convertConflictResolution( + vmcp *mcpv1alpha1.VirtualMCPServer, + agg *vmcpconfig.AggregationConfig, +) { // Convert conflict resolution strategy switch vmcp.Spec.Aggregation.ConflictResolution { case mcpv1alpha1.ConflictResolutionPrefix: @@ -290,32 +397,137 @@ func (*Converter) convertAggregation( PrefixFormat: "{workload}_", } } +} - // Convert per-workload tool configs - if len(vmcp.Spec.Aggregation.Tools) > 0 { - agg.Tools = make([]*vmcpconfig.WorkloadToolConfig, 0, len(vmcp.Spec.Aggregation.Tools)) - for _, toolConfig := range vmcp.Spec.Aggregation.Tools { - wtc := &vmcpconfig.WorkloadToolConfig{ - Workload: toolConfig.Workload, - Filter: toolConfig.Filter, - } +// convertToolConfigs converts per-workload tool configurations +func (c *Converter) convertToolConfigs( + ctx context.Context, + vmcp *mcpv1alpha1.VirtualMCPServer, + agg *vmcpconfig.AggregationConfig, +) error { + if len(vmcp.Spec.Aggregation.Tools) == 0 { + return nil + } - // Convert overrides - if len(toolConfig.Overrides) > 0 { - wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride) - for toolName, override := range toolConfig.Overrides { - wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{ - Name: override.Name, - Description: override.Description, - } - } + ctxLogger := log.FromContext(ctx) + agg.Tools = make([]*vmcpconfig.WorkloadToolConfig, 0, len(vmcp.Spec.Aggregation.Tools)) + + for _, toolConfig := range vmcp.Spec.Aggregation.Tools { + wtc := &vmcpconfig.WorkloadToolConfig{ + Workload: toolConfig.Workload, + Filter: toolConfig.Filter, + } + + if err := c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc); err != nil { + return err + } + c.applyInlineOverrides(toolConfig, wtc) + + agg.Tools = append(agg.Tools, wtc) + } + return nil +} + +// applyToolConfigRef resolves and applies MCPToolConfig reference +func (c *Converter) applyToolConfigRef( + ctx context.Context, + ctxLogger logr.Logger, + vmcp *mcpv1alpha1.VirtualMCPServer, + toolConfig mcpv1alpha1.WorkloadToolConfig, + wtc *vmcpconfig.WorkloadToolConfig, +) error { + if toolConfig.ToolConfigRef == nil { + return nil + } + + resolvedConfig, err := c.resolveMCPToolConfig(ctx, vmcp.Namespace, toolConfig.ToolConfigRef.Name) + if err != nil { + ctxLogger.Error(err, "failed to resolve MCPToolConfig reference", + "workload", toolConfig.Workload, + "toolConfigRef", toolConfig.ToolConfigRef.Name) + // Fail closed: return error when MCPToolConfig is configured but resolution fails + // This prevents deploying without tool filtering when explicit configuration is requested + return fmt.Errorf("MCPToolConfig resolution failed for %q: %w", + toolConfig.ToolConfigRef.Name, err) + } + + // Note: resolveMCPToolConfig never returns (nil, nil) - it either succeeds with + // (toolConfig, nil) or fails with (nil, error), so no nil check needed here + + c.mergeToolConfigFilter(wtc, resolvedConfig) + c.mergeToolConfigOverrides(wtc, resolvedConfig) + return nil +} + +// mergeToolConfigFilter merges filter from MCPToolConfig +func (*Converter) mergeToolConfigFilter( + wtc *vmcpconfig.WorkloadToolConfig, + resolvedConfig *mcpv1alpha1.MCPToolConfig, +) { + if len(wtc.Filter) == 0 && len(resolvedConfig.Spec.ToolsFilter) > 0 { + wtc.Filter = resolvedConfig.Spec.ToolsFilter + } +} + +// mergeToolConfigOverrides merges overrides from MCPToolConfig +func (*Converter) mergeToolConfigOverrides( + wtc *vmcpconfig.WorkloadToolConfig, + resolvedConfig *mcpv1alpha1.MCPToolConfig, +) { + if len(resolvedConfig.Spec.ToolsOverride) == 0 { + return + } + + if wtc.Overrides == nil { + wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride) + } + + for toolName, override := range resolvedConfig.Spec.ToolsOverride { + if _, exists := wtc.Overrides[toolName]; !exists { + wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{ + Name: override.Name, + Description: override.Description, } + } + } +} - agg.Tools = append(agg.Tools, wtc) +// applyInlineOverrides applies inline tool overrides +func (*Converter) applyInlineOverrides( + toolConfig mcpv1alpha1.WorkloadToolConfig, + wtc *vmcpconfig.WorkloadToolConfig, +) { + if len(toolConfig.Overrides) == 0 { + return + } + + if wtc.Overrides == nil { + wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride) + } + + for toolName, override := range toolConfig.Overrides { + wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{ + Name: override.Name, + Description: override.Description, } } +} - return agg +// resolveMCPToolConfig fetches an MCPToolConfig resource by name and namespace +func (c *Converter) resolveMCPToolConfig( + ctx context.Context, + namespace string, + name string, +) (*mcpv1alpha1.MCPToolConfig, error) { + toolConfig := &mcpv1alpha1.MCPToolConfig{} + err := c.k8sClient.Get(ctx, types.NamespacedName{ + Name: name, + Namespace: namespace, + }, toolConfig) + if err != nil { + return nil, fmt.Errorf("failed to get MCPToolConfig %s/%s: %w", namespace, name, err) + } + return toolConfig, nil } // convertCompositeTools converts CompositeToolSpec from CRD to vmcp config @@ -671,3 +883,25 @@ func (*Converter) convertOperational( return operational } + +// generateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange +// client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness. +// This must match the naming convention used by the deployment controller. +func generateUniqueTokenExchangeEnvVarName(configName string) string { + // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) + sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) + // Remove any remaining invalid characters (keep only alphanumeric and underscore) + sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized) +} + +// generateUniqueHeaderInjectionEnvVarName generates a unique environment variable name for header injection +// values, incorporating the ExternalAuthConfig name to ensure uniqueness. +// This must match the naming convention used by the deployment controller. +func generateUniqueHeaderInjectionEnvVarName(configName string) string { + // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) + sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) + // Remove any remaining invalid characters (keep only alphanumeric and underscore) + sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) +} From fcaf921f2c17085055b6d581b1b61795818b8fda Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 5 Dec 2025 15:48:55 +0100 Subject: [PATCH 3/5] fix ci --- .../virtualmcpserver_controller.go | 26 +---- .../virtualmcpserver_deployment.go | 4 +- .../virtualmcpserver_externalauth_test.go | 4 +- .../virtualmcpserver_vmcpconfig_test.go | 39 ++++++- .../pkg/controllerutil/externalauth.go | 38 +++++++ .../pkg/controllerutil/externalauth_test.go | 102 ++++++++++++++++++ cmd/thv-operator/pkg/vmcpconfig/converter.go | 41 ++----- 7 files changed, 191 insertions(+), 63 deletions(-) create mode 100644 cmd/thv-operator/pkg/controllerutil/externalauth.go create mode 100644 cmd/thv-operator/pkg/controllerutil/externalauth_test.go diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 2e73eab20..0387f491d 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -8,8 +8,6 @@ import ( "fmt" "maps" "reflect" - "regexp" - "strings" "time" appsv1 "k8s.io/api/apps/v1" @@ -1304,37 +1302,17 @@ func (*VirtualMCPServerReconciler) convertExternalAuthConfigToStrategy( if strategy.TokenExchange != nil && externalAuthConfig.Spec.TokenExchange != nil && externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { - strategy.TokenExchange.ClientSecretEnv = generateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) + strategy.TokenExchange.ClientSecretEnv = ctrlutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) } if strategy.HeaderInjection != nil && externalAuthConfig.Spec.HeaderInjection != nil && externalAuthConfig.Spec.HeaderInjection.ValueSecretRef != nil { - strategy.HeaderInjection.HeaderValueEnv = generateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name) + strategy.HeaderInjection.HeaderValueEnv = ctrlutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name) } return strategy, nil } -// generateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange -// client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness. -func generateUniqueTokenExchangeEnvVarName(configName string) string { - // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) - sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) - // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = envVarSanitizeRegex.ReplaceAllString(sanitized, "_") - return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized) -} - -// generateUniqueHeaderInjectionEnvVarName generates a unique environment variable name for header injection -// values, incorporating the ExternalAuthConfig name to ensure uniqueness. -func generateUniqueHeaderInjectionEnvVarName(configName string) string { - // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) - sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) - // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = envVarSanitizeRegex.ReplaceAllString(sanitized, "_") - return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) -} - // convertBackendAuthConfigToVMCP converts a BackendAuthConfig from CRD to vmcp config. func (r *VirtualMCPServerReconciler) convertBackendAuthConfigToVMCP( ctx context.Context, diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index 664a49476..191f3e80c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -433,7 +433,7 @@ func (r *VirtualMCPServerReconciler) getExternalAuthConfigSecretEnvVar( if externalAuthConfig.Spec.TokenExchange.ClientSecretRef == nil { return nil, nil // No secret to mount } - envVarName = generateUniqueTokenExchangeEnvVarName(externalAuthConfigName) + envVarName = ctrlutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfigName) secretRef = externalAuthConfig.Spec.TokenExchange.ClientSecretRef case mcpv1alpha1.ExternalAuthTypeHeaderInjection: @@ -443,7 +443,7 @@ func (r *VirtualMCPServerReconciler) getExternalAuthConfigSecretEnvVar( if externalAuthConfig.Spec.HeaderInjection.ValueSecretRef == nil { return nil, nil // No secret to mount } - envVarName = generateUniqueHeaderInjectionEnvVarName(externalAuthConfigName) + envVarName = ctrlutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfigName) secretRef = externalAuthConfig.Spec.HeaderInjection.ValueSecretRef case mcpv1alpha1.ExternalAuthTypeUnauthenticated: diff --git a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go index 1e13e3ec6..8b383564c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go @@ -950,7 +950,7 @@ func TestGenerateUniqueTokenExchangeEnvVarName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := generateUniqueTokenExchangeEnvVarName(tt.configName) + result := ctrlutil.GenerateUniqueTokenExchangeEnvVarName(tt.configName) assert.Contains(t, result, expectedPrefix) assert.Contains(t, result, tt.expectedSuffix) // Verify format: PREFIX_SUFFIX @@ -1007,7 +1007,7 @@ func TestGenerateUniqueHeaderInjectionEnvVarName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := generateUniqueHeaderInjectionEnvVarName(tt.configName) + result := ctrlutil.GenerateUniqueHeaderInjectionEnvVarName(tt.configName) assert.True(t, strings.HasPrefix(result, expectedPrefix+"_"), "Result should start with prefix") assert.True(t, strings.HasSuffix(result, tt.expectedSuffix), "Result should end with suffix") // Verify format: PREFIX_SUFFIX diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index a49dd870d..7f088186e 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -196,7 +196,8 @@ func TestConvertBackendAuthConfig(t *testing.T) { authConfig: &mcpv1alpha1.BackendAuthConfig{ Type: mcpv1alpha1.BackendAuthTypeDiscovered, }, - expectedType: mcpv1alpha1.BackendAuthTypeDiscovered, + // "discovered" type is converted to "unauthenticated" by the converter + expectedType: "unauthenticated", }, { name: "external auth config ref", @@ -206,7 +207,8 @@ func TestConvertBackendAuthConfig(t *testing.T) { Name: "auth-config", }, }, - expectedType: mcpv1alpha1.BackendAuthTypeExternalAuthConfigRef, + // For external_auth_config_ref, the type comes from the referenced MCPExternalAuthConfig + expectedType: "unauthenticated", }, } @@ -216,6 +218,10 @@ func TestConvertBackendAuthConfig(t *testing.T) { t.Parallel() vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmcp", + Namespace: "default", + }, Spec: mcpv1alpha1.VirtualMCPServerSpec{ GroupRef: mcpv1alpha1.GroupRef{ Name: "test-group", @@ -226,7 +232,34 @@ func TestConvertBackendAuthConfig(t *testing.T) { }, } - converter := newTestConverter(t, newNoOpMockResolver(t)) + // For external_auth_config_ref test, create the referenced MCPExternalAuthConfig + var converter *vmcpconfig.Converter + if tt.authConfig.Type == mcpv1alpha1.BackendAuthTypeExternalAuthConfigRef { + // Create a fake MCPExternalAuthConfig + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + } + + // Create converter with fake client that has the external auth config + scheme := runtime.NewScheme() + _ = mcpv1alpha1.AddToScheme(scheme) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(externalAuthConfig). + Build() + var err error + converter, err = vmcpconfig.NewConverter(newNoOpMockResolver(t), fakeClient) + require.NoError(t, err) + } else { + converter = newTestConverter(t, newNoOpMockResolver(t)) + } + config, err := converter.Convert(context.Background(), vmcpServer) require.NoError(t, err) diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth.go b/cmd/thv-operator/pkg/controllerutil/externalauth.go new file mode 100644 index 000000000..01ceff543 --- /dev/null +++ b/cmd/thv-operator/pkg/controllerutil/externalauth.go @@ -0,0 +1,38 @@ +// Package controllerutil provides utility functions for Kubernetes controllers. +package controllerutil + +import ( + "fmt" + "regexp" + "strings" +) + +// GenerateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange +// client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness. +// This function is used by both the converter and deployment controller to ensure consistent +// environment variable naming across the system. +// +// Example: For an ExternalAuthConfig named "my-auth-config", this returns: +// "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_MY_AUTH_CONFIG" +func GenerateUniqueTokenExchangeEnvVarName(configName string) string { + // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) + sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) + // Remove any remaining invalid characters (keep only alphanumeric and underscore) + sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized) +} + +// GenerateUniqueHeaderInjectionEnvVarName generates a unique environment variable name for header injection +// values, incorporating the ExternalAuthConfig name to ensure uniqueness. +// This function is used by both the converter and deployment controller to ensure consistent +// environment variable naming across the system. +// +// Example: For an ExternalAuthConfig named "my-auth-config", this returns: +// "TOOLHIVE_HEADER_INJECTION_VALUE_MY_AUTH_CONFIG" +func GenerateUniqueHeaderInjectionEnvVarName(configName string) string { + // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) + sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) + // Remove any remaining invalid characters (keep only alphanumeric and underscore) + sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) +} diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth_test.go b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go new file mode 100644 index 000000000..2b8c7ea48 --- /dev/null +++ b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go @@ -0,0 +1,102 @@ +package controllerutil + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestGenerateUniqueTokenExchangeEnvVarName tests the GenerateUniqueTokenExchangeEnvVarName function +func TestGenerateUniqueTokenExchangeEnvVarName(t *testing.T) { + t.Parallel() + + expectedPrefix := "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET" + tests := []struct { + name string + configName string + expectedSuffix string + }{ + { + name: "simple name", + configName: "test-config", + expectedSuffix: "TEST_CONFIG", + }, + { + name: "multiple hyphens", + configName: "my-test-config", + expectedSuffix: "MY_TEST_CONFIG", + }, + { + name: "with special characters", + configName: "test.config@123", + expectedSuffix: "TEST_CONFIG_123", + }, + { + name: "single character", + configName: "a", + expectedSuffix: "A", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := GenerateUniqueTokenExchangeEnvVarName(tt.configName) + assert.Contains(t, result, expectedPrefix) + assert.Contains(t, result, tt.expectedSuffix) + // Verify format: PREFIX_SUFFIX + assert.Contains(t, result, "_") + // Verify all characters are valid for env vars (uppercase, alphanumeric, underscore) + envVarPattern := regexp.MustCompile(`^[A-Z0-9_]+$`) + assert.Regexp(t, envVarPattern, result, "Result should be a valid environment variable name") + }) + } +} + +// TestGenerateUniqueHeaderInjectionEnvVarName tests the GenerateUniqueHeaderInjectionEnvVarName function +func TestGenerateUniqueHeaderInjectionEnvVarName(t *testing.T) { + t.Parallel() + + expectedPrefix := "TOOLHIVE_HEADER_INJECTION_VALUE" + tests := []struct { + name string + configName string + expectedSuffix string + }{ + { + name: "simple name", + configName: "test-config", + expectedSuffix: "TEST_CONFIG", + }, + { + name: "multiple hyphens", + configName: "my-test-config", + expectedSuffix: "MY_TEST_CONFIG", + }, + { + name: "with special characters", + configName: "test.config@123", + expectedSuffix: "TEST_CONFIG_123", + }, + { + name: "single character", + configName: "x", + expectedSuffix: "X", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := GenerateUniqueHeaderInjectionEnvVarName(tt.configName) + assert.True(t, regexp.MustCompile("^"+expectedPrefix+"_").MatchString(result), "Result should start with prefix") + assert.True(t, regexp.MustCompile(tt.expectedSuffix+"$").MatchString(result), "Result should end with suffix") + // Verify format: PREFIX_SUFFIX + assert.Contains(t, result, "_") + // Verify all characters are valid for env vars (uppercase, alphanumeric, underscore) + envVarPattern := regexp.MustCompile(`^[A-Z0-9_]+$`) + assert.Regexp(t, envVarPattern, result, "Result should be a valid environment variable name") + }) + } +} diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index c15dac904..a6d4f30c5 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -5,8 +5,6 @@ import ( "context" "encoding/json" "fmt" - "regexp" - "strings" "time" "github.com/go-logr/logr" @@ -17,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/spectoconfig" authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" @@ -270,14 +269,14 @@ func (c *Converter) convertBackendAuthConfig( crdConfig *mcpv1alpha1.BackendAuthConfig, ) (*authtypes.BackendAuthStrategy, error) { // If type is "discovered", return unauthenticated strategy - if crdConfig.Type == "discovered" { + if crdConfig.Type == mcpv1alpha1.BackendAuthTypeDiscovered { return &authtypes.BackendAuthStrategy{ - Type: "unauthenticated", + Type: authtypes.StrategyTypeUnauthenticated, }, nil } // If type is "external_auth_config_ref", resolve the MCPExternalAuthConfig - if crdConfig.Type == "external_auth_config_ref" { + if crdConfig.Type == mcpv1alpha1.BackendAuthTypeExternalAuthConfigRef { if crdConfig.ExternalAuthConfigRef == nil { return nil, fmt.Errorf("backend %s: external_auth_config_ref type requires externalAuthConfigRef field", backendName) } @@ -310,19 +309,19 @@ func (*Converter) convertExternalAuthConfigToStrategy( switch externalAuthConfig.Spec.Type { case mcpv1alpha1.ExternalAuthTypeUnauthenticated: - strategy.Type = "unauthenticated" + strategy.Type = authtypes.StrategyTypeUnauthenticated case mcpv1alpha1.ExternalAuthTypeHeaderInjection: if externalAuthConfig.Spec.HeaderInjection == nil { return nil, fmt.Errorf("headerInjection config is required when type is headerInjection") } - strategy.Type = "header_injection" + strategy.Type = authtypes.StrategyTypeHeaderInjection strategy.HeaderInjection = &authtypes.HeaderInjectionConfig{ HeaderName: externalAuthConfig.Spec.HeaderInjection.HeaderName, // The secret value will be mounted as an environment variable by the deployment controller // Use the same env var naming convention as the deployment controller - HeaderValueEnv: generateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name), + HeaderValueEnv: controllerutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name), } case mcpv1alpha1.ExternalAuthTypeTokenExchange: @@ -330,7 +329,7 @@ func (*Converter) convertExternalAuthConfigToStrategy( return nil, fmt.Errorf("tokenExchange config is required when type is tokenExchange") } - strategy.Type = "token_exchange" + strategy.Type = authtypes.StrategyTypeTokenExchange strategy.TokenExchange = &authtypes.TokenExchangeConfig{ TokenURL: externalAuthConfig.Spec.TokenExchange.TokenURL, ClientID: externalAuthConfig.Spec.TokenExchange.ClientID, @@ -343,7 +342,7 @@ func (*Converter) convertExternalAuthConfigToStrategy( if externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { // The secret value will be mounted as an environment variable by the deployment controller // Use the same env var naming convention as the deployment controller - strategy.TokenExchange.ClientSecretEnv = generateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) + strategy.TokenExchange.ClientSecretEnv = controllerutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) } default: @@ -883,25 +882,3 @@ func (*Converter) convertOperational( return operational } - -// generateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange -// client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness. -// This must match the naming convention used by the deployment controller. -func generateUniqueTokenExchangeEnvVarName(configName string) string { - // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) - sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) - // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") - return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized) -} - -// generateUniqueHeaderInjectionEnvVarName generates a unique environment variable name for header injection -// values, incorporating the ExternalAuthConfig name to ensure uniqueness. -// This must match the naming convention used by the deployment controller. -func generateUniqueHeaderInjectionEnvVarName(configName string) string { - // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) - sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) - // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") - return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) -} From a2d668ecb71906c6eafb455ca375f836c22b5343 Mon Sep 17 00:00:00 2001 From: taskbot Date: Tue, 9 Dec 2025 09:53:56 +0100 Subject: [PATCH 4/5] changes from review --- .../v1alpha1/mcpexternalauthconfig_webhook.go | 16 ++++- .../virtualmcpserver_controller.go | 5 +- .../pkg/controllerutil/externalauth.go | 8 ++- cmd/thv-operator/pkg/vmcpconfig/converter.go | 67 +++++++------------ deploy/charts/operator-crds/Chart.yaml | 2 +- deploy/charts/operator-crds/README.md | 2 +- pkg/vmcp/auth/converters/registry_test.go | 11 +++ 7 files changed, 60 insertions(+), 51 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go index ecae8b226..435d2b70c 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go @@ -24,14 +24,26 @@ var _ webhook.CustomValidator = &MCPExternalAuthConfig{} // ValidateCreate implements webhook.CustomValidator func (r *MCPExternalAuthConfig) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) { - return nil, r.validate() + var warnings admission.Warnings + if r.Spec.Type == ExternalAuthTypeUnauthenticated { + warnings = append(warnings, + "'unauthenticated' type disables authentication to the backend. "+ + "Only use for backends on trusted networks or when authentication is handled by network-level security.") + } + return warnings, r.validate() } // ValidateUpdate implements webhook.CustomValidator func (r *MCPExternalAuthConfig) ValidateUpdate( _ context.Context, _ runtime.Object, _ runtime.Object, ) (admission.Warnings, error) { - return nil, r.validate() + var warnings admission.Warnings + if r.Spec.Type == ExternalAuthTypeUnauthenticated { + warnings = append(warnings, + "'unauthenticated' type disables authentication to the backend. "+ + "Only use for backends on trusted networks or when authentication is handled by network-level security.") + } + return warnings, r.validate() } // ValidateDelete implements webhook.CustomValidator diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 0387f491d..f67ac941a 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -8,6 +8,7 @@ import ( "fmt" "maps" "reflect" + "strings" "time" appsv1 "k8s.io/api/apps/v1" @@ -64,10 +65,6 @@ type VirtualMCPServerReconciler struct { PlatformDetector *ctrlutil.SharedPlatformDetector } -var ( - envVarSanitizeRegex = regexp.MustCompile(`[^A-Z0-9_]`) -) - // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=virtualmcpservers,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=virtualmcpservers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpgroups,verbs=get;list;watch diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth.go b/cmd/thv-operator/pkg/controllerutil/externalauth.go index 01ceff543..08a5495a4 100644 --- a/cmd/thv-operator/pkg/controllerutil/externalauth.go +++ b/cmd/thv-operator/pkg/controllerutil/externalauth.go @@ -7,6 +7,10 @@ import ( "strings" ) +var ( + envVarSanitizer = regexp.MustCompile(`[^A-Z0-9_]`) +) + // GenerateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange // client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness. // This function is used by both the converter and deployment controller to ensure consistent @@ -18,7 +22,7 @@ func GenerateUniqueTokenExchangeEnvVarName(configName string) string { // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + sanitized = envVarSanitizer.ReplaceAllString(sanitized, "_") return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized) } @@ -33,6 +37,6 @@ func GenerateUniqueHeaderInjectionEnvVarName(configName string) string { // Sanitize config name for use in env var (uppercase, replace invalid chars with underscore) sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_")) // Remove any remaining invalid characters (keep only alphanumeric and underscore) - sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_") + sanitized = envVarSanitizer.ReplaceAllString(sanitized, "_") return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) } diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index a6d4f30c5..27b147652 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -18,6 +18,7 @@ import ( "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/spectoconfig" + "github.com/stacklok/toolhive/pkg/vmcp/auth/converters" authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" ) @@ -300,53 +301,37 @@ func (c *Converter) convertBackendAuthConfig( return nil, fmt.Errorf("backend %s: unknown auth type %q", backendName, crdConfig.Type) } -// convertExternalAuthConfigToStrategy converts MCPExternalAuthConfig to BackendAuthStrategy +// convertExternalAuthConfigToStrategy converts MCPExternalAuthConfig to BackendAuthStrategy. +// This uses the converter registry to consolidate conversion logic and apply token type normalization consistently. +// The registry pattern makes adding new auth types easier and ensures conversion happens in one place. func (*Converter) convertExternalAuthConfigToStrategy( _ context.Context, externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig, ) (*authtypes.BackendAuthStrategy, error) { - strategy := &authtypes.BackendAuthStrategy{} - - switch externalAuthConfig.Spec.Type { - case mcpv1alpha1.ExternalAuthTypeUnauthenticated: - strategy.Type = authtypes.StrategyTypeUnauthenticated - - case mcpv1alpha1.ExternalAuthTypeHeaderInjection: - if externalAuthConfig.Spec.HeaderInjection == nil { - return nil, fmt.Errorf("headerInjection config is required when type is headerInjection") - } - - strategy.Type = authtypes.StrategyTypeHeaderInjection - strategy.HeaderInjection = &authtypes.HeaderInjectionConfig{ - HeaderName: externalAuthConfig.Spec.HeaderInjection.HeaderName, - // The secret value will be mounted as an environment variable by the deployment controller - // Use the same env var naming convention as the deployment controller - HeaderValueEnv: controllerutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name), - } - - case mcpv1alpha1.ExternalAuthTypeTokenExchange: - if externalAuthConfig.Spec.TokenExchange == nil { - return nil, fmt.Errorf("tokenExchange config is required when type is tokenExchange") - } - - strategy.Type = authtypes.StrategyTypeTokenExchange - strategy.TokenExchange = &authtypes.TokenExchangeConfig{ - TokenURL: externalAuthConfig.Spec.TokenExchange.TokenURL, - ClientID: externalAuthConfig.Spec.TokenExchange.ClientID, - Audience: externalAuthConfig.Spec.TokenExchange.Audience, - Scopes: externalAuthConfig.Spec.TokenExchange.Scopes, - SubjectTokenType: externalAuthConfig.Spec.TokenExchange.SubjectTokenType, - } + // Use the converter registry to convert to typed strategy + registry := converters.DefaultRegistry() + converter, err := registry.GetConverter(externalAuthConfig.Spec.Type) + if err != nil { + return nil, err + } - // If client secret ref is set, use an environment variable - if externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { - // The secret value will be mounted as an environment variable by the deployment controller - // Use the same env var naming convention as the deployment controller - strategy.TokenExchange.ClientSecretEnv = controllerutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) - } + // Convert to typed BackendAuthStrategy (applies token type normalization) + strategy, err := converter.ConvertToStrategy(externalAuthConfig) + if err != nil { + return nil, fmt.Errorf("failed to convert external auth config to strategy: %w", err) + } - default: - return nil, fmt.Errorf("unknown external auth type: %s", externalAuthConfig.Spec.Type) + // Enrich with unique env var names per ExternalAuthConfig to avoid conflicts + // when multiple configs of the same type reference different secrets + if strategy.TokenExchange != nil && + externalAuthConfig.Spec.TokenExchange != nil && + externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { + strategy.TokenExchange.ClientSecretEnv = controllerutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name) + } + if strategy.HeaderInjection != nil && + externalAuthConfig.Spec.HeaderInjection != nil && + externalAuthConfig.Spec.HeaderInjection.ValueSecretRef != nil { + strategy.HeaderInjection.HeaderValueEnv = controllerutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name) } return strategy, nil diff --git a/deploy/charts/operator-crds/Chart.yaml b/deploy/charts/operator-crds/Chart.yaml index 87e93efd0..89424dc63 100644 --- a/deploy/charts/operator-crds/Chart.yaml +++ b/deploy/charts/operator-crds/Chart.yaml @@ -2,5 +2,5 @@ apiVersion: v2 name: toolhive-operator-crds description: A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. type: application -version: 0.0.76 +version: 0.0.77 appVersion: "0.0.1" diff --git a/deploy/charts/operator-crds/README.md b/deploy/charts/operator-crds/README.md index 8d77792f6..81af44a6a 100644 --- a/deploy/charts/operator-crds/README.md +++ b/deploy/charts/operator-crds/README.md @@ -1,6 +1,6 @@ # ToolHive Operator CRDs Helm Chart -![Version: 0.0.76](https://img.shields.io/badge/Version-0.0.76-informational?style=flat-square) +![Version: 0.0.77](https://img.shields.io/badge/Version-0.0.77-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. diff --git a/pkg/vmcp/auth/converters/registry_test.go b/pkg/vmcp/auth/converters/registry_test.go index 3a2b1db90..58afa4d43 100644 --- a/pkg/vmcp/auth/converters/registry_test.go +++ b/pkg/vmcp/auth/converters/registry_test.go @@ -79,6 +79,12 @@ func TestDefaultRegistry(t *testing.T) { require.NoError(t, err) require.NotNil(t, headerInjectionConverter) assert.Equal(t, "header_injection", headerInjectionConverter.StrategyType()) + + // Test unauthenticated converter + unauthenticatedConverter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeUnauthenticated) + require.NoError(t, err) + require.NotNil(t, unauthenticatedConverter) + assert.Equal(t, "unauthenticated", unauthenticatedConverter.StrategyType()) }) } @@ -100,6 +106,10 @@ func TestNewRegistry(t *testing.T) { headerInjectionConverter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeHeaderInjection) require.NoError(t, err) assert.NotNil(t, headerInjectionConverter) + + unauthenticatedConverter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeUnauthenticated) + require.NoError(t, err) + assert.NotNil(t, unauthenticatedConverter) }) t.Run("creates independent instances", func(t *testing.T) { @@ -124,6 +134,7 @@ func TestNewRegistry(t *testing.T) { }{ {mcpv1alpha1.ExternalAuthTypeTokenExchange, "token_exchange"}, {mcpv1alpha1.ExternalAuthTypeHeaderInjection, "header_injection"}, + {mcpv1alpha1.ExternalAuthTypeUnauthenticated, "unauthenticated"}, } for _, tc := range testCases { From 1fc3af1ff4197f855eb89bb1331790c3818c8ebb Mon Sep 17 00:00:00 2001 From: taskbot Date: Tue, 9 Dec 2025 10:06:40 +0100 Subject: [PATCH 5/5] fix ci --- .../mcpexternalauthconfig_webhook_test.go | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go index d2837217d..ade6d6441 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go @@ -13,10 +13,12 @@ func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { t.Parallel() tests := []struct { - name string - config *MCPExternalAuthConfig - expectError bool - errorMsg string + name string + config *MCPExternalAuthConfig + expectError bool + errorMsg string + expectWarning bool + warningMsg string }{ { name: "valid unauthenticated", @@ -29,7 +31,9 @@ func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { Type: ExternalAuthTypeUnauthenticated, }, }, - expectError: false, + expectError: false, + expectWarning: true, + warningMsg: "'unauthenticated' type disables authentication to the backend. Only use for backends on trusted networks or when authentication is handled by network-level security.", }, { name: "unauthenticated with tokenExchange should fail", @@ -45,8 +49,10 @@ func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { }, }, }, - expectError: true, - errorMsg: "tokenExchange must not be set when type is 'unauthenticated'", + expectError: true, + errorMsg: "tokenExchange must not be set when type is 'unauthenticated'", + expectWarning: true, + warningMsg: "'unauthenticated' type disables authentication to the backend. Only use for backends on trusted networks or when authentication is handled by network-level security.", }, { name: "unauthenticated with headerInjection should fail", @@ -66,8 +72,10 @@ func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { }, }, }, - expectError: true, - errorMsg: "headerInjection must not be set when type is 'unauthenticated'", + expectError: true, + errorMsg: "headerInjection must not be set when type is 'unauthenticated'", + expectWarning: true, + warningMsg: "'unauthenticated' type disables authentication to the backend. Only use for backends on trusted networks or when authentication is handled by network-level security.", }, { name: "valid tokenExchange", @@ -198,8 +206,13 @@ func TestMCPExternalAuthConfig_ValidateCreate(t *testing.T) { require.NoError(t, err) } - // Warnings should always be nil for now - assert.Nil(t, warnings) + // Check warnings + if tt.expectWarning { + require.Len(t, warnings, 1, "expected exactly one warning") + assert.Equal(t, tt.warningMsg, string(warnings[0])) + } else { + assert.Nil(t, warnings, "expected no warnings") + } }) } } @@ -220,7 +233,9 @@ func TestMCPExternalAuthConfig_ValidateUpdate(t *testing.T) { // ValidateUpdate should use the same logic as ValidateCreate warnings, err := config.ValidateUpdate(context.Background(), nil, config) require.NoError(t, err) - assert.Nil(t, warnings) + // Should have warning for unauthenticated type + require.Len(t, warnings, 1, "expected exactly one warning") + assert.Equal(t, "'unauthenticated' type disables authentication to the backend. Only use for backends on trusted networks or when authentication is handled by network-level security.", string(warnings[0])) // Test invalid update invalidConfig := &MCPExternalAuthConfig{ @@ -239,7 +254,9 @@ func TestMCPExternalAuthConfig_ValidateUpdate(t *testing.T) { warnings, err = invalidConfig.ValidateUpdate(context.Background(), nil, invalidConfig) require.Error(t, err) assert.Contains(t, err.Error(), "tokenExchange must not be set when type is 'unauthenticated'") - assert.Nil(t, warnings) + // Should still have warning for unauthenticated type even when validation fails + require.Len(t, warnings, 1, "expected exactly one warning") + assert.Equal(t, "'unauthenticated' type disables authentication to the backend. Only use for backends on trusted networks or when authentication is handled by network-level security.", string(warnings[0])) } func TestMCPExternalAuthConfig_ValidateDelete(t *testing.T) {