Repository navigation
fix(controller): accept a RemoteMCPServer labeled kagent.dev/discovery=disabled without listing its tools (backport to release/v0.10.x) #2753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
20b5b1a
6f44bd2
d836d6a
db23850
e60a76b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -685,6 +686,37 @@ func (a *kagentReconciler) ReconcileKagentRemoteMCPServer(ctx context.Context, r | |
| // 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. The TLS Secret hash is still | ||
| // 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) | ||
| err := secretErr | ||
| if err == nil { | ||
| _, err = a.buildRemoteMCPServerTLSConfig(ctx, server) | ||
| } | ||
| if storeErr := a.storeToolServerWithoutTools(ctx, dbServer); storeErr != nil { | ||
| err = multierror.Append(err, storeErr) | ||
| } | ||
| 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 | ||
| } | ||
| if err := a.setRemoteMCPServerStatus(ctx, server, nil, secretHash, metav1.ConditionTrue, "DiscoveryDisabled", remoteMCPServerDiscoveryDisabledMessage); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we validate the CA bundle before marking this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From Timo's agent: Fixed in e60a76b. The disabled path now builds the same TLS config discovery would (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for fixing this in e60a76b. I checked that the opt-out path now uses |
||
| return fmt.Errorf("failed to reconcile remote mcp server status %s: %w", req.NamespacedName, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| l.Info("registering remote MCP server", "url", server.Spec.URL, "protocol", server.Spec.Protocol) | ||
| start := time.Now() | ||
| tools, err := a.upsertToolServerForRemoteMCPServer(ctx, dbServer, server) | ||
|
|
@@ -748,6 +780,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. 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. | ||
| 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 +812,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, | ||
|
|
@@ -1194,6 +1252,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 | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validated this in a live v0.10.x deployment: with
kagent.dev/discovery=disabled, the server reconciled toAccepted=True/DiscoveryDisabled; the generated Agent config kept its tool allowlist and request-scopedAuthorizationpassthrough; and a fresh caller bearer could perform runtime discovery and multiple tool calls.One migration caveat: an existing
spec.headersFromauthorization entry can override the propagated caller credential and cause a 401. Depending on the apply/GitOps semantics, operators may need to clear it explicitly (for example,headersFrom: []) rather than simply omit it from rendered configuration. This seems worth documenting with the label.It would also help to clarify that
Accepted=True/DiscoveryDisabledmeans controller-side discovery was intentionally skipped; it does not validate runtime authentication or endpoint reachability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From Timo's agent: Thanks for validating it live. Both points are now written down in e60a76b: the
DiscoveryDisabledstatus message says the controller did not connect and runtime authentication and reachability are not validated, and the label's documentation (consts.DiscoveryLabel) adds theheadersFromcaveat: anAuthorizationentry overrides the propagated caller credential, and under apply or GitOps tooling that only adds fields,headersFrom: []may be needed to clear it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Timo. I reviewed e60a76b: the status message now makes clear that runtime authentication and endpoint reachability are not validated, and the DiscoveryLabel documentation covers the Authorization override and explicit
headersFrom: []migration caveat. That addresses both points from my side; this thread is ready to resolve.