From 20b5b1acb77c842e58a7830e176c117e7e7b86be Mon Sep 17 00:00:00 2001 From: Timo Derstappen Date: Tue, 8 Sep 2026 21:20:36 +0200 Subject: [PATCH 1/4] fix(controller): accept a RemoteMCPServer labeled kagent.dev/discovery=disabled without listing its tools (backport to release/v0.10.x) A RemoteMCPServer that authenticates every caller has no credential to offer the controller: agents forward the caller token at run time (KAGENT_PROPAGATE_TOKEN), but the controller's own discovery request carries no bearer, the server answers 401 and the CR stays Accepted=False (ReconcileFailed: Unauthorized) although every agent that references it works. spec.headersFrom is not a way out: its values are resolved into each agent's config and applied last by the runtime, so a controller credential there would replace the caller token in every agent. The kagent.dev/discovery=disabled label (already the predicate that keeps a Service or MCPServer out of auto-discovery) now opts a RemoteMCPServer out of tool discovery: the reconciler skips the listing, reports Accepted=True with reason DiscoveryDisabled, clears status.discoveredTools and keeps the server in the database with no tools. The controller also reacts to label changes, so the opt-out and its removal apply at once rather than on the next periodic refresh. Backport of #2752. Signed-off-by: Timo Derstappen --- .../predicates/discovery_disabled.go | 8 +- .../controller/reconciler/reconciler.go | 46 +++++++ .../remote_mcp_server_discovery_test.go | 125 ++++++++++++++++++ .../remote_mcp_server_controller.go | 7 +- go/core/pkg/consts/labels.go | 15 +++ 5 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go create mode 100644 go/core/pkg/consts/labels.go diff --git a/go/core/internal/controller/predicates/discovery_disabled.go b/go/core/internal/controller/predicates/discovery_disabled.go index e9bd0034e..3baba11c2 100644 --- a/go/core/internal/controller/predicates/discovery_disabled.go +++ b/go/core/internal/controller/predicates/discovery_disabled.go @@ -20,9 +20,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" -) -const discoveryLabel = "kagent.dev/discovery" + "github.com/kagent-dev/kagent/go/core/pkg/consts" +) // DiscoveryDisabledPredicate filters out resources with the discovery label set to disabled type DiscoveryDisabledPredicate struct { @@ -55,6 +55,6 @@ func isDiscoveryDisabled(obj client.Object) bool { return false } - discoveryLabelValue, exists := labels[discoveryLabel] - return exists && discoveryLabelValue == "disabled" + discoveryLabelValue, exists := labels[consts.DiscoveryLabel] + return exists && discoveryLabelValue == consts.DiscoveryDisabled } diff --git a/go/core/internal/controller/reconciler/reconciler.go b/go/core/internal/controller/reconciler/reconciler.go index de07032b3..54e6418aa 100644 --- a/go/core/internal/controller/reconciler/reconciler.go +++ b/go/core/internal/controller/reconciler/reconciler.go @@ -37,6 +37,7 @@ import ( agent_translator "github.com/kagent-dev/kagent/go/core/internal/controller/translator/agent" "github.com/kagent-dev/kagent/go/core/internal/utils" "github.com/kagent-dev/kagent/go/core/internal/version" + "github.com/kagent-dev/kagent/go/core/pkg/consts" "github.com/modelcontextprotocol/go-sdk/mcp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -676,6 +677,25 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r GroupKind: server.GroupVersionKind().GroupKind().String(), } + if remoteMCPServerDiscoveryDisabled(server) { + // The operator opted this server out of discovery (a server that + // authenticates every caller has no credential to offer the controller). + // Accept it without connecting: the status carries no tools, the DB keeps + // the server with an empty inventory, and agents resolve the tool list at + // run time with the credentials they carry. + l.Info("skipping tool discovery for remote MCP server", "url", server.Spec.URL, "label", consts.DiscoveryLabel) + if _, err := a.dbClient.StoreToolServer(ctx, dbServer); err != nil { + return fmt.Errorf("failed to store toolServer %s: %w", dbServer.Name, err) + } + if err := a.dbClient.RefreshToolsForServer(ctx, dbServer.Name, dbServer.GroupKind); err != nil { + return fmt.Errorf("failed to clear tools for toolServer %s: %w", dbServer.Name, err) + } + if err := a.setRemoteMCPServerStatus(ctx, server, nil, "", metav1.ConditionTrue, "DiscoveryDisabled", remoteMCPServerDiscoveryDisabledMessage); err != nil { + return fmt.Errorf("failed to reconcile remote mcp server status %s: %w", req.NamespacedName, err) + } + return nil + } + // Compute the TLS-Secret hash before tool discovery so the status // reflects the operator's current spec.tls.caCertSecretRef contents. // Agents that mount this Secret read the hash from Status.SecretHash @@ -748,6 +768,17 @@ func (a *kagentReconciler) computeRemoteMCPServerSecretHash(ctx context.Context, return computeStatusSecretHash([]secretRef{{NamespacedName: nn, Secret: secret}}), nil } +// remoteMCPServerDiscoveryDisabledMessage explains an Accepted RemoteMCPServer +// that publishes no discovered tools because its operator opted out of discovery. +const remoteMCPServerDiscoveryDisabledMessage = "Tool discovery is disabled by the " + consts.DiscoveryLabel + "=" + consts.DiscoveryDisabled + + " label; agents resolve the tool list at run time" + +// remoteMCPServerDiscoveryDisabled reports whether the operator opted the server +// out of tool discovery with the kagent.dev/discovery=disabled label. +func remoteMCPServerDiscoveryDisabled(server *v1alpha2.RemoteMCPServer) bool { + return server.Labels[consts.DiscoveryLabel] == consts.DiscoveryDisabled +} + func (a *kagentReconciler) reconcileRemoteMCPServerStatus( ctx context.Context, server *v1alpha2.RemoteMCPServer, @@ -769,6 +800,21 @@ func (a *kagentReconciler) reconcileRemoteMCPServerStatus( reason = "Reconciled" message = "Remote MCP server configuration accepted" } + return a.setRemoteMCPServerStatus(ctx, server, discoveredTools, secretHash, status, reason, message) +} + +// setRemoteMCPServerStatus writes the Accepted condition, the discovered tools +// and the Secret hash, skipping the update when nothing changed so the +// reconciler does not loop on its own status writes. +func (a *kagentReconciler) setRemoteMCPServerStatus( + ctx context.Context, + server *v1alpha2.RemoteMCPServer, + discoveredTools []*v1alpha2.MCPTool, + secretHash string, + status metav1.ConditionStatus, + reason string, + message string, +) error { conditionChanged := meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{ Type: v1alpha2.AgentConditionTypeAccepted, Status: status, diff --git a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go new file mode 100644 index 000000000..858c99b24 --- /dev/null +++ b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go @@ -0,0 +1,125 @@ +package reconciler + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/kagent-dev/kagent/go/api/database" + "github.com/kagent-dev/kagent/go/api/v1alpha2" + "github.com/kagent-dev/kagent/go/core/pkg/consts" +) + +// recordingToolServerStore is the slice of database.Client the RemoteMCPServer +// reconciler touches when discovery is disabled. Every other method panics +// (nil interface), which is the assertion that nothing else is reached. +type recordingToolServerStore struct { + database.Client + stored *database.ToolServer + refreshed *struct { + name, groupKind string + tools []*v1alpha2.MCPTool + } +} + +func (s *recordingToolServerStore) StoreToolServer(_ context.Context, toolServer *database.ToolServer) (*database.ToolServer, error) { + s.stored = toolServer + return toolServer, nil +} + +func (s *recordingToolServerStore) RefreshToolsForServer(_ context.Context, serverName string, groupKind string, tools ...*v1alpha2.MCPTool) error { + s.refreshed = &struct { + name, groupKind string + tools []*v1alpha2.MCPTool + }{serverName, groupKind, tools} + return nil +} + +// TestReconcileKagentRemoteMCPServer_DiscoveryDisabled verifies that a +// RemoteMCPServer labeled kagent.dev/discovery=disabled is Accepted without the +// controller connecting to it: the URL below is unroutable, so any dial would +// fail the reconcile, and the recording store panics on anything but the two +// catalog writes the opt-out performs. +func TestReconcileKagentRemoteMCPServer_DiscoveryDisabled(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha2.AddToScheme(scheme)) + + server := &v1alpha2.RemoteMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "per-caller", + Namespace: "default", + Generation: 2, + Labels: map[string]string{consts.DiscoveryLabel: consts.DiscoveryDisabled}, + }, + Spec: v1alpha2.RemoteMCPServerSpec{ + Description: "authenticates every caller", + URL: "http://192.0.2.1:1/mcp", + Protocol: v1alpha2.RemoteMCPServerProtocolStreamableHttp, + }, + Status: v1alpha2.RemoteMCPServerStatus{ + DiscoveredTools: []*v1alpha2.MCPTool{{Name: "stale"}}, + Conditions: []metav1.Condition{{ + Type: v1alpha2.AgentConditionTypeAccepted, Status: metav1.ConditionFalse, Reason: "ReconcileFailed", Message: "Unauthorized", + }}, + }, + } + kube := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(server). + WithObjects(server). + Build() + store := &recordingToolServerStore{} + reconciler := &kagentReconciler{kube: kube, dbClient: store} + + require.NoError(t, reconciler.ReconcileKagentRemoteMCPServer(context.Background(), + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "per-caller"}})) + + updated := &v1alpha2.RemoteMCPServer{} + require.NoError(t, kube.Get(context.Background(), client.ObjectKeyFromObject(server), updated)) + + accepted := meta.FindStatusCondition(updated.Status.Conditions, v1alpha2.AgentConditionTypeAccepted) + require.NotNil(t, accepted) + assert.Equal(t, metav1.ConditionTrue, accepted.Status) + assert.Equal(t, "DiscoveryDisabled", accepted.Reason) + assert.Equal(t, remoteMCPServerDiscoveryDisabledMessage, accepted.Message) + assert.Equal(t, server.Generation, accepted.ObservedGeneration) + assert.Equal(t, server.Generation, updated.Status.ObservedGeneration) + assert.Empty(t, updated.Status.DiscoveredTools, "the stale inventory is cleared") + + require.NotNil(t, store.stored) + assert.Equal(t, "default/per-caller", store.stored.Name) + assert.Equal(t, "authenticates every caller", store.stored.Description) + require.NotNil(t, store.refreshed) + assert.Equal(t, store.stored.Name, store.refreshed.name) + assert.Equal(t, store.stored.GroupKind, store.refreshed.groupKind) + assert.Empty(t, store.refreshed.tools) +} + +func TestRemoteMCPServerDiscoveryDisabled(t *testing.T) { + for name, tc := range map[string]struct { + labels map[string]string + want bool + }{ + "no labels": {labels: nil, want: false}, + "other label": {labels: map[string]string{"app": "x"}, want: false}, + "other value": {labels: map[string]string{consts.DiscoveryLabel: "enabled"}, want: false}, + "disabled": {labels: map[string]string{consts.DiscoveryLabel: consts.DiscoveryDisabled}, want: true}, + "disabled, mixed": {labels: map[string]string{"app": "x", consts.DiscoveryLabel: consts.DiscoveryDisabled}, want: true}, + } { + t.Run(name, func(t *testing.T) { + server := &v1alpha2.RemoteMCPServer{ObjectMeta: metav1.ObjectMeta{Labels: tc.labels}} + assert.Equal(t, tc.want, remoteMCPServerDiscoveryDisabled(server)) + }) + } +} diff --git a/go/core/internal/controller/remote_mcp_server_controller.go b/go/core/internal/controller/remote_mcp_server_controller.go index a609fb6e3..ed549a755 100644 --- a/go/core/internal/controller/remote_mcp_server_controller.go +++ b/go/core/internal/controller/remote_mcp_server_controller.go @@ -79,7 +79,12 @@ func (r *RemoteMCPServerController) SetupWithManager(mgr ctrl.Manager) error { WithOptions(controller.Options{ NeedLeaderElection: new(true), }). - For(&v1alpha2.RemoteMCPServer{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + // A label change does not bump the generation, and the discovery opt-out + // (kagent.dev/discovery=disabled) is a label: watch both so the opt-out + // and its removal apply at once instead of on the next periodic refresh. + For(&v1alpha2.RemoteMCPServer{}, builder.WithPredicates(predicate.Or[client.Object]( + predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}, + ))). Watches( &corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { diff --git a/go/core/pkg/consts/labels.go b/go/core/pkg/consts/labels.go new file mode 100644 index 000000000..7fc1d4b4b --- /dev/null +++ b/go/core/pkg/consts/labels.go @@ -0,0 +1,15 @@ +package consts + +// DiscoveryLabel opts a tool server out of the controller's tool discovery. A +// RemoteMCPServer labeled `kagent.dev/discovery=disabled` is Accepted without the +// controller listing its tools: its status publishes no discovered tools, and the +// agents that reference it resolve the tool list at run time with the credentials +// they carry — for a server that authenticates every caller (a propagated caller +// token, which the controller does not hold), the controller-side listing would +// otherwise fail and mark the server as not Accepted forever. Any other value, or +// no label, keeps discovery on. The same label excludes a Service or MCPServer +// from automatic discovery (predicates.DiscoveryDisabledPredicate). +const DiscoveryLabel = "kagent.dev/discovery" + +// DiscoveryDisabled is the DiscoveryLabel value that turns discovery off. +const DiscoveryDisabled = "disabled" From 6f44bd211a02e64d0abb3223b1e620a18fc17210 Mon Sep 17 00:00:00 2001 From: Timo Derstappen Date: Tue, 8 Sep 2026 21:31:40 +0200 Subject: [PATCH 2/4] fix(controller): keep publishing the TLS Secret hash when discovery is disabled The opt-out path wrote an empty Status.SecretHash, which agents fold into their rollout hash for in-place CA rotations. Compute the hash before the opt-out branch, publish it with the DiscoveryDisabled condition, and keep failing the server on a broken spec.tls Secret reference. Signed-off-by: Timo Derstappen --- .../controller/reconciler/reconciler.go | 30 +++++--- .../remote_mcp_server_discovery_test.go | 73 +++++++++++++++++++ 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/go/core/internal/controller/reconciler/reconciler.go b/go/core/internal/controller/reconciler/reconciler.go index 54e6418aa..99277dbfd 100644 --- a/go/core/internal/controller/reconciler/reconciler.go +++ b/go/core/internal/controller/reconciler/reconciler.go @@ -677,12 +677,23 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r GroupKind: server.GroupVersionKind().GroupKind().String(), } + // Compute the TLS-Secret hash before tool discovery so the status + // reflects the operator's current spec.tls.caCertSecretRef contents. + // Agents that mount this Secret read the hash from Status.SecretHash + // at translate time so an in-place cert rotation triggers a rollout + // even when the Secret name (and therefore the cert path) didn't + // change. Missing-Secret errors are folded into the registration + // error path so a misconfigured RMS still surfaces a Failed condition. + secretHash, secretErr := a.computeRemoteMCPServerSecretHash(ctx, server) + if remoteMCPServerDiscoveryDisabled(server) { // The operator opted this server out of discovery (a server that // authenticates every caller has no credential to offer the controller). // Accept it without connecting: the status carries no tools, the DB keeps // the server with an empty inventory, and agents resolve the tool list at - // run time with the credentials they carry. + // run time with the credentials they carry. The TLS Secret hash is still + // published (agents fold it into their rollout hash), and a broken + // spec.tls Secret reference still fails the server. l.Info("skipping tool discovery for remote MCP server", "url", server.Spec.URL, "label", consts.DiscoveryLabel) if _, err := a.dbClient.StoreToolServer(ctx, dbServer); err != nil { return fmt.Errorf("failed to store toolServer %s: %w", dbServer.Name, err) @@ -690,21 +701,18 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r if err := a.dbClient.RefreshToolsForServer(ctx, dbServer.Name, dbServer.GroupKind); err != nil { return fmt.Errorf("failed to clear tools for toolServer %s: %w", dbServer.Name, err) } - if err := a.setRemoteMCPServerStatus(ctx, server, nil, "", metav1.ConditionTrue, "DiscoveryDisabled", remoteMCPServerDiscoveryDisabledMessage); err != nil { + if secretErr != nil { + if err := a.reconcileRemoteMCPServerStatus(ctx, server, nil, secretHash, secretErr); err != nil { + return fmt.Errorf("failed to reconcile remote mcp server status %s: %w", req.NamespacedName, err) + } + return nil + } + if err := a.setRemoteMCPServerStatus(ctx, server, nil, secretHash, metav1.ConditionTrue, "DiscoveryDisabled", remoteMCPServerDiscoveryDisabledMessage); err != nil { return fmt.Errorf("failed to reconcile remote mcp server status %s: %w", req.NamespacedName, err) } return nil } - // Compute the TLS-Secret hash before tool discovery so the status - // reflects the operator's current spec.tls.caCertSecretRef contents. - // Agents that mount this Secret read the hash from Status.SecretHash - // at translate time so an in-place cert rotation triggers a rollout - // even when the Secret name (and therefore the cert path) didn't - // change. Missing-Secret errors are folded into the registration - // error path so a misconfigured RMS still surfaces a Failed condition. - secretHash, secretErr := a.computeRemoteMCPServerSecretHash(ctx, server) - l.Info("registering remote MCP server", "url", server.Spec.URL, "protocol", server.Spec.Protocol) start := time.Now() tools, err := a.upsertToolServerForRemoteMCPServer(ctx, dbServer, server) diff --git a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go index 858c99b24..00fbb155b 100644 --- a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go +++ b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -106,6 +107,78 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabled(t *testing.T) { assert.Empty(t, store.refreshed.tools) } +// TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash verifies +// that the opt-out still publishes the TLS Secret hash agents fold into their +// rollout hash, and still fails the server on a broken spec.tls reference. +func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash(t *testing.T) { + newServer := func() *v1alpha2.RemoteMCPServer { + return &v1alpha2.RemoteMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "per-caller-tls", + Namespace: "default", + Generation: 1, + Labels: map[string]string{consts.DiscoveryLabel: consts.DiscoveryDisabled}, + }, + Spec: v1alpha2.RemoteMCPServerSpec{ + Description: "authenticates every caller, pinned CA", + URL: "https://192.0.2.1:1/mcp", + Protocol: v1alpha2.RemoteMCPServerProtocolStreamableHttp, + TLS: &v1alpha2.TLSConfig{CACertSecretRef: "ca", CACertSecretKey: "ca.crt"}, + }, + } + } + caSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "ca", Namespace: "default"}, + Data: map[string][]byte{"ca.crt": []byte("-----BEGIN CERTIFICATE-----")}, + } + + for name, tc := range map[string]struct { + objects []client.Object + wantAccepted metav1.ConditionStatus + wantReason string + wantHash bool + }{ + "CA Secret present: Accepted, hash published": { + objects: []client.Object{caSecret}, wantAccepted: metav1.ConditionTrue, wantReason: "DiscoveryDisabled", wantHash: true, + }, + "CA Secret missing: the broken reference still fails the server": { + wantAccepted: metav1.ConditionFalse, wantReason: "ReconcileFailed", + }, + } { + t.Run(name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha2.AddToScheme(scheme)) + server := newServer() + kube := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(server). + WithObjects(append([]client.Object{server}, tc.objects...)...). + Build() + store := &recordingToolServerStore{} + reconciler := &kagentReconciler{kube: kube, dbClient: store} + + require.NoError(t, reconciler.ReconcileKagentRemoteMCPServer(context.Background(), + reconcile.Request{NamespacedName: client.ObjectKeyFromObject(server)})) + + updated := &v1alpha2.RemoteMCPServer{} + require.NoError(t, kube.Get(context.Background(), client.ObjectKeyFromObject(server), updated)) + accepted := meta.FindStatusCondition(updated.Status.Conditions, v1alpha2.AgentConditionTypeAccepted) + require.NotNil(t, accepted) + assert.Equal(t, tc.wantAccepted, accepted.Status) + assert.Equal(t, tc.wantReason, accepted.Reason) + assert.Empty(t, updated.Status.DiscoveredTools) + if tc.wantHash { + assert.NotEmpty(t, updated.Status.SecretHash, "the TLS Secret hash is published although discovery is disabled") + } else { + assert.Empty(t, updated.Status.SecretHash) + assert.Contains(t, accepted.Message, "failed to get TLS secret") + } + require.NotNil(t, store.stored, "the server is stored even when its TLS reference is broken") + }) + } +} + func TestRemoteMCPServerDiscoveryDisabled(t *testing.T) { for name, tc := range map[string]struct { labels map[string]string From d836d6a48067045a695d4b64983a38d0f151be4d Mon Sep 17 00:00:00 2001 From: Timo Derstappen Date: Tue, 15 Sep 2026 23:48:33 +0200 Subject: [PATCH 3/4] fix(controller): write the opted-out RemoteMCPServer inventory once The discovery-disabled path stored the server and cleared its tools on every periodic refresh, a Postgres write every 60 s for a row that never changes. Reuse the tool-snapshot cache from #2796: the empty inventory is written when the snapshot changes and skipped otherwise. The snapshot is taken over an empty, non-nil tool list so it differs from the row ensureToolServerRow writes after a failed discovery. Signed-off-by: Timo Derstappen --- .../controller/reconciler/reconciler.go | 29 +++++++++++++++---- .../remote_mcp_server_discovery_test.go | 19 ++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/go/core/internal/controller/reconciler/reconciler.go b/go/core/internal/controller/reconciler/reconciler.go index 99277dbfd..c9fbec52a 100644 --- a/go/core/internal/controller/reconciler/reconciler.go +++ b/go/core/internal/controller/reconciler/reconciler.go @@ -695,11 +695,8 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r // published (agents fold it into their rollout hash), and a broken // spec.tls Secret reference still fails the server. l.Info("skipping tool discovery for remote MCP server", "url", server.Spec.URL, "label", consts.DiscoveryLabel) - if _, err := a.dbClient.StoreToolServer(ctx, dbServer); err != nil { - return fmt.Errorf("failed to store toolServer %s: %w", dbServer.Name, err) - } - if err := a.dbClient.RefreshToolsForServer(ctx, dbServer.Name, dbServer.GroupKind); err != nil { - return fmt.Errorf("failed to clear tools for toolServer %s: %w", dbServer.Name, err) + if err := a.storeToolServerWithoutTools(ctx, dbServer); err != nil { + return err } if secretErr != nil { if err := a.reconcileRemoteMCPServerStatus(ctx, server, nil, secretHash, secretErr); err != nil { @@ -1248,6 +1245,28 @@ func (a *kagentReconciler) upsertToolServerForRemoteMCPServer(ctx context.Contex return tools, nil } +// storeToolServerWithoutTools persists the server with an empty inventory, +// once: the periodic refresh re-enters the discovery-disabled path every +// interval and must not write to Postgres while nothing changed. The snapshot +// is taken over an empty, non-nil tool list so it differs from the row +// ensureToolServerRow writes after a failed discovery, which leaves the +// previously discovered tools in place. +func (a *kagentReconciler) storeToolServerWithoutTools(ctx context.Context, toolServer *database.ToolServer) error { + noTools := []*v1alpha2.MCPTool{} + if a.toolSnapshotUnchanged(toolServer, noTools) { + return nil + } + a.evictToolSnapshot(toolServer.Name, toolServer.GroupKind) + if _, err := a.dbClient.StoreToolServer(ctx, toolServer); err != nil { + return fmt.Errorf("failed to store toolServer %s: %w", toolServer.Name, err) + } + if err := a.dbClient.RefreshToolsForServer(ctx, toolServer.Name, toolServer.GroupKind); err != nil { + return fmt.Errorf("failed to clear tools for toolServer %s: %w", toolServer.Name, err) + } + a.rememberToolSnapshot(toolServer, noTools) + return nil +} + func toolSnapshotKey(name, groupKind string) string { return name + "\x00" + groupKind } diff --git a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go index 00fbb155b..e0b2a4ad4 100644 --- a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go +++ b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go @@ -31,10 +31,12 @@ type recordingToolServerStore struct { name, groupKind string tools []*v1alpha2.MCPTool } + writes int } func (s *recordingToolServerStore) StoreToolServer(_ context.Context, toolServer *database.ToolServer) (*database.ToolServer, error) { s.stored = toolServer + s.writes++ return toolServer, nil } @@ -43,6 +45,7 @@ func (s *recordingToolServerStore) RefreshToolsForServer(_ context.Context, serv name, groupKind string tools []*v1alpha2.MCPTool }{serverName, groupKind, tools} + s.writes++ return nil } @@ -105,6 +108,22 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabled(t *testing.T) { assert.Equal(t, store.stored.Name, store.refreshed.name) assert.Equal(t, store.stored.GroupKind, store.refreshed.groupKind) assert.Empty(t, store.refreshed.tools) + assert.Equal(t, 2, store.writes, "the server row and the empty inventory are written once") + + // The periodic refresh re-enters the same path; nothing changed, so the + // database is not touched again. + require.NoError(t, reconciler.ReconcileKagentRemoteMCPServer(context.Background(), + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "per-caller"}})) + assert.Equal(t, 2, store.writes, "an unchanged opted-out server is not rewritten") + + // A description change is a new snapshot and is written again. + require.NoError(t, kube.Get(context.Background(), client.ObjectKeyFromObject(server), updated)) + updated.Spec.Description = "renamed" + require.NoError(t, kube.Update(context.Background(), updated)) + require.NoError(t, reconciler.ReconcileKagentRemoteMCPServer(context.Background(), + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "per-caller"}})) + assert.Equal(t, 4, store.writes) + assert.Equal(t, "renamed", store.stored.Description) } // TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash verifies From e60a76bd5eaf7d70a56efcdf2f102578b0a4a296 Mon Sep 17 00:00:00 2001 From: Timo Derstappen Date: Fri, 2 Oct 2026 11:24:21 +0200 Subject: [PATCH 4/4] fix(controller): fail opted-out RemoteMCPServer on bad CA or store error With kagent.dev/discovery=disabled the reconciler now rejects what discovery would have rejected before dialing: a CA bundle without a valid certificate fails the server instead of looking healthy. A failure to persist the empty inventory sets Accepted=False and clears the stale discoveredTools, as the discovery error path does. The status message and the label's documentation state that DiscoveryDisabled does not validate runtime authentication or reachability, and that a spec.headersFrom Authorization entry overrides the propagated caller credential and may need an explicit `headersFrom: []` under GitOps tooling. Signed-off-by: Timo Derstappen --- .../controller/reconciler/reconciler.go | 21 ++-- .../remote_mcp_server_discovery_test.go | 107 ++++++++++++++++-- go/core/pkg/consts/labels.go | 8 ++ 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/go/core/internal/controller/reconciler/reconciler.go b/go/core/internal/controller/reconciler/reconciler.go index c9fbec52a..207be9f1e 100644 --- a/go/core/internal/controller/reconciler/reconciler.go +++ b/go/core/internal/controller/reconciler/reconciler.go @@ -692,14 +692,21 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r // Accept it without connecting: the status carries no tools, the DB keeps // the server with an empty inventory, and agents resolve the tool list at // run time with the credentials they carry. The TLS Secret hash is still - // published (agents fold it into their rollout hash), and a broken - // spec.tls Secret reference still fails the server. + // published (agents fold it into their rollout hash), and the server + // still fails on what discovery would have rejected before dialing: a + // broken spec.tls Secret reference or a CA bundle without a valid + // certificate. A failure to persist the empty inventory fails it too, + // with the stale tools cleared from the status. l.Info("skipping tool discovery for remote MCP server", "url", server.Spec.URL, "label", consts.DiscoveryLabel) - if err := a.storeToolServerWithoutTools(ctx, dbServer); err != nil { - return err + err := secretErr + if err == nil { + _, err = a.buildRemoteMCPServerTLSConfig(ctx, server) + } + if storeErr := a.storeToolServerWithoutTools(ctx, dbServer); storeErr != nil { + err = multierror.Append(err, storeErr) } - if secretErr != nil { - if err := a.reconcileRemoteMCPServerStatus(ctx, server, nil, secretHash, secretErr); err != nil { + if err != nil { + if err := a.reconcileRemoteMCPServerStatus(ctx, server, nil, secretHash, err); err != nil { return fmt.Errorf("failed to reconcile remote mcp server status %s: %w", req.NamespacedName, err) } return nil @@ -776,7 +783,7 @@ func (a *kagentReconciler) computeRemoteMCPServerSecretHash(ctx context.Context, // remoteMCPServerDiscoveryDisabledMessage explains an Accepted RemoteMCPServer // that publishes no discovered tools because its operator opted out of discovery. const remoteMCPServerDiscoveryDisabledMessage = "Tool discovery is disabled by the " + consts.DiscoveryLabel + "=" + consts.DiscoveryDisabled + - " label; agents resolve the tool list at run time" + " label; agents resolve the tool list at run time. The controller did not connect: runtime authentication and reachability are not validated" // remoteMCPServerDiscoveryDisabled reports whether the operator opted the server // out of tool discovery with the kagent.dev/discovery=disabled label. diff --git a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go index e0b2a4ad4..ff37729cf 100644 --- a/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go +++ b/go/core/internal/controller/reconciler/remote_mcp_server_discovery_test.go @@ -2,7 +2,16 @@ package reconciler import ( "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "errors" + "math/big" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -49,6 +58,34 @@ func (s *recordingToolServerStore) RefreshToolsForServer(_ context.Context, serv return nil } +// failingToolServerStore fails the first catalog write the opt-out performs. +type failingToolServerStore struct { + database.Client +} + +func (failingToolServerStore) StoreToolServer(context.Context, *database.ToolServer) (*database.ToolServer, error) { + return nil, errors.New("database unavailable") +} + +// selfSignedCAPEM returns a freshly generated, PEM-encoded CA certificate. +func selfSignedCAPEM(t *testing.T) []byte { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-ca"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + BasicConstraintsValid: true, + KeyUsage: x509.KeyUsageCertSign, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + require.NoError(t, err) + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + // TestReconcileKagentRemoteMCPServer_DiscoveryDisabled verifies that a // RemoteMCPServer labeled kagent.dev/discovery=disabled is Accepted without the // controller connecting to it: the URL below is unroutable, so any dial would @@ -128,7 +165,8 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabled(t *testing.T) { // TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash verifies // that the opt-out still publishes the TLS Secret hash agents fold into their -// rollout hash, and still fails the server on a broken spec.tls reference. +// rollout hash, and still fails the server on a broken spec.tls reference or a +// CA bundle discovery would reject. func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash(t *testing.T) { newServer := func() *v1alpha2.RemoteMCPServer { return &v1alpha2.RemoteMCPServer{ @@ -146,9 +184,11 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash(t *test }, } } - caSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "ca", Namespace: "default"}, - Data: map[string][]byte{"ca.crt": []byte("-----BEGIN CERTIFICATE-----")}, + caSecret := func(data []byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "ca", Namespace: "default"}, + Data: map[string][]byte{"ca.crt": data}, + } } for name, tc := range map[string]struct { @@ -156,12 +196,17 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash(t *test wantAccepted metav1.ConditionStatus wantReason string wantHash bool + wantMessage string }{ "CA Secret present: Accepted, hash published": { - objects: []client.Object{caSecret}, wantAccepted: metav1.ConditionTrue, wantReason: "DiscoveryDisabled", wantHash: true, + objects: []client.Object{caSecret(selfSignedCAPEM(t))}, wantAccepted: metav1.ConditionTrue, wantReason: "DiscoveryDisabled", wantHash: true, }, "CA Secret missing: the broken reference still fails the server": { - wantAccepted: metav1.ConditionFalse, wantReason: "ReconcileFailed", + wantAccepted: metav1.ConditionFalse, wantReason: "ReconcileFailed", wantMessage: "failed to get TLS secret", + }, + "CA bundle without a valid certificate: rejected as discovery would": { + objects: []client.Object{caSecret([]byte("-----BEGIN CERTIFICATE-----"))}, wantAccepted: metav1.ConditionFalse, wantReason: "ReconcileFailed", + wantHash: true, wantMessage: "does not contain valid PEM certificates", }, } { t.Run(name, func(t *testing.T) { @@ -191,13 +236,61 @@ func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledKeepsSecretHash(t *test assert.NotEmpty(t, updated.Status.SecretHash, "the TLS Secret hash is published although discovery is disabled") } else { assert.Empty(t, updated.Status.SecretHash) - assert.Contains(t, accepted.Message, "failed to get TLS secret") + } + if tc.wantMessage != "" { + assert.Contains(t, accepted.Message, tc.wantMessage) } require.NotNil(t, store.stored, "the server is stored even when its TLS reference is broken") }) } } +// TestReconcileKagentRemoteMCPServer_DiscoveryDisabledStoreFailure verifies +// that a server switched to discovery disabled is not left Accepted with its +// old inventory when the empty catalog cannot be persisted. +func TestReconcileKagentRemoteMCPServer_DiscoveryDisabledStoreFailure(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha2.AddToScheme(scheme)) + + server := &v1alpha2.RemoteMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "switched-off", + Namespace: "default", + Generation: 3, + Labels: map[string]string{consts.DiscoveryLabel: consts.DiscoveryDisabled}, + }, + Spec: v1alpha2.RemoteMCPServerSpec{ + URL: "http://192.0.2.1:1/mcp", + Protocol: v1alpha2.RemoteMCPServerProtocolStreamableHttp, + }, + Status: v1alpha2.RemoteMCPServerStatus{ + DiscoveredTools: []*v1alpha2.MCPTool{{Name: "discovered-before-the-switch"}}, + Conditions: []metav1.Condition{{ + Type: v1alpha2.AgentConditionTypeAccepted, Status: metav1.ConditionTrue, Reason: "Reconciled", Message: "Remote MCP server configuration accepted", + }}, + }, + } + kube := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(server). + WithObjects(server). + Build() + reconciler := &kagentReconciler{kube: kube, dbClient: failingToolServerStore{}} + + require.NoError(t, reconciler.ReconcileKagentRemoteMCPServer(context.Background(), + reconcile.Request{NamespacedName: client.ObjectKeyFromObject(server)})) + + updated := &v1alpha2.RemoteMCPServer{} + require.NoError(t, kube.Get(context.Background(), client.ObjectKeyFromObject(server), updated)) + accepted := meta.FindStatusCondition(updated.Status.Conditions, v1alpha2.AgentConditionTypeAccepted) + require.NotNil(t, accepted) + assert.Equal(t, metav1.ConditionFalse, accepted.Status) + assert.Equal(t, "ReconcileFailed", accepted.Reason) + assert.Contains(t, accepted.Message, "database unavailable") + assert.Empty(t, updated.Status.DiscoveredTools, "the old inventory is cleared") +} + func TestRemoteMCPServerDiscoveryDisabled(t *testing.T) { for name, tc := range map[string]struct { labels map[string]string diff --git a/go/core/pkg/consts/labels.go b/go/core/pkg/consts/labels.go index 7fc1d4b4b..9aa5360e1 100644 --- a/go/core/pkg/consts/labels.go +++ b/go/core/pkg/consts/labels.go @@ -9,6 +9,14 @@ package consts // otherwise fail and mark the server as not Accepted forever. Any other value, or // no label, keeps discovery on. The same label excludes a Service or MCPServer // from automatic discovery (predicates.DiscoveryDisabledPredicate). +// +// Accepted=True with reason DiscoveryDisabled means only that the controller +// skipped discovery on purpose and the spec.tls configuration is usable; it +// does not validate runtime authentication or endpoint reachability. When +// switching a server to the propagated caller credential, clear any +// spec.headersFrom entry that sets Authorization: it overrides the caller's +// token. Under server-side apply or GitOps tooling that only adds fields, +// omitting the entry may leave it in place, so set `headersFrom: []` explicitly. const DiscoveryLabel = "kagent.dev/discovery" // DiscoveryDisabled is the DiscoveryLabel value that turns discovery off.