feat(http): implement tls serving certs for server#739
feat(http): implement tls serving certs for server#739mjudeikis wants to merge 1 commit intocontainers:mainfrom
Conversation
|
Still, a few other things are needed. Not straight forward as I expected :D |
manusa
left a comment
There was a problem hiding this comment.
Thanks for taking care of this!
The TLS implementation looks good overall. A few notes:
Dockerfile change: As discussed in the comments, this should be handled in #752. Once #752 is merged with the ENTRYPOINT/CMD split pattern:
ENTRYPOINT ["/app/kubernetes-mcp-server"]
CMD ["--port", "8080"]The Helm chart will work correctly because:
- The deployment template uses
args(notcommand), which replacesCMDin Kubernetes - With your
extraArgsaddition, the final args become:["--config", "/path/to/config.toml", "--tls-cert=...", "--tls-key=..."] - The port is then specified via the config file (
config.port), not the Dockerfile
So the recommendation is to drop the Dockerfile change from this PR and let #752 handle it with the proper backward-compatible approach.
The extraArgs pattern: This is a good addition that provides flexibility for TLS and other future configuration options. It appends to the base args rather than replacing them entirely.
858496b to
af83a2e
Compare
|
@manusa ready. tests look like flakes, and I don't have access to rerun |
manusa
left a comment
There was a problem hiding this comment.
Thx for this improvement!
I think it's looking good, I added a few minor comments.
02bd492 to
6ff4ba5
Compare
Signed-off-by: Mangirdas Judeikis <[email protected]>
6ff4ba5 to
a64e4a3
Compare
|
@manusa hope this is what you expected in your comments :) |
|
@mjudeikis-bot follow this pr and let me know if my attention here is needed. |
|
On it — monitoring this PR. Current status: @manusa has Action needed: @manusa asked at 14:24 today: "shall I merge that one first?" (referring to #798, the CI job for README dirtiness checks) — that question appears unanswered. You may want to respond before @manusa does a final re-review. |
|
|
||
| # -- Extra arguments to pass to the kubernetes-mcp-server command line. | ||
| # -- Useful for passing TLS keys or other configuration options. It can also be configured using ConfigMap. | ||
| extraArgs: [] |
There was a problem hiding this comment.
When TLS is enabled via extraArgs, the default livenessProbe and readinessProbe in values.yaml use httpGet with port: http. These HTTP probes will fail against an HTTPS endpoint since the server won't respond to plain HTTP.
Since TLS is configured through opaque extraArgs, the chart has no way to auto-set scheme: HTTPS on the probes. Users would need to manually override probes, volumes, and volume mounts — which is error-prone.
Minimal fix: Document the required probe override in the comments here (e.g., show that users must also set livenessProbe.httpGet.scheme: HTTPS).
Better approach: Add a first-class tls section to the chart values instead of relying on extraArgs for TLS:
tls:
enabled: false
secretName: ""
mountPath: /etc/tlsWhen tls.enabled is true, the chart template would automatically:
- Inject
--tls-certand--tls-keyargs - Mount the TLS secret as a volume
- Set
scheme: HTTPSon health probes
This is the standard pattern used by most Helm charts (cert-manager webhook, ingress-nginx, etc.) and provides a much better UX — users just set tls.enabled: true and tls.secretName: my-cert.
The extraArgs value is still useful as a generic escape hatch for other flags.
|
|
||
| // TLS configuration for the HTTP server | ||
| // TLSCert is the path to the TLS certificate file for HTTPS | ||
| TLSCert string `toml:"tls_cert,omitempty"` |
There was a problem hiding this comment.
These new TOML options should also be documented in docs/configuration.md. Other similar fields like certificate_authority and server_url are already documented in the configuration reference table there.
| return fmt.Errorf("certificate-authority must be a valid file path: %w", err) | ||
| } | ||
| } | ||
| // Validate TLS configuration |
There was a problem hiding this comment.
Nit: TLS is only meaningful in HTTP mode (when port is set). Consider also validating that port is configured when TLS cert/key are provided:
if tlsCert != "" && m.StaticConfig.Port == "" {
return fmt.Errorf("--tls-cert and --tls-key require --port to be set (TLS is only supported in HTTP mode)")
}This would give users a clear early error instead of silently ignoring the TLS config in STDIO mode.
manusa
left a comment
There was a problem hiding this comment.
Hi, thx for taking care of the previous comments.
I reported a few more minor things 😅 otherwise, I think we should be ready to go 🚀
|
👋 @mjudeikis — new review comments from @manusa just landed (2026-02-26). Flagging as you asked: 3 items need attention:
All three are blocking or near-blocking for merge. Helm probe fix is the most critical. |
Add tls service certs, allowing to provide TLS certificates to the server itself.
This allows usages:
In general simplified configuration when neither ingress or gateway is available