From 02277efe27e1aa9ceab4ebdb80782d43893e2fc4 Mon Sep 17 00:00:00 2001 From: Tim Flannagan Date: Fri, 6 Nov 2026 22:52:08 +0000 Subject: [PATCH] refactor(api): move MCP header resolution out of v1alpha3 (#3563) # Description `go/api/v1alpha3` is the public API that external clients and wrapper types import, but it pulled in controller-runtime and `database/sql` for a few helpers that do I/O. This moves those helpers out so the API package only describes data. `RemoteMCPServerSpec` loses its `sql.Scanner`/`driver.Valuer` methods, which nothing used. `RemoteMCPServer.ResolveHeaders`, `ValueRef.Resolve`, and `ValueSource.Resolve` move into the tool service's MCP client, their only caller, and `go/api/utils` is removed. The translator keeps its existing `HeadersFrom` handling, which compiles Secret-backed headers into egress credentials rather than reading them. Header resolution behaves the same, except errors are now wrapped and name the failing header. Go consumers that called the removed methods need to read the referenced Secret or ConfigMap themselves. Signed-off-by: timflannagan (cherry picked from commit c6adf72f9cc110f039c7a93e6c1dd5b4eb565698) Signed-off-by: Eitan Yarmush --- go/api/utils/utils.go | 39 ------------- go/api/v1alpha3/common_types.go | 39 ------------- go/api/v1alpha3/remotemcpserver_types.go | 39 ------------- go/core/internal/service/tool/mcp.go | 49 +++++++++++++++- go/core/internal/service/tool/mcp_test.go | 70 +++++++++++++++++++++++ 5 files changed, 118 insertions(+), 118 deletions(-) delete mode 100644 go/api/utils/utils.go diff --git a/go/api/utils/utils.go b/go/api/utils/utils.go deleted file mode 100644 index 1225656e89..0000000000 --- a/go/api/utils/utils.go +++ /dev/null @@ -1,39 +0,0 @@ -package utils - -import ( - "context" - "fmt" - - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// GetSecretValue fetches a value from a Secret -func GetSecretValue(ctx context.Context, c client.Client, ref client.ObjectKey, key string) (string, error) { - secret := &corev1.Secret{} - err := c.Get(ctx, ref, secret) - if err != nil { - return "", fmt.Errorf("failed to find Secret %s: %v", ref.String(), err) - } - - value, exists := secret.Data[key] - if !exists { - return "", fmt.Errorf("key %s not found in Secret %s", key, ref.String()) - } - return string(value), nil -} - -// GetConfigMapValue fetches a value from a ConfigMap -func GetConfigMapValue(ctx context.Context, c client.Client, ref client.ObjectKey, key string) (string, error) { - configMap := &corev1.ConfigMap{} - err := c.Get(ctx, ref, configMap) - if err != nil { - return "", fmt.Errorf("failed to find ConfigMap for %s: %v", ref.String(), err) - } - - value, exists := configMap.Data[key] - if !exists { - return "", fmt.Errorf("key %s not found in ConfigMap %s", key, ref) - } - return value, nil -} diff --git a/go/api/v1alpha3/common_types.go b/go/api/v1alpha3/common_types.go index 98379f776c..fd1d7d6051 100644 --- a/go/api/v1alpha3/common_types.go +++ b/go/api/v1alpha3/common_types.go @@ -17,16 +17,12 @@ limitations under the License. package v1alpha3 import ( - "context" "fmt" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/kagent-dev/kagent/go/api/utils" ) // ObjectReference identifies a resource; an omitted namespace means the owner's namespace. @@ -147,21 +143,6 @@ type ValueSource struct { Key string `json:"key"` } -func (s *ValueSource) Resolve(ctx context.Context, client client.Client, namespace string) (string, error) { - if s == nil { - return "", fmt.Errorf("ValueSource cannot be nil") - } - - switch s.Type { - case ConfigMapValueSource: - return utils.GetConfigMapValue(ctx, client, types.NamespacedName{Namespace: namespace, Name: s.Name}, s.Key) - case SecretValueSource: - return utils.GetSecretValue(ctx, client, types.NamespacedName{Namespace: namespace, Name: s.Name}, s.Key) - default: - return "", fmt.Errorf("unknown value source type: %s", s.Type) - } -} - // ValueRef represents a configuration value // +kubebuilder:validation:ExactlyOneOf=value;valueFrom type ValueRef struct { @@ -173,23 +154,3 @@ type ValueRef struct { // +optional ValueFrom *ValueSource `json:"valueFrom,omitempty"` } - -func (r *ValueRef) Resolve(ctx context.Context, client client.Client, namespace string) (string, string, error) { - if r == nil { - return "", "", fmt.Errorf("ValueRef cannot be nil") - } - - switch { - case r.Value != "": - return r.Name, r.Value, nil - case r.ValueFrom != nil: - value, err := r.ValueFrom.Resolve(ctx, client, namespace) - if err != nil { - return "", "", fmt.Errorf("failed to resolve value for ref %s: %v", r.Name, err) - } - - return r.Name, value, nil - default: - return r.Name, "", nil - } -} diff --git a/go/api/v1alpha3/remotemcpserver_types.go b/go/api/v1alpha3/remotemcpserver_types.go index d3453ed56e..b28fa3be0c 100644 --- a/go/api/v1alpha3/remotemcpserver_types.go +++ b/go/api/v1alpha3/remotemcpserver_types.go @@ -17,15 +17,8 @@ limitations under the License. package v1alpha3 import ( - "context" - "database/sql" - "database/sql/driver" - "encoding/json" - "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" ) // +kubebuilder:validation:Enum=SSE;STREAMABLE_HTTP @@ -95,22 +88,6 @@ type RemoteMCPServerSpec struct { TLS *TLSConfig `json:"tls,omitempty"` } -var _ sql.Scanner = (*RemoteMCPServerSpec)(nil) - -func (t *RemoteMCPServerSpec) Scan(src any) error { - switch v := src.(type) { - case []uint8: - return json.Unmarshal(v, t) - } - return nil -} - -var _ driver.Valuer = (*RemoteMCPServerSpec)(nil) - -func (t RemoteMCPServerSpec) Value() (driver.Value, error) { - return json.Marshal(t) -} - // RemoteMCPServerStatus defines the observed state of RemoteMCPServer. type RemoteMCPServerStatus struct { // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster @@ -153,22 +130,6 @@ type RemoteMCPServer struct { Status RemoteMCPServerStatus `json:"status,omitempty"` } -// ResolveHeaders resolves all HeadersFrom entries using the object's namespace. -func (r *RemoteMCPServer) ResolveHeaders(ctx context.Context, client client.Client) (map[string]string, error) { - result := map[string]string{} - - for _, h := range r.Spec.HeadersFrom { - k, v, err := h.Resolve(ctx, client, r.Namespace) - if err != nil { - return nil, fmt.Errorf("failed to resolve header: %v", err) - } - - result[k] = v - } - - return result, nil -} - // +kubebuilder:object:root=true // RemoteMCPServerList contains a list of RemoteMCPServer. type RemoteMCPServerList struct { diff --git a/go/core/internal/service/tool/mcp.go b/go/core/internal/service/tool/mcp.go index 1f4a067e54..999d5169c3 100644 --- a/go/core/internal/service/tool/mcp.go +++ b/go/core/internal/service/tool/mcp.go @@ -12,7 +12,9 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "go.opentelemetry.io/otel/trace" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kagent-dev/kagent/go/api/v1alpha3" @@ -135,7 +137,7 @@ func (c *RuntimeMCPClient) connect(ctx context.Context, ref MCPServerRef) (*mcp. } connectCtx, cancel := context.WithTimeout(ctx, timeout) - headers, err := server.ResolveHeaders(connectCtx, c.kubeClient) + headers, err := resolveHeaders(connectCtx, c.kubeClient, server.Namespace, server.Spec.HeadersFrom) if err != nil { cancel() return nil, nil, fmt.Errorf("failed to resolve RemoteMCPServer headers: %w", err) @@ -276,3 +278,48 @@ func (t *mcpAppsHeaderTransport) RoundTrip(request *http.Request) (*http.Respons } return t.base.RoundTrip(request) } + +// resolveHeaders reads RemoteMCPServer headers, loading ValueFrom entries from +// Secrets or ConfigMaps in the server's namespace. +func resolveHeaders(ctx context.Context, kube client.Reader, namespace string, refs []v1alpha3.ValueRef) (map[string]string, error) { + headers := make(map[string]string, len(refs)) + for _, ref := range refs { + value, err := resolveHeaderValue(ctx, kube, namespace, ref) + if err != nil { + return nil, fmt.Errorf("failed to resolve header %s: %w", ref.Name, err) + } + headers[ref.Name] = value + } + return headers, nil +} + +func resolveHeaderValue(ctx context.Context, kube client.Reader, namespace string, ref v1alpha3.ValueRef) (string, error) { + if ref.ValueFrom == nil { + return ref.Value, nil + } + key := types.NamespacedName{Namespace: namespace, Name: ref.ValueFrom.Name} + switch ref.ValueFrom.Type { + case v1alpha3.SecretValueSource: + secret := &corev1.Secret{} + if err := kube.Get(ctx, key, secret); err != nil { + return "", fmt.Errorf("failed to get Secret %s: %w", key, err) + } + value, ok := secret.Data[ref.ValueFrom.Key] + if !ok { + return "", fmt.Errorf("key %s not found in Secret %s", ref.ValueFrom.Key, key) + } + return string(value), nil + case v1alpha3.ConfigMapValueSource: + configMap := &corev1.ConfigMap{} + if err := kube.Get(ctx, key, configMap); err != nil { + return "", fmt.Errorf("failed to get ConfigMap %s: %w", key, err) + } + value, ok := configMap.Data[ref.ValueFrom.Key] + if !ok { + return "", fmt.Errorf("key %s not found in ConfigMap %s", ref.ValueFrom.Key, key) + } + return value, nil + default: + return "", fmt.Errorf("unknown value source type %q", ref.ValueFrom.Type) + } +} diff --git a/go/core/internal/service/tool/mcp_test.go b/go/core/internal/service/tool/mcp_test.go index 9f5b347849..402b56a9bf 100644 --- a/go/core/internal/service/tool/mcp_test.go +++ b/go/core/internal/service/tool/mcp_test.go @@ -7,6 +7,7 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -117,3 +118,72 @@ func TestRuntimeMCPClientResolveServerMatrix(t *testing.T) { }) } } + +func TestResolveHeaders(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "creds", Namespace: "team"}, + Data: map[string][]byte{"token": []byte("secret-token")}, + } + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "settings", Namespace: "team"}, + Data: map[string]string{"tenant": "acme"}, + } + + t.Run("Given inline, Secret, and ConfigMap headers, when the server headers are resolved, assert that every value is loaded from the server namespace", func(t *testing.T) { + kube := toolTestKube(t, secret, configMap) + + headers, err := resolveHeaders(t.Context(), kube, "team", []v1alpha3.ValueRef{ + {Name: "X-Static", Value: "static"}, + {Name: "Authorization", ValueFrom: &v1alpha3.ValueSource{Type: v1alpha3.SecretValueSource, Name: "creds", Key: "token"}}, + {Name: "X-Tenant", ValueFrom: &v1alpha3.ValueSource{Type: v1alpha3.ConfigMapValueSource, Name: "settings", Key: "tenant"}}, + }) + + require.NoError(t, err) + assert.Equal(t, map[string]string{ + "X-Static": "static", + "Authorization": "secret-token", + "X-Tenant": "acme", + }, headers) + }) + + scenarios := []struct { + name string + objects []client.Object + ref v1alpha3.ValueRef + wantError string + }{ + { + name: "Given the Secret lives in another namespace, when a Secret header is resolved, assert that resolution fails", + objects: []client.Object{&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "creds", Namespace: "other"}, Data: secret.Data}}, + ref: v1alpha3.ValueRef{Name: "Authorization", ValueFrom: &v1alpha3.ValueSource{Type: v1alpha3.SecretValueSource, Name: "creds", Key: "token"}}, + wantError: "failed to get Secret team/creds", + }, + { + name: "Given the Secret lacks the key, when a Secret header is resolved, assert that resolution fails", + objects: []client.Object{secret}, + ref: v1alpha3.ValueRef{Name: "Authorization", ValueFrom: &v1alpha3.ValueSource{Type: v1alpha3.SecretValueSource, Name: "creds", Key: "missing"}}, + wantError: "key missing not found in Secret team/creds", + }, + { + name: "Given the ConfigMap lacks the key, when a ConfigMap header is resolved, assert that resolution fails", + objects: []client.Object{configMap}, + ref: v1alpha3.ValueRef{Name: "X-Tenant", ValueFrom: &v1alpha3.ValueSource{Type: v1alpha3.ConfigMapValueSource, Name: "settings", Key: "missing"}}, + wantError: "key missing not found in ConfigMap team/settings", + }, + { + name: "Given an unknown source type, when the header is resolved, assert that resolution fails", + ref: v1alpha3.ValueRef{Name: "X-Other", ValueFrom: &v1alpha3.ValueSource{Type: "Vault", Name: "x", Key: "y"}}, + wantError: `unknown value source type "Vault"`, + }, + } + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + kube := toolTestKube(t, scenario.objects...) + + headers, err := resolveHeaders(t.Context(), kube, "team", []v1alpha3.ValueRef{scenario.ref}) + + require.ErrorContains(t, err, scenario.wantError) + assert.Nil(t, headers) + }) + } +}