Serve Prometheus metrics on a separate diagnostics listener

[amirejaz]

Summary

An external good-faith security report against a toolhive-doc-mcp deployment
flagged the proxy diagnostics endpoints. Finding E of #6271: /metrics is registered
as an explicit ServeMux path, and since Go resolves the most specific pattern first
it always outranks the / catch-all, so it shares the port that serves MCP traffic.

In Kubernetes that port is the one deployments route publicly — the operator binds
the proxy to 0.0.0.0 (mcpserver_runconfig.go) and the Service maps it — so
enabling metrics put an unauthenticated endpoint on a publicly routable listener,
exposing tool names, MCP method names, client names, and traffic volumes.

What this changes, precisely (thanks @JAORMX for pushing on the original framing,
which overstated it): the fix does not authenticate, rate limit, or audit
/metrics. The diagnostics listener carries no middleware by design, and the operator
binds it to 0.0.0.0, so the endpoint is just as unauthenticated on its own port and
remains reachable from other pods.

What it does buy is control by port. Kubernetes NetworkPolicy matches on pods,
ports, and protocols and cannot filter on HTTP path, so while /metrics shares the
transport port there is no way to express "allow MCP traffic, deny metrics scraping" —
any policy permitting MCP clients also permits scraping. On its own port that becomes
expressible, and the safe outcome no longer depends on every deployment getting its
route rules right. Route-level controls (Gateway API, Ingress path rules) close the
north-south half but leave pod-to-pod traffic untouched. This is why etcd
(--listen-metrics-urls), controller-runtime (--metrics-bind-address), and the
ToolHive operator itself all put metrics on a separate port.

• Bind /metrics to a dedicated diagnostics listener (pkg/diagnostics). The runner
  no longer passes the Prometheus handler to the transport — that omission is what
  stops the proxies mounting /metrics on the application mux at all.
• Return 404 for /metrics on the application listener, mirroring how /health
  already guards itself, so the path is not silently proxied to the backend instead.
• Default to port 9464, the OpenTelemetry spec's Prometheus exporter default
  (OTEL_EXPORTER_PROMETHEUS_PORT), falling back to an available port when taken.
• Leave /health on the application listener: Kubernetes probes target it, and Drop build fingerprint from proxy /health response #6280
  already removed its build fingerprint.

/metrics is off by default at every layer, so this only affects deployments that
explicitly enabled it.

Part of #6271

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

New coverage:

• pkg/diagnostics/server_test.go — construction validation, metrics served on its
  own listener, only /metrics served (no catch-all), start-twice, idempotent stop.
• pkg/runner/diagnostics_test.go — the core property: with metrics enabled the
  listener binds a port distinct from the application port; plus port resolution,
  host defaulting, and nil-telemetry-config handling.
• pkg/transport/proxy/transparent/transparent_test.go — the reachability half:
  with no handler supplied, /metrics returns 404 on the application listener and
  does not fall through to the backend.

All touched packages pass under -race. go vet ./pkg/... ./test/e2e/... is clean.

Two caveats, both pre-existing and unrelated:

• task lint-fix reports one gosec G115 finding in cmd/thv/app/upgrade.go:204,
  which this PR does not touch.
• pkg/plugins/pluginsvc currently fails TestValidateOCIRegistryHost and
  TestParseGitReference_RefAndSubdir locally. Both reproduce identically on a clean
  origin/main (3c4dec3) and appear host/DNS-dependent, so they look like flakes
  worth a separate look — flagging in case CI shows them here.

Three E2E tests that scraped /metrics on the proxy port were updated to target the
diagnostics port. I have not run task test-e2e locally (needs a container runtime);
please let CI exercise it.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

The new PrometheusPort field is additive and lives on pkg/telemetry.Config, which
is not embedded in the operator CRDs.

Changes

File	Change
pkg/diagnostics/server.go	New: diagnostics listener serving /metrics, with the rationale for why it must not share the app listener and a warning against adding debug handlers to it
pkg/runner/diagnostics.go	New: runner start/stop plus port resolution
pkg/runner/runner.go	Start the diagnostics listener instead of setting transportConfig.PrometheusHandler; release it on every exit from Run; stop it in Cleanup
pkg/telemetry/config.go	Add PrometheusPort; correct the now-wrong "served on the main transport port" doc
pkg/telemetry/middleware.go	Drop the startup log that reported the application port for /metrics
pkg/transport/proxy/{transparent,streamable,httpsse}	404 /metrics when no handler is supplied
docs/observability.md	Document the diagnostics port, port selection, and a cardinality warning

Does this introduce a user-facing change?

Yes, two changes.

1. /metrics moves to a dedicated port. For deployments with
enablePrometheusMetricsPath enabled, /metrics moves off the transport port onto a
dedicated diagnostics port (default 9464). Scrape configs pointed at the transport
port need updating, and the diagnostics port should be kept off any internet-facing
Service or Ingress and restricted with a NetworkPolicy.

This is a deliberate break: leaving the endpoint where it is means leaving it outside
the middleware chain. Worth a release note.

2. /metrics is no longer proxied to the backend. Previously, when metrics were
disabled, /metrics fell through to the catch-all and was forwarded to the backend
MCP server. It now returns 404 on the application listener. A remote MCP server that
exposes its own /metrics behind thv proxy would no longer be reachable at that
path. This mirrors how /health has always been shadowed, and it is what stops the
path silently reaching the backend now that no handler is registered — but it is a
behaviour change worth calling out.

Special notes for reviewers

I self-reviewed this before opening it for review and fixed four things; the commit
history separates the original change from the hardening pass.

The one worth knowing about: the diagnostics listener leaked on most of Run's exit
paths. Cleanup is reached only via stopMCPServer, which the early error returns
skip, as does the graceful container-exit branch that returns nil. Under
workloads.Manager's exponential-backoff restart loop, every failed attempt would
have stranded a goroutine and a bound port — and because 9464 stayed held, the next
attempt would silently land on a different port, breaking the stable scrape target
this PR exists to provide, in exactly the crash-looping scenario where metrics matter
most. Fixed with an unconditional defer (the stop is nil-safe and idempotent).

Also fixed: missing ReadTimeout/WriteTimeout/MaxHeaderBytes (this listener has
no middleware, so nothing else bounds a trickled body); a missing test for the 404
behaviour; and a package-doc warning against ever registering pprof here.

Three things I'd still like scrutiny on:

1. The port default. I chose a fixed 9464 over an auto-assigned port so
   Kubernetes scrapers have a predictable target — an arbitrary port would have broken
   in-cluster scraping with no way to fix it until a CRD field lands. The trade-off is
   that a second CLI workload on one machine falls back to a different port (logged at
   startup). Alternative considered: authenticating /metrics instead of moving it.
   Rejected because ToolHive's OIDC middleware cannot validate Kubernetes
   service-account tokens, so Prometheus could not scrape it; the TokenReview approach
   (kube-rbac-proxy / controller-runtime WithAuthenticationAndAuthorization) would be
   a Kubernetes-only auth path and more code.

2. Hard-failing Run if the diagnostics port cannot bind. FindOrUsePort falls
   back to an available port, so this needs a machine with no free ports — but there is
   a TOCTOU window between its check and our bind, so in principle a transient race
   could fail the workload over metrics. I kept the hard failure: the feature is
   opt-in, the failure is loud at startup, and silently degraded observability is
   exactly what Telemetry + diagnostics hardening: bound label length (OOM), overflow lock, tool/prompt cardinality, diagnostics bypass, /health fingerprint #6271 complains about. Happy to soften it to a warning if you disagree.

3. Dead capability left in place. The proxies still accept a prometheusHandler,
   but nothing supplies one now. I left the plumbing rather than mix a dead-code
   removal into a security fix — happy to strip it here or in a follow-up.

Follow-ups, not in scope here: vMCP (pkg/vmcp/server/server.go:600 registers its own
unauthenticated /metrics), an operator CRD field and CLI flag for the port, a
separate bind host for diagnostics, and surfacing the resolved port in workload status
rather than only the startup log.

I also added the cardinality warning to docs/observability.md that #6271 notes is
missing. Say the word if you'd rather that went in its own docs PR.

Generated with Claude Code

[Copilot]

Pull request overview

This PR addresses a security gap where /metrics could bypass the proxy middleware chain (auth/body limits/rate limiting/audit) due to ServeMux longest-match behavior, by moving Prometheus metrics to a dedicated diagnostics listener and ensuring /metrics is never proxied to backends when disabled.

Changes:

• Add a dedicated diagnostics HTTP server (pkg/diagnostics) to serve only /metrics on a separate listener (default port 9464, with fallback when taken).
• Update runners and proxies so transports no longer mount a Prometheus handler on the application mux, and return 404 for /metrics when no handler is supplied.
• Update telemetry config/docs and adjust unit/e2e tests for the new diagnostics listener behavior.

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/e2e/telemetry_middleware_e2e_test.go	Updates best-effort metrics probing to prefer the diagnostics default port and use diagnostics.MetricsPath.
test/e2e/telemetry_metrics_validation_e2e_test.go	Updates metrics URL construction to target diagnostics listener (currently assumes default port).
test/e2e/osv_authz_test.go	Updates metrics URL construction to target diagnostics listener (currently assumes default port).
pkg/transport/proxy/transparent/transparent_test.go	Adds regression test ensuring /metrics is 404 and not proxied to backend when no handler is supplied.
pkg/transport/proxy/transparent/transparent_proxy.go	Returns 404 for /metrics when no Prometheus handler is provided; adds rationale comments.
pkg/transport/proxy/streamable/streamable_proxy.go	Returns 404 for /metrics when no Prometheus handler is provided.
pkg/transport/proxy/httpsse/http_proxy.go	Returns 404 for /metrics when no Prometheus handler is provided.
pkg/telemetry/middleware.go	Stops logging the application port for /metrics; keeps setting Prometheus handler on the runner.
pkg/telemetry/middleware_test.go	Updates expectations to avoid reading the application port during Prometheus handler wiring.
pkg/telemetry/config.go	Adds PrometheusPort and updates docs to reflect dedicated diagnostics listener behavior.
pkg/runner/runner.go	Starts/stops the diagnostics server from Run/Cleanup and stops passing handler to transport config.
pkg/runner/diagnostics.go	Implements runner start/stop logic for the diagnostics server and port selection behavior.
pkg/runner/diagnostics_test.go	Adds unit tests ensuring metrics bind on a separate port, host defaulting, and idempotent shutdown.
pkg/diagnostics/server.go	Introduces the diagnostics server implementation with timeouts and port fallback logic.
pkg/diagnostics/server_test.go	Adds unit tests for validation, serving only /metrics, and stop/start semantics.
docs/server/swagger.yaml	Documents prometheusPort and updates Prometheus metrics exposure description.
docs/server/swagger.json	Generated API docs update for the telemetry config fields/descriptions.
docs/server/docs.go	Generated API docs template update for the telemetry config fields/descriptions.
docs/observability.md	Documents diagnostics port behavior, port selection, and cardinality warning.
docs/cli/thv_run.md	Updates CLI docs text to reflect metrics being on a dedicated diagnostics port.
cmd/thv/app/run_flags.go	Updates flag help text to reflect metrics moving off the transport port.

Files not reviewed (1)

• docs/server/docs.go: Generated file

---

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

[codecov]

Codecov Report

❌ Patch coverage is 74.01575% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.92%. Comparing base (3c4dec3) to head (edfa2e0).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/diagnostics/server.go	78.31%	13 Missing and 5 partials ⚠️
pkg/runner/runner.go	27.27%	5 Missing and 3 partials ⚠️
pkg/runner/diagnostics.go	76.00%	3 Missing and 3 partials ⚠️
pkg/transport/proxy/httpsse/http_proxy.go	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6296      +/-   ##
==========================================
+ Coverage   72.85%   72.92%   +0.06%     
==========================================
  Files         743      744       +1     
  Lines       77681    78289     +608     
==========================================
+ Hits        56596    57093     +497     
- Misses      17118    17203      +85     
- Partials     3967     3993      +26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

Wouldn't a better fix have been to only route the relevant routes via the Gateway API kubernetes resource? in the end, service everything on the same interface is not really a problem, the problem is exposing it, and this is already a solved problem with how kubernetes exposes routes. Seems to me like this is more of a documentation issue than a functional one.

[amirejaz]

You're right that the description oversells this, and that's my error to fix — the "bypasses the middleware chain" wording implies we restore auth, rate limiting and audit for /metrics, and we don't. The diagnostics listener has no middleware, and the operator binds it to 0.0.0.0, so /metrics stays unauthenticated and in-cluster reachable, just on 9464. The only real delta is that it's off the port deployments route publicly.

On the substantive question though — I don't think routing alone covers it. NetworkPolicy is L3/L4 and can't filter on path, so while /metrics shares the application port there's no way to allow MCP traffic and deny scraping. A separate port gives you that primitive; Gateway API only governs what reaches the gateway, not pod-to-pod traffic. Same reason etcd and controller-runtime put metrics on their own ports. So routing plus docs closes the north-south half and leaves east-west unaddressed.

Also worth noting we create a plain corev1.Service for MCPServer today — no Ingress or HTTPRoute — so route-scoping is either new operator work or docs-and-hope.

Your point does move the bar though: the justification is "port separation is enforceable and safe by default", not "fixes a bypass". If you don't think that earns a breaking change, better to settle it now before we build out the migration story.

[JAORMX]

@amirejaz I wasn't talking about a NetworkPolicy. I agree it's not enough if we'd only leverage that. But production deployments of MCP servers tend to use either an HTTPRoute (or several) or an Ingress resource. We should document that MCP servers should expose only the paths needed instead of a blanket / which is what a simple non-production grade deployment would do. That's what I meant.

[amirejaz]

Agreed, and we have no guidance on that today — the docs cover how to reach a workload externally but never say to scope the route to specific paths.

Worth deciding where it should live. The advice applies beyond metrics: /health and the .well-known endpoints stay on the transport port by design, and anything added to that mux later lands there too. So it's really "expose only the paths you need, not /" as general deployment guidance rather than something metrics-specific — happy to add a short section here, or do it separately if you'd rather not mix it into this change.

And to check I've got you — you're not arguing against the separate port, just that path-scoped routes should be documented alongside it?

[JAORMX]

Let's add the docs here just to keep it all in one place. and keep this change too.

[amirejaz]

Added in edfa2e003 — new "Scope external routes to the paths you need" section in docs/observability.md, right after the NetworkPolicy example, with a note that the two are separate controls (a policy governs which pods may connect, a route governs which paths are published).

The part I think is most useful is a table of what actually lives on the transport port and whether each path belongs on an external route, so nobody has to read the proxy source to work it out: /mcp for streamable-http, /sse + /messages for sse, /.well-known/oauth-protected-resource for RFC 9728 discovery, the embedded auth server routes only when that's enabled, and /health marked as internal since probes reach it in-cluster. Plus an HTTPRoute example publishing only the MCP endpoint and discovery, and a note that Ingress needs pathType: Exact/Prefix rather than a single / prefix.

One thing worth a second opinion: this puts general route-scoping advice in a telemetry doc. That's what you asked for and it does keep it together, but it may want moving to a deployment/hardening page later — happy either way.