feat: add reth chart#644
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR introduces a complete Helm chart for deploying Reth (Ethereum client) on Kubernetes, including chart metadata, documentation, Grafana dashboards for monitoring, Helm template helpers, and Kubernetes manifests templates for StatefulSet, Services, RBAC, and configuration management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
cjorge-graphops
force-pushed
the
feat/reth-chart
branch
from
818735a to
bff3908
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (24)
charts/reth/templates/reth/servicemonitor.yaml (2)
32-39: Consider also surfacingbearerTokenSecret,tlsConfig, and similar pass-throughs.You already pass-through
interval,scrapeTimeout,relabelings, andmetricRelabelings. For consistency with how other charts in this repo expose ServiceMonitor knobs, you may also want to allowbearerTokenSecret,tlsConfig,scheme, andparams. Optional, but improves flexibility for users in TLS/secured environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/servicemonitor.yaml` around lines 32 - 39, Add pass-through fields for bearerTokenSecret, tlsConfig, scheme, and params to the ServiceMonitor template alongside the existing relabelings/metricRelabelings. Specifically, in the template that references .Values.prometheus.serviceMonitors (the same block that renders relabelings and metricRelabelings) conditionally render keys for bearerTokenSecret, tlsConfig, scheme, and params using the corresponding .Values.prometheus.serviceMonitors.<name> entries so users can set those knobs via values.yaml; keep the same nindent/templating style and guard each with an if check like the existing blocks.
23-24: Confirmed: the metrics path/is correct for Reth.Reth's
--metricslistener serves Prometheus metrics at the root path/(e.g., http://localhost:9001/), so the hardcoded value matches the actual endpoint. For forward compatibility and to allow potential configuration overrides across Reth versions, consider making this path templated fromvalues.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/servicemonitor.yaml` around lines 23 - 24, The ServiceMonitor currently hardcodes the metrics path as "path: /" together with "port: http-metrics"; change this to use a templated value from values.yaml (e.g., replace the literal "path: /" in the ServiceMonitor manifest with something like "path: {{ .Values.reth.metricsPath | default \"/\" }}"), add a corresponding values.yaml key (e.g., reth.metricsPath: "/") so operators can override it, and ensure the template uses the default "/" to preserve current behavior if the value is not set.charts/reth/templates/NOTES.txt (1)
1-2: Consider populatingNOTES.txtwith post-install guidance.An empty
NOTES.txtis functional but is a missed opportunity to give users actionable post-install information (e.g., how to port-forward to the RPC/Engine endpoints, how to check sync status, where the JWT secret can be retrieved, dashboard install hints). Many sibling charts in this repo provide such guidance — consider adding it for consistency and UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/NOTES.txt` around lines 1 - 2, Populate the Helm chart's NOTES.txt (charts/reth/templates/NOTES.txt) with concise post-install guidance: include example kubectl port-forward commands for RPC and Engine services, kubectl commands to check pod status and sync logs (e.g., kubectl logs/exec into the reth pod), the command to retrieve the JWT secret from the reth secret, and a short note about installing or accessing any dashboard/metrics endpoints; keep messages templated using Helm placeholders (.Release.Name, .Values) so they render per-release and mirror sibling charts' style for consistency.charts/reth/templates/dashboards.yaml (2)
13-13: Watch out for the 1 MiB ConfigMap size limit.
(.Files.Glob "dashboards/*.json").AsConfigbundles every dashboard JSON into a single ConfigMap. With several dashboards (the chart already adds reth-discovery, reth-persistence, reth-state-growth, etc.), the ConfigMap can approach Kubernetes' 1 MiB hard limit and fail to apply. Consider either:
- Splitting into one ConfigMap per dashboard file, or
- Documenting the limit and/or compressing/minifying the JSON before bundling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/dashboards.yaml` at line 13, The current use of (.Files.Glob "dashboards/*.json").AsConfig bundles all dashboard JSON into a single ConfigMap risking Kubernetes' 1 MiB limit; update charts/reth/templates/dashboards.yaml to generate one ConfigMap per dashboard file instead of a single AsConfig bundle by iterating over (.Files.Glob "dashboards/*.json") (e.g., range over .Files.Glob and create a ConfigMap resource per file using the file base name for metadata.name) or alternatively add a preprocessing step to minify/compress each JSON before bundling—ensure you reference the glob expression (.Files.Glob "dashboards/*.json") and adjust any consumer references to expect per-file ConfigMaps.
9-9: Quote the dashboard label key for safety.
{{ $values.dashboardsConfigMapLabel }}: ...emits the key unquoted. If a user setsdashboardsConfigMapLabelto something containing characters like/or.(common for Grafana sidecar conventions, e.g.,grafana.com/dashboard), the unquoted YAML mapping key may parse fine but it's brittle; quoting avoids surprises.🔧 Proposed fix
- {{ $values.dashboardsConfigMapLabel }}: {{ $values.dashboardsConfigMapLabelValue | quote }} + {{ $values.dashboardsConfigMapLabel | quote }}: {{ $values.dashboardsConfigMapLabelValue | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/dashboards.yaml` at line 9, The mapping key is emitted unquoted; update the template to quote the key so labels like grafana.com/dashboard or values with dots/slashes are safe: change the left-hand expression from {{ $values.dashboardsConfigMapLabel }} to {{ $values.dashboardsConfigMapLabel | quote }} (keep the value expression {{ $values.dashboardsConfigMapLabelValue | quote }} as-is) so the generated YAML mapping uses a quoted key and avoids parsing brittleness.charts/reth/templates/reth/poddisruptionbudget.yaml (1)
4-22: Operational note: single-replica StatefulSet +minAvailable: 1blocks node drains.Reth nodes are typically deployed as a single StatefulSet replica (one chain database). Combined with the default
minAvailable: 1, any voluntary eviction (kubectl drain, cluster autoscaler scale-down, node upgrades) on the host node will hang indefinitely. Consider documenting this in the chart README and/or defaultingpodDisruptionBudget.enabledtofalse, leaving it as opt-in for HA setups with multiple replicas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/poddisruptionbudget.yaml` around lines 4 - 22, The PodDisruptionBudget currently defaults to creating minAvailable: 1 which will block drains for single-replica StatefulSets; change the chart so podDisruptionBudget is opt-in: set values.podDisruptionBudget.enabled default to false in values.yaml and add a clear note in the chart README explaining that PDBs should be enabled only for HA (multi-replica) setups; optionally, if you prefer automatic safety, update the template (charts/reth/templates/reth/poddisruptionbudget.yaml) to avoid emitting minAvailable: 1 for single-replica deployments (use maxUnavailable or compute minAvailable 0 when replicas == 1) and reference values.podDisruptionBudget.maxUnavailable and values.podDisruptionBudget.minAvailable in that logic.charts/reth/Chart.yaml (1)
1-25: Consider adding chart discoverability metadata.Optionally consider adding standard chart metadata for parity with other charts in this repo and better discoverability on Artifact Hub:
📝 Suggested additions
apiVersion: v2 name: reth description: Deploy and scale [Reth](https://github.com/paradigmxyz/reth) inside Kubernetes with ease +home: https://github.com/paradigmxyz/reth +sources: + - https://github.com/paradigmxyz/reth +keywords: + - ethereum + - reth + - blockchain + - execution-client +maintainers: + - name: graphops + url: https://github.com/graphops +icon: https://raw.githubusercontent.com/paradigmxyz/reth/main/assets/reth-prod.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/Chart.yaml` around lines 1 - 25, Add standard Helm chart discoverability metadata to Chart.yaml: include fields like keywords (array: e.g., "ethereum", "reth", "blockchain"), home (project URL), sources (array with the GitHub repo), maintainers (array of objects with name and email), and icon (URL to a project logo) and consider adding annotations or a description tag for Artifact Hub metadata; update the existing Chart.yaml entries (apiVersion, name, description, type, version, appVersion) to include these new keys so the chart matches the repo's other charts and improves discoverability.charts/reth/templates/rbac.yaml (1)
28-31: Add explicitnamespaceto the RoleBinding subject for consistency.The
ClusterRoleBindingblock below explicitly setsnamespace: {{ .Release.Namespace }}on its subject (Line 56), but the namespacedRoleBindinghere omits it. The API server defaults it correctly for a namespaced binding, so this isn't a bug — but adding it makes the rendered manifest self-documenting and avoids confusion if the template is ever copied or rendered with--include-crdsstyle tooling.♻️ Proposed change
subjects: - kind: ServiceAccount name: {{ include "reth.serviceAccountName" . }} + namespace: {{ .Release.Namespace }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/rbac.yaml` around lines 28 - 31, Add an explicit namespace field to the RoleBinding subject to match the ClusterRoleBinding pattern: update the subjects block that uses the ServiceAccount name from include "reth.serviceAccountName" so it also sets namespace to {{ .Release.Namespace }}; locate the RoleBinding template where subjects contains kind: ServiceAccount and name: {{ include "reth.serviceAccountName" . }} and add namespace: {{ .Release.Namespace }} to make the rendered manifest self-documenting and consistent.charts/reth/templates/dashboards-operator.yaml (2)
45-47:hasKey $op "suspend"always evaluates true given the default values.The chart's
values.yamlshipsgrafana.operatorDashboards.suspend: false(per the README on line 176), sohasKey $op "suspend"will be true on every render andsuspend: falsewill always be emitted into the spec. If the intent was "only rendersuspendwhen the user explicitly opts in", drop the default fromvalues.yaml(leave it absent) so users who don't set it skip the field. If the intent is to always emit it, simplify to a plainsuspend: {{ default false $op.suspend }}and drop thehasKeyguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/dashboards-operator.yaml` around lines 45 - 47, The template guard using hasKey $op "suspend" always returns true because values.yaml defines grafana.operatorDashboards.suspend: false; either remove the default key from values.yaml so the hasKey check only passes when the user explicitly sets $op.suspend, or simplify the template by removing the hasKey guard and emit suspend using a default value (e.g., suspend: {{ default false $op.suspend }}) so the field is always present; update the dashboards-operator.yaml template accordingly (referencing hasKey, $op.suspend, and the grafana.operatorDashboards.suspend values key) depending on which behavior you want.
10-10: Sanitized name suffix can collide and may not be DNS-1123 safe.
base $path | replace "." "-"applies a single transform, sooverview.jsonbecomesoverview-json. If a futuredashboards/overview-json(or similarly named) file is added it would collide and produce duplicateGrafanaDashboardresources. Also, only.is replaced — any non-DNS-1123 characters in a filename (uppercase,_, etc.) would render an invalidmetadata.name. Consider a stricter sanitizer (e.g.lower,regexReplaceAll "[^a-z0-9-]" ... "-") and/or stripping the.jsonextension explicitly withtrimSuffix ".json"before composing the suffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/dashboards-operator.yaml` at line 10, The generated metadata.name uses include "reth.fullname" $ and a sanitized suffix built from base $path which currently only replaces "." with "-" and can produce collisions or non-DNS-1123 names; change the suffix generation to first strip the .json extension with trimSuffix ".json", convert to lowercase with lower, run regexReplaceAll "[^a-z0-9-]" "-" to replace any invalid characters, collapse/normalize repeated "-" (e.g. regexReplaceAll "-+" "-"), then trimSuffix "-" and finally trunc 63 to ensure DNS-1123 compliance and avoid collisions (apply these transforms where base $path is used in the name expression).charts/reth/values.yaml (6)
167-184: Deprecaterpc.jwtSecretliteral in favor ofrpc.jwtSecretFromExistingSecret.Storing the JWT as a literal in
values.yamlis a common pitfall — it lands in source control, inhelm get valuesoutput, and on any cached chart values. The validation atstatefulset.yamllines 45-50 prevents combining the two but doesn't discourage the literal form. Consider adding a deprecation note in the field comment, or removing the literal option entirely and only supportingexistingSecret. (See also the related concern about both paths embedding the secret on the process command line.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 167 - 184, Deprecate the rpc.jwtSecret literal and require rpc.jwtSecretFromExistingSecret: update the values.yaml comment for rpc.jwtSecret to mark it deprecated and instruct users to supply the secret via rpc.jwtSecretFromExistingSecret (name/key), and add validation in the chart templates (the same area that enforces mutual exclusion for jwtSecret vs jwtSecretFromExistingSecret) to fail the release if rpc.jwtSecret is set (with a clear error message), ensuring only the existingSecret path is permitted and avoiding embedding raw JWTs in values or command lines; reference the rpc.jwtSecret and rpc.jwtSecretFromExistingSecret keys and the existing mutual-exclusion validation logic so reviewers can locate and update the checks.
27-47: RBAC defaults grant Get on Nodes cluster-wide — least-privilege note.
clusterRulesgrantsget nodescluster-wide so the init container can read the node'sExternalIP. This is the minimum required for the NodePort discovery flow but is still a cluster-scoped permission. If the chart ever runs withp2p.service.enabled: falseor withp2p.service.advertiseIPset, this ClusterRole/Binding shouldn't be created — and the matchingrbac.yaml(per the cross-file snippet) already gates on$needsNodeLookup/$needsServiceLookup. ✅ Just document in the values.yaml comment that the cluster-scoped permission is only created when actually needed, so security reviewers don't get tripped up by the values-file defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 27 - 47, Update the rbac values comment to explicitly note that the cluster-scoped clusterRules (the `get nodes` permission) is a default in values.yaml but is only actually created when needed by the templates; mention that the ClusterRole/Binding creation is gated by the template conditionals (`$needsNodeLookup` / `$needsServiceLookup`) and that this will not be generated if `p2p.service.enabled` is false or `p2p.service.advertiseIP` is set, so reviewers understand the cluster-wide permission is not always applied; reference `rbac.create` and `clusterRules` in the comment so it's clear where the setting lives.
222-234:logging.file.maxFiles: 0interacts oddly withreadOnlyRootFilesystem: true.The documented intent is "0 disables file logging under read-only root filesystems" (line 231-232), but
_helpers.tplline 383 unconditionally emits--log.file.max-files=0regardless of whetherlogging.file.directoryis set. If a user setslogging.file.directorybut forgets to bumpmaxFiles, they'll get file logging silently disabled. Consider emitting--log.file.max-filesonly when bothlogging.file.directoryis non-empty andmaxFiles > 0, or document the dependency more prominently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 222 - 234, The values.yaml default sets logging.file.maxFiles: 0 which is intended to disable file logging on read-only roots, but the template in _helpers.tpl currently always emits the CLI flag --log.file.max-files=0; update the template logic in _helpers.tpl (the block that renders the --log.file.max-files flag) so the flag is only rendered when logging.file.directory is non-empty and logging.file.maxFiles > 0, or alternatively add a clear comment in values.yaml documenting that maxFiles must be >0 when directory is set; target the flag emission logic for change and keep the flag name --log.file.max-files as the conditional subject.
336-340:publishNotReadyAddresses.headless: falseis unusual for a StatefulSet headless Service.The conventional pattern for StatefulSet headless Services is
publishNotReadyAddresses: trueso that pod DNS records (<pod-name>.<headless-service>.<ns>.svc.cluster.local) resolve while pods are still becoming ready — this is important for peer-to-peer bootstrap and intra-cluster RPC clients. With this default, peers attempting to connect to a Reth pod that isn't yet "ready" (e.g., during long initial sync where readiness depends on metrics or RPC ports) will not get DNS results and may fail to bootstrap.Recommend defaulting
publishNotReadyAddresses.headless: truewhile keepingmain: false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 336 - 340, Change the default for the StatefulSet headless service so pod DNS entries resolve before readiness: set publishNotReadyAddresses.headless to true (leave publishNotReadyAddresses.main as false). Update any comments to reflect this is intentional for headless (peer/bootstrap) scenarios tied to the StatefulSet headless service so clients can resolve pod.<headless-service> DNS while pods are still initializing.
408-447: P2P service defaults and validation interplay — sanity-check matrix.Defaults:
p2p.service.enabled: false,p2p.service.type: NodePort,p2p.service.nodePort: null. The validation instatefulset.yamllines 18-23 fails whenenabled=true && nodePort=null. This is correct, but it means the only "happy path" for someone enabling P2P is to also setnodePortexplicitly. Consider documenting this requirement directly in the comment forp2p.service.enabled(line 409) so users don't have to discover it via a render-timefail. Same applies toloadBalancerIPfor the LoadBalancer path when using non-cloud LB controllers (e.g., MetalLB).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 408 - 447, Update the p2p.service.enabled comment to document the required companion settings: state that when enabling p2p.service.enabled and using p2p.service.type: NodePort you must also set p2p.service.nodePort (non-null) because the statefulset validation will fail otherwise; similarly note that when using p2p.service.type: LoadBalancer on non-cloud LB controllers you may need to set p2p.service.loadBalancerIP (and/or externalIPs) to avoid rendering-time failures. Reference the p2p.service.enabled, p2p.service.type, p2p.service.nodePort and p2p.service.loadBalancerIP keys so users see the exact fields to set.
431-447: Update the kubectl init container image to a maintained source and version.
lachlanevenson/k8s-kubectl:v1.25.4is a community-maintained image whose 1.25.4 tag dates back to late 2022, well outside the upstream Kubernetes support window. Running an outdated kubectl client against a modern cluster works most of the time but isn't guaranteed (skew policy is ±1 minor). Consider switching tobitnami/kubectl(actively maintained and currently at v1.35.3) and bumping the tag to a version closer to your supported clusters' kube-apiserver version.This is a supply-chain hygiene improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/values.yaml` around lines 431 - 447, The initContainer image configuration uses the outdated repository/image and tag (initContainer.image.repository: lachlanevenson/k8s-kubectl and initContainer.image.tag: v1.25.4); update initContainer.image.repository to the maintained image (e.g., bitnami/kubectl) and bump initContainer.image.tag to a current Kubernetes-aligned version (for example v1.35.3 or a tag matching your supported kube-apiserver minor version), keep or verify initContainer.image.pullPolicy remains IfNotPresent (or set to IfNotPresent/Always per your CI policy), and ensure the rest of the initContainer settings (securityContext, capabilities) remain unchanged.charts/reth/templates/reth/statefulset.yaml (4)
30-32:maxPeersvalidation: clarify intent for users who setmaxPeers: 0.
(ne $values.p2p.maxPeers nil)correctly treats unset/null as "not provided" and any explicit value (including0) as "set". This is consistent with the corresponding emission in_helpers.tpl(line 282-284). The validation is correct; just note that an explicitmaxPeers: 0is treated as "configured" and will conflict withmaxInbound/maxOutbound, which may surprise users. A short note in the values.yaml comments for these fields would be helpful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/statefulset.yaml` around lines 30 - 32, The validation treats any explicit value (including 0) for reth.p2p.maxPeers as "set" and will conflict with reth.p2p.maxInboundPeers or reth.p2p.maxOutboundPeers; update the values.yaml comments for the p2p section to explicitly state that setting maxPeers: 0 is considered configured and will cause the same validation failure as any other explicit value, and reference the template symbols reth.p2p.maxPeers, reth.p2p.maxInboundPeers, reth.p2p.maxOutboundPeers (and the corresponding emission logic in _helpers.tpl) so users understand the behavior and avoid surprises.
96-104:atoionrollingUpdatePartitionis convoluted.
default 0 ($values.rollingUpdatePartition | quote | atoi)quotes the value, parses it back to int, and applies a default of0. SincerollingUpdatePartitionis already an integer invalues.yaml(line 275), the round-trip is unnecessary andatoiof a non-numeric value silently returns0, hiding misconfiguration. Simplify:♻️ Proposed simplification
- partition: {{ default 0 ($values.rollingUpdatePartition | quote | atoi) }} + partition: {{ default 0 $values.rollingUpdatePartition }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/statefulset.yaml` around lines 96 - 104, The template is overly complex and unsafe: replace the partition expression that currently uses quote and atoi with a direct default on the integer value so misconfigurations aren't silently masked; update the rollingUpdate.partition line in charts/reth/templates/reth/statefulset.yaml to use the values key directly (e.g., partition: {{ default 0 $values.rollingUpdatePartition }}) while keeping the surrounding updateStrategy/rollingUpdate blocks and the referenced symbols updateStrategyType and rollingUpdatePartition unchanged.
4-6:$jwtEnabledand$jwtSecretNametruthiness forexistingSecret.name.Defaults set
jwt.existingSecret.name: nullandjwt.existingSecret.key: null(values.yaml lines 120, 122). Note that on Line 6 the default for$jwtSecretKeyonly kicks in when$values.jwt.existingSecret.keyis falsy — so users who setexistingSecret.namebut rely on the default key get"jwt.hex"as expected. Looks correct, just calling out that any non-nullexistingSecret.keyusers provide is used verbatim (no validation that the key actually exists in the referenced Secret). Consider documenting this in the values.yaml comment forjwt.existingSecret.key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/statefulset.yaml` around lines 4 - 6, The templates use $jwtEnabled, $jwtSecretName and $jwtSecretKey derived from values.jwt.existingSecret.name and .key so if users set existingSecret.name but leave existingSecret.key null the default "jwt.hex" is applied; however any non-null values.jwt.existingSecret.key is used verbatim without validation against the referenced Secret. Update the values.yaml comment for jwt.existingSecret.key to explicitly document that the provided key name will be used as-is (no Helm-side existence check) and that setting existingSecret.name alone will trigger $jwtEnabled while $jwtSecretKey falls back to "jwt.hex" when key is omitted; reference the template variables $jwtEnabled, $jwtSecretName, $jwtSecretKey (and the include "reth.fullname" / $componentName logic) in the comment so users understand the mapping.
311-315: Add ahelm templatesnapshot test to verify P2P port configuration across service types.The container port logic is correct:
$p2pContainerPortresolves via the helper (lines 92-98 in_helpers.tpl) top2p.service.nodePortin NodePort mode andp2p.portin LoadBalancer mode. Both--portand--discovery.portargs (lines 243, 260 in_helpers.tpl) use the same$p2pPortvariable, ensuring the container listens on the correct port in both modes. The Service correctly targets the container port by name.No snapshot tests currently exist in the repository to document this behavior. Add a test verifying that:
- NodePort mode:
containerPort,--port, and--discovery.portall match the configuredp2p.service.nodePort- LoadBalancer mode:
containerPort,--port, and--discovery.portall matchp2p.port🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/statefulset.yaml` around lines 311 - 315, Add a new Helm template snapshot test that renders the chart twice (NodePort and LoadBalancer modes) and asserts the P2P port values are consistent: in NodePort mode set p2p.service.type=NodePort and p2p.service.nodePort to a test value and verify the rendered StatefulSet containerPort (named udp-p2p), and the container args (--port and --discovery.port) all equal that nodePort; in LoadBalancer mode set p2p.service.type=LoadBalancer and p2p.port to a test value and verify the same three places (containerPort and both args) equal that port. Use the helper variables referenced in templates ($p2pContainerPort and $p2pPort) as the source of truth in your assertions and add the snapshot/test file to the chart tests suite so future template changes will be caught.charts/reth/templates/reth/service.yaml (2)
25-48: Headless Service exposing RPC/engine/metrics ports — intentional?The headless Service (
-headless) exposeshttp-jsonrpc,ws-rpc,http-engineapi, andhttp-metricsin addition to P2P ports. Headless Services are typically used for stable per-pod DNS resolution, while user-facing RPC and metrics already have dedicated Services (main,-engine,-metrics). Including these on the headless Service is harmless but means clients can also discover RPC endpoints via the headless name, which conflicts with the topology-aware/main service annotations applied to the main service only. Consider trimming the headless Service to onlytcp-p2p/udp-p2punless there's a documented use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/service.yaml` around lines 25 - 48, The headless Service template is exposing user-facing ports (http-jsonrpc, ws-rpc, http-engineapi, http-metrics) via the -headless Service even though those have dedicated non-headless services; update the charts/reth/templates/reth/service.yaml headless block to only include P2P ports (tcp-p2p and udp-p2p) by removing or gating the sections that render when $values.http.enabled, $values.ws.enabled, $values.authrpc.enabled, and $values.metrics.enabled so only the P2P entries remain; ensure you keep the existing conditional pattern ({{- if ... }}) and port naming conventions (targetPort names like tcp-p2p/udp-p2p) so the main, -engine, and -metrics services remain the sole exporters of RPC/metrics endpoints.
184-222: P2P NodePort service is hardcoded to pod-0; document the single-replica constraint.The Service name (
-p2p-0), thepod:label (line 188), and the selector (line 222) all pin to<fullname>-reth-0. Combined withreth.replicasforcingreplicas: 1when P2P is enabled (_helpers.tplline 144-145), this is consistent — but a user who setsreplicaCount: 3together withp2p.service.enabled: truewill getreplicas: 1silently overridden, which is surprising and hard to diagnose. Recommend either:
- Adding a
failinstatefulset.yamlwhenreplicaCount > 1 && p2pEnabledso users see a clear error, or- Documenting this clearly in the values.yaml comment for
p2p.service.enabled.The current values.yaml comment at line 112 (
Number of Reth Pods to run when dedicated P2P exposure is disabled) hints at this but is easy to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/reth/service.yaml` around lines 184 - 222, The templates silently force replicas to 1 when P2P service is enabled; add an explicit check in the statefulset template to fail early when users set replicaCount > 1 while p2p.service.enabled is true. In statefulset.yaml add a helm conditional that uses the fail function (e.g., {{- if and (gt .Values.replicas 1) .Values.p2p.service.enabled }} {{ fail "Message..." }} {{- end }}) referencing .Values.replicas and .Values.p2p.service.enabled so the chart errors with a clear message instead of silently overriding replicas; also mention the same constraint in values.yaml comment for p2p.service.enabled if you prefer documentation instead of failing.charts/reth/templates/_helpers.tpl (2)
76-90: Inconsistentdefaultforp2p.service.typebetweenisNodePortandisLoadBalancer.
isNodePortdefaults a missingtypeto"NodePort"(line 77), butisLoadBalancerdefaults a missingtypeto""(line 85). In practicevalues.yamlsetstype: NodePortso both helpers behave deterministically, but the asymmetry is fragile — anyone pruning the values default could end up with both helpers returningfalse, which is then explicitly caught by thefailinstatefulset.yamlline 18-20. Consider aligning the defaults, e.g.:♻️ Suggested alignment
{{- define "reth.p2p.isLoadBalancer" -}} -{{- if and .p2p .p2p.service .p2p.service.enabled (eq (default "" .p2p.service.type) "LoadBalancer") -}} +{{- if and .p2p .p2p.service .p2p.service.enabled (eq (default "NodePort" .p2p.service.type) "LoadBalancer") -}} true {{- else -}} false {{- end -}} {{- end -}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/_helpers.tpl` around lines 76 - 90, The two helpers reth.p2p.isNodePort and reth.p2p.isLoadBalancer use inconsistent defaults for .p2p.service.type (isNodePort defaults to "NodePort" while isLoadBalancer defaults to ""), which can make both return false if the key is missing; update the reth.p2p.isLoadBalancer helper to use the same default (e.g., default "NodePort") or otherwise align the defaulting behavior with reth.p2p.isNodePort so the checks are deterministic; modify the conditional in the define "reth.p2p.isLoadBalancer" to use (default "NodePort" .p2p.service.type) (or the chosen common default) to match reth.p2p.isNodePort.
132-141:reth.isIPAddressIPv6 regex is too permissive.
^[0-9A-Fa-f:]+$matches obviously invalid strings like:::::org:-free random hex. In practice the value reaches Reth and fails downstream, but the validation message atstatefulset.yamlline 24-25 promises an "IP address" check. If you want a tighter check without writing a full IPv6 regex, consider deferring to Helm'sregexMatchfor canonical forms or accepting that this is a soft guard and clarifying the comment. Low-priority polish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/reth/templates/_helpers.tpl` around lines 132 - 141, The IPv6 check in the reth.isIPAddress helper is too permissive (currently "^[0-9A-Fa-f:]+$"); tighten it to a more realistic IPv6 pattern or defer to a canonical-match approach: replace $ipv6 with a regex that requires 1–4 hex digits per hextet and at least one colon (for example something like "^[0-9A-Fa-f]{1,4}(:[0-9A-Fa-f]{1,4}){1,7}$") or use Helm's regexMatch against a canonical IPv6 form, then update the helper reth.isIPAddress to use that stricter $ipv6 and/or add a clarifying comment that this is a soft guard if you choose not to implement full validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/reth/README.md`:
- Line 339: The description for reth.updateStrategyType in values.yaml contains
an unescaped pipe (“(OnDelete|RollingUpdate)”) which breaks the generated README
table; update the description for the values key reth.updateStrategyType in
values.yaml to escape the pipe (e.g. "(OnDelete\\|RollingUpdate)" or use
"|") so the markdown table stays intact, then regenerate the README so the
change is reflected in charts/reth/README.md.
In `@charts/reth/templates/_helpers.tpl`:
- Around line 143-149: The template silently forces replicas to 1 when
p2p.service.enabled is true via the helper reth.replicas; add a validation in
the StatefulSet template (statefulset.yaml) that checks the user's replicaCount
against p2p.service.enabled and emits a template fail() (or a clear helm
template error) if p2p.service.enabled is true but .replicaCount is set and not
equal to 1, referencing the helper name "reth.replicas" and the p2p flag
"p2p.service.enabled" so users get an explicit error instead of a silent
override.
In `@charts/reth/templates/reth/poddisruptionbudget.yaml`:
- Around line 13-17: The template currently uses truthy checks that treat 0 as
empty; replace them with explicit presence checks: use hasKey on
$values.podDisruptionBudget for "maxUnavailable" and "minAvailable" (e.g., if
hasKey $values.podDisruptionBudget "maxUnavailable") and emit the value directly
when present; for minAvailable, check hasKey first and use the provided value,
otherwise fall back to 1 (do not use default which treats 0 as empty). Update
the conditional branches around $values.podDisruptionBudget.maxUnavailable and
$values.podDisruptionBudget.minAvailable accordingly.
In `@charts/reth/templates/reth/servicemonitor.yaml`:
- Line 16: The jobLabel currently uses the literal template value "{{
.Release.Name }}" which makes Prometheus look for a label with that exact key;
update the jobLabel in the ServiceMonitor (the jobLabel field) to the actual
label key used on the Service—app.kubernetes.io/instance—so Prometheus will use
the release name stored on the Service via reth.labels; locate the jobLabel
entry in servicemonitor.yaml and replace the template string with the literal
label key "app.kubernetes.io/instance".
In `@charts/reth/templates/reth/statefulset.yaml`:
- Around line 248-281: The template currently loads
rpc.jwtSecretFromExistingSecret into an env var (RETH_RPC_JWT_SECRET) and then
injects it into argv in the command wrapper (the command block that uses set --
"--rpc.jwtsecret=${RETH_RPC_JWT_SECRET}" "$@"), which leaks the secret via
process args; instead, mount the secret as a file and pass its file path to
reth. Update the StatefulSet template to add a projected/secret volume and a
volumeMount (e.g. mount key as /rpc-jwt/jwt.hex) using the existing
rpc.jwtSecretFromExistingSecret.name/key, remove the env var usage (remove
RETH_RPC_JWT_SECRET injection), and modify the command/args handling (or
reth.computedArgs generation used in _helpers.tpl) to pass
--rpc.jwtsecret=/rpc-jwt/jwt.hex (the file path) rather than embedding the
secret value; also update any helper in _helpers.tpl that currently renders the
literal secret into argv to use the file path approach.
- Around line 191-209: Remove the dead initialization/guard and stop silently
falling back to public IP services when resolving the NodePort external IP:
delete the redundant EXTERNAL_IP=""; if [ -z "$EXTERNAL_IP" ] block and replace
the public-IP fallback loop that queries SITES with a fail-fast path that logs a
clear error and exits when kubectl get nodes "${NODE_NAME}" yields no ExternalIP
(or make the fallback conditional on a new opt-in flag). Reference EXTERNAL_IP,
NODE_NAME, SITES, and the --nat=extip behavior in your change and update
docs/values (e.g., reth.p2p.service.advertiseIP) so users can supply an explicit
advertise IP instead of relying on the egress public IP.
- Around line 210-229: The hostname check currently runs inside the polling loop
and can abort even after EXTERNAL_IP was just set; change the logic so
EXTERNAL_HOSTNAME is only treated as an error when EXTERNAL_IP is still empty
after the loop (or skip the hostname check whenever EXTERNAL_IP is non-empty).
Concretely, keep the kubectl retrievals for EXTERNAL_IP and EXTERNAL_HOSTNAME
inside the while loop but remove the exit 1/echo hostname block from the loop
and add a single check after the loop that echoes the hostname error and exits
only if EXTERNAL_IP is empty and EXTERNAL_HOSTNAME is non-empty; ensure the
variables EXTERNAL_IP and EXTERNAL_HOSTNAME and the service name "{{ include
"reth.fullname" . }}-{{ $componentName }}-p2p" are used unchanged.
- Around line 147-149: The volume mount key for the ConfigMap is using the
ternary that can pick up configFile.existingConfigMap.key, which breaks when
configFile.inline is true because the generated inline ConfigMap always uses
base $configPath (e.g. "reth.toml"); change the items[].key logic to branch on
configFile.inline and hardcode the key to base $configPath for that branch,
otherwise keep the existing ternary that falls back to
existingSecret/existingConfigMap keys; update the template around items: - key:
to use an if $values.configFile.inline ... else ... end conditional referencing
configPath, configFile.inline, existingSecret.key and existingConfigMap.key.
In `@charts/reth/values.yaml`:
- Around line 277-311: The startupProbe (startupProbe.failureThreshold ×
startupProbe.periodSeconds) is too short for archive initial sync and combined
with livenessProbe.enabled: true can cause CrashLoopBackOff; update the chart to
either (A) increase startupProbe.failureThreshold to cover long syncs (e.g.,
~24h expressed as failureThreshold when periodSeconds is 10) by default, or (B)
set livenessProbe.enabled to false by default and document that operators should
enable livenessProbe after the node is fully synced; modify the values under the
readinessProbe/livenessProbe/startupProbe blocks (referencing
startupProbe.failureThreshold, startupProbe.periodSeconds, and
livenessProbe.enabled) to implement one of these safer defaults and add a short
comment/note explaining the chosen behavior.
---
Nitpick comments:
In `@charts/reth/Chart.yaml`:
- Around line 1-25: Add standard Helm chart discoverability metadata to
Chart.yaml: include fields like keywords (array: e.g., "ethereum", "reth",
"blockchain"), home (project URL), sources (array with the GitHub repo),
maintainers (array of objects with name and email), and icon (URL to a project
logo) and consider adding annotations or a description tag for Artifact Hub
metadata; update the existing Chart.yaml entries (apiVersion, name, description,
type, version, appVersion) to include these new keys so the chart matches the
repo's other charts and improves discoverability.
In `@charts/reth/templates/_helpers.tpl`:
- Around line 76-90: The two helpers reth.p2p.isNodePort and
reth.p2p.isLoadBalancer use inconsistent defaults for .p2p.service.type
(isNodePort defaults to "NodePort" while isLoadBalancer defaults to ""), which
can make both return false if the key is missing; update the
reth.p2p.isLoadBalancer helper to use the same default (e.g., default
"NodePort") or otherwise align the defaulting behavior with reth.p2p.isNodePort
so the checks are deterministic; modify the conditional in the define
"reth.p2p.isLoadBalancer" to use (default "NodePort" .p2p.service.type) (or the
chosen common default) to match reth.p2p.isNodePort.
- Around line 132-141: The IPv6 check in the reth.isIPAddress helper is too
permissive (currently "^[0-9A-Fa-f:]+$"); tighten it to a more realistic IPv6
pattern or defer to a canonical-match approach: replace $ipv6 with a regex that
requires 1–4 hex digits per hextet and at least one colon (for example something
like "^[0-9A-Fa-f]{1,4}(:[0-9A-Fa-f]{1,4}){1,7}$") or use Helm's regexMatch
against a canonical IPv6 form, then update the helper reth.isIPAddress to use
that stricter $ipv6 and/or add a clarifying comment that this is a soft guard if
you choose not to implement full validation.
In `@charts/reth/templates/dashboards-operator.yaml`:
- Around line 45-47: The template guard using hasKey $op "suspend" always
returns true because values.yaml defines grafana.operatorDashboards.suspend:
false; either remove the default key from values.yaml so the hasKey check only
passes when the user explicitly sets $op.suspend, or simplify the template by
removing the hasKey guard and emit suspend using a default value (e.g., suspend:
{{ default false $op.suspend }}) so the field is always present; update the
dashboards-operator.yaml template accordingly (referencing hasKey, $op.suspend,
and the grafana.operatorDashboards.suspend values key) depending on which
behavior you want.
- Line 10: The generated metadata.name uses include "reth.fullname" $ and a
sanitized suffix built from base $path which currently only replaces "." with
"-" and can produce collisions or non-DNS-1123 names; change the suffix
generation to first strip the .json extension with trimSuffix ".json", convert
to lowercase with lower, run regexReplaceAll "[^a-z0-9-]" "-" to replace any
invalid characters, collapse/normalize repeated "-" (e.g. regexReplaceAll "-+"
"-"), then trimSuffix "-" and finally trunc 63 to ensure DNS-1123 compliance and
avoid collisions (apply these transforms where base $path is used in the name
expression).
In `@charts/reth/templates/dashboards.yaml`:
- Line 13: The current use of (.Files.Glob "dashboards/*.json").AsConfig bundles
all dashboard JSON into a single ConfigMap risking Kubernetes' 1 MiB limit;
update charts/reth/templates/dashboards.yaml to generate one ConfigMap per
dashboard file instead of a single AsConfig bundle by iterating over
(.Files.Glob "dashboards/*.json") (e.g., range over .Files.Glob and create a
ConfigMap resource per file using the file base name for metadata.name) or
alternatively add a preprocessing step to minify/compress each JSON before
bundling—ensure you reference the glob expression (.Files.Glob
"dashboards/*.json") and adjust any consumer references to expect per-file
ConfigMaps.
- Line 9: The mapping key is emitted unquoted; update the template to quote the
key so labels like grafana.com/dashboard or values with dots/slashes are safe:
change the left-hand expression from {{ $values.dashboardsConfigMapLabel }} to
{{ $values.dashboardsConfigMapLabel | quote }} (keep the value expression {{
$values.dashboardsConfigMapLabelValue | quote }} as-is) so the generated YAML
mapping uses a quoted key and avoids parsing brittleness.
In `@charts/reth/templates/NOTES.txt`:
- Around line 1-2: Populate the Helm chart's NOTES.txt
(charts/reth/templates/NOTES.txt) with concise post-install guidance: include
example kubectl port-forward commands for RPC and Engine services, kubectl
commands to check pod status and sync logs (e.g., kubectl logs/exec into the
reth pod), the command to retrieve the JWT secret from the reth secret, and a
short note about installing or accessing any dashboard/metrics endpoints; keep
messages templated using Helm placeholders (.Release.Name, .Values) so they
render per-release and mirror sibling charts' style for consistency.
In `@charts/reth/templates/rbac.yaml`:
- Around line 28-31: Add an explicit namespace field to the RoleBinding subject
to match the ClusterRoleBinding pattern: update the subjects block that uses the
ServiceAccount name from include "reth.serviceAccountName" so it also sets
namespace to {{ .Release.Namespace }}; locate the RoleBinding template where
subjects contains kind: ServiceAccount and name: {{ include
"reth.serviceAccountName" . }} and add namespace: {{ .Release.Namespace }} to
make the rendered manifest self-documenting and consistent.
In `@charts/reth/templates/reth/poddisruptionbudget.yaml`:
- Around line 4-22: The PodDisruptionBudget currently defaults to creating
minAvailable: 1 which will block drains for single-replica StatefulSets; change
the chart so podDisruptionBudget is opt-in: set
values.podDisruptionBudget.enabled default to false in values.yaml and add a
clear note in the chart README explaining that PDBs should be enabled only for
HA (multi-replica) setups; optionally, if you prefer automatic safety, update
the template (charts/reth/templates/reth/poddisruptionbudget.yaml) to avoid
emitting minAvailable: 1 for single-replica deployments (use maxUnavailable or
compute minAvailable 0 when replicas == 1) and reference
values.podDisruptionBudget.maxUnavailable and
values.podDisruptionBudget.minAvailable in that logic.
In `@charts/reth/templates/reth/service.yaml`:
- Around line 25-48: The headless Service template is exposing user-facing ports
(http-jsonrpc, ws-rpc, http-engineapi, http-metrics) via the -headless Service
even though those have dedicated non-headless services; update the
charts/reth/templates/reth/service.yaml headless block to only include P2P ports
(tcp-p2p and udp-p2p) by removing or gating the sections that render when
$values.http.enabled, $values.ws.enabled, $values.authrpc.enabled, and
$values.metrics.enabled so only the P2P entries remain; ensure you keep the
existing conditional pattern ({{- if ... }}) and port naming conventions
(targetPort names like tcp-p2p/udp-p2p) so the main, -engine, and -metrics
services remain the sole exporters of RPC/metrics endpoints.
- Around line 184-222: The templates silently force replicas to 1 when P2P
service is enabled; add an explicit check in the statefulset template to fail
early when users set replicaCount > 1 while p2p.service.enabled is true. In
statefulset.yaml add a helm conditional that uses the fail function (e.g., {{-
if and (gt .Values.replicas 1) .Values.p2p.service.enabled }} {{ fail
"Message..." }} {{- end }}) referencing .Values.replicas and
.Values.p2p.service.enabled so the chart errors with a clear message instead of
silently overriding replicas; also mention the same constraint in values.yaml
comment for p2p.service.enabled if you prefer documentation instead of failing.
In `@charts/reth/templates/reth/servicemonitor.yaml`:
- Around line 32-39: Add pass-through fields for bearerTokenSecret, tlsConfig,
scheme, and params to the ServiceMonitor template alongside the existing
relabelings/metricRelabelings. Specifically, in the template that references
.Values.prometheus.serviceMonitors (the same block that renders relabelings and
metricRelabelings) conditionally render keys for bearerTokenSecret, tlsConfig,
scheme, and params using the corresponding
.Values.prometheus.serviceMonitors.<name> entries so users can set those knobs
via values.yaml; keep the same nindent/templating style and guard each with an
if check like the existing blocks.
- Around line 23-24: The ServiceMonitor currently hardcodes the metrics path as
"path: /" together with "port: http-metrics"; change this to use a templated
value from values.yaml (e.g., replace the literal "path: /" in the
ServiceMonitor manifest with something like "path: {{ .Values.reth.metricsPath |
default \"/\" }}"), add a corresponding values.yaml key (e.g., reth.metricsPath:
"/") so operators can override it, and ensure the template uses the default "/"
to preserve current behavior if the value is not set.
In `@charts/reth/templates/reth/statefulset.yaml`:
- Around line 30-32: The validation treats any explicit value (including 0) for
reth.p2p.maxPeers as "set" and will conflict with reth.p2p.maxInboundPeers or
reth.p2p.maxOutboundPeers; update the values.yaml comments for the p2p section
to explicitly state that setting maxPeers: 0 is considered configured and will
cause the same validation failure as any other explicit value, and reference the
template symbols reth.p2p.maxPeers, reth.p2p.maxInboundPeers,
reth.p2p.maxOutboundPeers (and the corresponding emission logic in _helpers.tpl)
so users understand the behavior and avoid surprises.
- Around line 96-104: The template is overly complex and unsafe: replace the
partition expression that currently uses quote and atoi with a direct default on
the integer value so misconfigurations aren't silently masked; update the
rollingUpdate.partition line in charts/reth/templates/reth/statefulset.yaml to
use the values key directly (e.g., partition: {{ default 0
$values.rollingUpdatePartition }}) while keeping the surrounding
updateStrategy/rollingUpdate blocks and the referenced symbols
updateStrategyType and rollingUpdatePartition unchanged.
- Around line 4-6: The templates use $jwtEnabled, $jwtSecretName and
$jwtSecretKey derived from values.jwt.existingSecret.name and .key so if users
set existingSecret.name but leave existingSecret.key null the default "jwt.hex"
is applied; however any non-null values.jwt.existingSecret.key is used verbatim
without validation against the referenced Secret. Update the values.yaml comment
for jwt.existingSecret.key to explicitly document that the provided key name
will be used as-is (no Helm-side existence check) and that setting
existingSecret.name alone will trigger $jwtEnabled while $jwtSecretKey falls
back to "jwt.hex" when key is omitted; reference the template variables
$jwtEnabled, $jwtSecretName, $jwtSecretKey (and the include "reth.fullname" /
$componentName logic) in the comment so users understand the mapping.
- Around line 311-315: Add a new Helm template snapshot test that renders the
chart twice (NodePort and LoadBalancer modes) and asserts the P2P port values
are consistent: in NodePort mode set p2p.service.type=NodePort and
p2p.service.nodePort to a test value and verify the rendered StatefulSet
containerPort (named udp-p2p), and the container args (--port and
--discovery.port) all equal that nodePort; in LoadBalancer mode set
p2p.service.type=LoadBalancer and p2p.port to a test value and verify the same
three places (containerPort and both args) equal that port. Use the helper
variables referenced in templates ($p2pContainerPort and $p2pPort) as the source
of truth in your assertions and add the snapshot/test file to the chart tests
suite so future template changes will be caught.
In `@charts/reth/values.yaml`:
- Around line 167-184: Deprecate the rpc.jwtSecret literal and require
rpc.jwtSecretFromExistingSecret: update the values.yaml comment for
rpc.jwtSecret to mark it deprecated and instruct users to supply the secret via
rpc.jwtSecretFromExistingSecret (name/key), and add validation in the chart
templates (the same area that enforces mutual exclusion for jwtSecret vs
jwtSecretFromExistingSecret) to fail the release if rpc.jwtSecret is set (with a
clear error message), ensuring only the existingSecret path is permitted and
avoiding embedding raw JWTs in values or command lines; reference the
rpc.jwtSecret and rpc.jwtSecretFromExistingSecret keys and the existing
mutual-exclusion validation logic so reviewers can locate and update the checks.
- Around line 27-47: Update the rbac values comment to explicitly note that the
cluster-scoped clusterRules (the `get nodes` permission) is a default in
values.yaml but is only actually created when needed by the templates; mention
that the ClusterRole/Binding creation is gated by the template conditionals
(`$needsNodeLookup` / `$needsServiceLookup`) and that this will not be generated
if `p2p.service.enabled` is false or `p2p.service.advertiseIP` is set, so
reviewers understand the cluster-wide permission is not always applied;
reference `rbac.create` and `clusterRules` in the comment so it's clear where
the setting lives.
- Around line 222-234: The values.yaml default sets logging.file.maxFiles: 0
which is intended to disable file logging on read-only roots, but the template
in _helpers.tpl currently always emits the CLI flag --log.file.max-files=0;
update the template logic in _helpers.tpl (the block that renders the
--log.file.max-files flag) so the flag is only rendered when
logging.file.directory is non-empty and logging.file.maxFiles > 0, or
alternatively add a clear comment in values.yaml documenting that maxFiles must
be >0 when directory is set; target the flag emission logic for change and keep
the flag name --log.file.max-files as the conditional subject.
- Around line 336-340: Change the default for the StatefulSet headless service
so pod DNS entries resolve before readiness: set
publishNotReadyAddresses.headless to true (leave publishNotReadyAddresses.main
as false). Update any comments to reflect this is intentional for headless
(peer/bootstrap) scenarios tied to the StatefulSet headless service so clients
can resolve pod.<headless-service> DNS while pods are still initializing.
- Around line 408-447: Update the p2p.service.enabled comment to document the
required companion settings: state that when enabling p2p.service.enabled and
using p2p.service.type: NodePort you must also set p2p.service.nodePort
(non-null) because the statefulset validation will fail otherwise; similarly
note that when using p2p.service.type: LoadBalancer on non-cloud LB controllers
you may need to set p2p.service.loadBalancerIP (and/or externalIPs) to avoid
rendering-time failures. Reference the p2p.service.enabled, p2p.service.type,
p2p.service.nodePort and p2p.service.loadBalancerIP keys so users see the exact
fields to set.
- Around line 431-447: The initContainer image configuration uses the outdated
repository/image and tag (initContainer.image.repository:
lachlanevenson/k8s-kubectl and initContainer.image.tag: v1.25.4); update
initContainer.image.repository to the maintained image (e.g., bitnami/kubectl)
and bump initContainer.image.tag to a current Kubernetes-aligned version (for
example v1.35.3 or a tag matching your supported kube-apiserver minor version),
keep or verify initContainer.image.pullPolicy remains IfNotPresent (or set to
IfNotPresent/Always per your CI policy), and ensure the rest of the
initContainer settings (securityContext, capabilities) remain unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa3e0c6f-d75a-41ff-b4c8-bafcd4267fd2
📒 Files selected for processing (22)
charts/reth/Chart.yamlcharts/reth/README.mdcharts/reth/README.md.gotmplcharts/reth/dashboards/PROVENANCE.mdcharts/reth/dashboards/overview.jsoncharts/reth/dashboards/reth-discovery.jsoncharts/reth/dashboards/reth-mempool.jsoncharts/reth/dashboards/reth-persistence.jsoncharts/reth/dashboards/reth-state-growth.jsoncharts/reth/templates/NOTES.txtcharts/reth/templates/_helpers.tplcharts/reth/templates/dashboards-operator.yamlcharts/reth/templates/dashboards.yamlcharts/reth/templates/rbac.yamlcharts/reth/templates/reth/configmap.yamlcharts/reth/templates/reth/jwt-secret.yamlcharts/reth/templates/reth/poddisruptionbudget.yamlcharts/reth/templates/reth/service.yamlcharts/reth/templates/reth/servicemonitor.yamlcharts/reth/templates/reth/statefulset.yamlcharts/reth/templates/serviceaccount.yamlcharts/reth/values.yaml
| | reth.terminationGracePeriodSeconds | Amount of time to wait before force-killing the Reth process | int | `60` | | ||
| | reth.tolerations | | list | `[]` | | ||
| | reth.topologySpreadConstraints | Pod topology spread constraints | list | `[]` | | ||
| | reth.updateStrategyType | Choice of StatefulSet updateStrategy (OnDelete|RollingUpdate) | string | `"RollingUpdate"` | |
There was a problem hiding this comment.
Unescaped | in (OnDelete|RollingUpdate) breaks the values table.
The pipe character inside the description is being parsed as a table column separator, producing 5 cells in a 4-column row (markdownlint MD056 flagged this). Renderers like GitHub or Helm Hub will misalign the row and drop content. Since this README is generated from values.yaml, escape the pipe at the source so it survives README regeneration:
- # `@param` reth.updateStrategyType -- Choice of StatefulSet updateStrategy (OnDelete|RollingUpdate)
+ # `@param` reth.updateStrategyType -- Choice of StatefulSet updateStrategy (OnDelete\|RollingUpdate)(or use |). Then regenerate the README.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 339-339: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/README.md` at line 339, The description for
reth.updateStrategyType in values.yaml contains an unescaped pipe
(“(OnDelete|RollingUpdate)”) which breaks the generated README table; update the
description for the values key reth.updateStrategyType in values.yaml to escape
the pipe (e.g. "(OnDelete\\|RollingUpdate)" or use "|") so the markdown
table stays intact, then regenerate the README so the change is reflected in
charts/reth/README.md.
| {{- define "reth.replicas" -}} | ||
| {{- if (include "reth.p2p.enabled" . | trim | eq "true") -}} | ||
| {{- print 1 -}} | ||
| {{- else -}} | ||
| {{- print (default 1 .replicaCount) -}} | ||
| {{- end -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
Silent override of replicaCount when P2P is enabled.
reth.replicas returns 1 whenever p2p.service.enabled: true regardless of replicaCount. There's no fail or warning if a user explicitly sets replicaCount: 3. Combined with the -p2p-0-pinned NodePort Service in service.yaml, this is the right runtime behavior, but a misconfiguration like that should be surfaced. Recommend adding a validation in statefulset.yaml:
🛠️ Suggested validation
+{{- if and $p2pEnabled (gt (int (default 1 $values.replicaCount)) 1) }}
+{{- fail "reth.replicaCount must be 1 when reth.p2p.service.enabled=true (the chart only exposes pod-0 via the dedicated P2P Service)" }}
+{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/_helpers.tpl` around lines 143 - 149, The template
silently forces replicas to 1 when p2p.service.enabled is true via the helper
reth.replicas; add a validation in the StatefulSet template (statefulset.yaml)
that checks the user's replicaCount against p2p.service.enabled and emits a
template fail() (or a clear helm template error) if p2p.service.enabled is true
but .replicaCount is set and not equal to 1, referencing the helper name
"reth.replicas" and the p2p flag "p2p.service.enabled" so users get an explicit
error instead of a silent override.
| {{- if $values.podDisruptionBudget.maxUnavailable }} | ||
| maxUnavailable: {{ $values.podDisruptionBudget.maxUnavailable }} | ||
| {{- else }} | ||
| minAvailable: {{ default 1 $values.podDisruptionBudget.minAvailable }} | ||
| {{- end }} |
There was a problem hiding this comment.
Truthiness check on maxUnavailable/minAvailable mishandles legitimate zero values.
Two edge cases:
- Line 13:
{{- if $values.podDisruptionBudget.maxUnavailable }}is falsy when the user explicitly setsmaxUnavailable: 0(a valid PDB value meaning "block all voluntary evictions"). The template would silently fall through tominAvailableinstead. - Line 16:
default 1 $values.podDisruptionBudget.minAvailable—defaulttreats0as empty in Go templates, so a user-suppliedminAvailable: 0would be silently replaced with1.
Prefer explicit nil checks (or use hasKey/kindIs) over truthiness:
🔧 Proposed fix
- {{- if $values.podDisruptionBudget.maxUnavailable }}
+ {{- if ne $values.podDisruptionBudget.maxUnavailable nil }}
maxUnavailable: {{ $values.podDisruptionBudget.maxUnavailable }}
{{- else }}
- minAvailable: {{ default 1 $values.podDisruptionBudget.minAvailable }}
+ minAvailable: {{ if ne $values.podDisruptionBudget.minAvailable nil }}{{ $values.podDisruptionBudget.minAvailable }}{{ else }}1{{ end }}
{{- end }}📝 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.
| {{- if $values.podDisruptionBudget.maxUnavailable }} | |
| maxUnavailable: {{ $values.podDisruptionBudget.maxUnavailable }} | |
| {{- else }} | |
| minAvailable: {{ default 1 $values.podDisruptionBudget.minAvailable }} | |
| {{- end }} | |
| {{- if ne $values.podDisruptionBudget.maxUnavailable nil }} | |
| maxUnavailable: {{ $values.podDisruptionBudget.maxUnavailable }} | |
| {{- else }} | |
| minAvailable: {{ if ne $values.podDisruptionBudget.minAvailable nil }}{{ $values.podDisruptionBudget.minAvailable }}{{ else }}1{{ end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/poddisruptionbudget.yaml` around lines 13 - 17,
The template currently uses truthy checks that treat 0 as empty; replace them
with explicit presence checks: use hasKey on $values.podDisruptionBudget for
"maxUnavailable" and "minAvailable" (e.g., if hasKey $values.podDisruptionBudget
"maxUnavailable") and emit the value directly when present; for minAvailable,
check hasKey first and use the provided value, otherwise fall back to 1 (do not
use default which treats 0 as empty). Update the conditional branches around
$values.podDisruptionBudget.maxUnavailable and
$values.podDisruptionBudget.minAvailable accordingly.
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| jobLabel: "{{ .Release.Name }}" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does the jobLabel field in a Prometheus Operator ServiceMonitor spec do? Should it be a label key or a literal value?
💡 Result:
The jobLabel field in a Prometheus Operator ServiceMonitor spec specifies the name of a label key from the associated Kubernetes Service object. Its value from that Service label is used as the value for the 'job' label on all scraped metrics. It should be a label key (e.g., "k8s-app"), not a literal value. If empty or the label doesn't exist on the Service, the job label defaults to the Service's name. Examples from sources: - If jobLabel: "foo" and Service has label foo: "bar", then metrics get job="bar". - Common usage: jobLabel: "k8s-app" to use the Service's k8s-app label value as the job.
Citations:
- 1: https://doc.crds.dev/github.com/prometheus-operator/prometheus-operator/monitoring.coreos.com/ServiceMonitor/v1@v0.50.0
- 2: https://github.com/prometheus-operator/prometheus-operator/blob/e4c727291acc543dab531bc4aaf16637067c1b86/pkg/apis/monitoring/v1/servicemonitor_types.go
- 3: https://prometheus-operator.dev/docs/api-reference/api/
- 4: https://github.com/prometheus-operator/prometheus-operator/blob/main/Documentation/user-guides/running-exporters.md
🏁 Script executed:
# Find the reth chart files
find charts/reth/templates -type f -name "*.yaml" | head -20Repository: graphops/launchpad-charts
Length of output: 496
🏁 Script executed:
# Look for the Service definition
find charts/reth/templates -type f -name "*service*" -o -name "*svc*"Repository: graphops/launchpad-charts
Length of output: 198
🏁 Script executed:
# Check the servicemonitor.yaml file to see the full context
cat charts/reth/templates/reth/servicemonitor.yamlRepository: graphops/launchpad-charts
Length of output: 1498
🏁 Script executed:
# Look for how reth.labels is defined - check values files and _helpers
find charts/reth -name "_helpers.tpl" -o -name "values*.yaml" | xargs catRepository: graphops/launchpad-charts
Length of output: 31656
🏁 Script executed:
cat charts/reth/templates/reth/service.yamlRepository: graphops/launchpad-charts
Length of output: 8631
Fix jobLabel to reference the label key instead of using a literal value.
The jobLabel field specifies a label key on the target Service whose value Prometheus uses as the job label. Currently, setting it to "{{ .Release.Name }}" causes Prometheus to look for a label with that literal key name on the Service, which doesn't exist. Prometheus then silently falls back to a default job value instead of using the release name.
The Service already has the app.kubernetes.io/instance label (set by reth.labels) with the release name as its value. Reference that label key instead:
Suggested fix
- jobLabel: "{{ .Release.Name }}"
+ jobLabel: app.kubernetes.io/instance📝 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.
| jobLabel: "{{ .Release.Name }}" | |
| jobLabel: app.kubernetes.io/instance |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/servicemonitor.yaml` at line 16, The jobLabel
currently uses the literal template value "{{ .Release.Name }}" which makes
Prometheus look for a label with that exact key; update the jobLabel in the
ServiceMonitor (the jobLabel field) to the actual label key used on the
Service—app.kubernetes.io/instance—so Prometheus will use the release name
stored on the Service via reth.labels; locate the jobLabel entry in
servicemonitor.yaml and replace the template string with the literal label key
"app.kubernetes.io/instance".
| items: | ||
| - key: {{ default "reth.toml" (ternary $values.configFile.existingSecret.key $values.configFile.existingConfigMap.key (not (empty $values.configFile.existingSecret.name))) }} | ||
| path: {{ base $configPath }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect inline ConfigMap to confirm the key it uses
fd configmap.yaml charts/reth/templates --exec cat {}Repository: graphops/launchpad-charts
Length of output: 619
🏁 Script executed:
#!/bin/bash
# Find and examine the values.yaml file
fd "values.yaml" charts/reth --exec cat {} | head -200Repository: graphops/launchpad-charts
Length of output: 7192
🏁 Script executed:
#!/bin/bash
# Search for configFile structure in values.yaml to see the defaults
rg -A 15 "configFile:" charts/reth/values.yamlRepository: graphops/launchpad-charts
Length of output: 724
🏁 Script executed:
#!/bin/bash
# Check the condition that gates inline ConfigMap creation
rg -B 5 -A 10 "if.*inline" charts/reth/templates/reth/configmap.yamlRepository: graphops/launchpad-charts
Length of output: 559
🏁 Script executed:
#!/bin/bash
# Find and examine the StatefulSet template around the volume items section
rg -B 30 "items:" charts/reth/templates/reth/statefulset.yaml | tail -50Repository: graphops/launchpad-charts
Length of output: 1684
🏁 Script executed:
#!/bin/bash
# Check if there's a condition gating the configFile volume mount in StatefulSet
rg -B 10 -A 5 "configFile\|inline" charts/reth/templates/reth/statefulset.yamlRepository: graphops/launchpad-charts
Length of output: 51
Use hardcoded key for inline ConfigMap instead of relying on existingConfigMap.key.
When configFile.inline is true, the generated ConfigMap uses {{ base $configPath }} (which resolves to "reth.toml") as its key. However, the volume items[].key uses a ternary that defaults to existingConfigMap.key, which can be user-configured. If a user sets configFile.existingConfigMap.key: "custom.toml" while using configFile.inline, the volume will attempt to mount a non-existent key from the generated ConfigMap, causing the mount to fail at runtime.
Replace the ternary with conditional logic that hardcodes the key for the inline branch:
Suggested fix
items:
- key: {{ if $values.configFile.inline }}{{ base $configPath }}{{ else }}{{ default "reth.toml" (ternary $values.configFile.existingSecret.key $values.configFile.existingConfigMap.key (not (empty $values.configFile.existingSecret.name))) }}{{ end }}
path: {{ base $configPath }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/statefulset.yaml` around lines 147 - 149, The
volume mount key for the ConfigMap is using the ternary that can pick up
configFile.existingConfigMap.key, which breaks when configFile.inline is true
because the generated inline ConfigMap always uses base $configPath (e.g.
"reth.toml"); change the items[].key logic to branch on configFile.inline and
hardcode the key to base $configPath for that branch, otherwise keep the
existing ternary that falls back to existingSecret/existingConfigMap keys;
update the template around items: - key: to use an if $values.configFile.inline
... else ... end conditional referencing configPath, configFile.inline,
existingSecret.key and existingConfigMap.key.
| set -ex; | ||
| {{- if $p2pNodePort }} | ||
| EXTERNAL_IP=""; | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| EXTERNAL_IP="$(kubectl get nodes "${NODE_NAME}" -o jsonpath='{.status.addresses[?(@.type=="ExternalIP")].address}')"; | ||
| fi | ||
| until [ -n "$EXTERNAL_IP" ]; do | ||
| for SITE in $SITES; do | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| EXTERNAL_IP="$(curl --silent --max-time 5 "$SITE")"; | ||
| if [ -n "$EXTERNAL_IP" ]; then | ||
| break; | ||
| fi | ||
| fi | ||
| done | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| sleep 1; | ||
| fi | ||
| done |
There was a problem hiding this comment.
NodePort external IP discovery: dead code and risky public-IP fallback.
Two concerns in the NodePort branch:
- Lines 193-194:
EXTERNAL_IP=""; if [ -z "$EXTERNAL_IP" ]; then— the guard is always true since the variable was just initialized empty. Dead code that should be removed. - Lines 197-209: When
kubectl get nodesreturns noExternalIP, the script falls back to public IP echo services (v4.ident.me,api.ipify.org,ifconfig.me). The IP these services return is the egress IP of the pod's outbound traffic, which is not necessarily the same as the NodePort's reachable ingress IP (e.g., asymmetric NAT, different egress vs. ingress routes). Advertising a wrong NAT IP via--nat=extipwill silently break P2P inbound connectivity.
Recommend either failing fast with a clear error when Node ExternalIP is missing (and instructing the user to set reth.p2p.service.advertiseIP), or making the public-IP fallback opt-in.
🛠️ Suggested cleanup of the dead branch
set -ex;
{{- if $p2pNodePort }}
- EXTERNAL_IP="";
- if [ -z "$EXTERNAL_IP" ]; then
- EXTERNAL_IP="$(kubectl get nodes "${NODE_NAME}" -o jsonpath='{.status.addresses[?(@.type=="ExternalIP")].address}')";
- fi
+ EXTERNAL_IP="$(kubectl get nodes "${NODE_NAME}" -o jsonpath='{.status.addresses[?(@.type=="ExternalIP")].address}')";
until [ -n "$EXTERNAL_IP" ]; do📝 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.
| set -ex; | |
| {{- if $p2pNodePort }} | |
| EXTERNAL_IP=""; | |
| if [ -z "$EXTERNAL_IP" ]; then | |
| EXTERNAL_IP="$(kubectl get nodes "${NODE_NAME}" -o jsonpath='{.status.addresses[?(@.type=="ExternalIP")].address}')"; | |
| fi | |
| until [ -n "$EXTERNAL_IP" ]; do | |
| for SITE in $SITES; do | |
| if [ -z "$EXTERNAL_IP" ]; then | |
| EXTERNAL_IP="$(curl --silent --max-time 5 "$SITE")"; | |
| if [ -n "$EXTERNAL_IP" ]; then | |
| break; | |
| fi | |
| fi | |
| done | |
| if [ -z "$EXTERNAL_IP" ]; then | |
| sleep 1; | |
| fi | |
| done | |
| set -ex; | |
| {{- if $p2pNodePort }} | |
| EXTERNAL_IP="$(kubectl get nodes "${NODE_NAME}" -o jsonpath='{.status.addresses[?(@.type=="ExternalIP")].address}')"; | |
| until [ -n "$EXTERNAL_IP" ]; do | |
| for SITE in $SITES; do | |
| if [ -z "$EXTERNAL_IP" ]; then | |
| EXTERNAL_IP="$(curl --silent --max-time 5 "$SITE")"; | |
| if [ -n "$EXTERNAL_IP" ]; then | |
| break; | |
| fi | |
| fi | |
| done | |
| if [ -z "$EXTERNAL_IP" ]; then | |
| sleep 1; | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/statefulset.yaml` around lines 191 - 209, Remove
the dead initialization/guard and stop silently falling back to public IP
services when resolving the NodePort external IP: delete the redundant
EXTERNAL_IP=""; if [ -z "$EXTERNAL_IP" ] block and replace the public-IP
fallback loop that queries SITES with a fail-fast path that logs a clear error
and exits when kubectl get nodes "${NODE_NAME}" yields no ExternalIP (or make
the fallback conditional on a new opt-in flag). Reference EXTERNAL_IP,
NODE_NAME, SITES, and the --nat=extip behavior in your change and update
docs/values (e.g., reth.p2p.service.advertiseIP) so users can supply an explicit
advertise IP instead of relying on the egress public IP.
| {{- else if $p2pLoadBalancer }} | ||
| EXTERNAL_IP=""; | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| while [ -z "$EXTERNAL_IP" ]; do | ||
| EXTERNAL_IP="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.status.loadBalancer.ingress[0].ip}')"; | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| EXTERNAL_IP="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.spec.loadBalancerIP}')"; | ||
| fi | ||
| EXTERNAL_HOSTNAME="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')"; | ||
| if [ -n "$EXTERNAL_HOSTNAME" ]; then | ||
| echo "Reth requires an IP address for --nat=extip, but the LoadBalancer reported hostname: ${EXTERNAL_HOSTNAME}" >&2; | ||
| echo "Set reth.p2p.service.advertiseIP to a stable external IP, or configure the LoadBalancer to allocate an IP address." >&2; | ||
| exit 1; | ||
| fi | ||
| if [ -z "$EXTERNAL_IP" ]; then | ||
| sleep 2; | ||
| fi | ||
| done | ||
| fi | ||
| {{- end }} |
There was a problem hiding this comment.
LoadBalancer endpoint discovery: hostname check can race with IP allocation.
The hostname guard on lines 218-223 runs every iteration of the polling loop. Some cloud LB controllers populate .status.loadBalancer.ingress[0].hostname first and .ip shortly after (or both), and some controllers populate only hostname. If both are present, the script will exit 1 even though a usable IP was just retrieved. Move the hostname error check to fire only when no IP is available after the loop, or skip it entirely when EXTERNAL_IP is already non-empty:
🛠️ Proposed fix
while [ -z "$EXTERNAL_IP" ]; do
EXTERNAL_IP="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.status.loadBalancer.ingress[0].ip}')";
if [ -z "$EXTERNAL_IP" ]; then
EXTERNAL_IP="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.spec.loadBalancerIP}')";
fi
- EXTERNAL_HOSTNAME="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')";
- if [ -n "$EXTERNAL_HOSTNAME" ]; then
- echo "Reth requires an IP address for --nat=extip, but the LoadBalancer reported hostname: ${EXTERNAL_HOSTNAME}" >&2;
- echo "Set reth.p2p.service.advertiseIP to a stable external IP, or configure the LoadBalancer to allocate an IP address." >&2;
- exit 1;
- fi
if [ -z "$EXTERNAL_IP" ]; then
+ EXTERNAL_HOSTNAME="$(kubectl get service "{{ include "reth.fullname" . }}-{{ $componentName }}-p2p" -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')";
+ if [ -n "$EXTERNAL_HOSTNAME" ]; then
+ echo "Reth requires an IP address for --nat=extip, but the LoadBalancer reported hostname: ${EXTERNAL_HOSTNAME}" >&2;
+ echo "Set reth.p2p.service.advertiseIP to a stable external IP, or configure the LoadBalancer to allocate an IP address." >&2;
+ exit 1;
+ fi
sleep 2;
fi
done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/statefulset.yaml` around lines 210 - 229, The
hostname check currently runs inside the polling loop and can abort even after
EXTERNAL_IP was just set; change the logic so EXTERNAL_HOSTNAME is only treated
as an error when EXTERNAL_IP is still empty after the loop (or skip the hostname
check whenever EXTERNAL_IP is non-empty). Concretely, keep the kubectl
retrievals for EXTERNAL_IP and EXTERNAL_HOSTNAME inside the while loop but
remove the exit 1/echo hostname block from the loop and add a single check after
the loop that echoes the hostname error and exits only if EXTERNAL_IP is empty
and EXTERNAL_HOSTNAME is non-empty; ensure the variables EXTERNAL_IP and
EXTERNAL_HOSTNAME and the service name "{{ include "reth.fullname" . }}-{{
$componentName }}-p2p" are used unchanged.
| {{- if or $values.env $rpcJwtSecretFromExistingSecret }} | ||
| env: | ||
| {{- if $rpcJwtSecretFromExistingSecret }} | ||
| - name: RETH_RPC_JWT_SECRET | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ $values.rpc.jwtSecretFromExistingSecret.name }} | ||
| key: {{ default "jwt.hex" $values.rpc.jwtSecretFromExistingSecret.key }} | ||
| {{- end }} | ||
| {{- range $key, $val := $values.env }} | ||
| - name: {{ $key }} | ||
| value: {{ $val | quote }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- if $needsRuntimeWrapper }} | ||
| command: | ||
| - sh | ||
| - -ec | ||
| - | | ||
| {{- if $p2pNeedsEndpointInit }} | ||
| while IFS='=' read -r key value; do | ||
| case "$key" in | ||
| EXTERNAL_IP) EXTERNAL_IP="$value" ;; | ||
| esac | ||
| done < /env/reth-p2p | ||
| : "${EXTERNAL_IP:?missing EXTERNAL_IP}" | ||
| set -- "--nat=extip:${EXTERNAL_IP}" "$@" | ||
| {{- end }} | ||
| {{- if $rpcJwtSecretFromExistingSecret }} | ||
| : "${RETH_RPC_JWT_SECRET:?missing RETH_RPC_JWT_SECRET}" | ||
| set -- "--rpc.jwtsecret=${RETH_RPC_JWT_SECRET}" "$@" | ||
| {{- end }} | ||
| exec reth node "$@" | ||
| - reth-node |
There was a problem hiding this comment.
RPC JWT secret leaks onto the command line / process listing.
When rpc.jwtSecretFromExistingSecret.name is set, the secret is loaded as RETH_RPC_JWT_SECRET env var (line 251-255) and then injected into argv via set -- "--rpc.jwtsecret=${RETH_RPC_JWT_SECRET}" "$@" (line 278). This re-introduces the very leakage that "from existing secret" sourcing is meant to avoid: anyone with pods/exec or with read access to /proc/<pid>/cmdline (sidecars, shared PID namespace, debuggers, audit logs) can observe the JWT.
Reth's --rpc.jwtsecret accepts a file path. Prefer materializing the secret as a mounted file and passing the path instead. For example, mount rpc.jwtSecretFromExistingSecret as a volume (similar to how jwt material is mounted at /jwt/jwt.hex) and pass --rpc.jwtsecret=/rpc-jwt/jwt.hex directly via reth.computedArgs — no runtime wrapper required for this path.
The same concern applies to the literal rpc.jwtSecret path in _helpers.tpl (lines 314-316), which renders the secret directly into the rendered manifest and the argv. A chart-managed Secret + file mount is preferable in both cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/templates/reth/statefulset.yaml` around lines 248 - 281, The
template currently loads rpc.jwtSecretFromExistingSecret into an env var
(RETH_RPC_JWT_SECRET) and then injects it into argv in the command wrapper (the
command block that uses set -- "--rpc.jwtsecret=${RETH_RPC_JWT_SECRET}" "$@"),
which leaks the secret via process args; instead, mount the secret as a file and
pass its file path to reth. Update the StatefulSet template to add a
projected/secret volume and a volumeMount (e.g. mount key as /rpc-jwt/jwt.hex)
using the existing rpc.jwtSecretFromExistingSecret.name/key, remove the env var
usage (remove RETH_RPC_JWT_SECRET injection), and modify the command/args
handling (or reth.computedArgs generation used in _helpers.tpl) to pass
--rpc.jwtsecret=/rpc-jwt/jwt.hex (the file path) rather than embedding the
secret value; also update any helper in _helpers.tpl that currently renders the
literal secret into argv to use the file path approach.
| readinessProbe: | ||
| # -- Enable the default Reth readiness probe. Uses the metrics endpoint when enabled, otherwise falls back to TCP on an enabled Reth service port. | ||
| enabled: true | ||
| # -- Full Kubernetes readinessProbe override. Leave empty to use the chart's workload-aware default. | ||
| custom: {} | ||
| # -- How often to run the readiness probe | ||
| periodSeconds: 10 | ||
| # -- Seconds after which the readiness probe times out | ||
| timeoutSeconds: 5 | ||
| # -- Consecutive readiness probe failures before the Pod is marked not ready | ||
| failureThreshold: 3 | ||
|
|
||
| livenessProbe: | ||
| # -- Enable the default Reth liveness probe. Uses the metrics endpoint when enabled, otherwise falls back to TCP on an enabled Reth service port. | ||
| enabled: true | ||
| # -- Full Kubernetes livenessProbe override. Leave empty to use the chart's workload-aware default. | ||
| custom: {} | ||
| # -- How often to run the liveness probe | ||
| periodSeconds: 30 | ||
| # -- Seconds after which the liveness probe times out | ||
| timeoutSeconds: 5 | ||
| # -- Consecutive liveness probe failures before Kubernetes restarts the container | ||
| failureThreshold: 6 | ||
|
|
||
| startupProbe: | ||
| # -- Enable the default Reth startup probe. Keeps liveness/readiness gated while Reth opens its database and starts serving endpoints. | ||
| enabled: true | ||
| # -- Full Kubernetes startupProbe override. Leave empty to use the chart's workload-aware default. | ||
| custom: {} | ||
| # -- How often to run the startup probe | ||
| periodSeconds: 10 | ||
| # -- Seconds after which the startup probe times out | ||
| timeoutSeconds: 5 | ||
| # -- Consecutive startup probe failures before Kubernetes restarts the container | ||
| failureThreshold: 120 |
There was a problem hiding this comment.
Probe defaults vs. initial-sync reality on archive nodes.
startupProbe is configured for failureThreshold: 120 × periodSeconds: 10 = ~20 minutes before Kubernetes restarts the container (line 307, 311). On a fresh mainnet archive sync, Reth can take days to begin serving the metrics endpoint reliably (or even just to reach the point where / responds 200). With the default livenessProbe.enabled: true (line 291) and a startup probe that gives up after 20 minutes, a freshly-deployed archive node can land in a CrashLoopBackOff that prevents any sync progress.
Recommend either:
- Raising
startupProbe.failureThresholdsubstantially (e.g., 24h worth —failureThreshold: 8640×periodSeconds: 10) to cover initial sync, or - Disabling
livenessProbeby default and documenting that operators should enable it once their node has reached "synced" state.
This is the kind of default that can surprise operators in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/reth/values.yaml` around lines 277 - 311, The startupProbe
(startupProbe.failureThreshold × startupProbe.periodSeconds) is too short for
archive initial sync and combined with livenessProbe.enabled: true can cause
CrashLoopBackOff; update the chart to either (A) increase
startupProbe.failureThreshold to cover long syncs (e.g., ~24h expressed as
failureThreshold when periodSeconds is 10) by default, or (B) set
livenessProbe.enabled to false by default and document that operators should
enable livenessProbe after the node is fully synced; modify the values under the
readinessProbe/livenessProbe/startupProbe blocks (referencing
startupProbe.failureThreshold, startupProbe.periodSeconds, and
livenessProbe.enabled) to implement one of these safer defaults and add a short
comment/note explaining the chosen behavior.
1 participant
Summary by CodeRabbit
New Features
Documentation