Skip to content

kind: replace the mesh-config node templating with a sam-node Helm chart - #366

Merged
aojea merged 18 commits into
google:mainfrom
kaisoz:kaisoz/simplify-kind-devex
Sep 7, 2026
Merged

kind: replace the mesh-config node templating with a sam-node Helm chart#366
aojea merged 18 commits into
google:mainfrom
kaisoz:kaisoz/simplify-kind-devex

Conversation

@kaisoz

@kaisoz kaisoz commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

The kind dev environment templated each sam-node (and its optional example service) out of mesh-config.yaml + node.template.yaml via 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:

  • New charts/sam-node: deploys one node — ServiceAccount, config ConfigMap (services + attenuation from values), Deployment with the sam-node container and an optional service container sharing localhost (rendered when service.image is set). controlPlaneUrl is required; audience defaults to sam-mesh-audience; the node API binds loopback by default. Covered by helm-unittest (make helm-test) and wired into make helm-lint.
  • development/kind/run.sh deploys infrastructure only (control plane, router, console, Dex) — no sam-nodes. The cluster shrinks to one control-plane and two workers (one labeled sam-role: control-plane for the router); tmux shows control-plane and router panes.
  • Examples become chart releases: each development/examples/* ships a values.yaml describing just its service, stacked on development/kind/sam-node.values.yaml (kind wiring). Deploying is docker build + kind load + helm install -f base -f example. The new chat-a2a example is migrated too.
  • e2e deploys calc-mcp through the chart — CI now exercises the exact flow developers use. mesh-config.yaml, mesh-config.e2e.yaml, node.template.yaml, and all per-example sam-node-config.yaml files are deleted.
  • Docs updated: dev kubernetes guide, development index, warm-agent-pool / gemini-buddy / chat-a2a use cases, and a pointer in the production kubernetes guide (the chart packages the same pattern as the hand-written manifests there).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread development/deploy-kind-service.sh Outdated
Comment thread development/kind/run-local-node.sh Outdated
# 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}"; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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}"; }

Comment on lines +1 to +6
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "sam-node.fullname" . }}
labels:
{{- include "sam-node.labels" . | nindent 4 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 }}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread charts/sam-node/templates/deployment.yaml Outdated
Comment thread charts/sam-node/values.yaml
Comment thread charts/sam-node/tests/deployment_test.yaml
@aojea

aojea commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

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

@aojea
aojea merged commit 935dcf2 into google:main Sep 7, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants