Skip to content

[backport]: manifest servicemonitor cleanup + label selector for controller #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zdtsw merged 3 commits into from
Apr 2, 2026
Merged

[backport]: manifest servicemonitor cleanup + label selector for controller #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f8f11c
remove: replacement after selector is removed
zdtsw Mar 27, 2026
7bb6e17
update: add label
zdtsw Mar 27, 2026
220eb4a
Merge branch 'main' into jira_5614
zdtsw Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions config/default/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,6 @@ patches:
target:
kind: Deployment

# [NAMESPACE-SELECTOR] Ensure the ServiceMonitor namespaceSelector matches the
# deployment namespace. Without this, the hardcoded value in monitor.yaml won't
# update when an overlay changes the namespace (e.g. deploying into 'opendatahub'
# or `redhat-ods-operator`).
replacements:
- source:
kind: Deployment
name: controller-manager
fieldPath: metadata.namespace
targets:
- select:
kind: ServiceMonitor
group: monitoring.coreos.com
version: v1
name: controller-manager-metrics-monitor
fieldPaths:
- spec.namespaceSelector.matchNames.0

# Uncomment the patches line if you enable Metrics and CertManager
# [METRICS-WITH-CERTS] To enable metrics protected with certManager, uncomment the following line.
# This patch will protect the metrics with certManager self-signed certs.
Expand Down
3 changes: 2 additions & 1 deletion config/prometheus/servicemonitor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ spec:
selector:
matchLabels:
control-plane: controller-manager
app.kubernetes.io/name: workload-variant-autoscaler
Comment on lines 9 to +12
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

endpoints:
- port: http
scheme: http
interval: 30s
path: /metrics
path: /metrics
Comment on lines 14 to +17
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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/token

As 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.

Suggested change
- 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.

Loading