-
Notifications
You must be signed in to change notification settings - Fork 352
Allow cross-namespace tool references #1136
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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>
f77568a to
9107df4
Compare
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
|
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. |
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