Skip to content

Expose /debug/pprof on operator and metrics-exporter HTTP servers - #2017

Closed
dentiny wants to merge 1 commit into
Altinity:0.27.2from
dentiny:hjiang/register-pprof
Closed

Expose /debug/pprof on operator and metrics-exporter HTTP servers#2017
dentiny wants to merge 1 commit into
Altinity:0.27.2from
dentiny:hjiang/register-pprof

Conversation

@dentiny

@dentiny dentiny commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Closes #2016

Hi team, I'd like to register pprof endpoint for profiling purpose, which is useful to understand production issues such as OOM.

@dentiny
dentiny force-pushed the hjiang/register-pprof branch from 95b7460 to 58fc138 Compare June 30, 2026 00:23
@dentiny
dentiny marked this pull request as ready for review June 30, 2026 00:25
@sunsingerus sunsingerus added the planned for review This feature is planned for review label Jul 7, 2026

@sunsingerus sunsingerus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @dentiny — enabling profiling to debug OOM (#2016) is genuinely useful and something we want. I have to request changes, though, because as written this exposes /debug/pprof in a way that is a security regression for every existing install. Details and a recommended path below.

Why this can't merge as-is

import _ "net/http/pprof" runs the package init(), which registers /debug/pprof/* on http.DefaultServeMux. Both HTTP servers here serve that mux via the nil handler:

  • pkg/metrics/operator/machinery.go:110 — operator metrics, default :9999, bound 0.0.0.0 (all interfaces) unconditionally.
  • pkg/metrics/clickhouse/rest_server.go:207 — metrics-exporter, default :8888, 0.0.0.0 in the default (Plain) IPC mode.

Both ports are published cluster-wide by the clickhouse-operator-metrics Service and scraped by Prometheus, and we ship no NetworkPolicy. So after this change any pod in any namespace can reach /debug/pprof on the operator, unauthenticated:

  • Info disclosure/debug/pprof/heap and /allocs dump live operator memory, which holds ClickHouse credentials, IPC tokens, and Kubernetes API bearer tokens; /cmdline leaks all flags; /goroutine?debug=2 leaks full stacks.
  • DoS/debug/pprof/profile?seconds=N and /trace?seconds=N take an attacker-controlled duration and share the single listener/mux with /metrics, degrading the reconcile loop.
  • Bypasses IPC Secure mode — the token + loopback middleware wraps only the /chi handler; pprof is registered at the mux root, outside that protection, even in Secure mode.
  • On by default — a blank import active at init(), with no flag/config/opt-in, so every install silently gains this endpoint on upgrade.

A "just use a separate mux" fix is not sufficient

Moving pprof to a private *http.ServeMux alone does not help: net/http/pprof's init() registers on the global DefaultServeMux regardless of whether the import is blank or named, so as long as machinery.go / rest_server.go serve the nil handler they keep exposing it. (Verified: a named import still leaves DefaultServeMux responding 200 on /debug/pprof/.)

Recommended approach for a mergeable version

  1. Don't register on DefaultServeMux. Simplest is to use runtime/pprof (no init() side-effect) to serve heap/allocs/goroutine — the OOM-relevant profiles — from a private mux. Alternatively keep net/http/pprof but convert the :8888/:9999 servers to explicit, non-nil muxes so the polluted default mux is never served.
  2. Bind loopback-only (127.0.0.1), never on the metrics ports and never behind the Service — reachable via kubectl port-forward.
  3. Default OFF, opt-in via a flag, consistent with the existing -debug / -fips-info flags.
  4. Consider omitting profile/trace (the CPU/trace DoS vectors) unless separately gated, and log a warning when pprof is enabled so it is visible in kubectl logs.

Happy to review a revised version along these lines — thanks again for tackling the OOM-debuggability gap.

@sunsingerus sunsingerus added changes requested waiting reply Altinity is waiting for a reply and removed planned for review This feature is planned for review labels Jul 8, 2026
Signed-off-by: dentinyhao <dentinyhao@gmail.com>
@dentiny
dentiny force-pushed the hjiang/register-pprof branch from 58fc138 to 5f6c917 Compare July 14, 2026 20:55
@dentiny

dentiny commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Thank you so much for the detailed review! I made a few changes in the updated commit:

  • runtime/pprof is used instead of net/http/pprof, so it does not auto-register anything on http.DefaultServeMux
  • Forced pprof to bind only to loopback via validateLoopbackAddress
  • Operator flags is added to decide whether to enable profiling and which port to use
  • Explicit private muxes is used instead of global one

@dentiny
dentiny requested a review from sunsingerus July 14, 2026 20:58
@sunsingerus sunsingerus added planned for review This feature is planned for review and removed waiting reply Altinity is waiting for a reply labels Jul 16, 2026
@sunsingerus

Copy link
Copy Markdown
Collaborator

This PR does not even compile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested planned for review This feature is planned for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants