diff --git a/api/v1alpha1/mcp_route.go b/api/v1alpha1/mcp_route.go index e8416d98e4..1d5d9a097c 100644 --- a/api/v1alpha1/mcp_route.go +++ b/api/v1alpha1/mcp_route.go @@ -152,8 +152,10 @@ type MCPBackendSecurityPolicy struct { } // MCPBackendAPIKey defines the configuration for the API Key Authentication to a backend. +// When both `header` and `queryParam` are unspecified, the API key will be injected into the "Authorization" header by default. // // +kubebuilder:validation:XValidation:rule="(has(self.secretRef) && !has(self.inline)) || (!has(self.secretRef) && has(self.inline))", message="exactly one of secretRef or inline must be set" +// +kubebuilder:validation:XValidation:rule="!(has(self.header) && has(self.queryParam))", message="only one of header or queryParam can be set" type MCPBackendAPIKey struct { // secretRef is the Kubernetes secret which contains the API keys. // The key of the secret should be "apiKey". @@ -170,10 +172,23 @@ type MCPBackendAPIKey struct { // When the header is "Authorization", the injected header value will be // prefixed with "Bearer ". // + // Either one of Header or QueryParam can be specified to inject the API key. + // // +kubebuilder:validation:Optional // +kubebuilder:validation:MinLength=1 // +optional Header *string `json:"header,omitempty"` + + // QueryParam is the HTTP query parameter to inject the API key into. + // For example, if QueryParam is set to "api_key", and the API key is "mysecretkey", the request URL will be modified to include + // "?api_key=mysecretkey". + // + // Either one of Header or QueryParam can be specified to inject the API key. + // + // +kubebuilder:validation:Optional + // +kubebuilder:validation:MinLength=1 + // +optional + QueryParam *string `json:"queryParam,omitempty"` } // MCPRouteSecurityPolicy defines the security policy for a MCPRoute. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index feffa039be..388014dfad 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1137,6 +1137,11 @@ func (in *MCPBackendAPIKey) DeepCopyInto(out *MCPBackendAPIKey) { *out = new(string) **out = **in } + if in.QueryParam != nil { + in, out := &in.QueryParam, &out.QueryParam + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MCPBackendAPIKey. diff --git a/internal/controller/gateway.go b/internal/controller/gateway.go index 9e605cb178..8228a75b11 100644 --- a/internal/controller/gateway.go +++ b/internal/controller/gateway.go @@ -495,7 +495,6 @@ func mcpConfig(mcpRoutes []aigv1a1.MCPRoute) (_ *filterapi.MCPConfig, hasEffecti mcpBackend := filterapi.MCPBackend{ // MCPRoute doesn't support cross-namespace backend reference so just use the name. Name: filterapi.MCPBackendName(b.Name), - Path: ptr.Deref(b.Path, "/mcp"), } if b.ToolSelector != nil { mcpBackend.ToolSelector = &filterapi.MCPToolSelector{ diff --git a/internal/controller/mcp_route.go b/internal/controller/mcp_route.go index 6ca5b1b170..45a93abbd6 100644 --- a/internal/controller/mcp_route.go +++ b/internal/controller/mcp_route.go @@ -6,14 +6,12 @@ package controller import ( - "cmp" "context" "fmt" "strings" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/go-logr/logr" - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -499,26 +497,76 @@ func mcpBackendRefFilterName(mcpRoute *aigv1a1.MCPRoute, backendName gwapiv1.Obj // which is set by the MCP proxy based on its routing logic. // This route rule will eventually be moved to the backend listener in the extension server. func (c *MCPRouteController) mcpBackendRefToHTTPRouteRule(ctx context.Context, mcpRoute *aigv1a1.MCPRoute, ref *aigv1a1.MCPRouteBackendRef) (gwapiv1.HTTPRouteRule, error) { - var apiKey *aigv1a1.MCPBackendAPIKey - if ref.SecurityPolicy != nil && ref.SecurityPolicy.APIKey != nil { - apiKey = ref.SecurityPolicy.APIKey - } - // Ensure the HTTPRouteFilter for this backend with its optional security configuration. - filterName := mcpBackendRefFilterName(mcpRoute, ref.Name) - err := c.ensureMCPBackendRefHTTPFilter(ctx, filterName, apiKey, mcpRoute) + egFilterName := mcpBackendRefFilterName(mcpRoute, ref.Name) + err := c.ensureMCPBackendRefHTTPFilter(ctx, egFilterName, mcpRoute) if err != nil { return gwapiv1.HTTPRouteRule{}, fmt.Errorf("failed to ensure MCP backend API key HTTP filter: %w", err) } + filters := []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: "gateway.envoyproxy.io", + Kind: "HTTPRouteFilter", + Name: gwapiv1.ObjectName(egFilterName), + }, + }, + } + + fullPathPtr := ptr.Deref(ref.Path, defaultMCPPath) - filters := []gwapiv1.HTTPRouteFilter{{ - Type: gwapiv1.HTTPRouteFilterExtensionRef, - ExtensionRef: &gwapiv1.LocalObjectReference{ - Group: "gateway.envoyproxy.io", - Kind: "HTTPRouteFilter", - Name: gwapiv1.ObjectName(filterName), + // Add credential injection if apiKey is specified. + if ref.SecurityPolicy != nil && ref.SecurityPolicy.APIKey != nil { + apiKey := ref.SecurityPolicy.APIKey + + apiKeyLiteral, err := c.readAPIKey(ctx, mcpRoute.Namespace, apiKey) + if err != nil { + return gwapiv1.HTTPRouteRule{}, fmt.Errorf("failed to read API key for backend %s: %w", ref.Name, err) + } + switch { + case apiKey.QueryParam != nil: + fullPathPtr = fmt.Sprintf("%s?%s=%s", fullPathPtr, *apiKey.QueryParam, apiKeyLiteral) + case apiKey.Header != nil: + header := *apiKey.Header + if header == "Authorization" { + apiKeyLiteral = "Bearer " + apiKeyLiteral + } + filters = append(filters, + gwapiv1.HTTPRouteFilter{ + Type: gwapiv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gwapiv1.HTTPHeaderFilter{ + Set: []gwapiv1.HTTPHeader{ + {Name: gwapiv1.HTTPHeaderName(header), Value: apiKeyLiteral}, + }, + }, + }, + ) + default: + filters = append(filters, + gwapiv1.HTTPRouteFilter{ + Type: gwapiv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gwapiv1.HTTPHeaderFilter{ + Set: []gwapiv1.HTTPHeader{ + {Name: "Authorization", Value: "Bearer " + apiKeyLiteral}, + }, + }, + }, + ) + } + } + + filters = append(filters, + gwapiv1.HTTPRouteFilter{ + Type: gwapiv1.HTTPRouteFilterURLRewrite, + URLRewrite: &gwapiv1.HTTPURLRewriteFilter{ + Path: &gwapiv1.HTTPPathModifier{ + Type: gwapiv1.FullPathHTTPPathModifier, + ReplaceFullPath: ptr.To(fullPathPtr), + }, + }, }, - }} + ) return gwapiv1.HTTPRouteRule{ Matches: []gwapiv1.HTTPRouteMatch{ { @@ -555,7 +603,7 @@ func mcpRouteHeaderValue(mcpRoute *aigv1a1.MCPRoute) string { } // ensureMCPBackendRefHTTPFilter ensures that an HTTPRouteFilter exists for the given backend reference in the MCPRoute. -func (c *MCPRouteController) ensureMCPBackendRefHTTPFilter(ctx context.Context, filterName string, apiKey *aigv1a1.MCPBackendAPIKey, mcpRoute *aigv1a1.MCPRoute) error { +func (c *MCPRouteController) ensureMCPBackendRefHTTPFilter(ctx context.Context, filterName string, mcpRoute *aigv1a1.MCPRoute) error { // Rewrite the hostname to the backend service name. // This allows Envoy to route to public MCP services with SNI matching the service name. // This could be a standalone filter and moved to the main mcp gateway route logic. @@ -575,25 +623,6 @@ func (c *MCPRouteController) ensureMCPBackendRefHTTPFilter(ctx context.Context, if err := ctrlutil.SetControllerReference(mcpRoute, filter, c.client.Scheme()); err != nil { return fmt.Errorf("failed to set controller reference for HTTPRouteFilter: %w", err) } - - // add credential injection if apiKey is specified. - if apiKey != nil { - secretName := fmt.Sprintf("%s-credential", filterName) - if secretErr := c.ensureCredentialSecret(ctx, mcpRoute.Namespace, secretName, apiKey, mcpRoute); secretErr != nil { - return fmt.Errorf("failed to ensure credential secret: %w", secretErr) - } - header := cmp.Or(ptr.Deref(apiKey.Header, ""), "Authorization") - filter.Spec.CredentialInjection = &egv1a1.HTTPCredentialInjectionFilter{ - Header: ptr.To(header), - Overwrite: ptr.To(true), - Credential: egv1a1.InjectedCredential{ - ValueRef: gwapiv1.SecretObjectReference{ - Name: gwapiv1.ObjectName(secretName), - }, - }, - } - } - // Create or Update the HTTPRouteFilter. var existingFilter egv1a1.HTTPRouteFilter err := c.client.Get(ctx, client.ObjectKey{Name: filterName, Namespace: mcpRoute.Namespace}, &existingFilter) @@ -617,64 +646,19 @@ func (c *MCPRouteController) ensureMCPBackendRefHTTPFilter(ctx context.Context, return nil } -func (c *MCPRouteController) ensureCredentialSecret(ctx context.Context, namespace, secretName string, apiKey *aigv1a1.MCPBackendAPIKey, mcpRoute *aigv1a1.MCPRoute) error { - var credentialValue string +func (c *MCPRouteController) readAPIKey(ctx context.Context, namespace string, apiKey *aigv1a1.MCPBackendAPIKey) (string, error) { key := ptr.Deref(apiKey.Inline, "") if key == "" { secretRef := apiKey.SecretRef secret, err := c.kube.CoreV1().Secrets(namespace).Get(ctx, string(secretRef.Name), metav1.GetOptions{}) if err != nil { - return fmt.Errorf("failed to get secret for API key: %w", err) + return "", fmt.Errorf("failed to get secret for API key: %w", err) } if k, ok := secret.Data["apiKey"]; ok { key = string(k) } else if key, ok = secret.StringData["apiKey"]; !ok { - return fmt.Errorf("secret %s/%s does not contain 'apiKey' key", namespace, secretRef.Name) + return "", fmt.Errorf("secret %s/%s does not contain 'apiKey' key", namespace, secretRef.Name) } } - - // Only prepend the "Bearer " prefix if the header is not set or is set to "Authorization". - header := cmp.Or(ptr.Deref(apiKey.Header, ""), "Authorization") - if header == "Authorization" { - credentialValue = fmt.Sprintf("Bearer %s", key) - } else { - credentialValue = key - } - - existingSecret, secretErr := c.kube.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{}) - if secretErr != nil && !apierrors.IsNotFound(secretErr) { - return fmt.Errorf("failed to get credential secret: %w", secretErr) - } - - secretData := map[string][]byte{ - egv1a1.InjectedCredentialKey: []byte(credentialValue), - } - - if apierrors.IsNotFound(secretErr) { - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: namespace, - }, - Data: secretData, - } - - if mcpRoute != nil { - if err := ctrlutil.SetControllerReference(mcpRoute, secret, c.client.Scheme()); err != nil { - return fmt.Errorf("failed to set controller reference for credential secret: %w", err) - } - } - - c.logger.Info("Creating credential secret", "namespace", namespace, "name", secretName) - if _, err := c.kube.CoreV1().Secrets(namespace).Create(ctx, secret, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("failed to create credential secret: %w", err) - } - } else if existingSecret.Data == nil || string(existingSecret.Data[egv1a1.InjectedCredentialKey]) != credentialValue { - existingSecret.Data = secretData - c.logger.Info("Updating credential secret", "namespace", namespace, "name", secretName) - if _, err := c.kube.CoreV1().Secrets(namespace).Update(ctx, existingSecret, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("failed to update credential secret: %w", secretErr) - } - } - return nil + return key, nil } diff --git a/internal/controller/mcp_route_test.go b/internal/controller/mcp_route_test.go index b98de8eabd..1e9bfc64d3 100644 --- a/internal/controller/mcp_route_test.go +++ b/internal/controller/mcp_route_test.go @@ -268,13 +268,36 @@ func TestMCPRouteController_mcpRuleWithAPIKeyBackendSecurity(t *testing.T) { ctrlr := NewMCPRouteController(c, kubeClient, logr.Discard(), eventCh.Ch) tests := []struct { - name string - header *string - wantHeader string - wantCredential []byte + name string + key *aigv1a1.MCPBackendAPIKey + expRequestHeader *internalapi.Header + refPath *string + expPath string }{ - {"default header", nil, "Authorization", []byte("Bearer secretvalue")}, - {"custom header", ptr.To("X-Api-Key"), "X-Api-Key", []byte("secretvalue")}, + { + name: "inline API key default header", + key: &aigv1a1.MCPBackendAPIKey{Inline: ptr.To("inline-key")}, + expRequestHeader: &internalapi.Header{"Authorization", "Bearer inline-key"}, + expPath: "/mcp", + }, + { + name: "inline API key custom header", + key: &aigv1a1.MCPBackendAPIKey{Inline: ptr.To("inline-key"), Header: ptr.To("X-API-KEY")}, + expRequestHeader: &internalapi.Header{"X-API-KEY", "inline-key"}, + expPath: "/mcp", + }, + { + name: "secret ref API key default header", + key: &aigv1a1.MCPBackendAPIKey{SecretRef: &gwapiv1.SecretObjectReference{Name: "some-secret"}}, + expRequestHeader: &internalapi.Header{"Authorization", "Bearer secretvalue"}, + refPath: ptr.To("/some/path"), + expPath: "/some/path", + }, + { + name: "query param API key", + key: &aigv1a1.MCPBackendAPIKey{Inline: ptr.To("inline-key"), QueryParam: ptr.To("api_key")}, + expPath: "/mcp?api_key=inline-key", + }, } for _, tt := range tests { @@ -286,14 +309,8 @@ func TestMCPRouteController_mcpRuleWithAPIKeyBackendSecurity(t *testing.T) { Name: "svc-a", Namespace: ptr.To(gwapiv1.Namespace("default")), }, - SecurityPolicy: &aigv1a1.MCPBackendSecurityPolicy{ - APIKey: &aigv1a1.MCPBackendAPIKey{ - Header: tt.header, - SecretRef: &gwapiv1.SecretObjectReference{ - Name: "some-secret", - }, - }, - }, + SecurityPolicy: &aigv1a1.MCPBackendSecurityPolicy{APIKey: tt.key}, + Path: tt.refPath, }, ) require.NoError(t, err) @@ -305,28 +322,44 @@ func TestMCPRouteController_mcpRuleWithAPIKeyBackendSecurity(t *testing.T) { require.Equal(t, "svc-a", headers[0].Value) require.Equal(t, internalapi.MCPRouteHeader, string(headers[1].Name)) require.Contains(t, headers[1].Value, "route-a") - require.Len(t, httpRule.Filters, 1) - require.Equal(t, gwapiv1.HTTPRouteFilterExtensionRef, httpRule.Filters[0].Type) - require.NotNil(t, httpRule.Filters[0].ExtensionRef) - require.Equal(t, gwapiv1.Group("gateway.envoyproxy.io"), httpRule.Filters[0].ExtensionRef.Group) - require.Equal(t, gwapiv1.Kind("HTTPRouteFilter"), httpRule.Filters[0].ExtensionRef.Kind) - require.Contains(t, string(httpRule.Filters[0].ExtensionRef.Name), internalapi.MCPPerBackendHTTPRouteFilterPrefix) + // The first filter is the EG extension ref filter for URL host rewrite. + egFilter := httpRule.Filters[0] + require.Equal(t, gwapiv1.HTTPRouteFilterExtensionRef, egFilter.Type) + require.NotNil(t, egFilter.ExtensionRef) + require.Equal(t, gwapiv1.Group("gateway.envoyproxy.io"), egFilter.ExtensionRef.Group) + require.Equal(t, gwapiv1.Kind("HTTPRouteFilter"), egFilter.ExtensionRef.Kind) + require.Contains(t, string(egFilter.ExtensionRef.Name), internalapi.MCPPerBackendHTTPRouteFilterPrefix) var httpFilter egv1a1.HTTPRouteFilter - err = c.Get(t.Context(), types.NamespacedName{Namespace: "default", Name: string(httpRule.Filters[0].ExtensionRef.Name)}, &httpFilter) - require.NoError(t, err) - require.NotNil(t, httpFilter.Spec.CredentialInjection) - require.Equal(t, tt.wantHeader, ptr.Deref(httpFilter.Spec.CredentialInjection.Header, "")) - require.Equal(t, httpFilter.Name+"-credential", string(httpFilter.Spec.CredentialInjection.Credential.ValueRef.Name)) - - secret, err := kubeClient.CoreV1().Secrets("default").Get(t.Context(), - string(httpFilter.Spec.CredentialInjection.Credential.ValueRef.Name), metav1.GetOptions{}) + err = c.Get(t.Context(), types.NamespacedName{Namespace: "default", Name: string(egFilter.ExtensionRef.Name)}, &httpFilter) require.NoError(t, err) - require.Equal(t, tt.wantCredential, secret.Data[egv1a1.InjectedCredentialKey]) - require.NotNil(t, httpFilter.Spec.URLRewrite) require.NotNil(t, httpFilter.Spec.URLRewrite.Hostname) require.Equal(t, egv1a1.BackendHTTPHostnameModifier, httpFilter.Spec.URLRewrite.Hostname.Type) + + if tt.expRequestHeader != nil { + // The second filter is the request header modifier for API key injection. + reqHeaderFilter := httpRule.Filters[1] + require.Equal(t, gwapiv1.HTTPRouteFilterRequestHeaderModifier, reqHeaderFilter.Type) + require.NotNil(t, reqHeaderFilter.RequestHeaderModifier) + found := false + for _, set := range reqHeaderFilter.RequestHeaderModifier.Set { + if set.Name == gwapiv1.HTTPHeaderName(tt.expRequestHeader.Key()) && + set.Value == tt.expRequestHeader.Value() { + found = true + break + } + } + require.Truef(t, found, "Expected request header modifier not found in %v", reqHeaderFilter.RequestHeaderModifier.Set) + } + + // Verify the last filter is the path rewrite filter. + pathRewriteFilter := httpRule.Filters[len(httpRule.Filters)-1] + require.Equal(t, gwapiv1.HTTPRouteFilterURLRewrite, pathRewriteFilter.Type) + require.NotNil(t, pathRewriteFilter.URLRewrite) + require.NotNil(t, pathRewriteFilter.URLRewrite.Path) + require.Equal(t, gwapiv1.FullPathHTTPPathModifier, pathRewriteFilter.URLRewrite.Path.Type) + require.Equal(t, tt.expPath, *pathRewriteFilter.URLRewrite.Path.ReplaceFullPath) }) } } @@ -348,31 +381,13 @@ func TestMCPRouteController_ensureMCPBackendRefHTTPFilter(t *testing.T) { require.NoError(t, err) filterName := mcpBackendRefFilterName(mcpRoute, "some-name") - err = ctrlr.ensureMCPBackendRefHTTPFilter(t.Context(), filterName, &aigv1a1.MCPBackendAPIKey{ - SecretRef: &gwapiv1.SecretObjectReference{ - Name: "test-secret", - }, - }, mcpRoute) + err = ctrlr.ensureMCPBackendRefHTTPFilter(t.Context(), filterName, mcpRoute) require.NoError(t, err) // Verify HTTPRouteFilter was created. var httpFilter egv1a1.HTTPRouteFilter err = c.Get(t.Context(), types.NamespacedName{Namespace: "default", Name: filterName}, &httpFilter) require.NoError(t, err) - - // Verify filter has credential injection configured. - require.NotNil(t, httpFilter.Spec.CredentialInjection) - require.Equal(t, "Authorization", ptr.Deref(httpFilter.Spec.CredentialInjection.Header, "")) - require.Equal(t, filterName+"-credential", string(httpFilter.Spec.CredentialInjection.Credential.ValueRef.Name)) - - // Update the route without API key and ensure the filter is deleted. - err = ctrlr.ensureMCPBackendRefHTTPFilter(t.Context(), filterName, nil, mcpRoute) - require.NoError(t, err) - - // Check that the HTTPRouteFilter doesn't have CredentialInjection anymore. - err = c.Get(t.Context(), types.NamespacedName{Namespace: "default", Name: filterName}, &httpFilter) - require.NoError(t, err) - require.Nil(t, httpFilter.Spec.CredentialInjection) } func TestMCPRouteController_syncGateways_NamespaceCrossReference(t *testing.T) { diff --git a/internal/filterapi/mcpconfig.go b/internal/filterapi/mcpconfig.go index d70a231860..5e119b81a1 100644 --- a/internal/filterapi/mcpconfig.go +++ b/internal/filterapi/mcpconfig.go @@ -38,9 +38,6 @@ type MCPBackend struct { // This name is set in [internalapi.MCPBackendHeader] header to route the request to the specific backend. Name MCPBackendName `json:"name"` - // Path is the HTTP endpoint path of the backend MCP server. - Path string `json:"path"` - // ToolSelector filters the tools exposed by this backend. If not set, all tools are exposed. ToolSelector *MCPToolSelector `json:"toolSelector,omitempty"` } diff --git a/internal/mcpproxy/config_test.go b/internal/mcpproxy/config_test.go index 7fd7576db1..e0ad4d4a0a 100644 --- a/internal/mcpproxy/config_test.go +++ b/internal/mcpproxy/config_test.go @@ -77,9 +77,9 @@ func TestLoadConfig_BasicConfiguration(t *testing.T) { { Name: "route1", Backends: []filterapi.MCPBackend{ - {Name: "backend1", Path: "/mcp1"}, + {Name: "backend1"}, { - Name: "backend2", Path: "/mcp2", + Name: "backend2", ToolSelector: &filterapi.MCPToolSelector{ Include: []string{"tool1", "tool2"}, IncludeRegex: []string{"^test.*"}, @@ -90,8 +90,8 @@ func TestLoadConfig_BasicConfiguration(t *testing.T) { { Name: "route2", Backends: []filterapi.MCPBackend{ - {Name: "backend3", Path: "/mcp3"}, - {Name: "backend4", Path: "/mcp4"}, + {Name: "backend3"}, + {Name: "backend4"}, }, }, }, @@ -130,7 +130,7 @@ func TestLoadConfig_ToolsChangedNotification(t *testing.T) { routes: map[filterapi.MCPRouteName]*mcpProxyConfigRoute{ "route1": { backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend1": {Name: "backend1", Path: "/mcp1"}, + "backend1": {Name: "backend1"}, }, toolSelectors: map[filterapi.MCPBackendName]*toolSelector{}, }, @@ -147,8 +147,8 @@ func TestLoadConfig_ToolsChangedNotification(t *testing.T) { { Name: "route1", Backends: []filterapi.MCPBackend{ - {Name: "backend1", Path: "/mcp1"}, - {Name: "backend2", Path: "/mcp2"}, // Added backend + {Name: "backend1"}, + {Name: "backend2"}, // Added backend }, }, }, @@ -178,7 +178,7 @@ func TestLoadConfig_NoToolsChangedNotification(t *testing.T) { routes: map[filterapi.MCPRouteName]*mcpProxyConfigRoute{ "route1": { backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend1": {Name: "backend1", Path: "/mcp1"}, + "backend1": {Name: "backend1"}, }, toolSelectors: map[filterapi.MCPBackendName]*toolSelector{}, }, @@ -195,7 +195,7 @@ func TestLoadConfig_NoToolsChangedNotification(t *testing.T) { { Name: "route1", Backends: []filterapi.MCPBackend{ - {Name: "backend1", Path: "/mcp1"}, // Same backend + {Name: "backend1"}, // Same backend }, }, }, @@ -229,7 +229,6 @@ func TestLoadConfig_InvalidRegex(t *testing.T) { Backends: []filterapi.MCPBackend{ { Name: "backend1", - Path: "/mcp1", ToolSelector: &filterapi.MCPToolSelector{ IncludeRegex: []string{"[invalid"}, // Invalid regex }, @@ -256,7 +255,7 @@ func TestLoadConfig_ToolSelectorChange(t *testing.T) { routes: map[filterapi.MCPRouteName]*mcpProxyConfigRoute{ "route1": { backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend1": {Name: "backend1", Path: "/mcp1"}, + "backend1": {Name: "backend1"}, }, toolSelectors: map[filterapi.MCPBackendName]*toolSelector{ "backend1": { @@ -279,7 +278,6 @@ func TestLoadConfig_ToolSelectorChange(t *testing.T) { Backends: []filterapi.MCPBackend{ { Name: "backend1", - Path: "/mcp1", ToolSelector: &filterapi.MCPToolSelector{ Include: []string{"tool1", "tool2"}, // Different tools }, @@ -315,11 +313,11 @@ func TestLoadConfig_ToolOrderDoesNotMatter(t *testing.T) { // Initialize proxy with initial configuration directly proxy := &ProxyConfig{ mcpProxyConfig: &mcpProxyConfig{ - backendListenerAddr: "http://localhost:8080", + backendListenerAddr: "http://localhost:8080/", routes: map[filterapi.MCPRouteName]*mcpProxyConfigRoute{ "route1": { backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend1": {Name: "backend1", Path: "/mcp1"}, + "backend1": {Name: "backend1"}, }, toolSelectors: map[filterapi.MCPBackendName]*toolSelector{ "backend1": { @@ -351,7 +349,6 @@ func TestLoadConfig_ToolOrderDoesNotMatter(t *testing.T) { Backends: []filterapi.MCPBackend{ { Name: "backend1", - Path: "/mcp1", ToolSelector: &filterapi.MCPToolSelector{ Include: []string{"tool-c", "tool-a", "tool-b"}, // Different order IncludeRegex: []string{"^exact$", ".*suffix$", "^prefix.*"}, // Different order diff --git a/internal/mcpproxy/handlers.go b/internal/mcpproxy/handlers.go index 5a953651bd..af43188667 100644 --- a/internal/mcpproxy/handlers.go +++ b/internal/mcpproxy/handlers.go @@ -1021,10 +1021,6 @@ func (m *mcpRequestContext) recordResponse(ctx context.Context, rawMsg jsonrpc.M } } -func (m *mcpRequestContext) mcpEndpointForBackend(backend filterapi.MCPBackend) string { - return m.backendListenerAddr + backend.Path -} - func (m *mcpRequestContext) handleResourceReadRequest(ctx context.Context, s *session, w http.ResponseWriter, req *jsonrpc.Request, p *mcp.ReadResourceParams) error { backendName, resourceName, err := upstreamResourceURI(p.URI) if err != nil { diff --git a/internal/mcpproxy/handlers_test.go b/internal/mcpproxy/handlers_test.go index 356dab28af..ef9710e6c7 100644 --- a/internal/mcpproxy/handlers_test.go +++ b/internal/mcpproxy/handlers_test.go @@ -59,13 +59,13 @@ func newTestMCPProxyWithTracer(t tracingapi.MCPTracer) *mcpRequestContext { "backend1": {include: map[string]struct{}{"test-tool": {}}}, }, backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend1": {Name: "backend1", Path: "/mcp"}, - "backend2": {Name: "backend2", Path: "/"}, + "backend1": {Name: "backend1"}, + "backend2": {Name: "backend2"}, }, }, "test-route-another": { backends: map[filterapi.MCPBackendName]filterapi.MCPBackend{ - "backend3": {Name: "backend3", Path: "/mcp"}, + "backend3": {Name: "backend3"}, }, }, }, diff --git a/internal/mcpproxy/mcpproxy.go b/internal/mcpproxy/mcpproxy.go index 9fcf0bf8ae..68cb4c8a95 100644 --- a/internal/mcpproxy/mcpproxy.go +++ b/internal/mcpproxy/mcpproxy.go @@ -357,7 +357,7 @@ func (m *mcpRequestContext) invokeJSONRPCRequest(ctx context.Context, routeName if err != nil { return nil, fmt.Errorf("failed to encode MCP message: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, m.mcpEndpointForBackend(backend), bytes.NewReader(encoded)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, m.backendListenerAddr, bytes.NewReader(encoded)) if err != nil { return nil, fmt.Errorf("failed to create MCP notifications/initialized request: %w", err) } diff --git a/internal/mcpproxy/mcpproxy_test.go b/internal/mcpproxy/mcpproxy_test.go index 7d268715b0..8b3dd5b5e4 100644 --- a/internal/mcpproxy/mcpproxy_test.go +++ b/internal/mcpproxy/mcpproxy_test.go @@ -354,7 +354,7 @@ func TestInitializeSession_Success(t *testing.T) { proxy := newTestMCPProxy() proxy.backendListenerAddr = backendServer.URL - res, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend", Path: "/a/b/c"}, &mcp.InitializeParams{}) + res, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend"}, &mcp.InitializeParams{}) require.NoError(t, err) require.Equal(t, gatewayToMCPServerSessionID("test-session-123"), res.sessionID) @@ -372,7 +372,7 @@ func TestInitializeSession_InitializeFailure(t *testing.T) { proxy := newTestMCPProxy() proxy.backendListenerAddr = backendServer.URL - sessionID, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend", Path: "/a/b/c"}, &mcp.InitializeParams{}) + sessionID, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend"}, &mcp.InitializeParams{}) require.Error(t, err) require.Empty(t, sessionID) @@ -400,7 +400,7 @@ func TestInitializeSession_NotificationsInitializedFailure(t *testing.T) { proxy := newTestMCPProxy() proxy.backendListenerAddr = backendServer.URL - sessionID, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend", Path: "/aaaaaaaaaaaaaa"}, &mcp.InitializeParams{}) + sessionID, err := proxy.initializeSession(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend"}, &mcp.InitializeParams{}) require.Error(t, err) require.Empty(t, sessionID) @@ -409,7 +409,7 @@ func TestInitializeSession_NotificationsInitializedFailure(t *testing.T) { func TestInvokeJSONRPCRequest_Success(t *testing.T) { backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/aaaaaaaaaaaaaa", r.URL.Path) + require.Equal(t, "/", r.URL.Path) require.Equal(t, "test-backend", r.Header.Get("x-ai-eg-mcp-backend")) require.Equal(t, "test-session", r.Header.Get(sessionIDHeader)) require.Equal(t, "application/json", r.Header.Get("Content-Type")) @@ -420,7 +420,7 @@ func TestInvokeJSONRPCRequest_Success(t *testing.T) { m := newTestMCPProxy() m.backendListenerAddr = backendServer.URL - resp, err := m.invokeJSONRPCRequest(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend", Path: "/aaaaaaaaaaaaaa"}, &compositeSessionEntry{ + resp, err := m.invokeJSONRPCRequest(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend"}, &compositeSessionEntry{ sessionID: "test-session", }, &jsonrpc.Request{}) @@ -432,8 +432,7 @@ func TestInvokeJSONRPCRequest_Success(t *testing.T) { func TestInvokeJSONRPCRequest_NoSessionID(t *testing.T) { backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Check the path equals /mcp. - require.Equal(t, "/mcp", r.URL.Path) + require.Equal(t, "/", r.URL.Path) require.Equal(t, "test-backend", r.Header.Get("x-ai-eg-mcp-backend")) require.Empty(t, r.Header.Get(sessionIDHeader)) w.WriteHeader(http.StatusOK) @@ -443,7 +442,7 @@ func TestInvokeJSONRPCRequest_NoSessionID(t *testing.T) { m := newTestMCPProxy() m.backendListenerAddr = backendServer.URL - resp, err := m.invokeJSONRPCRequest(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend", Path: "/mcp"}, &compositeSessionEntry{ + resp, err := m.invokeJSONRPCRequest(t.Context(), "route1", filterapi.MCPBackend{Name: "test-backend"}, &compositeSessionEntry{ sessionID: "", }, &jsonrpc.Request{}) diff --git a/internal/mcpproxy/session.go b/internal/mcpproxy/session.go index 35d30d5bb4..65eca30736 100644 --- a/internal/mcpproxy/session.go +++ b/internal/mcpproxy/session.go @@ -55,17 +55,7 @@ func (s *session) Close() error { // Stateless backend, nothing to do. continue } - // Make DELETE request to the MCP server to close the session. - backend, err := s.reqCtx.getBackendForRoute(s.route, backendName) - if err != nil { - s.reqCtx.l.Error("failed to get backend for route", - slog.String("backend", backendName), - slog.String("session_id", string(sessionID)), - slog.String("error", err.Error()), - ) - continue - } - req, err := http.NewRequest(http.MethodDelete, s.reqCtx.mcpEndpointForBackend(backend), nil) + req, err := http.NewRequest(http.MethodDelete, s.reqCtx.backendListenerAddr, nil) if err != nil { s.reqCtx.l.Error("failed to create DELETE request to MCP server to close session", slog.String("backend", backendName), @@ -334,7 +324,7 @@ func (s *session) sendRequestPerBackend(ctx context.Context, eventChan chan<- *s body = bytes.NewReader(encodedReq) } - req, err := http.NewRequestWithContext(ctx, httpMethod, s.reqCtx.mcpEndpointForBackend(backend), body) + req, err := http.NewRequestWithContext(ctx, httpMethod, s.reqCtx.backendListenerAddr, body) if err != nil { return fmt.Errorf("failed to create GET request: %w", err) } diff --git a/internal/mcpproxy/session_test.go b/internal/mcpproxy/session_test.go index 7e4799a1cd..7bca61757f 100644 --- a/internal/mcpproxy/session_test.go +++ b/internal/mcpproxy/session_test.go @@ -131,7 +131,7 @@ func TestSendRequestPerBackend_SetsOriginalPathHeaders(t *testing.T) { ch := make(chan *sseEvent, 1) ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) defer cancel() - err := s.sendRequestPerBackend(ctx, ch, "test-route", filterapi.MCPBackend{Name: "backend1", Path: "/mcp"}, &compositeSessionEntry{ + err := s.sendRequestPerBackend(ctx, ch, "test-route", filterapi.MCPBackend{Name: "backend1"}, &compositeSessionEntry{ sessionID: "sess1", }, http.MethodGet, nil) require.NoError(t, err) diff --git a/manifests/charts/ai-gateway-crds-helm/templates/aigateway.envoyproxy.io_mcproutes.yaml b/manifests/charts/ai-gateway-crds-helm/templates/aigateway.envoyproxy.io_mcproutes.yaml index 06caed8df9..417901544c 100644 --- a/manifests/charts/ai-gateway-crds-helm/templates/aigateway.envoyproxy.io_mcproutes.yaml +++ b/manifests/charts/ai-gateway-crds-helm/templates/aigateway.envoyproxy.io_mcproutes.yaml @@ -147,12 +147,23 @@ spec: defaults to "Authorization". When the header is "Authorization", the injected header value will be prefixed with "Bearer ". + + Either one of Header or QueryParam can be specified to inject the API key. minLength: 1 type: string inline: description: Inline contains the API key as an inline string. type: string + queryParam: + description: |- + QueryParam is the HTTP query parameter to inject the API key into. + For example, if QueryParam is set to "api_key", and the API key is "mysecretkey", the request URL will be modified to include + "?api_key=mysecretkey". + + Either one of Header or QueryParam can be specified to inject the API key. + minLength: 1 + type: string secretRef: description: |- secretRef is the Kubernetes secret which contains the API keys. @@ -202,6 +213,8 @@ spec: - message: exactly one of secretRef or inline must be set rule: (has(self.secretRef) && !has(self.inline)) || (!has(self.secretRef) && has(self.inline)) + - message: only one of header or queryParam can be set + rule: '!(has(self.header) && has(self.queryParam))' type: object toolSelector: description: |- diff --git a/site/docs/api/api.mdx b/site/docs/api/api.mdx index 240eaa93b5..c75c840bc4 100644 --- a/site/docs/api/api.mdx +++ b/site/docs/api/api.mdx @@ -1858,6 +1858,7 @@ MCPAuthorizationTarget defines the target of an authorization rule. - [MCPBackendSecurityPolicy](#mcpbackendsecuritypolicy) MCPBackendAPIKey defines the configuration for the API Key Authentication to a backend. +When both `header` and `queryParam` are unspecified, the API key will be injected into the "Authorization" header by default. ##### Fields @@ -1877,7 +1878,12 @@ MCPBackendAPIKey defines the configuration for the API Key Authentication to a b name="header" type="string" required="false" - description="Header is the HTTP header to inject the API key into. If not specified,
defaults to `Authorization`.
When the header is `Authorization`, the injected header value will be
prefixed with `Bearer `." + description="Header is the HTTP header to inject the API key into. If not specified,
defaults to `Authorization`.
When the header is `Authorization`, the injected header value will be
prefixed with `Bearer `.
Either one of Header or QueryParam can be specified to inject the API key." +/> diff --git a/tests/crdcel/main_test.go b/tests/crdcel/main_test.go index 1bf9b64afd..3b14bfd448 100644 --- a/tests/crdcel/main_test.go +++ b/tests/crdcel/main_test.go @@ -244,6 +244,10 @@ func TestMCPRoutes(t *testing.T) { name: "backend_api_key_missing.yaml", expErr: "spec.backendRefs[0].securityPolicy.apiKey: Invalid value: \"object\": exactly one of secretRef or inline must be set", }, + { + name: "backend_api_key_both_header_and_query.yaml", + expErr: "only one of header or queryParam can be set", + }, { name: "jwks_missing.yaml", expErr: "spec.securityPolicy.oauth.jwks: Invalid value: \"object\": either remoteJWKS or localJWKS must be specified.", diff --git a/tests/crdcel/testdata/mcpgatewayroutes/backend_api_key_both_header_and_query.yaml b/tests/crdcel/testdata/mcpgatewayroutes/backend_api_key_both_header_and_query.yaml new file mode 100644 index 0000000000..49a1d43b87 --- /dev/null +++ b/tests/crdcel/testdata/mcpgatewayroutes/backend_api_key_both_header_and_query.yaml @@ -0,0 +1,25 @@ +# Copyright Envoy AI Gateway Authors +# SPDX-License-Identifier: Apache-2.0 +# The full text of the Apache license is available in the LICENSE file at +# the root of the repo. + +# This should fail validation: API key has both header and query param specified. +apiVersion: aigateway.envoyproxy.io/v1alpha1 +kind: MCPRoute +metadata: + name: backend-apikey-both-header-and-query + namespace: default +spec: + parentRefs: + - name: some-gateway + kind: Gateway + group: gateway.networking.k8s.io + backendRefs: + - name: mcp-service + kind: Service + port: 80 + securityPolicy: + apiKey: + inline: "my-api-key" + header: "X-API-KEY" + queryParam: "api_key" diff --git a/tests/data-plane-mcp/env.go b/tests/data-plane-mcp/env.go index 04e95bea60..908ad2ea68 100644 --- a/tests/data-plane-mcp/env.go +++ b/tests/data-plane-mcp/env.go @@ -88,25 +88,25 @@ func requireNewMCPEnv(t *testing.T, forceJSONResponse bool, writeTimeout time.Du { Name: "test-route", Backends: []filterapi.MCPBackend{ - {Name: "dumb-mcp-backend", Path: "/mcp"}, - {Name: "default-mcp-backend", Path: "/mcp"}, + {Name: "dumb-mcp-backend"}, + {Name: "default-mcp-backend"}, }, }, { Name: "yet-another-route", Backends: []filterapi.MCPBackend{ { - Name: "default-mcp-backend", Path: "/mcp", + Name: "default-mcp-backend", // This shouldn't affect any other routes. ToolSelector: &filterapi.MCPToolSelector{Include: []string{"non-existent"}}, }, - {Name: "dumb-mcp-backend", Path: "/mcp"}, + {Name: "dumb-mcp-backend"}, }, }, { Name: "awesome-route", Backends: []filterapi.MCPBackend{ - {Name: "dumb-mcp-backend", Path: "/mcp"}, + {Name: "dumb-mcp-backend"}, }, }, }, diff --git a/tests/data-plane-mcp/envoy.yaml b/tests/data-plane-mcp/envoy.yaml index be305d900b..9497699783 100644 --- a/tests/data-plane-mcp/envoy.yaml +++ b/tests/data-plane-mcp/envoy.yaml @@ -128,6 +128,7 @@ static_resources: cluster: dumb-mcp-backend idleTimeout: 3600s timeout: 120s + pathRewrite: /mcp - match: prefix: "/" headers: @@ -138,6 +139,7 @@ static_resources: cluster: default-mcp-backend idleTimeout: 3600s timeout: 120s + pathRewrite: /mcp - match: prefix: "/" headers: @@ -148,6 +150,7 @@ static_resources: autoHostRewrite: true cluster: context7 idleTimeout: 3600s + pathRewrite: /mcp timeout: 120s - match: prefix: "/" @@ -159,6 +162,7 @@ static_resources: autoHostRewrite: true cluster: github idleTimeout: 3600s + pathRewrite: /mcp/readonly timeout: 120s request_headers_to_add: - header: @@ -174,6 +178,7 @@ static_resources: autoHostRewrite: true cluster: kiwi idleTimeout: 3600s + pathRewrite: / timeout: 120s http_filters: - name: envoy.filters.http.header_to_metadata diff --git a/tests/data-plane-mcp/publicmcp_test.go b/tests/data-plane-mcp/publicmcp_test.go index 8ea00379db..f57a6bb769 100644 --- a/tests/data-plane-mcp/publicmcp_test.go +++ b/tests/data-plane-mcp/publicmcp_test.go @@ -29,8 +29,8 @@ func TestPublicMCPServers(t *testing.T) { { Name: "test-route", Backends: []filterapi.MCPBackend{ - {Name: "context7", Path: "/mcp"}, - {Name: "kiwi", Path: "/"}, + {Name: "context7"}, + {Name: "kiwi"}, }, }, }, @@ -42,7 +42,6 @@ func TestPublicMCPServers(t *testing.T) { mcpConfig.Routes[0].Backends = append(mcpConfig.Routes[0].Backends, filterapi.MCPBackend{ Name: "github", - Path: "/mcp/readonly", ToolSelector: &filterapi.MCPToolSelector{ IncludeRegex: []string{".*pull_requests?.*", ".*issues?.*"}, }, diff --git a/tests/e2e/mcp_route_test.go b/tests/e2e/mcp_route_test.go index adee300236..fac428f369 100644 --- a/tests/e2e/mcp_route_test.go +++ b/tests/e2e/mcp_route_test.go @@ -52,7 +52,7 @@ func TestMCP(t *testing.T) { }) t.Run("tenant route with another path suffix", func(t *testing.T) { testMCPRouteTools(t.Context(), t, client, fwd.Address(), "/mcp/another", []string{ - "mcp-backend__sum", + "mcp-backend-query-api-key__sum", }, nil, false, true) }) t.Run("tenant route with different path", func(t *testing.T) { diff --git a/tests/e2e/testdata/mcp_route.yaml b/tests/e2e/testdata/mcp_route.yaml index 6d18804377..1a04c25243 100644 --- a/tests/e2e/testdata/mcp_route.yaml +++ b/tests/e2e/testdata/mcp_route.yaml @@ -86,6 +86,47 @@ spec: port: 1063 targetPort: 1063 type: ClusterIP +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: mcp-backend-query-api-key + namespace: default +spec: + replicas: 1 + selector: + matchLabels: + app: mcp-backend-query-api-key + template: + metadata: + labels: + app: mcp-backend-query-api-key + spec: + containers: + - name: mcp-backend + image: docker.io/envoyproxy/ai-gateway-testmcpserver:latest + imagePullPolicy: IfNotPresent + ports: + - containerPort: 1063 + env: + - name: TEST_API_KEY + value: "test-api-key" + - name: TEST_API_KEY_QUERY_PARAM + value: "api_key" +--- +apiVersion: v1 +kind: Service +metadata: + name: mcp-backend-query-api-key + namespace: default +spec: + selector: + app: mcp-backend-query-api-key + ports: + - protocol: TCP + port: 1063 + targetPort: 1063 + type: ClusterIP --- apiVersion: v1 @@ -186,11 +227,12 @@ spec: group: gateway.networking.k8s.io namespace: default backendRefs: - - name: mcp-backend + - name: mcp-backend-query-api-key port: 1063 securityPolicy: apiKey: inline: "test-api-key" + queryParam: "api_key" toolSelector: include: - sum diff --git a/tests/internal/testmcp/server.go b/tests/internal/testmcp/server.go index c767639861..6a683937c7 100644 --- a/tests/internal/testmcp/server.go +++ b/tests/internal/testmcp/server.go @@ -83,7 +83,11 @@ func NewServer(opts *Options) (*http.Server, *mcp.Server) { }, ) - if apiKey := os.Getenv("TEST_API_KEY"); apiKey != "" { + // Setup API key auth when environment variable TEST_API_KEY is set. + apiKey := os.Getenv("TEST_API_KEY") + apiKeyQueryParam := os.Getenv("TEST_API_KEY_QUERY_PARAM") + // Query param auth takes precedence over header. + if apiKey != "" && apiKeyQueryParam == "" { header := strings.ToLower(cmp.Or(os.Getenv("TEST_API_KEY_HEADER"), "Authorization")) expectedValue := apiKey if header == "authorization" { @@ -119,7 +123,18 @@ func NewServer(opts *Options) (*http.Server, *mcp.Server) { notificationsCounts := newToolNotificationCounts(handlerCounts) mcp.AddTool(s, notificationsCounts.Tool, notificationsCounts.Handler) - handler := mcp.NewStreamableHTTPHandler(func(*http.Request) *mcp.Server { + handler := mcp.NewStreamableHTTPHandler(func(r *http.Request) *mcp.Server { + // Check for API key in query param if configured. + if apiKey != "" && apiKeyQueryParam != "" { + log.Printf("checking for API key in query param %q\n", apiKeyQueryParam) + queryParam := r.URL.Query().Get(apiKeyQueryParam) + if queryParam != apiKey { + // Returning nil will cause 400 response in the current implementation of NewStreamableHTTPHandler. + log.Printf("invalid API key in query param %q: %q\n", apiKeyQueryParam, queryParam) + return nil + } + log.Printf("valid API key in query param %q\n", apiKeyQueryParam) + } return s }, &mcp.StreamableHTTPOptions{JSONResponse: opts.ForceJSONResponse})