fix: disable probe and metrics endpoints by default#10
Conversation
The default values for --health-probe-bind-address and --metrics-bind-address were ":0", which in controller-runtime means "bind to a random free port" rather than "disabled". Because the chart runs cozy-proxy with hostNetwork=true, these showed up as two unexpected high-port listeners on the host. Switch the defaults to "0", the controller-runtime convention for disabling the server. Users who want health probes or metrics can pass an explicit bind address. Refs: #5 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the default bind addresses for the health probe and metrics endpoints to "0", allowing them to be disabled. The review feedback suggests improving the help text by providing example address formats and ensuring consistent pluralization for the metrics endpoint description.
| var metricsAddr string | ||
| flag.StringVar(&probeAddr, "health-probe-bind-address", ":0", "The address the probe endpoint binds to.") | ||
| flag.StringVar(&metricsAddr, "metrics-bind-address", ":0", "The address the metric endpoint binds to.") | ||
| flag.StringVar(&probeAddr, "health-probe-bind-address", "0", "The address the probe endpoint binds to. Set to \"0\" to disable.") |
There was a problem hiding this comment.
Consider adding an example of the expected address format (e.g., :8081) to the help text. Since the default is now the special value "0", providing an example helps users understand that they should provide a :port string to enable the endpoint.
| flag.StringVar(&probeAddr, "health-probe-bind-address", "0", "The address the probe endpoint binds to. Set to \"0\" to disable.") | |
| flag.StringVar(&probeAddr, "health-probe-bind-address", "0", "The address the probe endpoint binds to (e.g., :8081). Set to \"0\" to disable.") |
| flag.StringVar(&probeAddr, "health-probe-bind-address", ":0", "The address the probe endpoint binds to.") | ||
| flag.StringVar(&metricsAddr, "metrics-bind-address", ":0", "The address the metric endpoint binds to.") | ||
| flag.StringVar(&probeAddr, "health-probe-bind-address", "0", "The address the probe endpoint binds to. Set to \"0\" to disable.") | ||
| flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metric endpoint binds to. Set to \"0\" to disable.") |
There was a problem hiding this comment.
Consider adding an example of the expected address format (e.g., :8080) and using the plural 'metrics' in the help text to maintain consistency with the flag name (--metrics-bind-address).
| flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metric endpoint binds to. Set to \"0\" to disable.") | |
| flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to (e.g., :8080). Set to \"0\" to disable.") |
Summary
Change the defaults for
--health-probe-bind-addressand--metrics-bind-addressfrom":0"(bind to a random free port) to"0"(disabled), which is the controller-runtime convention for turning off the server.Why
netstaton a node running cozy-proxy showed two unexpected high-port listeners:Because the chart runs cozy-proxy with
hostNetwork: true, those ports were exposed directly on the host. Users who want health probes or metrics can still opt in by passing an explicit bind address via flag.Verified against controller-runtime v0.20.1:
pkg/metrics/server/server.go):BindAddress == "0"skips server creation entirely.pkg/manager/manager.go):addr == "" || addr == "0"returns a nil listener, so no probe server is started.":0"value instead fell through tonet.Listen("tcp", ":0"), which is exactly what produced the random high ports.Test plan
netstat -ltnpon the node shows no cozy-proxy listeners--health-probe-bind-address=:8081and confirm the probe endpoint comes up on that port--metrics-bind-address=:8080and confirm metrics are servedFixes #5