TLS support on RemoteMCPServer + companion-Secret API#1905
Conversation
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
…r-ca-cert Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
|
Warning Testing pausedMonthly snapshot limit reached. Update your plan to get more snapshots and resume testing. |
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end TLS support for RemoteMCPServer (CRD + controller tool-discovery + agent runtime wiring) and introduces a companion-Secret materialization API so operators can create/update a tool server and its referenced Secrets in one POST, aligning the surface area with existing ModelConfig.spec.tls patterns.
Changes:
- Extend
RemoteMCPServerwithspec.tls(reusingTLSConfig+ shared CEL validation) and propagate TLS settings into both controller-side MCP tool discovery and agent-side MCP transport params, including per-Secret CA bundle mounting to avoid volume collisions. - Add an optional
RemoteMCPServerURLRewriterextension point to rewrite dial URLs prior to tool discovery. - Add companion Secret support to
POST /api/toolservers(mirroring ModelConfig inline-Secret behavior), refactoring companion-secret logic into a shared helper and adding e2e/unit coverage.
Reviewed changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/types/index.ts | Adds RemoteMCPServerSpec.tls typing and optional secrets payload on toolserver create requests. |
| python/packages/kagent-adk/src/kagent/adk/types.py | Lifts TLS fields from nested MCP params and installs a TLS-aware httpx_client_factory for MCP sessions. |
| python/packages/kagent-adk/src/kagent/adk/models/_ssl.py | Updates TLS examples to match new per-Secret CA mount paths. |
| python/packages/kagent-adk/tests/unittests/test_mcp_tls.py | Unit tests for TLS field lifting + factory installation for HTTP/SSE MCP transports. |
| python/packages/kagent-adk/tests/unittests/models/test_tls_e2e.py | Adjusts expected CA path in SSL troubleshooting assertions. |
| python/packages/kagent-adk/tests/unittests/models/test_openai.py | Updates custom CA path expectations to include per-Secret directory. |
| helm/kagent-crds/templates/kagent.dev_remotemcpservers.yaml | Adds spec.tls schema + CEL validations to RemoteMCPServer CRD (Helm template). |
| helm/kagent-crds/templates/kagent.dev_modelconfigs.yaml | Moves TLS CEL validations into the TLSConfig schema for ModelConfig CRD (Helm template). |
| go/api/v1alpha2/modelconfig_types.go | Adds shared kubebuilder XValidations to TLSConfig and introduces TLSConfig.IsEmpty(). |
| go/api/v1alpha2/remotemcpserver_types.go | Adds RemoteMCPServerSpec.TLS *TLSConfig. |
| go/api/v1alpha2/zz_generated.deepcopy.go | Updates deepcopy generation for the new RemoteMCPServerSpec.TLS field. |
| go/api/config/crd/bases/kagent.dev_remotemcpservers.yaml | Adds spec.tls schema + CEL validations to RemoteMCPServer CRD (base YAML). |
| go/api/config/crd/bases/kagent.dev_modelconfigs.yaml | Moves TLS CEL validations into the TLSConfig schema for ModelConfig CRD (base YAML). |
| go/core/internal/controller/reconciler/reconciler.go | Applies RMS TLS to controller tool discovery HTTP client; adds optional URL rewrite hook. |
| go/core/internal/controller/reconciler/reconciler_test.go | Tests for RMS TLS config building, URL rewriting, and new TLS-aware HTTP client transport wiring. |
| go/core/internal/controller/reconciler/mcp_server_reconciler_test.go | Updates reconciler constructor usage for the new URL rewriter arg. |
| go/core/pkg/translator/adk_api_translator_types.go | Introduces RemoteMCPServerURLRewriter interface. |
| go/core/pkg/app/app.go | Plumbs URL rewriter through extension config into the reconciler. |
| go/core/internal/controller/translator/agent/adk_api_translator.go | Derives TLS wire fields via TLSConfig.IsEmpty(), mounts per-Secret CA bundles, and avoids mount/volume collisions. |
| go/core/internal/controller/translator/agent/compiler.go | Threads modelDeploymentData into MCP translation so RMS TLS can add mounts. |
| go/core/internal/controller/translator/agent/manifest_builder.go | Defers/stamps kagent.dev/config-hash after plugins so plugin mutations trigger rollouts. |
| go/core/internal/controller/translator/agent/manifest_builder_test.go | Adds regression coverage for post-plugin config-hash stamping (deployment + sandbox paths). |
| go/core/internal/controller/translator/agent/tls_mounting_test.go | Validates new per-Secret CA mount path/volume naming behavior and collision avoidance. |
| go/core/internal/controller/translator/agent/remotemcpserver_tls_test.go | Tests RMS TLS propagation into wire params and CA Secret mounts in the agent deployment. |
| go/core/internal/controller/translator/agent/testdata/outputs/tls-with-custom-ca.json | Updates golden output for per-Secret CA mount path and config hash changes. |
| go/core/internal/controller/translator/agent/testdata/outputs/tls-with-system-cas-disabled.json | Updates golden output for per-Secret CA mount path and config hash changes. |
| go/core/internal/httpserver/handlers/companion_secrets.go | Extracts shared companion-secret validation + create/update logic for multiple owner kinds. |
| go/core/internal/httpserver/handlers/modelconfig.go | Switches ModelConfig companion-secret creation to shared helper with explicit GVK. |
| go/core/internal/httpserver/handlers/toolservers.go | Adds secrets support to toolserver create request and uses shared companion-secret helper. |
| go/core/internal/httpserver/handlers/toolservers_test.go | Adds unit tests for toolserver companion-secret creation, grouping, and rejection cases. |
| go/core/test/e2e/remotemcpserver_tls_test.go | Adds e2e coverage for RMS TLS (private CA / skip-verify / SSE) and toolserver companion secrets. |
| go/core/test/e2e/mocks/invoke_remotemcpserver_tls_agent.json | Adds mock LLM script used by the RMS TLS e2e tests. |
| go/go.mod | Adds github.com/kagent-dev/mockmcp dependency for e2e fixture. |
| go/go.sum | Adds checksums for github.com/kagent-dev/mockmcp. |
| examples/modelconfig-with-tls.yaml | Updates troubleshooting paths to reflect per-Secret CA mount directory structure. |
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.
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
EItanya
left a comment
There was a problem hiding this comment.
Initial review. functionality makes complete sense, but there are a few code smells I'd like to try and work out
| // currently cannot supply TLS config of its own. | ||
| type RemoteMCPServerURLRewriter interface { | ||
| RewriteRemoteMCPServerURL(ctx context.Context, remoteMCPServer *v1alpha2.RemoteMCPServer) (string, error) | ||
| } |
There was a problem hiding this comment.
Can we just add this to the above plugin instead? Or is this a reconciler plugin?
| // This field follows the same pattern as APIKeySecretKey. | ||
| // Required when CACertSecretRef is set (unless DisableVerify is true). | ||
| // CACertSecretKey is the key within the Secret that contains the | ||
| // CA certificate data (PEM-encoded). Required when CACertSecretRef |
There was a problem hiding this comment.
Why is this required, can't we infer a single key if there is one? I see it's required today but not enforced via cel, so I suppose this is just a cleaner UX given the current constraint.
| } | ||
|
|
||
| cfg := &tls.Config{ | ||
| InsecureSkipVerify: tlsSpec.DisableVerify, //nolint:gosec // operator-authored test-fixture escape hatch |
There was a problem hiding this comment.
What does this comment mean?
| // the controller-resolved TLS Secret hash so callers can mix it into | ||
| // the agent's config hash — that's the signal that drives a rollout | ||
| // when the CA Secret rotates in place (same Secret name, new PEM). | ||
| if mdd != nil { |
| // +kubebuilder:validation:XValidation:message="caCertSecretKey requires caCertSecretRef",rule="!(has(self.caCertSecretKey) && size(self.caCertSecretKey) > 0 && (!has(self.caCertSecretRef) || size(self.caCertSecretRef) == 0))" | ||
| // +kubebuilder:validation:XValidation:message="caCertSecretRef requires caCertSecretKey",rule="!(has(self.caCertSecretRef) && size(self.caCertSecretRef) > 0 && (!has(self.caCertSecretKey) || size(self.caCertSecretKey) == 0))" | ||
| // +kubebuilder:validation:XValidation:message="disableSystemCAs requires caCertSecretRef or disableVerify (trust-nothing config rejects every upstream)",rule="!(has(self.disableSystemCAs) && self.disableSystemCAs && (!has(self.disableVerify) || !self.disableVerify) && (!has(self.caCertSecretRef) || size(self.caCertSecretRef) == 0))" |
There was a problem hiding this comment.
if we have tests around this it would be good to validate that these changes are still consistent with the validation rules they replace in line 328-330
| // rmsURLRewriter optionally transforms the URL the controller dials | ||
| // when discovering tools on a RemoteMCPServer. Nil means dial | ||
| // s.Spec.URL verbatim. See pkgtranslator.RemoteMCPServerURLRewriter. | ||
| rmsURLRewriter pkgtranslator.RemoteMCPServerURLRewriter |
There was a problem hiding this comment.
nit: prefer remoteMCPServerURLRewriter here instead. We use the fully qualified name elsewhere in our codebase so keeping that consistent would be good.
|
|
||
| cfg := &tls.Config{ | ||
| InsecureSkipVerify: tlsSpec.DisableVerify, //nolint:gosec // operator-authored test-fixture escape hatch | ||
| } |
There was a problem hiding this comment.
configuring this here seems like a potential code smell. disableVerify should be configured explictly by the user
| } else { | ||
| base = &http.Transport{TLSClientConfig: tlsConfig} | ||
| } |
There was a problem hiding this comment.
this seems to be dead code. http.DefaultTransport.(*http.Transport) will always evaluate to true
| if t, ok := http.DefaultTransport.(*http.Transport); ok { | ||
| clone := t.Clone() | ||
| clone.TLSClientConfig = tlsConfig | ||
| base = clone |
There was a problem hiding this comment.
this cloning and type assertion seems unnecessary to me. We don't ingest a transport in our arguments so we are deep cloning a default from the http library. We can just init a base transport here with the desired settings instead.
| _, _, p := tlsCAPaths(tlsConfig.CACertSecretRef, tlsConfig.CACertSecretKey) | ||
| caCertPath = &p | ||
| } | ||
| return |
There was a problem hiding this comment.
nit: prefer explictly reteturning insecureSkipVerify, disableSystemCAs, caCertPath instead of doing named returns
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: tlsConfig.CACertSecretRef, | ||
| DefaultMode: new(int32(0444)), // Read-only for all users | ||
| DefaultMode: new(int32(0444)), |
There was a problem hiding this comment.
nit: would be good to keep the comment
| } | ||
|
|
||
| populateTLSFields(&sapAICore.BaseModel, model.Spec.TLS) | ||
| sapAICore.TLSInsecureSkipVerify, sapAICore.TLSCACertPath, sapAICore.TLSDisableSystemCAs = deriveTLSFields(model.Spec.TLS) |
There was a problem hiding this comment.
nit: given that all baseModels use the same fields TLSInsecureSkipVerify , TLSCACertPath etc. feels like we are being overly verbose in our approach to replace populateTLSFields. We may be able to keep populateTLSFields instead and wrap any additional logic into that function.
Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
Description
Adds TLS configuration to
RemoteMCPServerso declarative agents can connect to MCP servers behind private CAs, self-signed certs, or corporate internal CAs. Mirrors the existingModelConfig.spec.tlsshape so the operator-facing surface is consistent across resource types.Things to note:
RemoteMCPServer.spec.tls *TLSConfigreuses theTLSConfigfromModelConfigand have shared CEL validationRemoteMCPServer.spec.tlsso that listing tools with a custom CA cert worksRemoteMCPServerURLRewriterextension point to allow for rewriting URLs ofRemoteMCPServersbefore being dialed by the Kagent ControlleraddTLSConfigurationmounts a per-Secret read-only volume at/etc/ssl/certs/custom/<secretName>/<key>(previously a single fixed-name volume that silently collided when multiple TLS sources referenced different Secrets on the same agent).POST /api/toolserversaccepts an optionalsecrets []SecretMaterialarray so operators can create an RMS or MCPServer and its companion Secrets in a single round-trip. Mirrors the existingModelConfiginline-Secret pattern.Testing
Go e2e (
go test ./test/e2e/...against a kind cluster with kagent installed):TestE2E_RMS_PrivateCAUpstream— private-CA HTTPS upstream via mockmcp; assertsStatus.DiscoveredToolspopulates,/api/toolserversreturns the same tools, and the agent invocation reaches mockmcp's tool handler.TestE2E_RMS_DisableVerify— same fixture, skip-verify path.TestE2E_RMS_SSE_TLS— SSE transport against a private-CA upstream.TestE2E_API_ToolServerCompanionSecrets— one-POST RMS+Secret creation with OwnerReference cascade.Uses kagent-dev/mockmcp as the upstream fixture (pinned to
mainHEAD commit for now).Out of scope
spec.tls— the TypeScript type binding is in this PR; rendering the form is a UI follow-up.