kind: replace the mesh-config node templating with a sam-node Helm chart - #366
Conversation
…ectory as several nodes
…s never block enrollment
There was a problem hiding this comment.
Code Review
This pull request introduces a new Helm chart, sam-node, to deploy SAM nodes with optional sidecar services, replacing the previous static node configuration templates. It also updates the local development scripts and documentation to use this new Helm-based deployment flow. The review feedback focuses on improving the robustness of the deployment and cleanup scripts—specifically by using label selectors for rollout checks and adding safety checks before directory deletion—and making the Helm chart more flexible by supporting optional ServiceAccount creation, custom names, and annotations.
| # Throwaway identity per run: the default data-dir keeps the biscuit from a | ||
| # previous cluster, and enrolling with it against a fresh control plane fails. | ||
| DATA_DIR="$(mktemp -d)" | ||
| cleanup() { [[ -n "${NODE_PID:-}" ]] && kill "${NODE_PID}" 2>/dev/null || true; rm -rf "${DATA_DIR}"; } |
There was a problem hiding this comment.
To prevent accidental deletion of critical directories (e.g., if DATA_DIR is empty or set to an unexpected path), it is highly recommended to perform a sanity check (ensuring the variable is non-empty and is a directory) before executing rm -rf.
| cleanup() { [[ -n "${NODE_PID:-}" ]] && kill "${NODE_PID}" 2>/dev/null || true; rm -rf "${DATA_DIR}"; } | |
| cleanup() { [[ -n "${NODE_PID:-}" ]] && kill "${NODE_PID}" 2>/dev/null || true; [[ -n "${DATA_DIR:-}" && -d "${DATA_DIR}" ]] && rm -rf "${DATA_DIR}"; } |
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ include "sam-node.fullname" . }} | ||
| labels: | ||
| {{- include "sam-node.labels" . | nindent 4 }} |
There was a problem hiding this comment.
To make the Helm chart production-ready and flexible for cloud environments (where ServiceAccounts often require specific annotations for IAM/Workload Identity mapping, or where users might want to reuse an existing ServiceAccount), it is highly recommended to make the ServiceAccount creation optional and support custom annotations.
{{- if .Values.serviceAccount.create -}}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ default (include "sam-node.fullname" .) .Values.serviceAccount.name }}
labels:
{{- include "sam-node.labels" . | nindent 4 }}
{{- with .Values.serviceAccount.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- end }}|
LGTM way simpler and easier to think about, there are some open comments, let me know if they need to be addressed or this is fine to merge as is |
The kind dev environment templated each sam-node (and its optional example service) out of
mesh-config.yaml+node.template.yamlvia envsubst, which was hard to discover and different from how anyone deploys a node in a real cluster. This PR replaces that with a first-class chart and plain Helm workflows:charts/sam-node: deploys one node — ServiceAccount, config ConfigMap (services+attenuationfrom values), Deployment with the sam-node container and an optional service container sharing localhost (rendered whenservice.imageis set).controlPlaneUrlis required; audience defaults tosam-mesh-audience; the node API binds loopback by default. Covered by helm-unittest (make helm-test) and wired intomake helm-lint.development/kind/run.shdeploys infrastructure only (control plane, router, console, Dex) — no sam-nodes. The cluster shrinks to one control-plane and two workers (one labeledsam-role: control-planefor the router); tmux shows control-plane and router panes.development/examples/*ships avalues.yamldescribing just its service, stacked ondevelopment/kind/sam-node.values.yaml(kind wiring). Deploying isdocker build+kind load+helm install -f base -f example. The newchat-a2aexample is migrated too.mesh-config.yaml,mesh-config.e2e.yaml,node.template.yaml, and all per-examplesam-node-config.yamlfiles are deleted.