Add CEL validation rule for RemoteMCPServer with CA Cert and AllowedNamespaces#1988
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes cross-namespace RemoteMCPServer usage by (1) deleting spec.allowedNamespaces from the RemoteMCPServer API/CRD and (2) making the reconciler reject any cross-namespace RemoteMCPServer reference, to prevent incorrect/dangling TLS CA Secret mounts due to Secrets being namespace-scoped at pod volume resolution time.
Changes:
- Removed
RemoteMCPServerSpec.AllowedNamespacesfrom the Go API types and regenerated generated code/CRDs. - Updated agent reconcile-time validation to deny all cross-namespace
RemoteMCPServerreferences with a clear error. - Updated/added tests, including a translator-level characterization test documenting the dangling Secret mount behavior in cross-namespace scenarios.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| helm/kagent-crds/templates/kagent.dev_remotemcpservers.yaml | Removes spec.allowedNamespaces from the Helm-packaged RemoteMCPServer CRD schema. |
| go/core/internal/controller/translator/agent/remotemcpserver_tls_test.go | Adds a translator characterization test showing why cross-namespace RMS TLS CA mounts are unsafe. |
| go/core/internal/controller/reconciler/reconciler.go | Makes validateMcpServerReference reject cross-namespace RemoteMCPServer references unconditionally. |
| go/core/internal/controller/reconciler/reconciler_test.go | Updates cross-namespace RMS validation tests to expect denial. |
| go/api/v1alpha2/zz_generated.deepcopy.go | Regenerates deepcopy code to reflect removal of AllowedNamespaces from RemoteMCPServerSpec. |
| go/api/v1alpha2/remotemcpserver_types.go | Removes RemoteMCPServerSpec.AllowedNamespaces from the public API type. |
| go/api/config/crd/bases/kagent.dev_remotemcpservers.yaml | Removes spec.allowedNamespaces from the generated RemoteMCPServer CRD schema. |
Files not reviewed (1)
- go/api/v1alpha2/zz_generated.deepcopy.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
EItanya
left a comment
There was a problem hiding this comment.
We can't do this, it's a breaking API change
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
ed3c5f1 to
53213d4
Compare
Makes sense. I have reworked this to be a CEL validation rule instead so you cannot create a RemoteMCPServer with both |
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
…ross-ns-remotemcpserver Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Changed approach to no longer break the API. I am instead adding CEL rules to reject when spec.allowedNamespaces and spec.tls.caCertSecretRef are both set.
Description
Adds a CEL admission rule on
RemoteMCPServerthat rejects setting bothspec.allowedNamespacesandspec.tls.caCertSecretRef. ARemoteMCPServerthat pins a CA Secret must be same-namespace-only; one that allows cross-namespace references must not pin a CA Secret.This closes a silent footgun in the cross-namespace + TLS combination without changing any working behavior: the
AllowedNamespacesfield stays, the reconciler is unchanged, and non-CA cross-namespace sharing keeps working exactly as before.Why
When an agent consumes a
RemoteMCPServerthat pins a CA bundle, the translator mounts that Secret onto the agent's pod by bare name (addTLSConfiguration). ASecretVolumeSourcehas no namespace field — Kubernetes always resolves it in the pod's own namespace, not theRemoteMCPServer's. So if the RMS is referenced cross-namespace, the CA mount either:Meanwhile the controller validates and hashes the Secret in the
RemoteMCPServer's namespace, so it reportsAccepted=truewhile the pod mount looks elsewhere. The cert reference itself is structurally same-namespace (caCertSecretRefis a bare name with no namespace field, resolved in the owner's namespace by design); the only thing that broke co-location was opening the RMS object up cross-namespace viaallowedNamespaces. Making the two mutually exclusive removes the unsound combination at admission time rather than letting it surface as a runtime mount failure on the consuming agent.Validation rule
Spec-level
XValidationonRemoteMCPServerSpec:The rule only rejects the genuinely-hazardous case — a CA Secret combined with an
allowedNamespacesthat actually permits other namespaces (from: Allorfrom: Selector).from: Same(or omitted/empty, which defaults to same-namespace) is always allowed alongside a CA Secret, since it carries no cross-namespace mount hazard.Regenerated
kagent.dev_remotemcpservers.yamland synced the helm CRD copy underhelm/kagent-crds/templates/.Tests
Added cases to the existing envtest CEL suite (
tlsconfig_cel_test.go), exercised against a real kube-apiserver:allowedNamespacesfrom: All+caCertSecretRef→ rejected.allowedNamespacesfrom: Selector+caCertSecretRef→ rejected.allowedNamespacesfrom: Same+caCertSecretRef→ accepted (same-namespace, no hazard).allowedNamespacesfrom: All, no TLS → accepted (cross-namespace sharing still works).caCertSecretRef, noallowedNamespaces→ accepted (same-namespace CA pinning still works).Compatibility
Not a breaking API change — no field is removed and no working configuration changes behavior. The only new effect is at admission: a manifest that combines a CA Secret with a cross-namespace-permitting
allowedNamespaces(from: All/from: Selector) is now rejected on create/update. That combination was already non-functional (dangling or wrong-CA mount), so this turns a silent runtime failure into an explicit, actionable admission error. Same-namespace configurations (from: Sameor omitted) are unaffected, and objects already stored are not re-validated until their next write.