From d33183b49b05239d41c8e2ea467509397fba6b64 Mon Sep 17 00:00:00 2001 From: User Date: Fri, 28 Aug 2026 15:44:38 +0200 Subject: [PATCH 1/2] Derive the storage NetworkPolicy node addresses at deploy time `allow-nfs-from-nodes` ships an ipBlock of 192.0.2.0/24 - RFC 5737 TEST-NET-1, routed nowhere - and the documented procedure was to hand-edit it before the first deploy. That is the wrong shape for a template. It puts cluster-specific configuration inside a tracked manifest, so every fork's tree diverges from upstream on exactly one line and every pull can conflict on it; and the edit is easy to forget, with a failure mode of a forty-minute hang whose message ("mount.nfs: Connection timed out") names nothing. k8s/storage/set-node-cidrs.sh now reads the node InternalIPs from whichever cluster kubectl points at and patches the RENDERED STREAM: kubectl kustomize --enable-helm k8s/storage/ \ | k8s/storage/set-node-cidrs.sh \ | kubectl apply -f - Nothing on disk is modified, and the placeholder stays as the shipped default so that bypassing the script still produces the loud, safe failure rather than a wide-open policy. It emits one /32 per node - tighter than a range a human would pick, which matters because a hostNetwork pod shares its node's address and is admitted whatever its labels say. It refuses rather than proceeding when a node address falls inside the pod network (the export is no_root_squash, so that hands every pod in the cluster root over every workspace), and - deliberately - when it cannot evaluate that overlap at all. The first draft used the `command -v python3 && ... || true` shape, which passes silently when an interpreter EXISTS but fails to run; the Windows Store stub does exactly that, and the check reported "ok" on an address that was inside the pod CIDR. It now branches on exit status: 0 clean, 1 overlap, anything else refuse. It also refuses on a Cilium with policy-cidr-match-mode unset. From 1.14 remote nodes carry the `remote-node` identity and CIDR rules do not select node identities without that flag, so the policy would be written correctly and silently ignored - the same hang, from a different cause. CI cannot catch this: kind runs kindnetd, which enforces ipBlock normally, so eight green kind jobs say nothing about whether Cilium will honour the rule. Both kind jobs now call this script instead of deriving the range from `docker network inspect kind`. That removes ~25 duplicated lines per job and, more to the point, means the path an operator actually runs is exercised eight times per run against a real two-node cluster, with assert_netpol_admits_every_node then proving the policy it wrote admits every kubelet. The inline version validated a mechanism no production cluster has. Two documentation corrections found while checking the runbook against what shipped. Section 3 described steps 1-9 as manual; all of them are manifests now, so they are kept as the reasoning behind the two applies rather than as a procedure. And its step 2 said to label `openms` with `enforce=baseline`, which the implementation reversed: k8s/base/namespace.yaml sets `warn` and `audit` only, on the stated grounds that the app's pods have no securityContext, hostPath, host namespaces or added capabilities. The runbook now records the reversal and flags that a fork adding a privileged sidecar should revisit it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018tuhTmRVKxCXo5gJSJU8e8 --- .github/workflows/build-and-test.yml | 88 ++++++------- docs/a16-storage-runbook.md | 86 +++++++++---- docs/kubernetes-deployment.md | 21 +-- k8s/storage/networkpolicy.yaml | 40 ++++-- k8s/storage/set-node-cidrs.sh | 185 +++++++++++++++++++++++++++ 5 files changed, 322 insertions(+), 98 deletions(-) create mode 100755 k8s/storage/set-node-cidrs.sh diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 07880038..d926394b 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -900,32 +900,24 @@ jobs: # podSelector and no ipBlock. Left alone, every mount on a node not # running Ganesha hangs, which the worker's DoNotSchedule spread # guarantees will happen to exactly one replica. - if ! grep -q 'cidr: 192.0.2.0/24' /tmp/storage.yaml; then - echo "::error::k8s/storage/networkpolicy.yaml no longer carries the 192.0.2.0/24 placeholder; this rewrite would silently no-op and every cross-node mount would hang" - exit 1 - fi - # Derived, not hardcoded: kind pins only its IPv6 subnet, so the IPv4 - # one comes from Docker's default address pool and is not 172.18/16 - # by contract. - NODE_CIDR=$(docker network inspect kind \ - -f '{{range .IPAM.Config}}{{.Subnet}} {{end}}' \ - | tr ' ' '\n' | grep -v ':' | grep -E '^[0-9.]+/[0-9]+$' | head -n1) - if [ -z "$NODE_CIDR" ]; then - echo "::error::could not read an IPv4 subnet from the kind docker network" - docker network inspect kind -f '{{json .IPAM.Config}}' - exit 1 - fi - echo "admitting kind nodes on 2049 from $NODE_CIDR" - sed -i "s|cidr: 192.0.2.0/24|cidr: ${NODE_CIDR}|" /tmp/storage.yaml - # kubeconform validated /tmp/storage-rendered.yaml, not the file that - # is actually applied. Re-check the one value the seds above compute - # rather than copy, so a malformed CIDR fails here and names itself - # instead of surfacing as an API-server rejection mid-retry-loop. - if ! grep -Eq '^[[:space:]]*cidr: [0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+$' /tmp/storage.yaml; then - echo "::error::the rewritten NetworkPolicy CIDR is not a bare IPv4 block" - grep -n 'cidr:' /tmp/storage.yaml - exit 1 - fi + # The SAME script the deploy documentation tells an operator to run, + # not a kind-only reimplementation of it. It reads node InternalIPs + # from the cluster, emits one /32 each, refuses if any of them falls + # inside the pod network, and refuses on a Cilium whose + # policy-cidr-match-mode would ignore CIDR rules against nodes. + # + # Running it here is the only validation it gets, and it is a real + # one: this executes eight times per run against a two-node cluster, + # and assert_netpol_admits_every_node downstream then proves the + # policy it wrote actually admits every kubelet. The inline version + # this replaces derived the range from `docker network inspect kind`, + # which no production cluster has - so the path an operator actually + # runs was exercised by nothing. + # + # Its own placeholder and CIDR-shape guards replace the ones that + # used to live here; it exits non-zero rather than emitting YAML. + k8s/storage/set-node-cidrs.sh < /tmp/storage.yaml > /tmp/storage-patched.yaml + mv /tmp/storage-patched.yaml /tmp/storage.yaml applied=0 for i in 1 2 3 4 5; do if kubectl apply -f /tmp/storage.yaml; then @@ -1585,32 +1577,24 @@ jobs: # podSelector and no ipBlock. Left alone, every mount on a node not # running Ganesha hangs, which the worker's DoNotSchedule spread # guarantees will happen to exactly one replica. - if ! grep -q 'cidr: 192.0.2.0/24' /tmp/storage.yaml; then - echo "::error::k8s/storage/networkpolicy.yaml no longer carries the 192.0.2.0/24 placeholder; this rewrite would silently no-op and every cross-node mount would hang" - exit 1 - fi - # Derived, not hardcoded: kind pins only its IPv6 subnet, so the IPv4 - # one comes from Docker's default address pool and is not 172.18/16 - # by contract. - NODE_CIDR=$(docker network inspect kind \ - -f '{{range .IPAM.Config}}{{.Subnet}} {{end}}' \ - | tr ' ' '\n' | grep -v ':' | grep -E '^[0-9.]+/[0-9]+$' | head -n1) - if [ -z "$NODE_CIDR" ]; then - echo "::error::could not read an IPv4 subnet from the kind docker network" - docker network inspect kind -f '{{json .IPAM.Config}}' - exit 1 - fi - echo "admitting kind nodes on 2049 from $NODE_CIDR" - sed -i "s|cidr: 192.0.2.0/24|cidr: ${NODE_CIDR}|" /tmp/storage.yaml - # kubeconform validated /tmp/storage-rendered.yaml, not the file that - # is actually applied. Re-check the one value the seds above compute - # rather than copy, so a malformed CIDR fails here and names itself - # instead of surfacing as an API-server rejection mid-retry-loop. - if ! grep -Eq '^[[:space:]]*cidr: [0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+$' /tmp/storage.yaml; then - echo "::error::the rewritten NetworkPolicy CIDR is not a bare IPv4 block" - grep -n 'cidr:' /tmp/storage.yaml - exit 1 - fi + # The SAME script the deploy documentation tells an operator to run, + # not a kind-only reimplementation of it. It reads node InternalIPs + # from the cluster, emits one /32 each, refuses if any of them falls + # inside the pod network, and refuses on a Cilium whose + # policy-cidr-match-mode would ignore CIDR rules against nodes. + # + # Running it here is the only validation it gets, and it is a real + # one: this executes eight times per run against a two-node cluster, + # and assert_netpol_admits_every_node downstream then proves the + # policy it wrote actually admits every kubelet. The inline version + # this replaces derived the range from `docker network inspect kind`, + # which no production cluster has - so the path an operator actually + # runs was exercised by nothing. + # + # Its own placeholder and CIDR-shape guards replace the ones that + # used to live here; it exits non-zero rather than emitting YAML. + k8s/storage/set-node-cidrs.sh < /tmp/storage.yaml > /tmp/storage-patched.yaml + mv /tmp/storage-patched.yaml /tmp/storage.yaml applied=0 for i in 1 2 3 4 5; do if kubectl apply -f /tmp/storage.yaml; then diff --git a/docs/a16-storage-runbook.md b/docs/a16-storage-runbook.md index aa9099cf..47cf1d98 100644 --- a/docs/a16-storage-runbook.md +++ b/docs/a16-storage-runbook.md @@ -135,32 +135,66 @@ Decision 4 provisions a **fresh** Cinder PVC in `template-app-storage` rather th migrating the existing one, so the old volume stays intact and untouched throughout. That is what makes section 5 cheap. -1. Create `template-app-storage`, labelled `pod-security.kubernetes.io/enforce=privileged`. -2. Label `openms` explicitly `enforce=baseline` — do not rely on the absent label. -3. Provision the new Cinder PVC in `template-app-storage`. -4. Deploy Ganesha with `existingClaim`, `Export_Id: 1`, `deviceBasedFsids: false`, - `strategy: Recreate`, 1 replica, and **explicit memory request and limit**. -5. Apply the default-deny ingress plus the two allow rules on 2049: the - pod-label-scoped one admitting `app: template-app` in `openms`, and an - `ipBlock` for the cluster's node addresses. **The second is not optional - and ships as a placeholder.** The provisioner emits in-tree `nfs:` PVs, - which the kubelet mounts from the node's own address in the host network - namespace — that matches no `podSelector`, so with the placeholder left in - place every mount hangs. Read the addresses from `kubectl get nodes -o wide` - and narrow the range as far as it will go; check it does not contain the pod - CIDR, which would hand every pod in the cluster root over every workspace. -6. Create the PVC in `openms` on the new StorageClass, mounted at the unchanged - path `/workspaces-streamlit-template`. It is a NEW claim, `workspaces-nfs-pvc`, - not an edit of `workspaces-pvc`: a bound PVC's spec is immutable apart from - `resources.requests`, so `kubectl apply` would be rejected outright, and the - only way past that is deleting a claim whose `cinder-csi` class reclaims with - `Delete` — destroying the volume section 5 rolls back to. -7. Seed `.demos` via the fixed initContainer above, and create the `.nfs-probe` - sentinel. -8. Repoint the streamlit and rq-worker Deployments at the new claim. Delete the - `nodeselector.yaml` patches; keep the `memory-tier-*` components as resource - patches; set requests == limits. -9. Deploy the storage canary and the sidebar indicator. +**Steps 1–9 below shipped as manifests and are no longer performed by hand.** +They are kept as a description of what the two applies do, and of the reasoning +behind each, because the reasoning is not recoverable from the YAML alone. The +whole cutover is: + +```bash +kubectl kustomize --enable-helm k8s/storage/ \ + | k8s/storage/set-node-cidrs.sh \ + | kubectl apply -f - + +kubectl -n template-app-storage rollout status statefulset -l app=nfs-server --timeout=300s +kubectl apply -k k8s/overlays/prod/ +``` + +Storage root first: it publishes the StorageClass the workspaces PVC claims, so +the other order leaves every pod `Pending` on a class that does not exist. + +1. `template-app-storage`, labelled `pod-security.kubernetes.io/enforce=privileged` + — `k8s/storage/namespace.yaml`. +2. ~~Label `openms` explicitly `enforce=baseline`.~~ **Reversed during + implementation.** `k8s/base/namespace.yaml` sets `warn` and `audit` at + baseline and does **not** set `enforce`. The reasoning is recorded there: the + app's own pods have no `securityContext`, no `hostPath`, no host namespaces + and no added capabilities anywhere in `k8s/base` or the overlay, so enforcing + buys nothing it does not already satisfy, while an enforced level that + tightens under a cluster upgrade can refuse to admit pods. The storage tier, + which does need more, is isolated in its own namespace for exactly this + reason. **A fork that adds a privileged sidecar to `openms` should revisit + this** — the audit trail will show the violation, but nothing will stop it. +3. The new Cinder PVC — `k8s/storage/nfs-backing-pvc.yaml`. +4. Ganesha with `existingClaim`, `Export_Id: 1`, `deviceBasedFsids: false`, + `strategy: Recreate`, 1 replica and explicit memory request and limit — + `k8s/storage/ganesha-values.yaml`. CI asserts all four + (`assert_storage_identity_values`, `assert_fsids_pinned`). +5. The default-deny ingress plus the two allow rules on 2049 — + `k8s/storage/networkpolicy.yaml`. The node rule is **not optional** and ships + as a placeholder: the provisioner emits in-tree `nfs:` PVs which the kubelet + mounts from the node's own address in the host network namespace, matching no + `podSelector`, so with the placeholder left in place every mount hangs. + `set-node-cidrs.sh` in the pipeline above supplies the real addresses — one + `/32` per node — and refuses if any of them falls inside the pod network, or + if the cluster runs a Cilium whose `policy-cidr-match-mode` would ignore the + rule. **Do not hand-edit the CIDR instead**; that puts cluster-specific + configuration into a tracked manifest. +6. The workspaces PVC in `openms` on the new StorageClass, at the unchanged path + `/workspaces-streamlit-template` — `k8s/base/workspace-pvc.yaml`. It is a NEW + claim, `workspaces-nfs-pvc`, not an edit of `workspaces-pvc`: a bound PVC's + spec is immutable apart from `resources.requests`, so `kubectl apply` would be + rejected outright, and the only way past that is deleting a claim whose + `cinder-csi` class reclaims with `Delete` — destroying the volume section 5 + rolls back to. +7. `.demos` seeding — now an initContainer running `docker/seed-demos.sh` + (`k8s/base/streamlit-deployment.yaml`), not the inline script in section 2. + The `.nfs-probe` sentinel is created by the worker's readiness probe on first + run (`src/workflow/health.py`). +8. The Deployments already mount the new claim; the `nodeselector.yaml` patches + are deleted and `assert_no_node_pinning_anywhere` is the ratchet that keeps + them deleted. `memory-tier-*` remain as resource patches with + requests == limits. +9. The sidebar indicator reads the heartbeats `probe_storage()` publishes. --- diff --git a/docs/kubernetes-deployment.md b/docs/kubernetes-deployment.md index 25e1aa66..879f21ef 100644 --- a/docs/kubernetes-deployment.md +++ b/docs/kubernetes-deployment.md @@ -322,22 +322,27 @@ kubectl -n openms rollout restart deployment/-streamlit ### Step 6 — Deploy -**Set the node CIDR first.** `k8s/storage/networkpolicy.yaml` ships `192.0.2.0/24` (RFC 5737 TEST-NET-1) as a placeholder, which admits nothing. The provisioner emits in-tree `nfs:` volumes, and the kubelet mounts those from the node's own address rather than from a pod IP, so without this every mount hangs on a CNI that enforces NetworkPolicy. Read the node addresses off the cluster and narrow the range as far as it will go: +Apply the storage tier **before** the overlay. It publishes the StorageClass the workspaces PVC claims; applied in the other order, every pod sits `Pending` on a class that does not exist yet: ```bash -kubectl get nodes -o wide # the INTERNAL-IP column -``` - -Then apply the storage tier **before** the overlay. It publishes the StorageClass the workspaces PVC claims; applied in the other order, every pod sits `Pending` on a class that does not exist yet: +kubectl kustomize --enable-helm k8s/storage/ \ + | k8s/storage/set-node-cidrs.sh \ + | kubectl apply -f - -```bash -kubectl kustomize --enable-helm k8s/storage/ | kubectl apply -f - kubectl -n template-app-storage rollout status statefulset -l app=nfs-server --timeout=300s kubectl apply -k k8s/overlays/prod/ ``` -The first of those needs Helm on `PATH`. Both applies are idempotent, and on an upgrade the storage one is usually a no-op. +**Do not skip `set-node-cidrs.sh`, and do not edit the CIDR by hand instead.** `k8s/storage/networkpolicy.yaml` ships `192.0.2.0/24` (RFC 5737 TEST-NET-1) as a placeholder, which admits nothing. The provisioner emits in-tree `nfs:` volumes and the kubelet mounts those from the node's own address rather than from a pod IP — matching no `podSelector` — so on a CNI that enforces NetworkPolicy, an `ipBlock` covering the nodes is the only thing that admits the mount. Left unset, every mount hangs with `mount.nfs: Connection timed out`, which names nothing. + +The script reads the node addresses from the cluster you are pointed at and patches the **rendered stream** — it never edits a tracked file, so your fork does not diverge from upstream on a cluster-specific line. It emits one `/32` per node, which is tighter than a range chosen by hand, and it refuses rather than proceeding when: + +- a node address falls inside the pod network (the export is `no_root_squash`, so that would give every pod in the cluster root over every workspace); +- it cannot evaluate that overlap at all; +- the cluster runs **Cilium** with `policy-cidr-match-mode` unset. From Cilium 1.14 remote nodes carry the `remote-node` identity and CIDR rules do not select node identities without that flag — so the policy would be correct and silently ignored, producing exactly the same hang. Fix the cluster, or override with `ALLOW_CILIUM_WITHOUT_NODE_CIDR_MATCH=1` if you know it admits node traffic another way. **Do not widen the CIDR to work around it.** + +It needs `kubectl`, `yq` and `python3` on `PATH`; the first apply also needs Helm. Both applies are idempotent, and on an upgrade the storage one is usually a no-op. ### Step 7 — Verify diff --git a/k8s/storage/networkpolicy.yaml b/k8s/storage/networkpolicy.yaml index a4087f14..71dff851 100644 --- a/k8s/storage/networkpolicy.yaml +++ b/k8s/storage/networkpolicy.yaml @@ -126,22 +126,38 @@ spec: # contain the POD network hands every pod in the cluster, NuXL included, root # over every workspace, and nothing about that is visible from the outside. # -# Set it to the range covering the INTERNAL IPs of the cluster's nodes. The -# addresses are in `kubectl get nodes -o wide` under INTERNAL-IP. Then check -# the range against the pod and service networks before applying - if it -# overlaps either, narrow it. One /32 per node is perfectly acceptable, and is -# what a small fixed cluster should use. Note that a hostNetwork pod shares -# its node's address and is admitted by this rule whatever its labels say; -# that is inherent to naming nodes by address, and is the reason the range -# should be as tight as the cluster allows. +# DO NOT EDIT THIS VALUE IN PLACE. Pipe the rendered root through +# k8s/storage/set-node-cidrs.sh, which reads the cluster's own node addresses +# and patches the STREAM: # -# docs/a16-storage-runbook.md section 3 step 5 carries this as an explicit cutover step. +# kubectl kustomize --enable-helm k8s/storage/ \ +# | k8s/storage/set-node-cidrs.sh \ +# | kubectl apply -f - +# +# Editing it here would put cluster-specific configuration inside a tracked +# manifest, so a fork's tree diverges from upstream on exactly this line and +# every pull can conflict on it. The script emits one /32 per node - tighter +# than any range a human would pick - and refuses outright if a node address +# falls inside the pod network, or if the cluster runs a Cilium that would +# ignore the rule (see below). The placeholder stays here so that an operator +# who bypasses the script still gets the loud, safe failure rather than a +# wide-open policy. +# +# A hostNetwork pod shares its node's address and is admitted by this rule +# whatever its labels say. That is inherent to naming nodes by address, and is +# why the script emits /32s rather than a covering range. +# +# docs/a16-storage-runbook.md section 3 step 5 and +# docs/kubernetes-deployment.md step 6 both carry this as a cutover step. # # Cilium, which is what the de.NBI user clusters run: from 1.14 remote nodes # carry the `remote-node` identity, and CIDR rules do not select node -# identities unless the agent runs with `policy-cidr-match-mode=nodes`. If -# mounts still hang with the correct CIDR in place, that flag is the next -# thing to check - not a wider selector here. +# identities unless the agent runs with `policy-cidr-match-mode=nodes`. The +# script detects this and refuses, because the symptom is otherwise identical +# to the placeholder being left in place - every mount hangs, with nothing +# naming the cause. CI cannot catch it: kind runs kindnetd, which enforces +# ipBlock normally. If mounts hang with correct /32s in place, that flag is +# the next thing to check - not a wider selector here. apiVersion: networking.k8s.io/v1 kind: NetworkPolicy metadata: diff --git a/k8s/storage/set-node-cidrs.sh b/k8s/storage/set-node-cidrs.sh new file mode 100755 index 00000000..d34b07fc --- /dev/null +++ b/k8s/storage/set-node-cidrs.sh @@ -0,0 +1,185 @@ +#!/usr/bin/env bash +# Inject the cluster's own node addresses into the storage NetworkPolicy. +# +# kubectl kustomize --enable-helm k8s/storage/ \ +# | k8s/storage/set-node-cidrs.sh \ +# | kubectl apply -f - +# +# WHY THIS EXISTS +# +# `allow-nfs-from-nodes` in networkpolicy.yaml ships an ipBlock of +# 192.0.2.0/24 - RFC 5737 TEST-NET-1, routed nowhere - because the addresses it +# actually needs are a property of the cluster, not of this repository. The +# provisioner emits in-tree `nfs:` PersistentVolumes, which the KUBELET mounts +# from the node's own address in the host network namespace. That matches no +# podSelector, so on a policy-enforcing CNI an ipBlock covering the nodes is the +# only thing standing between a worker and an indefinitely hanging mount. +# +# Until now the documented procedure was to hand-edit that value before the +# first deploy. That is the wrong shape for a template: it puts cluster-specific +# configuration inside a tracked manifest, so every fork's tree diverges from +# upstream on exactly one line and every `git pull` can conflict on it. Worse, +# the edit is easy to forget and its failure mode is a forty-minute hang whose +# message ("mount.nfs: Connection timed out") names nothing. +# +# So the placeholder STAYS as the shipped default - an operator who bypasses +# this script still gets the loud, safe failure rather than a wide-open policy - +# and the real value is computed here and patched into the RENDERED STREAM. +# Nothing on disk is modified. This is the same shape the CI kind jobs have used +# since the storage tier landed, which is the only version of this mechanism +# that has ever been exercised. +# +# WHAT IT EMITS +# +# One /32 per node, not a covering range. The tightest possible rule, and +# tighter than a human would reasonably pick by hand: a hostNetwork pod shares +# its node's address and is admitted by this rule whatever its labels say, so +# every address in the range that is not a node is a hole. +# +# Requires: kubectl (pointed at the target cluster), yq. + +set -euo pipefail + +POLICY_NAME="${POLICY_NAME:-allow-nfs-from-nodes}" +PLACEHOLDER="${PLACEHOLDER:-192.0.2.0/24}" + +die() { printf '\nERROR: %s\n' "$*" >&2; exit 1; } +note() { printf '%s\n' "$*" >&2; } + +for tool in kubectl yq; do + command -v "$tool" >/dev/null 2>&1 || die "$tool is not on PATH" +done + +manifest="$(cat)" +[ -n "$manifest" ] || die "no manifests on stdin; pipe 'kubectl kustomize --enable-helm k8s/storage/' into this script" + +# Refuse if the placeholder is gone. Without this the patch below silently +# no-ops on a manifest someone has already hand-edited, and the operator is +# left believing this script set the value when it did not. +printf '%s' "$manifest" | grep -q "cidr: $PLACEHOLDER" \ + || die "the rendered manifests do not contain 'cidr: $PLACEHOLDER'. + Either networkpolicy.yaml no longer carries the placeholder - in which + case this patch would silently do nothing - or the value has already + been set by hand, which this script is meant to replace." + +# --- the addresses ----------------------------------------------------------- + +nodes="$(kubectl get nodes \ + -o jsonpath='{range .items[*]}{.status.addresses[?(@.type=="InternalIP")].address}{"\n"}{end}' \ + 2>/dev/null | grep -v '^$' || true)" +[ -n "$nodes" ] || die "kubectl returned no node InternalIPs. Is kubectl pointed at the target cluster?" + +node_count="$(printf '%s\n' "$nodes" | wc -l | tr -d ' ')" +note "nodes: $(printf '%s' "$nodes" | tr '\n' ' ')" + +# --- the safety check that matters ------------------------------------------- +# +# The export is no_root_squash: anything that can reach 2049 acts as root on +# every user's workspace. A rule admitting the POD network would therefore hand +# every pod in the cluster - including other tenants' - root over every +# workspace, and nothing about that is visible from outside. +# +# A node address should never sit inside the pod or service CIDR, so this +# should never fire. It is here because the consequence of it firing unnoticed +# is unbounded, and because the check costs one API read. + +pod_cidr="$(kubectl -n kube-system get cm cilium-config \ + -o jsonpath='{.data.cluster-pool-ipv4-cidr}' 2>/dev/null || true)" +if [ -z "$pod_cidr" ]; then + pod_cidr="$(kubectl get nodes -o jsonpath='{.items[0].spec.podCIDR}' 2>/dev/null || true)" +fi + +if [ "${SKIP_POD_CIDR_CHECK:-}" = "1" ]; then + note "WARNING: SKIP_POD_CIDR_CHECK=1 - the pod-network overlap check was skipped." +elif [ -n "$pod_cidr" ]; then + # Exit codes, not output. `command -v python3 && python3 ... || true` looks + # equivalent and is not: an interpreter that EXISTS but fails to run - the + # Windows Store stub is one, a broken venv shim is another - yields empty + # output, which is indistinguishable from "checked, found nothing" and + # passes silently. That is the failure this whole check exists to prevent, + # reproduced one level up. So: 0 = clean, 1 = overlap, anything else = the + # check did not run, and the caller must not read that as clean. + set +e + overlap="$(NODES="$nodes" POD_CIDR="$pod_cidr" python3 -c ' +import ipaddress, os, sys +try: + net = ipaddress.ip_network(os.environ["POD_CIDR"], strict=False) +except ValueError: + sys.exit(3) +bad = [n for n in os.environ["NODES"].split() + if ipaddress.ip_address(n) in net] +if bad: + print(" ".join(bad)) + sys.exit(1) +sys.exit(0)' 2>/dev/null)" + rc=$? + set -e + case "$rc" in + 0) note "pod network $pod_cidr contains no node address - ok" ;; + 1) die "node address(es) $overlap fall inside the pod network $pod_cidr. + Admitting them would also admit every pod in the cluster, and the NFS + export is no_root_squash - that is root on every user's workspace. + Refusing." ;; + *) die "could not evaluate whether the node addresses overlap the pod + network $pod_cidr (python3 exited $rc). Refusing rather than assuming + they do not: getting this wrong exposes every workspace to every pod. + Install a working python3, or set SKIP_POD_CIDR_CHECK=1 having checked + by hand that no node address falls inside $pod_cidr." ;; + esac +else + note "NOTE: could not determine the pod CIDR, so the overlap check did not run. + Confirm by hand that no node address above falls inside the pod network." +fi + +# --- Cilium ------------------------------------------------------------------ +# +# Cilium is what the de.NBI user clusters run, and from 1.14 remote nodes carry +# the `remote-node` identity. CIDR rules do NOT select node identities unless +# the agent runs with policy-cidr-match-mode=nodes, so on a default Cilium the +# rule this script writes is correct and still ignored - and the symptom is the +# same silent hang the placeholder produces. +# +# CI cannot catch this: kind runs kindnetd, which enforces ipBlock normally. So +# eight green kind jobs say nothing about whether Cilium will honour this. + +if kubectl -n kube-system get cm cilium-config >/dev/null 2>&1; then + mode="$(kubectl -n kube-system get cm cilium-config \ + -o jsonpath='{.data.policy-cidr-match-mode}' 2>/dev/null || true)" + if [ "$mode" != "nodes" ]; then + if [ "${ALLOW_CILIUM_WITHOUT_NODE_CIDR_MATCH:-}" = "1" ]; then + note "WARNING: Cilium has policy-cidr-match-mode='${mode:-}', not 'nodes'. + Proceeding because ALLOW_CILIUM_WITHOUT_NODE_CIDR_MATCH=1. If mounts + hang, this is the first thing to re-check." + else + die "Cilium is installed and policy-cidr-match-mode is '${mode:-}', not 'nodes'. + From Cilium 1.14 remote nodes carry the 'remote-node' identity, and CIDR + rules do not select node identities unless that flag is set - so the + policy this script writes would be correct and silently ignored, and + every cross-node mount would hang with no message naming the cause. + + Fix the cluster (set policy-cidr-match-mode=nodes on the Cilium agent), + or re-run with ALLOW_CILIUM_WITHOUT_NODE_CIDR_MATCH=1 if you know this + cluster admits node traffic some other way. + + Do NOT widen the CIDR to work around this. A range wide enough to be + matched by a different mechanism is a range wide enough to expose the + export." + fi + else + note "Cilium policy-cidr-match-mode=nodes - CIDR rules select node identities, ok" + fi +fi + +# --- patch ------------------------------------------------------------------- + +blocks="[" +for ip in $nodes; do + blocks="$blocks{\"ipBlock\":{\"cidr\":\"$ip/32\"}}," +done +blocks="${blocks%,}]" + +note "admitting $node_count node(s) on TCP 2049, one /32 each" + +printf '%s' "$manifest" | yq " + (select(.kind == \"NetworkPolicy\" and .metadata.name == \"$POLICY_NAME\") + | .spec.ingress[0].from) = $blocks" From 3cbfe34bfbbff0cd0907ed1f4a8fa62188115600 Mon Sep 17 00:00:00 2001 From: User Date: Fri, 28 Aug 2026 16:07:43 +0200 Subject: [PATCH 2/2] Wrap the deploy sequence in one command The cutover was three commands that have to run in one specific order, with a pipeline step in the middle that is easy to leave out. Each way of getting it wrong fails silently and expensively: - overlay before storage root: every pod sits Pending on a StorageClass that does not exist, and the message says nothing about ordering; - `kubectl apply -k` on the storage root: no --enable-helm, so the Ganesha chart is never inflated; - skipping set-node-cidrs.sh: the shipped placeholder stays, and every workspace mount hangs on `mount.nfs: Connection timed out` naming nothing. k8s/deploy.sh runs exactly what the documented pipelines run, in the documented order, and adds only the waits between them plus a prerequisite check before anything is touched. It is a wrapper, not a new mechanism - there is nothing in it a reader has to take on trust. It confirms the target cluster first. Both namespaces are named the same on every cluster, so nothing in the later output would reveal that it went to the wrong one. Without a terminal it refuses rather than guessing; `--yes` is the deliberate override, and `--dry-run` renders and server-side validates both roots while applying nothing. The two roots still cannot be merged into one - the reasoning is at the top of k8s/storage/kustomization.yaml and is about the namespace transformer clobbering per-object namespaces - so this wraps the ordering rather than removing it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018tuhTmRVKxCXo5gJSJU8e8 --- docs/a16-storage-runbook.md | 11 ++- docs/kubernetes-deployment.md | 14 +++- k8s/deploy.sh | 136 ++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100755 k8s/deploy.sh diff --git a/docs/a16-storage-runbook.md b/docs/a16-storage-runbook.md index 47cf1d98..33348975 100644 --- a/docs/a16-storage-runbook.md +++ b/docs/a16-storage-runbook.md @@ -140,6 +140,12 @@ They are kept as a description of what the two applies do, and of the reasoning behind each, because the reasoning is not recoverable from the YAML alone. The whole cutover is: +```bash +k8s/deploy.sh # --dry-run first if you want to see it render and validate +``` + +which is, expanded: + ```bash kubectl kustomize --enable-helm k8s/storage/ \ | k8s/storage/set-node-cidrs.sh \ @@ -150,7 +156,10 @@ kubectl apply -k k8s/overlays/prod/ ``` Storage root first: it publishes the StorageClass the workspaces PVC claims, so -the other order leaves every pod `Pending` on a class that does not exist. +the other order leaves every pod `Pending` on a class that does not exist. The +script confirms which cluster it is pointed at before touching anything, because +both namespaces are named the same on every cluster and nothing in the later +output would tell you it had gone to the wrong one. 1. `template-app-storage`, labelled `pod-security.kubernetes.io/enforce=privileged` — `k8s/storage/namespace.yaml`. diff --git a/docs/kubernetes-deployment.md b/docs/kubernetes-deployment.md index 879f21ef..8d61b264 100644 --- a/docs/kubernetes-deployment.md +++ b/docs/kubernetes-deployment.md @@ -322,7 +322,19 @@ kubectl -n openms rollout restart deployment/-streamlit ### Step 6 — Deploy -Apply the storage tier **before** the overlay. It publishes the StorageClass the workspaces PVC claims; applied in the other order, every pod sits `Pending` on a class that does not exist yet: +```bash +k8s/deploy.sh # or --dry-run to render and validate, applying nothing +``` + +That is the whole deploy. It prints the context and cluster it is about to touch and asks for confirmation first (`--yes` to skip, required when there is no terminal), then does the three things below in the one order that works, waiting between them. + +**There is no single `kubectl apply -k` for this, and the reasons are worth knowing** — each is a silent, expensive failure if you do it by hand and get it wrong: + +1. **Two roots, in order.** `k8s/storage/` publishes the StorageClass `k8s/base/workspace-pvc.yaml` claims. The other order leaves every pod `Pending` on a class that does not exist, and the message says nothing about ordering. They cannot be merged into one root either — see the top of `k8s/storage/kustomization.yaml`, which is about the namespace transformer clobbering per-object namespaces. +2. **`kubectl apply -k` has no `--enable-helm`.** The storage root inflates the Ganesha chart, so it must be rendered and piped. +3. **The node addresses are not in the repo.** See below. + +Done by hand, it is: ```bash kubectl kustomize --enable-helm k8s/storage/ \ diff --git a/k8s/deploy.sh b/k8s/deploy.sh new file mode 100755 index 00000000..2bf5e569 --- /dev/null +++ b/k8s/deploy.sh @@ -0,0 +1,136 @@ +#!/usr/bin/env bash +# Deploy the whole thing, in the one order that works. +# +# k8s/deploy.sh # deploy +# k8s/deploy.sh --dry-run # render and check everything, apply nothing +# k8s/deploy.sh --yes # skip the context confirmation +# +# WHY A SCRIPT AND NOT `kubectl apply -k` +# +# Three constraints make a single apply impossible, and each of them is a +# silent, expensive failure when a human gets it wrong at the end of a long day: +# +# 1. TWO ROOTS, IN ORDER. k8s/storage/ publishes the StorageClass that +# k8s/base/workspace-pvc.yaml claims. Applied the other way round every pod +# sits Pending on a class that does not exist, and the message says nothing +# about ordering. They cannot be merged into one root either - the reasoning +# is at the top of k8s/storage/kustomization.yaml, and it is about the +# namespace transformer clobbering per-object namespaces. +# +# 2. `kubectl apply -k` HAS NO --enable-helm. The storage root inflates the +# Ganesha chart, so it has to be rendered and piped. +# +# 3. THE NODE ADDRESSES ARE NOT IN THE REPO. See set-node-cidrs.sh. Forgetting +# that step leaves the shipped placeholder in place and every workspace +# mount hangs on `mount.nfs: Connection timed out`, forty minutes later, +# naming nothing. +# +# So this wraps the sequence rather than inventing a mechanism. It applies +# exactly what the documented pipelines apply, in the documented order, and adds +# only the waits between them and a check that the prerequisites are present +# before anything is touched. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +STORAGE_ROOT="${STORAGE_ROOT:-k8s/storage}" +OVERLAY="${OVERLAY:-k8s/overlays/prod}" +STORAGE_NS="${STORAGE_NS:-template-app-storage}" + +DRY_RUN=0 +ASSUME_YES=0 +for arg in "$@"; do + case "$arg" in + --dry-run) DRY_RUN=1 ;; + --yes|-y) ASSUME_YES=1 ;; + -h|--help) sed -n '2,8p' "$0" | sed 's/^# \?//'; exit 0 ;; + *) printf 'unknown argument: %s\n' "$arg" >&2; exit 2 ;; + esac +done + +step() { printf '\n\033[1m==> %s\033[0m\n' "$*"; } +die() { printf '\nERROR: %s\n' "$*" >&2; exit 1; } + +# --- prerequisites, before anything is touched ------------------------------- + +for tool in kubectl helm yq python3; do + command -v "$tool" >/dev/null 2>&1 || die "$tool is not on PATH. + kubectl and helm render the storage root; yq and python3 are used by + set-node-cidrs.sh to write and bounds-check the node addresses." +done +[ -x "$STORAGE_ROOT/set-node-cidrs.sh" ] || die "$STORAGE_ROOT/set-node-cidrs.sh is missing or not executable" + +ctx="$(kubectl config current-context 2>/dev/null || true)" +[ -n "$ctx" ] || die "kubectl has no current context" +srv="$(kubectl config view --minify -o jsonpath='{.clusters[0].cluster.server}' 2>/dev/null || true)" + +printf '\ncontext : %s\ncluster : %s\nstorage : %s\noverlay : %s\n' \ + "$ctx" "${srv:-unknown}" "$STORAGE_ROOT" "$OVERLAY" + +# The context is confirmed because the two namespaces this touches are named the +# same on every cluster, so there is nothing in the later output that would tell +# you it went to the wrong one. +if [ "$DRY_RUN" -eq 0 ] && [ "$ASSUME_YES" -eq 0 ]; then + if [ -t 0 ]; then + printf '\nDeploy to this cluster? [y/N] ' + read -r reply + case "$reply" in [yY]*) ;; *) die "aborted" ;; esac + else + die "not a terminal and --yes was not given; refusing to guess which cluster you meant" + fi +fi + +# --- 1. storage root --------------------------------------------------------- + +step "Rendering $STORAGE_ROOT and injecting this cluster's node addresses" +rendered="$(mktemp)"; trap 'rm -f "$rendered"' EXIT +kubectl kustomize --enable-helm "$STORAGE_ROOT" \ + | "$STORAGE_ROOT/set-node-cidrs.sh" > "$rendered" +[ -s "$rendered" ] || die "the storage root rendered nothing" + +if [ "$DRY_RUN" -eq 1 ]; then + step "--dry-run: server-side validating the storage root" + kubectl apply --dry-run=server -f "$rendered" >/dev/null + step "--dry-run: server-side validating $OVERLAY" + kubectl apply --dry-run=server -k "$OVERLAY" >/dev/null + printf '\nBoth roots render and validate. Nothing was applied.\n' + exit 0 +fi + +step "Applying the storage root" +kubectl apply -f "$rendered" + +step "Waiting for the NFS server to become Ready" +# Before the overlay, not after: the workspaces PVC binds only once the +# provisioner is running, and a pod that starts first sits Pending on it. +kubectl -n "$STORAGE_NS" rollout status statefulset -l app=nfs-server --timeout=300s + +# --- 2. the app -------------------------------------------------------------- + +step "Applying $OVERLAY" +kubectl apply -k "$OVERLAY" + +ns="$(kubectl kustomize "$OVERLAY" | yq 'select(.kind == "Deployment") | .metadata.namespace' | head -n1)" +ns="${ns:-openms}" + +step "Waiting for the app to roll out" +for d in $(kubectl kustomize "$OVERLAY" | yq 'select(.kind == "Deployment") | .metadata.name'); do + kubectl -n "$ns" rollout status "deployment/$d" --timeout=300s +done + +# --- done -------------------------------------------------------------------- + +step "Deployed. Pod placement:" +kubectl -n "$ns" get pods -o wide + +cat <