Allow cross-namespace tool references#1136
Conversation
|
Hey @onematchfox, thanks for opening this. The reason we don't support this today is that tools, specifically I agree that we should look towards the Gateway API for solutions to namespace isolation. In particular we could use the I definitely don't want to create too much friction, but I want to make sure the API is secure by default. |
👍 Got it - had missed that
ReferenceGrant is quite onerous. How would you feel about using the mechanism behind restricting namespaces for route attachments instead? In essence, we could add a field, let's say E.g. apiVersion: kagent.dev/v1alpha2
kind: RemoteMCPServer
metadata:
name: kagent-tool-server
namespace: kagent
spec:
description: Official KAgent tool server
protocol: STREAMABLE_HTTP
sseReadTimeout: 5m0s
terminateOnClose: true
timeout: 30s
url: http://kagent-tools.kagent:8084/mcp
allowedAgents:
namespaces:
from: All |
62d6d80 to
3da82ac
Compare
Went ahead and implemented this so you can see what I mean. Let me know what you think. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for cross-namespace tool references, allowing agents in one namespace to reference tools (RemoteMCPServers and Agents) in other namespaces. The implementation follows the Gateway API pattern with a bidirectional handshake mechanism using the AllowedNamespaces field, which provides security controls for multi-tenant environments.
Key Changes
- Added
namespacefield toTypedLocalReferencein both UI types and Go API, enabling explicit namespace specification for tool references - Implemented Gateway API-style
AllowedNamespacessecurity policy on Agent and RemoteMCPServer CRDs withAll,Same, andSelectormodes - Updated UI to display tools with namespace prefix (e.g.,
namespace/toolname) and properly handle cross-namespace tool selection and matching - Modified Go translator and controller logic to validate cross-namespace references and resolve headers from the tool's namespace (not the agent's)
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ui/src/types/index.ts |
Added optional namespace field to TypedLocalReference interface |
ui/src/lib/toolUtils.ts |
Updated display name generation to include namespace, modified toolResponseToAgentTool to parse and store namespace separately |
ui/src/lib/__tests__/toolUtils.test.ts |
Added comprehensive test coverage for namespace handling in tool utilities |
ui/src/components/sidebars/AgentDetailsSidebar.tsx |
Updated to display tools with namespace qualification |
ui/src/components/create/ToolsSection.tsx |
Modified to pass and use agent namespace for tool display |
ui/src/components/create/SelectToolsDialog.tsx |
Enhanced tool matching logic to compare both name and namespace for agents |
ui/src/app/agents/new/page.tsx |
Passes agent namespace to ToolsSection component |
ui/src/app/actions/agents.ts |
Converts tool references to use separate namespace field, defaults to agent namespace when not specified |
go/api/v1alpha2/common_types.go |
Implements AllowedNamespaces type with validation logic for cross-namespace references |
go/api/v1alpha2/common_types_test.go |
Comprehensive unit tests for AllowedNamespaces functionality |
go/api/v1alpha2/agent_types.go |
Added AllowedNamespaces field to AgentSpec, added Namespace to TypedLocalReference, implemented NamespacedName() helper |
go/api/v1alpha2/remotemcpserver_types.go |
Added AllowedNamespaces to spec, moved header resolution to RemoteMCPServer method (uses tool's namespace) |
go/internal/controller/translator/agent/adk_api_translator.go |
Added cross-namespace validation checks, updated header resolution to use tool's namespace, ensures MCPServer and Service types reject cross-namespace refs |
go/internal/controller/translator/agent/adk_api_translator_test.go |
New unit tests for cross-namespace agent and RemoteMCPServer tool references with various AllowedNamespaces configurations |
go/internal/controller/agent_controller.go |
Updated tool matching logic to use NamespacedName() for proper cross-namespace comparison |
go/internal/controller/reconciler/reconciler.go |
Updated to pass RemoteMCPServer object (not just spec) for proper namespace-aware header resolution |
go/config/crd/bases/kagent.dev_agents.yaml |
Added allowedNamespaces field with validation rules, added namespace field to tool references |
go/config/crd/bases/kagent.dev_remotemcpservers.yaml |
Added allowedNamespaces field with validation rules |
helm/kagent-crds/templates/ |
Helm chart CRD templates updated to match base CRDs |
go/api/v1alpha2/zz_generated.deepcopy.go |
Generated deep copy methods for new AllowedNamespaces type |
go/internal/controller/translator/agent/testdata/ |
Test data for cross-namespace tool scenarios with expected outputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3da82ac to
1090e26
Compare
|
Rebased this PR and now linting is failing on a change that was part of #1154. |
1090e26 to
f77568a
Compare
|
Ok, I actually kind of love this approach! Maybe we should discuss it briefly in the community meeting tomorrow and then try to move ahead with it? |
Sounds good. See you then. |
Just realised you said community meeting (today) not contributor meeting (Thursday). Won't be able to attend today - but I should be able to attend on Thursday. Feel free to bring it up today as well though - would be great to get others' thoughts on the approach. |
f77568a to
9107df4
Compare
|
FYI @dimetron As discussed in the community meeting yesterday I have added validation to ensure that cross-namespaces references are not permitted outside of the namespaces that the controller is configured to watch. |
|
My latest refactors have given me a bit of room for thought. Whilst I personally like the route attachments pattern, it does have 2 shortcomings that are perhaps worth explicitly noting here:
On the other hand, the I wonder if:
Feel free to share thoughts. |
|
I would actually disagree that the namespace selector is "insecure".
The risk here is that other labels may be less consistent, or user-influenced. The namespace is already much safer to use than a label. While I've been able to make RemtoteMCPServer resources work with my ToolHive deployments, I'd like to be able to keep my agents in separate namespaces as well. I think the ReferenceGrant model makes sense, however, I would argue that it's almost out of scope for kagent. There are multiple other mechanisms available, even RBAC, that can allow/deny communication across namespaces. |
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Currently we default to using the agent namespace - but this no longer holds true so we need to explicitly look at the tools namespace instead. Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
b28a116 to
374c2a2
Compare
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
374c2a2 to
b8727b7
Compare
|
Now re-based. Hopefully I didn't mess anything up when handling conflicts 🤞 FYI @EItanya @supreme-gg-gg I see that e2e tests are now failing. Don't think it's related to this PR however. It seems more likely that a flaky test was introduced as part of #1202 - at least, I see that that PR suffered from the same issue. |
Opening in draft for now so I can get some 👀 on this and hopefully start a discussion as to how we can get this feature in place. The code as it stands now, works, based on my local testing. However, I understand that there may be some security concerns here - particularly since namespaces are often used for tenant isolation. Do these need to be addressed now? And if so, does anyone got any thoughts on ways to tackle this? I would personally look towards Gateway API for inspiration if needed. Closes kagent-dev#841 --------- Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Opening in draft for now so I can get some 👀 on this and hopefully start a discussion as to how we can get this feature in place. The code as it stands now, works, based on my local testing. However, I understand that there may be some security concerns here - particularly since namespaces are often used for tenant isolation. Do these need to be addressed now? And if so, does anyone got any thoughts on ways to tackle this? I would personally look towards Gateway API for inspiration if needed.
Closes #841