Fix: Secure TLS for Prometheus metrics endpoint#120
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe metrics server now reads serving certs from an OpenShift-provided secret mounted at ChangesMetrics Server TLS Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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)
config/prometheus/monitor.yaml (1)
18-19: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAvoid hardcoding one rendered namespace into
serverName.The rest of this manifest set still uses the
systemplaceholder namespace, but Line 19 pins hostname verification tometrics.openstack-lightspeed-system.svc. That only works for one rendered namespace; installing into any other namespace will break TLS verification against the service-ca cert. Please driveserverNamefrom the same namespace transform/replacement path as the Service resource instead of baking in a single output.🤖 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 `@config/prometheus/monitor.yaml` around lines 18 - 19, The Prometheus TLS config is hardcoding a rendered namespace in the serverName value, which will break hostname verification when the manifest is installed into a different namespace. Update the serverName in the Prometheus config to use the same namespace substitution/replacement mechanism as the Service resource and the rest of this manifest set, so it is generated dynamically from the target namespace instead of being fixed to metrics.openstack-lightspeed-system.svc.
🤖 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/main.go`:
- Around line 113-116: The metrics server setup is missing the TLS policy from
tlsOpts, so only the serving cert paths are being applied. Update the metrics
server options in main to also set TLSOpts: tlsOpts alongside CertDir, CertName,
and KeyName so the metrics endpoint inherits the same TLS 1.3 minimum and HTTP/2
disablement as the webhook server.
---
Nitpick comments:
In `@config/prometheus/monitor.yaml`:
- Around line 18-19: The Prometheus TLS config is hardcoding a rendered
namespace in the serverName value, which will break hostname verification when
the manifest is installed into a different namespace. Update the serverName in
the Prometheus config to use the same namespace substitution/replacement
mechanism as the Service resource and the rest of this manifest set, so it is
generated dynamically from the target namespace instead of being fixed to
metrics.openstack-lightspeed-system.svc.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9dc39c-bf0c-4d32-8c47-657fb3555182
📒 Files selected for processing (6)
cmd/main.goconfig/default/metrics_service.yamlconfig/manager/manager.yamlconfig/prometheus/ca-bundle-configmap.yamlconfig/prometheus/kustomization.yamlconfig/prometheus/monitor.yaml
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
2 times, most recently
from
073deac to
845c723
Compare
| # keyFile: /etc/metrics-certs/tls.key | ||
| insecureSkipVerify: true | ||
| caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt | ||
| serverName: metrics.system.svc |
There was a problem hiding this comment.
Hey! Just a heads-up that Kustomize won't automatically update this serverName field, leaving it stuck as metrics.system.svc. Because the actual service name changes after rendering, Prometheus will see a domain mismatch and TLS verification will fail. We'll need to either hardcode the final rendered name or use a Kustomize replacements block to inject it dynamically.
There was a problem hiding this comment.
Thanks Omkar for catching that, you are correct. I have since mad ethe correction. Cheers
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
66039c3 to
f09cae8
Compare
|
Thanks for the changes , LGTM:) |
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Nice start!:) Couple of changes required 🙈
| CertDir: "/tmp/k8s-metrics-server/serving-certs", | ||
| CertName: "tls.crt", | ||
| KeyName: "tls.key", |
There was a problem hiding this comment.
suggestion (blocking): Can we make it please so that these values are not hardcoded but passed to the operator's code via parameters? [1]
There was a problem hiding this comment.
Thanks for review!
I've implemented your suggestion to parameterise the cert paths.
|
/retest |
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
130ebec to
82a32b8
Compare
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
82a32b8 to
60ab3d0
Compare
|
/retest |
1 similar comment
|
/retest |
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
d1c6cb4 to
41737d2
Compare
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
3 times, most recently
from
8a82f38 to
57404d5
Compare
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
The blocking comment should be resolved before we merge:). Maybe something to discuss within the team.
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
5 times, most recently
from
58fe00f to
e6e96bd
Compare
Implement HTTPS serving for the operator's /metrics endpoint using OpenShift service-ca for automatic certificate management. Changes: - Add service-ca annotation to metrics Service for automatic cert generation - Mount certificate Secret in operator Pod - Configure metrics server to use service-ca certificates (TLS 1.3, HTTP/2 disabled) - Parameterize certificate paths via command-line flags (--cert-dir, --cert-name, --key-name) - Regenerate bundle manifests Note: ServiceMonitor TLS configuration (Prometheus scraping) will be handled in a follow-up ticket, as ../prometheus is currently commented out in the deployment. Fixes: OSPR-30557
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
e6e96bd to
4e0116a
Compare
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
/lgtm
The /metrics endpoint now works solely over HTTPS (tested with curl). Let's see how to jobs turn out but I think it should be alright.
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks Malinga! The patch looks quite straight forward now. I like the "always on TLS approach" as it feels production ready. So /lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lpiwowar, malingatembo, umago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks Team for all the reviews <3 |
4 participants
Followed pattern from
openstack-k8s-operators/telemetry-operatorCommit: 6111e80d by Jaromir Wysoglad (2024-02-28)
TLS implementation for Prometheus metrics in OpenShift
How adapted here
Jaromir's approach: Optional TLS via API (flexible, supports cert-manager or service-ca)
Our approach: Always-on TLS with service-ca only
Core pattern (5 components):
Result:
same security outcome as reference implementation
simpler
automatic certificate management
Files Changed
6 source files + 4 auto-generated bundle files (via make bundle)
Summary by CodeRabbit
Summary
New Features
Bug Fixes