[backport]: manifest servicemonitor cleanup + label selector for controller#44
Conversation
📝 WalkthroughWalkthroughRemoved a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/prometheus/servicemonitor.yaml`:
- Around line 14-17: The ServiceMonitor currently scrapes over plaintext
(scheme: http) and uses port: http which mismatches the target Service
advertising https/8443; update the ServiceMonitor spec to use scheme: https and
the correct port name or number (match the Service's port name/8443 from
config/default/metrics_service.yaml), and add proper TLS settings (tls_config
with certificate authority or set insecureSkipVerify: false) so scraping uses
TLS; ensure the port key (port: <port-name-or-8443>) and scheme: https are the
only changes so Prometheus can successfully and securely scrape the target.
- Around line 9-12: The ServiceMonitor currently only has selector.matchLabels
and is in namespace workload-variant-autoscaler-monitoring while the metrics
Service lives in workload-variant-autoscaler-system, so add a
spec.namespaceSelector with matchNames containing
"workload-variant-autoscaler-system" to allow cross-namespace target discovery;
update the ServiceMonitor resource (the spec.namespaceSelector.matchNames entry)
so the selector.matchLabels (control-plane: controller-manager and
app.kubernetes.io/name: workload-variant-autoscaler) can find the Service in the
other namespace.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1878cfa3-b670-4529-8a6f-9240a1bf1ef8
📒 Files selected for processing (2)
config/default/kustomization.yamlconfig/prometheus/servicemonitor.yaml
💤 Files with no reviewable changes (1)
- config/default/kustomization.yaml
| selector: | ||
| matchLabels: | ||
| control-plane: controller-manager | ||
| app.kubernetes.io/name: workload-variant-autoscaler |
There was a problem hiding this comment.
Restore cross-namespace target selection (namespaceSelector)
metadata.namespace is workload-variant-autoscaler-monitoring (Line 5), while the metrics Service is in workload-variant-autoscaler-system (config/default/metrics_service.yaml:1-18). Without spec.namespaceSelector.matchNames, this ServiceMonitor won’t discover any targets.
Suggested fix
spec:
+ namespaceSelector:
+ matchNames:
+ - workload-variant-autoscaler-system
selector:
matchLabels:
control-plane: controller-manager
app.kubernetes.io/name: workload-variant-autoscaler📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selector: | |
| matchLabels: | |
| control-plane: controller-manager | |
| app.kubernetes.io/name: workload-variant-autoscaler | |
| spec: | |
| namespaceSelector: | |
| matchNames: | |
| - workload-variant-autoscaler-system | |
| selector: | |
| matchLabels: | |
| control-plane: controller-manager | |
| app.kubernetes.io/name: workload-variant-autoscaler |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/prometheus/servicemonitor.yaml` around lines 9 - 12, The
ServiceMonitor currently only has selector.matchLabels and is in namespace
workload-variant-autoscaler-monitoring while the metrics Service lives in
workload-variant-autoscaler-system, so add a spec.namespaceSelector with
matchNames containing "workload-variant-autoscaler-system" to allow
cross-namespace target discovery; update the ServiceMonitor resource (the
spec.namespaceSelector.matchNames entry) so the selector.matchLabels
(control-plane: controller-manager and app.kubernetes.io/name:
workload-variant-autoscaler) can find the Service in the other namespace.
| - port: http | ||
| scheme: http | ||
| interval: 30s | ||
| path: /metrics No newline at end of file | ||
| path: /metrics |
There was a problem hiding this comment.
Major security/correctness gap: plaintext endpoint config (CWE-319) and likely port mismatch
Severity: Major. Using scheme: http on Line 15 allows cleartext metrics transport (CWE-319). In-cluster traffic inspection on compromised nodes can expose telemetry data. Also, the target metrics Service advertises https/8443 (config/default/metrics_service.yaml:1-18), so port: http may fail scraping.
Suggested fix
endpoints:
- - port: http
- scheme: http
+ - port: https
+ scheme: https
interval: 30s
- path: /metrics
+ path: /metrics
+ tlsConfig:
+ insecureSkipVerify: true
+ bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/tokenAs per coding guidelines, **: REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - port: http | |
| scheme: http | |
| interval: 30s | |
| path: /metrics | |
| \ No newline at end of file | |
| path: /metrics | |
| - port: https | |
| scheme: https | |
| interval: 30s | |
| path: /metrics | |
| tlsConfig: | |
| insecureSkipVerify: true | |
| bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/prometheus/servicemonitor.yaml` around lines 14 - 17, The
ServiceMonitor currently scrapes over plaintext (scheme: http) and uses port:
http which mismatches the target Service advertising https/8443; update the
ServiceMonitor spec to use scheme: https and the correct port name or number
(match the Service's port name/8443 from config/default/metrics_service.yaml),
and add proper TLS settings (tls_config with certificate authority or set
insecureSkipVerify: false) so scraping uses TLS; ensure the port key (port:
<port-name-or-8443>) and scheme: https are the only changes so Prometheus can
successfully and securely scrape the target.
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
2 participants
Description
backport from red-hat-data-services#64
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit