CNTRLPLANE-3973: Make sure unencrypted metrics bind to localhost only, and remove dead code - #6350
CNTRLPLANE-3973: Make sure unencrypted metrics bind to localhost only, and remove dead code#6350vincentdephily wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@vincentdephily: This pull request references CNTRLPLANE-3973 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vincentdephily The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughMetrics listeners now bind to localhost by default, reject non-loopback addresses, and run without configurable TLS settings. Controller and daemon startup wiring plus deployment manifests were updated to use the simplified listener configuration. ChangesMetrics Listener Simplification
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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 |
The `/metrics` endpoint is set up to be served unencrypted on localhost:8797 and proxied on *:9637 via kube-rbac-proxy. There were attempts to serve encrypted metrics in 0e8aab3 and 3db3e26, but neither actually switched to `ListenAndServeTLS()`. Don't try to serve TLS: just depend on the proxy and make sure that we're only binding to localhost. CNTRLPLANE-3973: machine-config-operator not respecting global TLS Profile
ea37bf1 to
10482b0
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/common/metrics.go`:
- Around line 146-152: Update the validation logging around net.SplitHostPort
and the loopback check to avoid including the operator-configured addr value,
which may expose internal hostnames. Log only generic validation-failure
messages or a redacted representation, while preserving the existing early
return and localhost enforcement behavior.
- Around line 163-166: Update the HTTP server setup in the metrics listener to
configure appropriate request deadlines, including ReadHeaderTimeout, and
replace the unbounded Shutdown(context.Background()) call with a context.Context
that has a finite timeout. Ensure shutdown uses that bounded context and is
properly canceled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 613fc20f-0f64-45bb-a4ed-7c42252de4bf
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gocmd/machine-config-daemon/start.gomanifests/machineconfigcontroller/deployment.yamlmanifests/machineconfigdaemon/daemonset.yamlpkg/controller/common/metrics.go
💤 Files with no reviewable changes (2)
- manifests/machineconfigdaemon/daemonset.yaml
- manifests/machineconfigcontroller/deployment.yaml
| host, _, err := net.SplitHostPort(addr) | ||
| if err != nil { | ||
| klog.Errorf("invalid metrics listen address %q: %v", addr, err) | ||
| return | ||
| } | ||
| if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() { | ||
| klog.Errorf("metrics is listening on %q, it should listen on localhost and be exposed via a TLS proxy", addr) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid logging raw rejected listener addresses.
addr is operator-configurable; invalid values may contain internal hostnames and are written directly to logs. Redact the address or log only the validation failure.
As per coding guidelines, “Flag logging that may expose … internal hostnames.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/common/metrics.go` around lines 146 - 152, Update the
validation logging around net.SplitHostPort and the loopback check to avoid
including the operator-configured addr value, which may expose internal
hostnames. Log only generic validation-failure messages or a redacted
representation, while preserving the existing early return and localhost
enforcement behavior.
Source: Coding guidelines
| klog.Infof("Starting metrics listener on %s", addr) | ||
| mux := http.NewServeMux() | ||
| mux.Handle("/metrics", promhttp.Handler()) | ||
| s := http.Server{ | ||
| TLSConfig: tlsConfig, | ||
| TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler)), | ||
| Addr: addr, | ||
| Handler: mux} | ||
| s := http.Server{Addr: addr, Handler: mux} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound request and shutdown time.
Without ReadHeaderTimeout, a same-pod client can retain connections while sending headers indefinitely; Shutdown(context.Background()) can then also wait forever. Configure server deadlines and use a bounded shutdown context.
As per path instructions, “context.Context for cancellation and timeouts.”
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 165-165: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{Addr: addr, Handler: mux}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/common/metrics.go` around lines 163 - 166, Update the HTTP
server setup in the metrics listener to configure appropriate request deadlines,
including ReadHeaderTimeout, and replace the unbounded
Shutdown(context.Background()) call with a context.Context that has a finite
timeout. Ensure shutdown uses that bounded context and is properly canceled.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/machine-config-controller/start.go`:
- Around line 59-60: The TLS flags are missing before MarkDeprecated is called,
and both errors are discarded. In cmd/machine-config-controller/start.go lines
59-60 and cmd/machine-config-daemon/start.go lines 60-61, register compatible
no-op flags named tls-cipher-suites and tls-min-version, then handle the errors
returned by each MarkDeprecated call so both binaries continue accepting and
deprecating these flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6427b432-15c3-47ae-9efd-906039959a1e
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gocmd/machine-config-daemon/start.gomanifests/machineconfigcontroller/deployment.yamlmanifests/machineconfigdaemon/daemonset.yamlpkg/controller/common/metrics.go
💤 Files with no reviewable changes (1)
- manifests/machineconfigdaemon/daemonset.yaml
…C and MCD These flags were a noop in practice (see 42c60ca), and there is no need to support them. CNTRLPLANE-3973: machine-config-operator not respecting global TLS Profile
10482b0 to
f6a6dec
Compare
|
@vincentdephily: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/verified by @kaleemsiddiqu |
openshift-ci-robot
added
the
verified
|
@kaleemsiddiqu: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified remove |
openshift-ci-robot
removed
the
verified
|
@kaleemsiddiqu: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
3 participants
- What I did
/metricsendpoint only binds localhost (kube-rbac-proxy is responsible for exposing the TLS endpoint)- How to verify it
There should be no behaviour change apart from the deprecated cli flags, this is a code cleanup.
- Description for the changelog
Deprecated ineffective
--tls-cipher-suitesand--tls-min-versionflags ofmachine-config-controllerandmachine-config-daemon: the metrics TLS config is always fecthed from the APIServer.Summary by CodeRabbit
/metricsendpoint is now served localhost-only and unencrypted by default.