feature/add-etcd-vm-node-scrape #614
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughThis pull request adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Config Loader
participant Values as Values.yaml
participant Proxy as kube-rbac-proxy DaemonSet
participant Scrape as VMNodeScrape/etcd-scrape
participant Etcd as etcd Service
Loader->>Values: Read scrapeRules.etcd.enabled flag
alt Flag is true
Values-->>Loader: Return true
Loader->>Proxy: Deploy etcd-proxy resources
Proxy-->>Loader: Resources initialized
Loader->>Scrape: Render etcd-scrape config
Scrape->>Etcd: Start scraping metrics
else Flag is false
Values-->>Loader: Return false
Loader->>Loader: Skip etcd scraping components
end
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (1)
120-120: Fix typo in resource name.The VMPodScrape name contains a typo:
etcd-managment-scrapeshould beetcd-management-scrape.- name: etcd-managment-scrape + name: etcd-management-scrapehack/download-dashboards.sh (1)
71-71: Updated Grafana Dashboard FilenameThe filename has been updated from
kube-etcd3.jsontokube-etcd.json, which aligns with the renaming mentioned in the PR objectives. Please verify that all downstream references (such as those in dashboards lists or documentation) reflect this change consistently.Note: There is a trailing
#TODOcomment on this line. Ensure this TODO is either resolved or tracked appropriately in your issue tracker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
dashboards/control-plane/kube-etcd3.json(0 hunks)hack/download-dashboards.sh(2 hunks)packages/core/platform/bundles/distro-full.yaml(1 hunks)packages/core/platform/bundles/distro-hosted.yaml(1 hunks)packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)packages/extra/etcd/Chart.yaml(1 hunks)packages/extra/etcd/templates/etcd-cluster.yaml(1 hunks)packages/extra/etcd/templates/podscrape.yaml(1 hunks)packages/extra/etcd/templates/prometheus-rules.yaml(1 hunks)packages/extra/monitoring/dashboards.list(1 hunks)packages/extra/versions_map(1 hunks)packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml(1 hunks)packages/system/monitoring-agents/templates/etcd-scrape.yaml(1 hunks)packages/system/monitoring-agents/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- dashboards/control-plane/kube-etcd3.json
✅ Files skipped from review due to trivial changes (2)
- packages/extra/etcd/Chart.yaml
- packages/extra/versions_map
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/monitoring-agents/templates/etcd-scrape.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 68-68: wrong indentation: expected 0 but found 2
(indentation)
[warning] 123-123: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (12)
packages/extra/monitoring/dashboards.list (1)
33-33: LGTM! Dashboard path updated.The update from
kube-etcd3tokube-etcdaligns with the broader changes to enhance etcd monitoring.packages/extra/etcd/templates/podscrape.yaml (1)
1-11: LGTM! VMPodScrape configuration looks good.The configuration correctly targets etcd pods using a specific label selector and appropriate HTTP scheme for internal pod scraping.
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (3)
19-19: Verify if host networking is required.The DaemonSet uses
hostNetwork: true. Please verify if this is necessary, as it could potentially expose the proxy to the host network.
125-126: Review TLS configuration for security concerns.The configuration uses
insecureSkipVerify: true, which bypasses certificate validation. Please verify if this is necessary and document the security implications.
24-24: Verify kube-rbac-proxy version.The image uses a fixed version
v0.11.0. Please verify if this is the latest stable version and consider using a version range or latest tag if appropriate.packages/core/platform/bundles/distro-hosted.yaml (1)
61-64: Enabling etcd Scrape RulesThe added
valuessection with the nestedscrapeRulesblock (settingetcd: enabled: true) is well placed under themonitoring-agentsrelease. This modification directly supports enhanced etcd monitoring in this bundle and is consistent with the overall PR objectives.packages/extra/etcd/templates/etcd-cluster.yaml (1)
43-48: Exposing Metrics via a Dedicated etcd ContainerThe addition of the
containerssection within the pod template—including a container namedetcdthat exposes themetricsport on 2381/TCP—effectively enables metrics scraping for the etcd cluster. Ensure that the application process running in the etcd container is configured to actually emit the expected metrics on this port.packages/core/platform/bundles/distro-full.yaml (1)
78-82: Consistent Enabling of etcd Metrics ScrapingThe introduction of the
scrapeRulesconfiguration (withetcd: enabled: true) under themonitoring-agentsrelease is consistent with the changes in other bundle files (for example, indistro-hosted.yaml). This consistency helps ensure that etcd metrics are scraped regardless of the deployment bundle selected.packages/extra/etcd/templates/prometheus-rules.yaml (1)
1-9: New PrometheusRule Resource for etcd MonitoringThe new PrometheusRule resource (
etcd-rules) is structured correctly and groups several alerts to monitor etcd cluster health and performance. The overall YAML syntax and use of templated labels are in line with Prometheus best practices.packages/core/platform/bundles/paas-hosted.yaml (1)
73-77: ScrapeRules Configuration for etcd Enabled
The new values block for themonitoring-agentsrelease now includesscrapeRuleswithetcd.enabled: true, which aligns with similar changes in other bundle files. This addition explicitly enables etcd metrics scraping for deployments relying on this bundle.packages/core/platform/bundles/paas-full.yaml (1)
100-104: Consistent Enabling of etcd ScrapeRules
Adding thescrapeRulesconfiguration withetcd.enabled: truein themonitoring-agentsrelease maintains consistency across bundle configurations. This ensures that etcd metrics are scraped in environments using thepaas-full.yamlbundle as well.packages/system/monitoring-agents/values.yaml (1)
363-366: ScrapeRules for etcd Default Set to False
In this system values file, the newscrapeRulessection setsetcd.enabledtofalseby default. This contrasts with the bundles where it is explicitly enabled. Verify that this discrepancy is intentional—perhaps to allow overriding via external configuration—and consider adding an inline comment or documentation note to clarify the intended behavior.
| scheme: http | ||
| scrapeTimeout: 5s | ||
| tlsConfig: | ||
| caFile: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt | ||
| insecureSkipVerify: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review TLS configuration for security concerns.
The configuration uses HTTP scheme while also specifying TLS config with insecureSkipVerify: true. This combination raises security concerns:
- HTTP scheme with TLS config is inconsistent.
insecureSkipVerify: truebypasses certificate validation.
Consider using HTTPS scheme if TLS is required, or verify if TLS can be safely disabled for this internal communication.
🏁 Script executed:
#!/bin/bash
# Check if etcd is configured to use TLS for metrics endpoint
rg -A 5 "metrics.*cert" packages/extra/etcd/Length of output: 46
🏁 Script executed:
#!/bin/bash
# Display the TLS settings and surrounding context from the etcd-scrape.yaml file for manual verification.
rg -C 5 "tlsConfig:" packages/system/monitoring-agents/templates/etcd-scrape.yamlLength of output: 307
Security Misconfiguration in TLS Settings
The file packages/system/monitoring-agents/templates/etcd-scrape.yaml is configured with an HTTP scheme yet also provides TLS settings with insecureSkipVerify: true. This combination is problematic because:
- HTTP scheme vs. TLS config: Using HTTP alongside a TLS configuration is inconsistent. If encryption is desired, the scheme should be HTTPS.
- Bypassing Certificate Validation: The use of
insecureSkipVerify: truedisables proper certificate validation, exposing the communication to potential security risks.
It is recommended to either:
- Update the scheme to
httpsand removeinsecureSkipVerify: trueso that certificate validation is enforced, or - Eliminate the TLS configuration entirely if secure communication is not needed for this internal endpoint.
| - alert: etcdHighNumberOfFailedGRPCRequests | ||
| annotations: | ||
| summary: "etcd cluster '{{`{{ $labels.job }}`}}': '{{`{{ $value }}`}}' of requests for '{{`{{ $labels.grpc_method }}`}}' failed on etcd instance '{{`{{ $labels.instance }}`}}'." | ||
| expr: | | ||
| 100 * sum(rate(grpc_server_handled_total{job=~".*etcd.*", grpc_code!="OK"}[5m])) BY (job, instance, grpc_service, grpc_method) | ||
| / | ||
| sum(rate(grpc_server_handled_total{job=~".*etcd.*"}[5m])) BY (job, instance, grpc_service, grpc_method) | ||
| > 1 | ||
| for: 10m |
There was a problem hiding this comment.
Duplicate Alert Definition: etcdHighNumberOfFailedGRPCRequests (First Instance)
This alert is defined here with a threshold of > 1 over a 10-minute period and a severity of warning. However, an identical alert name is used later with different parameters. Duplicate alert names can lead to conflicts in alert processing.
| - alert: etcdHighNumberOfFailedGRPCRequests | ||
| annotations: | ||
| summary: "etcd cluster '{{`{{ $labels.job }}`}}': '{{`{{ $value }}`}}' of requests for '{{`{{ $labels.grpc_method }}`}}' failed on etcd instance '{{`{{ $labels.instance }}`}}'." | ||
| expr: | | ||
| 100 * sum(rate(grpc_server_handled_total{job=~".*etcd.*", grpc_code!="OK"}[5m])) BY (job, instance, grpc_service, grpc_method) | ||
| / | ||
| sum(rate(grpc_server_handled_total{job=~".*etcd.*"}[5m])) BY (job, instance, grpc_service, grpc_method) | ||
| > 5 | ||
| for: 5m | ||
| labels: |
There was a problem hiding this comment.
Duplicate Alert Definition: etcdHighNumberOfFailedGRPCRequests (Second Instance)
This alert is defined again—this time with a threshold of > 5, a shorter duration of 5 minutes, and a severity of critical. Having two alerts with the same name but differing thresholds and durations is likely to cause ambiguity. Please consider consolidating these into a single alert or renaming one to clearly differentiate their intent.
| - alert: etcdHighNumberOfFailedHTTPRequests | ||
| annotations: | ||
| summary: "'{{`{{ $value }}`}}' of requests for '{{`{{ $labels.method }}`}}' failed on etcd instance '{{`{{ $labels.instance }}`}}'." | ||
| expr: | | ||
| sum(rate(etcd_http_failed_total{job=~".*etcd.*", code!="404"}[5m])) BY (method) / sum(rate(etcd_http_received_total{job=~".*etcd.*"}[5m])) BY (method) > 0.01 | ||
| for: 10m | ||
| labels: | ||
| severity: warning | ||
|
|
There was a problem hiding this comment.
Duplicate Alert Definition: etcdHighNumberOfFailedHTTPRequests (First Instance)
The first definition of this alert uses a threshold of > 0.01 for a 10-minute period with a severity of warning.
| - alert: etcdHighNumberOfFailedHTTPRequests | ||
| annotations: | ||
| summary: "'{{`{{ $value }}`}}' of requests for '{{`{{ $labels.method }}`}}' failed on etcd instance '{{`{{ $labels.instance }}`}}'." | ||
| expr: | | ||
| sum(rate(etcd_http_failed_total{job=~".*etcd.*", code!="404"}[5m])) BY (method) / sum(rate(etcd_http_received_total{job=~".*etcd.*"}[5m])) BY (method) > 0.05 | ||
| for: 10m | ||
| labels: | ||
| severity: critical | ||
|
|
There was a problem hiding this comment.
Duplicate Alert Definition: etcdHighNumberOfFailedHTTPRequests (Second Instance)
A second alert with the same name is defined immediately after, this time using a threshold of > 0.05 for 10 minutes and a severity of critical. Duplicate alert definitions with differing parameters can introduce confusion in alert handling. Please merge or differentiate these alerts accordingly.
klinch0
force-pushed
the
feature/add-etcd-VMNodeScrape
branch
from
653e737 to
4655657
Compare
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (2)
86-88: Review ClusterRole rule configuration.
The rule for theetcd-metrics-readerClusterRole includesnonResourceURLs: ["/metrics"]. Confirm that this approach is intentional rather than employing resource-based permissions, as this choice might impact the access control for metric endpoints.
119-121: Spelling inconsistency in resource name.
The VMPodScrape resource is namedetcd-managment-scrape. It appears that “management” is misspelled. Correcting this toetcd-management-scrapecould improve clarity and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/platform/bundles/distro-full.yaml(1 hunks)packages/core/platform/bundles/distro-hosted.yaml(1 hunks)packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml(1 hunks)packages/system/monitoring-agents/templates/etcd-scrape.yaml(1 hunks)packages/system/monitoring-agents/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/system/monitoring-agents/values.yaml
- packages/core/platform/bundles/paas-full.yaml
- packages/core/platform/bundles/paas-hosted.yaml
- packages/core/platform/bundles/distro-full.yaml
- packages/core/platform/bundles/distro-hosted.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 68-68: wrong indentation: expected 0 but found 2
(indentation)
[warning] 123-123: wrong indentation: expected 2 but found 4
(indentation)
packages/system/monitoring-agents/templates/etcd-scrape.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (5)
packages/system/monitoring-agents/templates/etcd-scrape.yaml (1)
1-1: Helm Templating Conditional Block and YAML Lint Note
The conditional block ({{- if .Values.scrapeRules.etcd.enabled }}) is correctly used to conditionally render the resource. Note that YAML lint tools may report a syntax error (e.g., "expected the node content, but found '-'") due to the Helm templating syntax; this is a known false positive and can be safely ignored.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (4)
25-27: Confirm upstream port configuration.
The container arguments include--upstream=http://127.0.0.1:2381/. Since etcd’s default client port is typically 2379, please verify that port 2381 is intentional and correctly configured for your environment.
67-70: Validate subjects list indentation.
YAMLlint has flagged an indentation warning for the list item undersubjects:(notably at line 68). Please review and adjust the indentation to ensure it complies with YAML best practices (e.g., ensure the dash is properly aligned relative to the parent key).🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 68-68: wrong indentation: expected 0 but found 2
(indentation)
122-123: Verify YAML indentation for podMetricsEndpoints.
YAMLlint reports a warning on line 123 about unexpected indentation (expected 2 spaces but found 4). Please double-check the indentation levels underpodMetricsEndpoints:to ensure they match the required YAML structure.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 123-123: wrong indentation: expected 2 but found 4
(indentation)
1-1:⚠️ Potential issueConditional rendering and Helm template syntax issue.
The file begins with{{- if not .Values.scrapeRules.etcd.enabled }}, which inverts the typical expectation that resources be rendered when etcd scraping is enabled. Verify whether the intended behavior is to deploy these resources when.Values.scrapeRules.etcd.enabledis true—in which case the condition should be updated to remove thenotmodifier. Additionally, YAMLlint reports a syntax error here; this may be a false positive due to Helm templating. Ensure your linting configuration supports Helm template directives.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| targetLabel: metrics_path | ||
| - replacement: etcd | ||
| targetLabel: job | ||
| scheme: http |
There was a problem hiding this comment.
Protocol Scheme vs. TLS Configuration Inconsistency
The scheme is explicitly set to http, while a TLS configuration block (lines 32–34) is still provided. TLS settings are generally applicable with HTTPS endpoints. Please verify your intent:
- If secure communication is desired, update the scheme to
httpsand adjust TLS settings accordingly, or - If HTTP is intended, consider removing the TLS block to reduce confusion.
| tlsConfig: | ||
| caFile: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt | ||
| insecureSkipVerify: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant TLS Configuration for HTTP Endpoint
The TLS configuration (including insecureSkipVerify: true) appears redundant when using http as the protocol. Removing these settings—if TLS is not required—can simplify the configuration and avoid potential misinterpretation.
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml
Outdated
Show resolved
Hide resolved
…yaml Co-authored-by: Andrei Kvapil <[email protected]>
…yaml Co-authored-by: Andrei Kvapil <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (2)
1-1: Helm Template Directive and Linter Compatibility
The Helm conditional directive ({{- if .Values.scrapeRules.etcd.enabled }}) is correctly used to conditionally render this block. Note that YAML linters may report a syntax error (as seen in the static analysis) because they do not recognize Helm templating. Consider configuring your linter to ignore Helm directives or use a Helm-aware linter.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
120-139: VMPodScrape Resource: Indentation and Naming Refinement
The VMPodScrape resource is well defined; however, please address the following points:
- Indentation Warning: The static analysis indicates an indentation issue on line 128 (expected 2 spaces but found 4). Verify that the list item under
podMetricsEndpointsis indented to meet your YAML style guidelines.- Naming Spelling: The metadata name
etcd-managment-scrapeappears to have a typographical error. Consider renaming it toetcd-management-scrapefor clarity and consistency.
A suggested diff for the renaming is provided below:- name: etcd-managment-scrape + name: etcd-management-scrapePlease review the indentation within the
podMetricsEndpointsblock to confirm it meets your expected YAML structure.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 128-128: wrong indentation: expected 2 but found 4
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml(1 hunks)packages/system/monitoring-agents/templates/etcd-scrape.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- packages/system/monitoring-agents/templates/etcd-scrape.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 73-73: wrong indentation: expected 0 but found 2
(indentation)
[warning] 128-128: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (8)
packages/system/monitoring-agents/templates/etcd-proxy-scrape.yaml (8)
2-39: DaemonSet Configuration Confirmation
The DaemonSet forkube-rbac-proxyis defined appropriately. The configuration correctly sets thenamespace, uses host networking, and specifies the node selector for control-plane nodes. The container’s arguments (e.g.--secure-listen-address=$(NODE_IP):9443and--upstream=http://127.0.0.1:2381/) follow previous recommendations. Ensure that the image version (v0.11.0) is intended and compatible with your environments.
42-47: ServiceAccount for kube-rbac-proxy
The ServiceAccount definition forkube-rbac-proxyin thecozy-monitoringnamespace is straightforward and correct.
50-61: ClusterRole for kube-rbac-proxy-auth
The ClusterRole is properly configured with the necessary permissions for token reviews and subject access reviews.
64-76: ClusterRoleBinding Indentation Check
The ClusterRoleBinding forkube-rbac-proxy-auth-bindingis correctly defined; however, note the static analysis warning regarding the indentation of thesubjectslist (e.g. line 73). YAML best practices typically require list items to be indented consistently (commonly 2 spaces under the key). Please verify that this indentation aligns with your team's style and adjust your linter configuration if necessary.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 73-73: wrong indentation: expected 0 but found 2
(indentation)
77-84: ServiceAccount for vm-scrape
The ServiceAccount forvm-scrapeis cleanly defined and placed in the correct namespace.
85-94: ClusterRole for etcd-metrics-reader – Permission Scope
The removal of thenonResourceURLs: ["/metrics"]line suggests an intentional change in the permission scope. Please confirm that this removal is deliberate and that the new or remaining permissions accurately allow the intended access. If further permissions are required for scraping metrics, consider verifying the access rules.
95-109: ClusterRoleBinding for etcd-metrics-reader: Indentation Consistency
Similar to the earlier binding, ensure that the list items under thesubjectskey (lines 106–108) are indented consistently according to your YAML formatting standards. This may require alignment adjustments if your YAML linter flags indentation issues.
110-119: Secret Definition for vm-token
The Secret is correctly defined as a service-account token forvm-scrapewith the appropriate annotation.
Summary by CodeRabbit
New Features
Bug Fixes
Chores