feat(sidecar+coordinator): add support for HTTPS /metrics

[zdtsw]

What type of PR is this?
/kind feature

What this PR does / why we need it:

• Both the sidecar's and the coordinator's /metrics endpoint currently only support HTTP.
• Adds a --metrics-cert-dir flag to each, pointing to a directory with hardcoded file name tls.crt/tls.key. The flag matches the router's existing flag. Also settable as the sidecar YAML key metrics-cert-dir and as the coordinator's server.metrics_cert_dir (env COORDINATOR_SERVER_METRICS_CERT_DIR).
• If the flag is not set, both keep serving /metrics over HTTP.
• If the flag is set but the cert files are missing or invalid both sidecar and coordinator:shut down instead of serving (no fallback to HTTP). The coordinator exits non-zero; the sidecar logs the error and exits 0.
• Valid certs are hot-reloaded, so rotating tls.crt/tls.key doesn't need a restart.

Which issue(s) this PR fixes:

Fixes #

Release note (write NONE if no user-facing change):

Add --metrics-cert-dir to the disagg-sidecar and the coordinator to serve the /metrics endpoint over HTTPS. Missing or invalid certificate files stop the component at startup.

[Copilot]

🟡 Changes recommended

The updated docs/metrics.md section documents enabling TLS but does not describe the implemented “log error and disable /metrics with no HTTP fallback” behavior, which is operationally significant.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an opt-in TLS mode for the sidecar’s /metrics endpoint so operators can serve Prometheus metrics over HTTPS using a provided tls.crt/tls.key, while keeping the data-plane proxy’s TLS configuration independent.

Changes:

• Introduces --metrics-cert-path (and YAML metrics-cert-path) to enable HTTPS for /metrics when set.
• Updates the metrics server startup path to use TLS with cert hot-reload, and to log-and-disable metrics on startup failure (instead of failing the whole proxy).
• Adds tests and documentation updates for the new flag.

File summaries

File	Description
pkg/sidecar/proxy/proxy.go	Extends sidecar runtime config with MetricsCertPath.
pkg/sidecar/proxy/options.go	Adds CLI/YAML wiring for --metrics-cert-path.
pkg/sidecar/proxy/options_test.go	Adds precedence/merge tests for YAML vs CLI for the new option.
pkg/sidecar/proxy/MORIIO_README.md	Documents HTTPS behavior and no-fallback semantics for /metrics.
pkg/sidecar/proxy/dns_metrics.go	Implements HTTPS serving and cert reloading for the metrics server; logs errors without taking down the proxy.
pkg/sidecar/proxy/dns_metrics_test.go	Adds coverage for HTTP/TLS metrics serving and invalid cert path behavior.
docs/metrics.md	Documents the new --metrics-cert-path flag for the sidecar metrics endpoint.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[zdtsw]

makrdown link checked fixed in #2782

[ahg-g]

@zdtsw pls rebase

[roytman]

Hi @zdtsw, could you also add something similar to the coordinator metrics?

[zdtsw]

Hi @zdtsw, could you also add something similar to the coordinator metrics?

I would follow the same pattern as in router:

• if not set in cli or yaml: http
• if get validated cert in the path: https
• if failed validated cert in the path: coordinator startup failes , rather than fall back to http or only disable /metrics
  is this the correct appraoch? @roytman

[roytman]

Hi @zdtsw, could you also add something similar to the coordinator metrics?

I would follow the same pattern as in router:

* if not set in cli or yaml: http

* if get validated cert in the path: https

* if failed validated cert in the path: coordinator startup failes , rather than fall back to http or only disable /metrics
  is this the correct appraoch? @roytman

Yes, please.

[ahg-g]

@roytman is this good to go?

[roytman]

@roytman is this good to go?

I'll go over it tomorrow.

[roytman]

This is a good PR: clear docs, thorough tests on both the coordinator and sidecar sides (plain HTTP, TLS success, missing/invalid cert, and the drain-on-fatal-error path for the coordinator), and a sensible split in failure behavior between the two components.
A few small comments left inline.

One broader question, more about the existing convention than this specific PR: the certificate and key file names (tls.crt, tls.key) are hardcoded, and only their containing directory is configurable via --metrics-cert-dir (. A more common pattern (e.g. vLLM) is to configure the full paths to each file separately:

vllm serve <model> \\
    --ssl-keyfile /path/to/key.pem \\
    --ssl-certfile /path/to/cert.pem

I know this isn't something this PR introduces; the sidecar's data-plane TLS (--cert-path) and the Router's --cert-path/--metrics-cert-dir already follow the same directory-plus-fixed-names convention, so this PR stays consistent with what's already there. It's not an architectural constraint of the reload mechanism, though: common.NewCertReloader watches the directory with fsnotify, which fires on any file change regardless of name, but its reload step has "tls.crt"/"tls.key" hardcoded as literal strings (pkg/common/certs.go). Making those configurable would be a small change to that one helper, not a redesign.

Not asking to change this PR, but raising it as a question for the project: if we ever need to support certs that don't come from a single "one directory, standard names" mount (e.g. cert-manager output with different filenames, or split cert/key locations), would we want a --ssl-certfile/--ssl-keyfile-style option alongside or instead of the directory one?
Worth a separate issue/discussion if others think it's worth supporting, not something to resolve here.
CC: @ahg-g @elevran

[roytman]

Today this swallows every error serveMetrics can return: a broken --metrics-cert-dir and an unrelated --metrics-port bind conflict get exactly the same treatment: one log line, then the proxy carries on with no /metrics endpoint at all, indefinitely.

Concrete case: cert dir is misconfigured (e.g. tls.key missing from the mounted volume). The sidecar starts fine, serves inference traffic normally, and just silently has no /metrics. Nothing pages anyone; the only signal is one log line, unless something is specifically watching for it. Compared to the coordinator (or vllm itself, where /metrics isn't a separate server or listener), which refuses to start on the same misconfiguration, loud, immediate, shows up as CrashLoopBackOff, gets caught during rollout instead of during an incident three weeks later when someone goes looking for a metric that isn't there.

Before this PR, any serveMetrics error propagated through the shared errgroup in Run and stopped the whole proxy, data plane included. This change was presumably meant to stop a metrics-only problem from taking down inference serving, which is a reasonable goal, but the failure modes here (bad cert, bad port) only happen at startup; CertReloader handles cert rotation failures internally later on without erroring out, so nothing here can kill an already-running data plane.

Given that, I don't think there's much risk in just letting the error propagate again and failing the sidecar on any metrics startup error, TLS or not, the same as vllm and the coordinator do. That's simpler than adding a sentinel to special-case only TLS failures, and it means a broken --metrics-port doesn't go unnoticed either.

[roytman]

Nit: This builds the path with string concatenation (s.config.MetricsCertDir + "/tls.crt"), while the coordinator's equivalent (cmd/coordinator/main.go, metricsTLSConfig) uses filepath.Join(certDir, "tls.crt"). Both are plain string fields, so this isn't a type difference, just two different styles for the same thing in the same PR.

Functionally, it doesn't matter here: if MetricsCertDir has a trailing slash, concatenation produces a double slash (/etc/certs//tls.crt), but POSIX collapses repeated slashes during path resolution, so tls.LoadX509KeyPair and the fsnotify directory watch both resolve it identically either way. So this is a style/consistency nit, not a correctness issue, worth matching the coordinator's filepath.Join just so the two near-identical functions read the same way, not because the current code is broken.

[roytman]

This hand-rolls ECDSA key + cert generation from scratch. The repo already has a helper for exactly this - internal/tls.CreateSelfSignedTLSCertificate (importable from cmd/coordinator since internal/tls sits at the module root), and it's already being reused in this same PR in pkg/sidecar/proxy/dns_metrics_test.go's writeSelfSignedCert (call the helper, then re-PEM-encode cert.Certificate[0] / cert.PrivateKey). Would avoid a third from-scratch implementation of test cert generation in the codebase.

[roytman]

I've opened a follow-up issue: #2817

[zdtsw]

This is a good PR: clear docs, thorough tests on both the coordinator and sidecar sides (plain HTTP, TLS success, missing/invalid cert, and the drain-on-fatal-error path for the coordinator), and a sensible split in failure behavior between the two components. A few small comments left inline.

One broader question, more about the existing convention than this specific PR: the certificate and key file names (tls.crt, tls.key) are hardcoded, and only their containing directory is configurable via --metrics-cert-dir (. A more common pattern (e.g. vLLM) is to configure the full paths to each file separately:

vllm serve <model> \\
    --ssl-keyfile /path/to/key.pem \\
    --ssl-certfile /path/to/cert.pem

I know this isn't something this PR introduces; the sidecar's data-plane TLS (--cert-path) and the Router's --cert-path/--metrics-cert-dir already follow the same directory-plus-fixed-names convention, so this PR stays consistent with what's already there. It's not an architectural constraint of the reload mechanism, though: common.NewCertReloader watches the directory with fsnotify, which fires on any file change regardless of name, but its reload step has "tls.crt"/"tls.key" hardcoded as literal strings (pkg/common/certs.go). Making those configurable would be a small change to that one helper, not a redesign.

Not asking to change this PR, but raising it as a question for the project: if we ever need to support certs that don't come from a single "one directory, standard names" mount (e.g. cert-manager output with different filenames, or split cert/key locations), would we want a --ssl-certfile/--ssl-keyfile-style option alongside or instead of the directory one? Worth a separate issue/discussion if others think it's worth supporting, not something to resolve here. CC: @ahg-g @elevran

Thanks for the detail review!
I've made the updates to address your comments:

• change the behavior on sidecar for the negative case if cert file is invalid: shutdown sidecar instead of only disable metrics
• use filepath.Join() than concat strings
• reuse the existing helper function in test

[roytman]

/lgtm for this PR! I'd be happy to get thoughts on the broader questions as well.