fix(valkey-operator): align ServiceMonitor with metrics.secure (skip-verify opt-in) - #231
fix(valkey-operator): align ServiceMonitor with metrics.secure (skip-verify opt-in)#231daanvinken wants to merge 4 commits into
Conversation
|
@daanvinken please rebase the branch |
When metrics.secure is true the metrics Service exposes port https, but the ServiceMonitor always targeted port http so scrapes failed. Use the matching port name, default scheme https, and insecureSkipVerify for the operator self-signed cert unless serviceMonitor overrides are set. Signed-off-by: daanvinken <daanvinken@tythus.com>
…itor Keep port/scheme aligned with metrics.secure. Do not default skip-verify; add metrics.serviceMonitor.insecureSkipVerify (false by default). Explicit tlsConfig still wins. Document secure scrape setup in the README. Signed-off-by: daanvinken <daanvinken@tythus.com>
0.4.1 already shipped on main with the log level description change. Move the ServiceMonitor secure-metrics fix notes to 0.4.2. Signed-off-by: daanvinken <daanvinken@tythus.com>
2db45ce to
d3dd681
Compare
|
Check |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
When metrics.secure is true, send the scraper ServiceAccount token (bearerTokenFile) so protected /metrics does not 401. Allow authorization or bearerTokenSecret overrides. Add helm unittest coverage for secure port, scheme, TLS precedence, skip-verify, and auth. Testing - helm lint ./valkey-operator - helm unittest ./valkey-operator Signed-off-by: daanvinken <daanvinken@tythus.com>
📝 WalkthroughWalkthroughThe chart now supports secure ServiceMonitor endpoints with HTTPS defaults, configurable authentication, TLS verification controls, precedence rules, Helm tests, documentation, and version 0.4.2 metadata. ChangesSecure ServiceMonitor behavior
Sequence Diagram(s)sequenceDiagram
participant Values as ServiceMonitor values
participant Template as ServiceMonitor template
participant Prometheus
Values->>Template: Provide scheme, authentication, and TLS settings
Template->>Prometheus: Render endpoint configuration
Prometheus->>Template: Scrape the configured metrics port
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
valkey-operator/README.md (1)
216-228: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winState the reader-binding prerequisite in the custom-CA example.
When this block is copied without an existing binding,
metrics.reader.binding.createremainsfalse. The ServiceMonitor still sends a token, but the Prometheus ServiceAccount is not bound tometrics-reader, so scrapes return401. Add the binding block or state that this example assumes a pre-existing binding.🤖 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 `@valkey-operator/README.md` around lines 216 - 228, Update the custom-CA configuration example near the metrics ServiceMonitor settings to include the required metrics.reader.binding.create configuration, or explicitly state that it assumes a pre-existing binding. Ensure copied configuration binds the Prometheus ServiceAccount to metrics-reader so authenticated scrapes do not return 401.
🤖 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 `@valkey-operator/README.md`:
- Around line 76-79: The ServiceMonitor configuration must prevent credentials
from being sent over HTTP when metrics.secure is true. Update the scheme
validation or credential-rendering logic around metrics.serviceMonitor.scheme,
bearerTokenFile, bearerTokenSecret, and authorization so an explicit http scheme
either fails validation or omits all credential fields in secure mode; preserve
HTTPS credential behavior. Add a Helm test covering this case and document the
restriction in the README.
---
Nitpick comments:
In `@valkey-operator/README.md`:
- Around line 216-228: Update the custom-CA configuration example near the
metrics ServiceMonitor settings to include the required
metrics.reader.binding.create configuration, or explicitly state that it assumes
a pre-existing binding. Ensure copied configuration binds the Prometheus
ServiceAccount to metrics-reader so authenticated scrapes do not return 401.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0244a443-b5db-485b-b733-573507f9a195
📒 Files selected for processing (6)
valkey-operator/CHANGELOG.mdvalkey-operator/Chart.yamlvalkey-operator/README.mdvalkey-operator/templates/servicemonitor.yamlvalkey-operator/tests/servicemonitor_test.yamlvalkey-operator/values.yaml
| tlsConfig: | ||
| {{- toYaml . | nindent 6 }} | ||
| {{- toYaml .Values.metrics.serviceMonitor.tlsConfig | nindent 6 }} | ||
| {{- else if and .Values.metrics.secure .Values.metrics.serviceMonitor.insecureSkipVerify }} |
There was a problem hiding this comment.
When metrics.secure=true and the ServiceMonitor is enabled, this renders an HTTPS endpoint without tlsConfig. The chart documents that the operator uses a self-signed metrics certificate, while insecureSkipVerify defaults to false, so Prometheus rejects the TLS handshake unless the user separately supplies a CA or enables skip verification. Make the generated configuration trust the certificate by default, or require and document an explicit CA-backed TLS configuration as part of enabling the secure ServiceMonitor.
Artifacts
Secure ServiceMonitor Helm render harness source
- Exact executable harness used to render the baseline and PR charts with Helm 3.19.4, inspect endpoints, and assert override precedence; it provides the reproducible check.
Baseline secure ServiceMonitor render output
- Helm 3.19.4 output for the parent chart at commit 01b4fa4 with secure metrics and the ServiceMonitor enabled; it shows the pre-PR monitor did not target HTTPS.
PR secure ServiceMonitor render output
- Helm 3.19.4 output for the PR chart with secure defaults and explicit override cases; it shows default TLS trust is absent while requested overrides render correctly.
CI-pinned Helm unit test output
- Observed output from the CI-pinned Helm 3.19.4 and helm-unittest 1.0.2 run; all 71 chart tests passed, including ServiceMonitor template tests.
There was a problem hiding this comment.
This is intentional.
metrics.secure means the operator serves metrics over HTTPS with authn/authz. It should not also mean "Prometheus, skip certificate verification." That would mix "encrypted channel" with "do not check the peer," which is easy to miss in production.
The operator's stock metrics cert is self-signed, so a scrape that only sets secure + serviceMonitor.enabled is not fully turnkey until you either:
- set
metrics.serviceMonitor.insecureSkipVerify: true(common for the self-signed case), or - set a real
tlsConfig(CA / cert), or - replace the metrics serving cert with one Prometheus already trusts.
That is documented in the README example (secure + reader binding + skip-verify). We prefer an explicit one-liner over baking skip-verify into the default secure path.
There was a problem hiding this comment.
Understood — that distinction is exactly right. Enabling HTTPS transport and disabling cert verification are separate concerns, and conflating them silently would be the worse default. The explicit opt-in via insecureSkipVerify: true is the correct model here.
Withdrawing the comment.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| {{- if .Values.metrics.serviceMonitor.scheme }} | ||
| scheme: {{ .Values.metrics.serviceMonitor.scheme }} | ||
| {{- else if .Values.metrics.secure }} | ||
| scheme: https | ||
| {{- end }} |
There was a problem hiding this comment.
Maybe a nit; would this pattern be better to match what you're doing on line 19?
| {{- if .Values.metrics.serviceMonitor.scheme }} | |
| scheme: {{ .Values.metrics.serviceMonitor.scheme }} | |
| {{- else if .Values.metrics.secure }} | |
| scheme: https | |
| {{- end }} | |
| {{- if .Values.metrics.serviceMonitor.scheme }} | |
| scheme: {{ .Values.metrics.serviceMonitor.scheme }} | |
| {{- else }} | |
| scheme: {{ if .Values.metrics.secure }}https{{ else }}http{{ end }} | |
| {{- end }} |
4 participants
Summary
Fixes ServiceMonitor scrapes when
metrics.secureis true, and makes TLS skip-verify opt-in rather than a silent default.Bug
With
metrics.secure: true, the metrics Service exposes porthttps, but the ServiceMonitor always targeted porthttp, so scrapes never hit the right endpoint.What we change
metrics.secure(https+ defaultscheme: httpswhen secure andserviceMonitor.schemeis unset).metrics.serviceMonitor.insecureSkipVerify(defaultfalse). Only when this is true (andtlsConfigis empty) do we injecttlsConfig.insecureSkipVerify: truefor the operator's self-signed metrics cert. ExplicitserviceMonitor.tlsConfigalways wins.Design question for reviewers
Should skip-verify stay disabled by default?
false(this PR)insecureSkipVerify: trueor a realtlsConfig(CA). No silent MITM-friendly default.trueWe prefer default false: "secure" should not imply "do not verify." The operator cert is self-signed, so skip-verify is still available as an explicit one-liner for the common case.
If maintainers prefer out-of-the-box scrapes, flipping the default to
trueis a one-line change; the important part is that it is named and documented, not hard-coded only in the template.Docs
README (Metrics / ServiceMonitor), values comments, and CHANGELOG 0.4.1 describe port/scheme alignment and the opt-in flag.
Not in this PR
Bearer token for secure metrics auth (
WithAuthenticationAndAuthorization) is a separate follow-up so scrapes do not get 401 after TLS is fixed.Testing