docker: ship vtop-node in the image, and check that it is there (#281) - #282
Merged
Conversation
A StatefulSet of vtop-node node processes — the co-located shape #237 built — with a headless Service for stable peer DNS, per-plane volume claims, meta node ids derived from the pod ordinal, and a config-checksum annotation so config changes roll the set. Readiness is /readyz and the chart documents that a fenced node going unready is correct behaviour to route around, not a probe to loosen; liveness is /healthz; a startup probe covers cold recovery. Nothing identity-shaped has a default: TLS secret names, the cluster id, per-replica broker UUIDs, range identities, and the client principal are all required, refuse rendering with the exact contract in the message, and are shape-checked by values.schema.json — a chart that defaulted any of them would be shipping credentials, which is issue #81's lesson. The PodDisruptionBudget holds maxUnavailable at 1 because three voters are a quorum; pods run non-root with a read-only rootfs; the ServiceMonitor is gated behind both a value and the CRD's presence.
Two of them made the chart unusable in the shapes it claimed to support. THE ADMIN SERVICE COULD NOT BOOTSTRAP THE CLUSTER. The client Service published the metadata admin port and NOTES told operators to port-forward it, but every pod presents a certificate whose SAN is that pod's OWN headless FQDN. A client routed to an arbitrary pod has no name it can verify, so TLS fails before `vtopctl meta init` sends a byte. A load-balanced endpoint and per-pod certificate identity are incompatible unless the operator also mints a shared SAN, which the chart cannot inspect and must not assume. The port is now off by default behind `service.exposeMetaAdmin`, documented with the exact condition that makes it safe, and NOTES forwards a SPECIFIC POD and prints the `server_name` to set — it defaults to "localhost", which never matches. NAMES COLLIDED AT LENGTH. `fullname` truncated to 63, then the headless Service appended "-headless" and truncated to 63 again — which for a long name yields the SAME string as the client Service, so the chart renders two Services with one name. Verified: an 80-character fullnameOverride produced a collision before this change and distinct 54/63-character names after. The reserved 9 characters also keep StatefulSet pod names and the statefulset.kubernetes.io/pod-name label inside the DNS limit. DNS EGRESS WAS UNRESTRICTED. The rule carried no `to` selector, so enabling the NetworkPolicy admitted port 53 to every destination — a standing hole in a default-deny policy, on the classic exfiltration channel. Scoped to kube-dns in kube-system, overridable via `networkPolicy.dnsTo`. `vtopctl node status` WAS BLOCKED BY THE POLICY THAT DOCUMENTS IT. That command goes replica-to-replica over the replication listener, not over the admin endpoint, and the client rule admitted only admin and native. Replica port added. podLabels and podAnnotations could override chart-managed keys. A user label redefining a selector label leaves the StatefulSet unable to match its own pods; a user annotation replacing checksum/config silently disables config-triggered rollouts, so pods keep running against a config that no longer exists. Both reserved with `omit`. THE TERMINATION GRACE PERIOD WAS DESCRIBED AS "long enough for a clean shutdown fsync". vtop-node installs no signal handler at all, so SIGTERM is ignored and every stop ends in SIGKILL. That is safe — acknowledgement follows fsync, recovery truncates a torn tail, and chaos scenario 04 verifies it — but it is the crash path, and the chart now says so instead of implying a graceful stop it never gets. Signal handling is tracked as #280, including the part that actually matters: a departing leader could release its range lease rather than making failover wait out a timer. Verified with helm v4.2.3: lint clean, full render 6 manifests, identity refusals still fire with their contracts intact, both omit guards drop hijacking keys, the opt-in restores the admin port, and the long-name cases render distinct in-limit names.
The chart had NO CI. Every check on its pull request skipped, so `helm lint` never ran, nothing rendered it, and the two real bugs review found — a Service that could not bootstrap the cluster, and names that collided at length — reached review with no automated signal at all. A branch existed that added the `helm` path filter and a comment referring to "the helm job below". There is no such job. The filter output was wired to nothing, which is the failure mode where CI looks configured and gates nothing. The job checks the two contracts the chart actually makes: RENDERING. Lint, render the documented happy path, and count the manifests — a chart that renders NOTHING also exits zero, so the exit code is not the signal. Every manifest must carry a kind and an apiVersion. REFUSING. Identities and credentials are never defaulted, which is the chart's security property: #81 exists because a lab compose shipped default credentials. A refusal that quietly stopped refusing would render a cluster whose nodes all share whatever the chart invented, and it would look exactly like success. Three refusals are asserted: no identities at all, no TLS secret names, and a nodeUuids list that does not match replicaCount. NAMES. Rendered at helm's maximum release-name length, asserting Service names are distinct and every name fits inside 63 characters — including room for the ordinal StatefulSet pods append. This is the check that would have caught the collision fixed alongside it. Stdlib only, no YAML library. Installing one adds a step that can quietly fail, and a check that stops checking while still reporting success is worse than no check. Helm's output is predictable enough to scan honestly. Every step was run locally against the chart before being written down: lint clean, 6 manifests, all three refusals refuse, and the long-name render produces distinct in-limit names.
… checked nothing MY OWN CI STEP DID NOT TEST WHAT IT CLAIMED. "refuses TLS secret names" set only cluster.id, so rendering aborted at the nodeUuids length check inside the ConfigMap — which renders before the StatefulSet whose volumes reference the Secrets. Helm exits non-zero either way, so the step PASSED while never once reaching the TLS refusal it names. That is the chart's core security property (#81 exists because a lab compose shipped default credentials), and a regression that silently stopped refusing TLS would have sailed straight through the check written to catch it. It now supplies every other identity so rendering reaches the TLS path, verified by confirming the refusal message actually cites TLS. A fourth refusal covers duplicate broker UUIDs. DUPLICATE nodeUuids WERE ACCEPTED. Two pods sharing a broker UUID share an IDENTITY: with data.lease.enabled they present the same one to the metadata plane and race each other for the same range lease, so the StatefulSet is not a set of replicas but two claimants wearing one name. The CN convention makes it worse — one certificate would be valid for both. Refused, naming the duplicates. STRING-VALUED NAMES RENDERED UNQUOTED. A Secret or storage class named `true` (or `no`, or a bare number) is emitted as a YAML boolean, and the manifest is rejected on a type error rather than anything meaningful. The schema keeps the INPUT a string; only quoting keeps the OUTPUT one. Verified: with --set-string tls.metaSecretName=true the rendered field is now "true". THE README STILL SENT PEOPLE TO THE ADMIN SERVICE. I fixed NOTES.txt in the previous commit and left the README pointing at a port that is now off by default — so the documented default workflow could not work. It now shows the per-pod port-forward, the server_name that must accompany it, and why the Service does not publish that port.
The published image contains only vtopctl. `docker/Dockerfile` built
`--bin vtopctl` and copied that one binary, so `vtop-node` — the process a
deployment actually runs — was never in it.
That makes the Helm chart undeployable against the image this project
publishes: the chart runs `vtop-node node`, so every pod would exec a binary
that is not there and CrashLoopBackOff on the first try. The chart's README
carried it as a hedge ("may not contain vtop-node"). It is not a hedge.
It is also inconsistent with the release archives, whose own comment says a
release with only the CLI "would be a steering wheel without the car". The
archives ship both; the image shipped the steering wheel.
Both binaries are now built and installed. The chart sets `command:` on the
container, so it bypasses the entrypoint and its hardcoded `exec vtopctl` —
no entrypoint change is needed for the chart, and the compose lab keeps
working exactly as before.
AND A CHECK THAT WOULD HAVE CAUGHT IT. CI built the image and asserted
nothing about its contents, so a chart could be written against a binary the
image never had and every job stayed green. The docker job now runs both
binaries with --version.
Verified by control rather than assertion: the new check passes against the
rebuilt image and FAILS against ghcr.io/allamiro/vtop-engine:0.2.0, which is
the bug it exists to catch.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 19 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="helm/vtop/templates/statefulset.yaml">
<violation number="1" location="helm/vtop/templates/statefulset.yaml:62">
P2: A valid PriorityClass name such as `true`, `no`, or a numeric label renders with the wrong YAML type and makes the pod template invalid; quote `priorityClassName` when rendering it.</violation>
<violation number="2" location="helm/vtop/templates/statefulset.yaml:111">
P2: A valid configured `image.binary` path containing whitespace or shell metacharacters can be parsed as shell syntax, causing the pod to fail before `vtop-node` starts; render it with shell-safe quoting (or constrain the value to a safe executable-path pattern).</violation>
</file>
<file name="helm/vtop/templates/_helpers.tpl">
<violation number="1" location="helm/vtop/templates/_helpers.tpl:159">
P2: A values file can render an invalid `node-<ordinal>.yaml` or a pod that exits before serving because DNS/server-name inputs are interpolated into quoted YAML without escaping or format validation. Rendering these values with `quote`/`toYaml` and constraining them to DNS/SNI-safe forms would keep malformed values from reaching the node.</violation>
</file>
<file name="helm/vtop/templates/NOTES.txt">
<violation number="1" location="helm/vtop/templates/NOTES.txt:5">
P2: NOTES.txt never flags that the default GHCR image (ghcr.io/allamiro/vtop-engine, tag defaulting to appVersion 0.2.0) still ships only vtopctl, so a user following these notes against the default image will CrashLoopBackOff on the first try. Add a short caveat directing users to point image.repository/tag at an image containing vtop-node until a fixed image is published, mirroring the note already carried in values.yaml.</violation>
</file>
<file name="helm/vtop/values.schema.json">
<violation number="1" location="helm/vtop/values.schema.json:193">
P2: The schema accepts maxUnavailable > 1 for a 3-replica metadata quorum even though the chart's own README casts this as a required schema validation ('maxUnavailable must not exceed 1 for replicaCount=3') and its PDB comment warns it breaks quorum. Encode the cross-field guard with draft-07 if/then so an invalid configuration fails `helm lint`/validation instead of being accepted until a drain takes down the cluster.</violation>
</file>
<file name="helm/vtop/README.md">
<violation number="1" location="helm/vtop/README.md:116">
P3: The Image caveat states in present tense that the image built by docker/Dockerfile ships only vtopctl, but this PR changes that same Dockerfile to build and install both vtop-node and vtopctl. Once the fixed image is published under default tag appVersion 0.2.0 (unchanged), a default install actually gets vtop-node while the README tells the operator the image cannot run the workload — the opposite of the fix. Consider scoping the caveat to the specific pre-fix published 0.2.0 artifact (e.g. 'the 0.2.0 image *currently on ghcr* ... until a rebuilt image is published with both binaries') so it self-expires once the fixed image ships.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| {{- with .Values.priorityClassName }} | ||
| priorityClassName: {{ . }} |
There was a problem hiding this comment.
P2: A valid PriorityClass name such as true, no, or a numeric label renders with the wrong YAML type and makes the pod template invalid; quote priorityClassName when rendering it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/templates/statefulset.yaml, line 62:
<comment>A valid PriorityClass name such as `true`, `no`, or a numeric label renders with the wrong YAML type and makes the pod template invalid; quote `priorityClassName` when rendering it.</comment>
<file context>
@@ -0,0 +1,235 @@
+ {{- toYaml . | nindent 8 }}
+ {{- end }}
+ {{- with .Values.priorityClassName }}
+ priorityClassName: {{ . }}
+ {{- end }}
+ securityContext:
</file context>
| - -ec | ||
| - | | ||
| ordinal="${HOSTNAME##*-}" | ||
| exec {{ .Values.image.binary }} node --config "/etc/vtop/config/node-${ordinal}.yaml" |
There was a problem hiding this comment.
P2: A valid configured image.binary path containing whitespace or shell metacharacters can be parsed as shell syntax, causing the pod to fail before vtop-node starts; render it with shell-safe quoting (or constrain the value to a safe executable-path pattern).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/templates/statefulset.yaml, line 111:
<comment>A valid configured `image.binary` path containing whitespace or shell metacharacters can be parsed as shell syntax, causing the pod to fail before `vtop-node` starts; render it with shell-safe quoting (or constrain the value to a safe executable-path pattern).</comment>
<file context>
@@ -0,0 +1,235 @@
+ - -ec
+ - |
+ ordinal="${HOSTNAME##*-}"
+ exec {{ .Values.image.binary }} node --config "/etc/vtop/config/node-${ordinal}.yaml"
+ env:
+ {{- if .Values.logFormat }}
</file context>
| # own entry, and one shared list keeps the rendered configs diffable. | ||
| peers: | ||
| {{- range $j := until (int $v.replicaCount) }} | ||
| - { id: {{ add1 $j }}, addr: "{{ include "vtop.podFqdn" (dict "root" $root "ordinal" $j) }}:{{ $v.ports.metaPeer }}", server_name: "{{ include "vtop.peerServerName" (dict "root" $root "ordinal" $j) }}" } |
There was a problem hiding this comment.
P2: A values file can render an invalid node-<ordinal>.yaml or a pod that exits before serving because DNS/server-name inputs are interpolated into quoted YAML without escaping or format validation. Rendering these values with quote/toYaml and constraining them to DNS/SNI-safe forms would keep malformed values from reaching the node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/templates/_helpers.tpl, line 159:
<comment>A values file can render an invalid `node-<ordinal>.yaml` or a pod that exits before serving because DNS/server-name inputs are interpolated into quoted YAML without escaping or format validation. Rendering these values with `quote`/`toYaml` and constraining them to DNS/SNI-safe forms would keep malformed values from reaching the node.</comment>
<file context>
@@ -0,0 +1,227 @@
+ # own entry, and one shared list keeps the rendered configs diffable.
+ peers:
+{{- range $j := until (int $v.replicaCount) }}
+ - { id: {{ add1 $j }}, addr: "{{ include "vtop.podFqdn" (dict "root" $root "ordinal" $j) }}:{{ $v.ports.metaPeer }}", server_name: "{{ include "vtop.peerServerName" (dict "root" $root "ordinal" $j) }}" }
+{{- end }}
+ tls:
</file context>
| {{- $ns := .Release.Namespace -}} | ||
| {{- $ids := list -}} | ||
| {{- range $j := until (int .Values.replicaCount) }}{{ $ids = append $ids (add1 $j) }}{{ end -}} | ||
| vtop {{ .Chart.AppVersion }} — {{ int .Values.replicaCount }} co-located node(s) as StatefulSet {{ $fullname }}. |
There was a problem hiding this comment.
P2: NOTES.txt never flags that the default GHCR image (ghcr.io/allamiro/vtop-engine, tag defaulting to appVersion 0.2.0) still ships only vtopctl, so a user following these notes against the default image will CrashLoopBackOff on the first try. Add a short caveat directing users to point image.repository/tag at an image containing vtop-node until a fixed image is published, mirroring the note already carried in values.yaml.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/templates/NOTES.txt, line 5:
<comment>NOTES.txt never flags that the default GHCR image (ghcr.io/allamiro/vtop-engine, tag defaulting to appVersion 0.2.0) still ships only vtopctl, so a user following these notes against the default image will CrashLoopBackOff on the first try. Add a short caveat directing users to point image.repository/tag at an image containing vtop-node until a fixed image is published, mirroring the note already carried in values.yaml.</comment>
<file context>
@@ -0,0 +1,61 @@
+{{- $ns := .Release.Namespace -}}
+{{- $ids := list -}}
+{{- range $j := until (int .Values.replicaCount) }}{{ $ids = append $ids (add1 $j) }}{{ end -}}
+vtop {{ .Chart.AppVersion }} — {{ int .Values.replicaCount }} co-located node(s) as StatefulSet {{ $fullname }}.
+
+1. Watch the pods come up:
</file context>
| } | ||
| } | ||
| }, | ||
| "podDisruptionBudget": { |
There was a problem hiding this comment.
P2: The schema accepts maxUnavailable > 1 for a 3-replica metadata quorum even though the chart's own README casts this as a required schema validation ('maxUnavailable must not exceed 1 for replicaCount=3') and its PDB comment warns it breaks quorum. Encode the cross-field guard with draft-07 if/then so an invalid configuration fails helm lint/validation instead of being accepted until a drain takes down the cluster.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/values.schema.json, line 193:
<comment>The schema accepts maxUnavailable > 1 for a 3-replica metadata quorum even though the chart's own README casts this as a required schema validation ('maxUnavailable must not exceed 1 for replicaCount=3') and its PDB comment warns it breaks quorum. Encode the cross-field guard with draft-07 if/then so an invalid configuration fails `helm lint`/validation instead of being accepted until a drain takes down the cluster.</comment>
<file context>
@@ -0,0 +1,252 @@
+ }
+ }
+ },
+ "podDisruptionBudget": {
+ "type": "object",
+ "additionalProperties": false,
</file context>
|
|
||
| ## Image caveat (read before installing) | ||
|
|
||
| `image.repository` defaults to `ghcr.io/allamiro/vtop-engine`, but as of |
There was a problem hiding this comment.
P3: The Image caveat states in present tense that the image built by docker/Dockerfile ships only vtopctl, but this PR changes that same Dockerfile to build and install both vtop-node and vtopctl. Once the fixed image is published under default tag appVersion 0.2.0 (unchanged), a default install actually gets vtop-node while the README tells the operator the image cannot run the workload — the opposite of the fix. Consider scoping the caveat to the specific pre-fix published 0.2.0 artifact (e.g. 'the 0.2.0 image currently on ghcr ... until a rebuilt image is published with both binaries') so it self-expires once the fixed image ships.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/README.md, line 116:
<comment>The Image caveat states in present tense that the image built by docker/Dockerfile ships only vtopctl, but this PR changes that same Dockerfile to build and install both vtop-node and vtopctl. Once the fixed image is published under default tag appVersion 0.2.0 (unchanged), a default install actually gets vtop-node while the README tells the operator the image cannot run the workload — the opposite of the fix. Consider scoping the caveat to the specific pre-fix published 0.2.0 artifact (e.g. 'the 0.2.0 image *currently on ghcr* ... until a rebuilt image is published with both binaries') so it self-expires once the fixed image ships.</comment>
<file context>
@@ -0,0 +1,231 @@
+
+## Image caveat (read before installing)
+
+`image.repository` defaults to `ghcr.io/allamiro/vtop-engine`, but as of
+appVersion 0.2.0 the image built by `docker/Dockerfile` ships **only
+`vtopctl`**, not `vtop-node`. Until the published image includes `vtop-node`,
</file context>
This was referenced Aug 7, 2026
Merged
allamiro
added a commit
that referenced
this pull request
Aug 7, 2026
A patch release for one reason: v0.2.0's container image cannot run a node. docker/Dockerfile built only --bin vtopctl, so the published image contained the operator CLI and not vtop-node, the process a deployment actually runs. Anyone pulling ghcr.io/allamiro/vtop-engine:0.2.0 for Kubernetes got a steering wheel and no car, and the Helm chart could not start a single pod against it. The failure is silent to whoever trusts the published artifact, which is why this is a release rather than a line in the next one: the fix is worthless sitting on main while the tag people actually pull stays broken. Fixed in #282, with a CI check that runs both binaries with --version and was verified by control — it fails against 0.2.0 and passes against the rebuild. CI had been building that image for two releases while asserting nothing about its contents. Also here, from making the chart real rather than rendered: #279 verifies the chart by DEPLOYING it. scripts/k8s-smoke.sh installs against a live cluster and asserts pods reach Ready, the metadata Raft group bootstraps and elects a leader, the committed offset equals what was produced, ranges really are independent per pod, and every record survives a force-deleted pod. A helm-deploy CI job runs it on kind, and it earned its place on its first run by catching the missing binary above. The eleven chart review findings and the helm lint/render/refusal job merged earlier, carried to main by #282 — whose description did not mention them. That was a branching mistake, recorded on #279 rather than quietly left in the history. No engine behaviour changed. Every crate version moves together because the workspace shares one version, not because each crate changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #281.
The published image contains only
vtopctl.docker/Dockerfilebuilt--bin vtopctland copied that one binary, sovtop-node— the process a deployment actually runs — was never in it.Why it matters
The Helm chart (#279) runs
vtop-node node, so every pod would exec a binary that is not there and CrashLoopBackOff on the first try. The chart is undeployable against the image this project publishes. Its README carries this as a hedge — "may not containvtop-node" — and it is not a hedge, it is the case.It also contradicts the release archives, whose own comment says a release with only the CLI "would be a steering wheel without the car". The archives ship both binaries. The image shipped the steering wheel.
The fix
Build and install both. No entrypoint change was needed: the chart sets
command:on the container, which bypasses the entrypoint and its hardcodedexec vtopctl, and the compose lab keeps using the entrypoint exactly as before.The check that would have caught it
CI built the image and asserted nothing about its contents, so a chart could be written against a binary the image never had and every job stayed green. The docker job now runs both binaries with
--version.Verified by control rather than by assertion:
The new check fails against the published image, which is the bug it exists to catch.
Note on the release
v0.2.0's image is affected and cannot run a node. Whether that warrants a v0.2.1 or just lands here for the next tag is a release call, not one I should make silently — flagging it rather than deciding it.
Summary by cubic
Fixes #281 by shipping
vtop-nodein the Docker image and adding CI to verify bothvtop-nodeandvtopctlare present. Also adds a Helm chart for deploying a co-located VTOP cluster with strict TLS/identity validation and CI gates to keep it honest.Bug Fixes
vtop-node,vtopctl) in the image; no entrypoint change needed for existing uses.vtop-node --versionandvtopctl --versionfrom the built image to catch missing binaries (fails on the previously published 0.2.0 image).New Features
helm/vtop: StatefulSet of co-locatedvtop-node nodepods with per-ordinal configs, headless Service, PDB, optional NetworkPolicy and ServiceMonitor, and clear NOTES/README.tls.metaSecretName,tls.dataSecretName, cluster id, per-replica broker UUIDs; rejects duplicate UUIDs; values are schema-checked.Written for commit 4d8a40d. Summary will update on new commits.