feat: handshake-node chart#418
Conversation
Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds the Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/publish-handshake-node-helm-chart.yml (1)
17-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable git credential persistence.
As indicated by static analysis, it's a security best practice to prevent
actions/checkoutfrom persisting theGITHUB_TOKENin the local Git configuration. Since this workflow only uses the token to authenticate with the container registry via Helm and does not execute Git pushes, git credential persistence is unnecessary and should be disabled.🛡️ Proposed fix to disable credential persistence
- - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish-handshake-node-helm-chart.yml at line 17, Update the actions/checkout step in the publish-handshake-node-helm-chart workflow to disable Git credential persistence by setting its persist-credentials option to false, while preserving the existing checkout action version and behavior.Source: Linters/SAST tools
charts/handshake-node/templates/statefulset.yaml (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSupport full
updateStrategyconfiguration.Currently, only the
typefield of theupdateStrategyis mapped. If a user tries to configure parameters likerollingUpdate: { maxUnavailable: 1 }(common for StatefulSets), they will be silently dropped. UsingtoYamlensures all strategy parameters are properly passed through.♻️ Proposed refactor
{{- if .Values.updateStrategy }} updateStrategy: - type: {{ .Values.updateStrategy.type }} + {{- toYaml .Values.updateStrategy | nindent 4 }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/handshake-node/templates/statefulset.yaml` around lines 11 - 14, Update the updateStrategy rendering in the StatefulSet template to serialize the complete .Values.updateStrategy object with toYaml and apply appropriate indentation, rather than mapping only updateStrategy.type, so nested settings such as rollingUpdate.maxUnavailable are preserved.charts/handshake-node/templates/service-p2p.yaml (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAllow
nodePortconfiguration forLoadBalancerservices.Kubernetes supports setting a static
nodePorton services of typeLoadBalancer. Expanding this condition allows users to avoid randomly assigned node ports when exposing the P2P service externally.♻️ Proposed refactor
- {{- if and (eq .Values.service.p2p.type "NodePort") .Values.service.p2p.nodePort }} + {{- if and (or (eq .Values.service.p2p.type "NodePort") (eq .Values.service.p2p.type "LoadBalancer")) .Values.service.p2p.nodePort }} nodePort: {{ .Values.service.p2p.nodePort }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/handshake-node/templates/service-p2p.yaml` around lines 29 - 31, Update the nodePort condition in the P2P service template to emit the configured value when service type is either NodePort or LoadBalancer, while retaining the existing requirement that .Values.service.p2p.nodePort is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/handshake-node/templates/statefulset.yaml`:
- Around line 70-77: Update the StatefulSet argument condition around the
“handshake-node.network” include so args are emitted only when the network is
regtest, simnet, or testnet, or when metrics/stratum require them. Extend the
network argument mapping to emit --testnet for the testnet value, while
preserving --regtest and --simnet behavior.
---
Nitpick comments:
In @.github/workflows/publish-handshake-node-helm-chart.yml:
- Line 17: Update the actions/checkout step in the
publish-handshake-node-helm-chart workflow to disable Git credential persistence
by setting its persist-credentials option to false, while preserving the
existing checkout action version and behavior.
In `@charts/handshake-node/templates/service-p2p.yaml`:
- Around line 29-31: Update the nodePort condition in the P2P service template
to emit the configured value when service type is either NodePort or
LoadBalancer, while retaining the existing requirement that
.Values.service.p2p.nodePort is set.
In `@charts/handshake-node/templates/statefulset.yaml`:
- Around line 11-14: Update the updateStrategy rendering in the StatefulSet
template to serialize the complete .Values.updateStrategy object with toYaml and
apply appropriate indentation, rather than mapping only updateStrategy.type, so
nested settings such as rollingUpdate.maxUnavailable are preserved.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7608333-7c38-4944-8996-9741d71d7408
📒 Files selected for processing (13)
.github/workflows/publish-handshake-node-helm-chart.ymlcharts/handshake-node/Chart.yamlcharts/handshake-node/README.mdcharts/handshake-node/templates/_helpers.tplcharts/handshake-node/templates/service-headless.yamlcharts/handshake-node/templates/service-metrics.yamlcharts/handshake-node/templates/service-p2p.yamlcharts/handshake-node/templates/service-rpc.yamlcharts/handshake-node/templates/service-stratum.yamlcharts/handshake-node/templates/serviceaccount.yamlcharts/handshake-node/templates/servicemonitor.yamlcharts/handshake-node/templates/statefulset.yamlcharts/handshake-node/values.yaml
| {{- $network := include "handshake-node.network" . }} | ||
| {{- if or (ne $network "main") .Values.metrics.enabled .Values.stratum.enabled }} | ||
| args: | ||
| {{- if eq $network "regtest" }} | ||
| - --regtest | ||
| {{- else if eq $network "simnet" }} | ||
| - --simnet | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent empty args: generation and missing --testnet flag.
If the network is configured as testnet, the condition ne $network "main" evaluates to true, causing the args: key to be emitted. However, because neither regtest nor simnet match, no arguments are appended (if metrics and stratum are disabled). This results in an empty args: key (evaluating to null) and missing the --testnet flag, which is required by the Handshake daemon to run on the testnet.
Fix the logic to explicitly check for networks that require arguments and add the --testnet mapping.
🛠️ Proposed fix
{{- $network := include "handshake-node.network" . }}
- {{- if or (ne $network "main") .Values.metrics.enabled .Values.stratum.enabled }}
+ {{- $needsNetworkArg := or (eq $network "testnet") (eq $network "regtest") (eq $network "simnet") }}
+ {{- if or $needsNetworkArg .Values.metrics.enabled .Values.stratum.enabled }}
args:
- {{- if eq $network "regtest" }}
+ {{- if eq $network "testnet" }}
+ - --testnet
+ {{- else if eq $network "regtest" }}
- --regtest
{{- else if eq $network "simnet" }}
- --simnet
{{- 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.
| {{- $network := include "handshake-node.network" . }} | |
| {{- if or (ne $network "main") .Values.metrics.enabled .Values.stratum.enabled }} | |
| args: | |
| {{- if eq $network "regtest" }} | |
| - --regtest | |
| {{- else if eq $network "simnet" }} | |
| - --simnet | |
| {{- end }} | |
| {{- $network := include "handshake-node.network" . }} | |
| {{- $needsNetworkArg := or (eq $network "testnet") (eq $network "regtest") (eq $network "simnet") }} | |
| {{- if or $needsNetworkArg .Values.metrics.enabled .Values.stratum.enabled }} | |
| args: | |
| {{- if eq $network "testnet" }} | |
| - --testnet | |
| {{- else if eq $network "regtest" }} | |
| - --regtest | |
| {{- else if eq $network "simnet" }} | |
| - --simnet | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/handshake-node/templates/statefulset.yaml` around lines 70 - 77,
Update the StatefulSet argument condition around the “handshake-node.network”
include so args are emitted only when the network is regtest, simnet, or
testnet, or when metrics/stratum require them. Extend the network argument
mapping to emit --testnet for the testnet value, while preserving --regtest and
--simnet behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish-handshake-node-helm-chart.yml">
<violation number="1" location=".github/workflows/publish-handshake-node-helm-chart.yml:1">
P3: Publishing behavior now has another full workflow copy to keep in sync; fixes to registry auth, action pins, or packaging must be applied across every chart workflow. A reusable `workflow_call` workflow with a chart-path input would centralize this logic.</violation>
</file>
<file name="charts/handshake-node/templates/service-metrics.yaml">
<violation number="1" location="charts/handshake-node/templates/service-metrics.yaml:20">
P2: Changing `metrics.service.port` alone makes this Service target that new port even though the daemon still listens on `metrics.listen` (default 12039), so scrapes receive connection failures. Model the listener target separately or derive it from `metrics.listen`; keep `service.port` as the Service-facing port.</violation>
</file>
<file name="charts/handshake-node/templates/service-p2p.yaml">
<violation number="1" location="charts/handshake-node/templates/service-p2p.yaml:27">
P2: A custom `config.listen` or `service.p2p.port` can make the P2P Service target a port the daemon is not listening on. Model the listener target separately or validate/document that these values must remain equal.</violation>
</file>
<file name="charts/handshake-node/templates/service-rpc.yaml">
<violation number="1" location="charts/handshake-node/templates/service-rpc.yaml:19">
P2: Overriding `rpcService.port` alone makes the Service send traffic to that new port while the daemon still listens on `config.rpclisten` (normally `12037`), so RPC becomes unreachable. Derive both from one value or validate that the listener port matches this Service port.</violation>
<violation number="2" location="charts/handshake-node/templates/service-rpc.yaml:19">
P1: With documented/default RPC settings, this Service has no reachable backend: the daemon listens only on `127.0.0.1`, but Service traffic arrives at the Pod IP. Configure an RPC bind address reachable from the Pod network (and its allowed-client policy) when rendering this Service.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| type: ClusterIP | ||
| ports: | ||
| - name: rpc | ||
| port: {{ .Values.rpcService.port }} |
There was a problem hiding this comment.
P1: With documented/default RPC settings, this Service has no reachable backend: the daemon listens only on 127.0.0.1, but Service traffic arrives at the Pod IP. Configure an RPC bind address reachable from the Pod network (and its allowed-client policy) when rendering this Service.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/handshake-node/templates/service-rpc.yaml, line 19:
<comment>With documented/default RPC settings, this Service has no reachable backend: the daemon listens only on `127.0.0.1`, but Service traffic arrives at the Pod IP. Configure an RPC bind address reachable from the Pod network (and its allowed-client policy) when rendering this Service.</comment>
<file context>
@@ -0,0 +1,24 @@
+ type: ClusterIP
+ ports:
+ - name: rpc
+ port: {{ .Values.rpcService.port }}
+ targetPort: rpc
+ protocol: TCP
</file context>
| @@ -0,0 +1,34 @@ | |||
| name: publish-handshake-node-helm-chart | |||
There was a problem hiding this comment.
P3: Publishing behavior now has another full workflow copy to keep in sync; fixes to registry auth, action pins, or packaging must be applied across every chart workflow. A reusable workflow_call workflow with a chart-path input would centralize this logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/publish-handshake-node-helm-chart.yml, line 1:
<comment>Publishing behavior now has another full workflow copy to keep in sync; fixes to registry auth, action pins, or packaging must be applied across every chart workflow. A reusable `workflow_call` workflow with a chart-path input would centralize this logic.</comment>
<file context>
@@ -0,0 +1,34 @@
+name: publish-handshake-node-helm-chart
+
+on:
</file context>
- statefulset: emit --testnet flag when network=testnet, gate args on networks that need them to avoid empty args on mainnet - statefulset: render full updateStrategy via toYaml so nested fields like rollingUpdate are preserved - service-p2p: allow explicit nodePort on LoadBalancer services - workflow: disable git credential persistence on actions/checkout Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
Chart.yaml - Raise kubeVersion to >=1.27.0-0 so the compatibility check aligns with the StatefulSet persistentVolumeClaimRetentionPolicy field the chart renders by default. _helpers.tpl - Validate network at render time; fail on unsupported values so labels and daemon flags cannot silently disagree. - New handshake-node.headlessName helper truncates to 54 chars before appending -headless so the Service name and StatefulSet serviceName stay within the 63-char DNS label limit under long fullname overrides. - rpcService.enabled short-circuits to false when rpc.enabled is false so the private RPC Service cannot be rendered without a backend. statefulset.yaml - Use headlessName helper for serviceName. - Derive HANDSHAKE_NODE_LISTEN and HANDSHAKE_NODE_RPCLISTEN from the corresponding Service port so bind and Service target port always match. Users can still override via config.listen/config.rpclisten. - Derive metrics/stratum listener addresses from allowPublic + Service port so metrics.allowPublic=false no longer leaves a public bind that the daemon rejects. - Require stratum.miningAddress and stratum.auth.existingSecret at render time whenever stratum public exposure is enabled. - Use stable matchLabels on volumeClaimTemplates so a chart-version bump does not attempt an immutable field change on upgrade. service-headless.yaml - Use the bounded headlessName helper (matches serviceName). service-stratum.yaml - Only render the Stratum Service when stratum.allowPublic=true so a ClusterIP is never created for a loopback-only listener. servicemonitor.yaml - Drop redundant | default filters on path/interval; values.yaml is the single source of truth. values.yaml - config.rpclisten default is empty (derived); RPC now listens on 0.0.0.0 so the ClusterIP-only RPC Service has a reachable backend. - metrics.listen and stratum.listen defaults are empty (derived from Service port + allowPublic). - Document testnet as a supported network. README.md - Prerequisites bumped to Kubernetes 1.27+. - Update metrics/stratum examples to reflect derived listeners and add the testnet value to the network reference row. Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
statefulset.yaml - Require config.rpcallowip whenever rpc.enabled=true. The derived 0.0.0.0 RPC bind combined with a ClusterIP Service accepts traffic from any in-cluster Pod, so an explicit CIDR allowlist is now mandatory (operators can still layer a NetworkPolicy on top). service-metrics.yaml, servicemonitor.yaml - Only render the metrics Service and ServiceMonitor when metrics.allowPublic=true. With allowPublic=false the daemon listens on loopback, so a ClusterIP or ServiceMonitor targeting the Pod IP would never reach the endpoint. values.yaml, README.md - Document config.rpcallowip as required when RPC is enabled. - Document that the metrics Service/ServiceMonitor are only rendered under public exposure. Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Reviewer feedback on PR #418: - README RPC example previously used a Service CIDR (10.0.0.0/8) with guidance mentioning "Pod/Service CIDR". rpcallowip matches client *source* addresses, so a Service CIDR (destination range) never matches; clarify to require a Pod source CIDR and note CNI SNAT. - Mainnet operations example enabled rpc without config.rpcallowip and no longer rendered after rpcallowip became required. Add the CIDR placeholder to the example. - Mirror the same SNAT/source-CIDR wording in values.yaml so the two docs stay consistent. Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Address PR #418 review from cubic-dev-ai: the mainnet operations example now mirrors the RPC Secret example's guidance that rpcallowip must be the observed client source CIDR (Pod CIDR unless CNI SNAT rewrites it to node IPs). Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
Signed-off-by: Tripura Repalle <tripura@blinklabs.io>
5a92fa3 to
fa6688c
Compare
1 participant
Closes: #417
Summary by cubic
Add a Helm chart to deploy
handshake-nodeas a secure, persistent HNS full node on Kubernetes, plus a GitHub Actions workflow that publishes it as an OCI chart to GHCR. Requires Kubernetes 1.27+.New Features
ClusterIP/NodePort/LoadBalancerwith optionalnodePort; optional RPC, metrics, and Stratum endpoints.ClusterIP-only; RPC Service renders only whenrpc.enabled=true; metricsService/ServiceMonitorrender only whenmetrics.allowPublic=true.networkat render; derive P2P/RPC/metrics/stratum listeners from Service ports andallowPublic; only create Stratum Service whenstratum.allowPublic=trueand requireminingAddress+ credentials for public; bounded headless Service name.main/testnet/regtest/simnet, health probes, resource/scheduling controls, PVC retention; CI packages and pushes toghcr.io/<repo>/chartson pushes tomain.Bug Fixes
--testnetonly when needed to avoid empty args on mainnet.config.rpcallowipwhenrpc.enabled=true.updateStrategyfields; allow explicitnodePortonLoadBalancer; ensure the private RPC Service never renders withoutrpc.enabled=true.rpcallowipmust match RPC client source CIDRs and note CNI SNAT; update the mainnet example accordingly.Written for commit fa6688c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation