feat: Add namespace support to resources_create_or_update similar to kubectl apply -n namespace#941
Conversation
…l apply -f Signed-off-by: phani2898 <phaneendra2898@gmail.com>
152a3d9 to
b92c508
Compare
| } | ||
| // If it's a namespaced resource and namespace wasn't provided, try to use the default configured one | ||
| if namespaced, nsErr := c.isNamespaced(&gvk); nsErr == nil && namespaced { | ||
| namespace = c.NamespaceOrDefault(namespace) |
There was a problem hiding this comment.
Doesn't this handle already the case that if the namespace is not provided, we fall back to default ?
Not sure on adding this parameter
|
|
||
| namespace := obj.GetNamespace() | ||
| // If a namespace override was provided, it takes precedence over the namespace in the resource metadata | ||
| if namespaceOverride != "" { |
There was a problem hiding this comment.
This seems to be an "override always wins silently" behavior 🤔
kubectl apply -n only sets the namespace for resources that don't already have one — it does not override an explicit metadata.namespace in the manifest.
When both -n <namespace> and metadata.namespace are set and they differ, kubectl returns an error, that the provided namespace does not match the passed in namespace
A more correct implementation would only apply the override when the resource has no namespace set (matching kubectl's behavior):
if namespaceOverride != "" && namespace != "" && namespaceOverride != namespace {
return nil, fmt.Errorf("the namespace from the provided object %q does not match the namespace %q", namespace, namespaceOverride)
}
if namespaceOverride != "" && namespace == "" {
namespace = namespaceOverride
obj.SetNamespace(namespace)
}There was a problem hiding this comment.
Perhaps worth to bake this into NamespaceOrDefault logic, instead of nesting the code above?
manusa
left a comment
There was a problem hiding this comment.
Thanks for looking into this and for linking it to #536!
The existing implementation already respects the namespace when provided in the resource YAML/JSON metadata.namespace field. The flow in resourcesCreateOrUpdate (pkg/kubernetes/resources.go:188-192) is:
obj.GetNamespace()extracts the namespace from the parsed resource metadataNamespaceOrDefault()returns it as-is when non-empty, only falling back to the configured default when it's empty
So if the resource YAML includes metadata.namespace: my-namespace, the resource is already created in my-namespace today without any code changes.
The root cause of #536 is that the tool description doesn't mention namespace handling, so the LLM doesn't know it can include metadata.namespace in the YAML to control placement. In the MCP context, the LLM can trivially add metadata.namespace to the YAML before calling the tool — it just needs to know it should.
Adding a new parameter to the core toolset would widen the API surface unnecessarily and introduce semantic complexity (e.g., what happens with cluster-scoped resources, should the override win over an explicit namespace in the manifest, etc.).
Instead, we'd like to address this with two smaller changes:
1. Improve the resource parameter description in pkg/toolsets/core/resources.go (line 102) to mention namespace handling:
"resource": {
Type: "string",
Description: "A JSON or YAML containing a representation of the Kubernetes resource. Should include top-level fields such as apiVersion, kind, metadata, and spec. For namespaced resources, include metadata.namespace to specify the target namespace; if omitted, the configured default namespace is used",
},2. Add a test proving a non-default namespace in the YAML is respected. The existing tests all use namespace: default which coincides with the envtest default, so they'd pass even if namespace handling was broken.
Add a subtest in TestResourcesCreateOrUpdate (pkg/mcp/resources_test.go, after the existing create/update subtests around line 565) that creates a ConfigMap with namespace: ns-1 in the YAML metadata (the ns-1 namespace is already set up in common_test.go:136) and verifies it lands there:
s.Run("resources_create_or_update respects namespace from resource metadata", func() {
configMapYaml := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a-cm-ns-from-metadata\n namespace: ns-1\ndata:\n key: value\n"
result, err := s.CallTool("resources_create_or_update", map[string]interface{}{
"resource": configMapYaml,
})
s.Run("returns success", func() {
s.Nilf(err, "call tool failed %v", err)
s.Falsef(result.IsError, "call tool should not fail, got: %v", result.Content)
})
s.Run("creates ConfigMap in the namespace specified in the resource metadata", func() {
cm, cmErr := client.CoreV1().ConfigMaps("ns-1").Get(s.T().Context(), "a-cm-ns-from-metadata", metav1.GetOptions{})
s.Require().Nilf(cmErr, "ConfigMap not found in ns-1 namespace")
s.Equalf("ns-1", cm.Namespace, "ConfigMap should be in the namespace from the resource metadata")
})
s.Run("does not create ConfigMap in the default namespace", func() {
_, cmErr := client.CoreV1().ConfigMaps("default").Get(s.T().Context(), "a-cm-ns-from-metadata", metav1.GetOptions{})
s.Errorf(cmErr, "ConfigMap should not exist in the default namespace")
})
})These two changes address #536 while keeping the core toolset lean. Would you be open to reworking the PR in this direction?
|
Hi @phani2898, are you still working on this? have you seen my previous comment? |
Adds optional namespace support to resources_create_or_update, similar to kubectl apply -n.
Many manifests are rendered without a hardcoded namespace and expect the namespace to be specified at deployment time. Currently this requires manually patching manifests before applying them through the MCP server.
With this change, a namespace can be provided during the operation and will be applied to resources that do not already define metadata.namespace.
Resolves: #536