Serve vMCP metrics on a separate diagnostics listener

[amirejaz]

Summary

Virtual MCP registers /metrics on the mux that serves MCP traffic — under a comment
that acknowledged it was unauthenticated. That's the vMCP half of Finding E in #6271;
#6296 moved the proxy half. Until both move, the endpoint still shares the port
deployments route publicly.

• Serve /metrics on the dedicated diagnostics listener (pkg/diagnostics), so access
  can be governed by port. NetworkPolicy matches on pods, ports, and protocols and
  cannot filter on HTTP path, so while /metrics shares the MCP port there is no way
  to express "allow MCP traffic, deny metrics scraping".
• Honour the same migration switch as the proxy path, so this is not a breaking
  change on its own. While metricsOnTransportPort is on (the default), /metrics
  stays reachable on the MCP port too and a deprecation warning names it. Once it is
  off, the application mux 404s rather than letting /metrics fall through to the /
  MCP handler.
• Add Server.DiagnosticsAddress() so the resolved port is discoverable.

Stacked on #6370, which introduces the switch. Review that one first; this PR's
diff is against it, and it will be retargeted to main once #6370 merges.

What this does not do: it does not authenticate, rate limit, or audit /metrics.
The diagnostics listener carries no middleware, and the host is inherited from the MCP
listener (0.0.0.0 under the operator), so the endpoint stays reachable from other
pods. Restricting who can reach that port is what protects it — see the NetworkPolicy
example in docs/observability.md.

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)

New coverage in pkg/vmcp/server/diagnostics_test.go: no listener without telemetry,
metrics bound to a port distinct from the MCP port, configured port honoured, host
defaulting, idempotent stop. TestServeHandlerRegistersMetricsWhenTelemetryEnabled is
renamed to TestServeHandlerDoesNotServeMetrics and now asserts the 404, since its old
assertion described the behaviour this PR removes.

Three telemetry tests scraped /metrics off the MCP address and now go through
DiagnosticsAddress() — necessary rather than cosmetic, since the listener falls back
to an available port when the configured one is taken, so the address cannot be
constructed.

Remaining local failures (TestValidateOCIRegistryHost, TestParseGitReference_*) and
lint findings (cmd/thv/app/upgrade.go, pkg/vmcp/config/crd_cli_roundtrip_test.go)
are pre-existing on main in files this PR does not touch.

Does this introduce a user-facing change?

Not on merge. /metrics becomes available on the diagnostics port
(prometheusPort, default 9464) while remaining on the MCP port, so existing
scrape configurations keep working until the window closes.

When it does close, this is the half that matters more.

The blast radius is larger here than for the proxy half. Nothing shipped enables the
metrics path for MCPServer, but three vMCP artifacts do — examples/vmcp-config.yaml,
docs/operator/virtualmcpserver-api.md, and
docs/operator/virtualmcpserver-observability.md — so more deployments plausibly have
it on. The example and the observability guide are updated to point at the diagnostics
port.

This should ship in the same release as #6296 and #6370, so operators get one
migration event rather than the same feature moving twice. None of it has shipped —
v0.44.0 was cut before #6296 merged.

Special notes for reviewers

DiagnosticsAddress() is worth a look beyond vMCP: the resolved diagnostics port
currently only appears in a startup log, which is why the E2E metrics helpers in #6296
hardcode 9464 (Copilot flagged this, and those threads are still open). This accessor
is the vMCP-side answer; the equivalent for the proxy path — surfacing it in workload
status — is still outstanding.

TestServeHandlerMetricsOnTransportPort pins both ends of the migration — default and
explicitly-on serve on the MCP port, opted-out 404s — so neither the window nor the
cutover can regress unnoticed.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 87.03704% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.78%. Comparing base (b30ca2b) to head (a535b01).

Files with missing lines	Patch %	Lines
pkg/vmcp/server/diagnostics.go	87.50%	5 Missing ⚠️
pkg/vmcp/server/server.go	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6368      +/-   ##
==========================================
- Coverage   77.82%   77.78%   -0.05%     
==========================================
  Files         760      761       +1     
  Lines       72997    73044      +47     
==========================================
+ Hits        56808    56815       +7     
- Misses      16184    16224      +40     
  Partials        5        5              

☔ 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.

[jhrozek]

Read through this one carefully — the port split and the reasoning in pkg/diagnostics both look right to me. Three things I'd want changed before it goes in, all small. A few smaller nits (the local 127.0.0.1 constant, the unguarded diagnosticsServer field vs the exported DiagnosticsAddress, and freePort's find-then-release race in the new test) I'm happy to leave as follow-ups.

[jhrozek]

The three proxies all use diagnostics.NotServedHereHandler() here rather than a bare 404 (streamable_proxy.go:282, transparent_proxy.go:1334, http_proxy.go:300), and its doc comment spells out why:

A bare 404 is indistinguishable from a typo, and the failure is otherwise silent from the server side: an upgrade moves the endpoint and the operator sees only a Prometheus target going down.

vMCP would be the one place that doesn't get that, so once metricsOnTransportPort goes false an operator whose scraper is still pointed at the MCP port gets an empty 404 and has to know to grep the startup log. The handler puts that instruction in the body.

Suggested change

	mux.HandleFunc(diagnostics.MetricsPath, http.NotFound)
	mux.Handle(diagnostics.MetricsPath, diagnostics.NotServedHereHandler())

[jhrozek]

This runs after go s.httpServer.Serve(listener) and before close(s.ready), so if the diagnostics listener fails to bind we return from Start with the MCP listener still serving in a background goroutine and ready never closed — anything blocked on Ready() waits forever.

The runner avoids this by starting diagnostics before transport setup (runner.go:378), so there's nothing to unwind on that path. Could we move this call up above the net.Listen for parity? diagnostics.New's validation would then fail before anything is bound, and no cleanup code is needed.

Separate note: pkg/diagnostics argues that losing a port race shouldn't take down the whole workload (bind retries for exactly that reason). Turning an exhausted retry into a fatal vMCP startup error is a defensible call, but it's the opposite of what that comment assumes, so it'd be worth a line saying it's deliberate.

[jhrozek]

metricsOnTransportPort and prometheusPort are only reachable through the inline spec.config.telemetry path (which embeds telemetry.Config directly). Both are deliberately absent from MCPTelemetryConfig — the path this same doc labels "preferred" a few paragraphs up — with the reasoning written out in spectoconfig/telemetry_drift_test.go:74-82.

Since this section sits under both examples, a reader using telemetryConfigRef will try to set a field that doesn't exist. Worth one sentence saying which knob lives where, and that telemetryConfigRef users get 9464 plus the transport-port copy until the window closes.