diff --git a/src/compute-plane-services/nvsnap/cmd/agent/main.go b/src/compute-plane-services/nvsnap/cmd/agent/main.go index 8ecc62a44..55b85a587 100644 --- a/src/compute-plane-services/nvsnap/cmd/agent/main.go +++ b/src/compute-plane-services/nvsnap/cmd/agent/main.go @@ -71,6 +71,8 @@ func main() { "NvSnap-server base URL for peer-fanout catalog lookups (e.g. http://nvsnap-server.nvsnap-system.svc.cluster.local:8080). Empty disables cross-node cascade.") flag.StringVar(&config.NodeIP, "node-ip", os.Getenv("HOST_IP"), "This agent's reachable address from peers (downward API status.hostIP when hostNetwork:true). Empty disables peer registration.") + flag.StringVar(&config.AdvertiseIP, "advertise-ip", os.Getenv("POD_IP"), + "Address peers dial to reach this agent (downward API status.podIP; equals the node IP under hostNetwork). Falls back to --node-ip when empty. See GH #490.") flag.StringVar(&config.BlobStoreURL, "blob-store-url", os.Getenv("NVSNAP_BLOB_STORE_URL"), "NvSnap-blobstore base URL for Phase 5d.2 durable backstop (e.g. http://nvsnap-blobstore.nvsnap-system.svc.cluster.local:9000). Empty disables capture-side upload AND cascade tier-3 fallback.") // Cross-cluster replication (docs/design/cross-cluster-replication.md). @@ -90,6 +92,11 @@ func main() { flag.StringVar(&config.FSStorePath, "fsstore-path", os.Getenv("NVSNAP_FSSTORE_PATH"), "Path to a shared filesystem mounted on every node (Lustre/Weka/EFS/Filestore/NFS). When set, captures are published here and the restore cascade copies from this path before peer fanout. Empty disables.") flag.StringVar(&config.ListenAddr, "listen", ":8081", "Listen address") + // The token itself is env-only, never a flag: flag values show up in the + // pod spec and in `ps`, and this is a credential. + var authMode string + flag.StringVar(&authMode, "auth-mode", os.Getenv("NVSNAP_AGENT_AUTH_MODE"), + "Agent API authentication: disabled (default), permissive (check, log failures, still serve), or required (401). Token comes from NVSNAP_AGENT_TOKEN. See GH #486.") flag.StringVar(&config.CheckpointDir, "checkpoint-dir", "/var/lib/nvsnap/checkpoints", "Checkpoint storage directory (in-agent-container path)") flag.StringVar(&config.CheckpointHostDir, "checkpoint-host-dir", "/var/lib/containerd/nvsnap-checkpoints", "Host path that backs --checkpoint-dir (must match the DaemonSet hostPath mount; used to translate paths for the capture-write writer Job)") flag.StringVar(&config.CRIUPath, "criu-path", "/usr/local/sbin/criu", "Path to CRIU binary (on host filesystem)") @@ -183,6 +190,8 @@ func main() { "Strategy for restore-side overlay mount prep: inline (do mounts during admission, default) or init-container (delegate to nvsnap-mount-prep init container on the restored pod)") flag.StringVar(&config.Webhook.MountPrepInitImage, "webhook-mount-prep-init-image", "", "Image ref for the nvsnap-mount-prep init container injected when --webhook-restore-prep-strategy=init-container. Must contain /nvsnap-mount-prep (the agent image satisfies this).") + flag.StringVar(&config.Webhook.AgentBaseURL, "webhook-agent-base-url", os.Getenv("NVSNAP_WEBHOOK_AGENT_BASE_URL"), + "Base URL the injected nvsnap-mount-prep init container uses to reach its node-local agent. Empty uses http://$(NVSNAP_HOST_IP):, which requires hostPort. Set to the internalTrafficPolicy:Local Service under pod networking. See GH #490.") flag.IntVar(&config.Webhook.AgentHostPort, "webhook-agent-host-port", 8081, "Port the nvsnap-mount-prep init container reaches the agent on (matches --listen and the agent DaemonSet's hostPort).") @@ -204,6 +213,19 @@ func main() { "imagePullSecret name for the mount-holder pod (created by operators in the workload namespace). Defaults to nvsnap-agent-pull; set to '-' to disable.") flag.Parse() + + // Fail startup on a bad mode rather than falling back to disabled: an + // operator who typo'd --auth-mode should hear about it now, not discover + // months later that the API was open the whole time. + var authErr error + if config.AuthMode, authErr = agent.ParseAuthMode(authMode); authErr != nil { + logrus.WithError(authErr).Fatal("invalid --auth-mode") + } + config.AuthToken = os.Getenv("NVSNAP_AGENT_TOKEN") + if config.AuthMode != agent.AuthDisabled && config.AuthToken == "" { + logrus.Fatalf("--auth-mode=%s requires NVSNAP_AGENT_TOKEN to be set", config.AuthMode) + } + config.RootfsCapture.WarmupDelay = time.Duration(rootfsWarmupSec) * time.Second for _, b := range strings.Split(replicationPeerBuckets, ",") { if b = strings.TrimSpace(b); b != "" { diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go index 0bd86bc49..55fff09f8 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go @@ -40,6 +40,9 @@ limitations under the License. // NVSNAP_POD_UID (required) downward API: metadata.uid // NVSNAP_RESTORE_HASH (required) full sha256 of the capture // NVSNAP_AGENT_URL (required) e.g. http://$(HOST_IP):8081 +// NVSNAP_AGENT_TOKEN (optional) bearer token for the agent API (GH #486); +// empty sends no header, which is correct while the +// agent still runs with auth disabled // NVSNAP_CAPTURE_NODE (optional) where capture data lives; empty=this node // NVSNAP_PREP_MOUNTS (required) JSON-encoded []VolumeMeta from the manifest // NVSNAP_PREP_DEADLINE (optional) duration; default 15m @@ -196,6 +199,7 @@ func startWithRetry(agentURL string, req prepRequest) error { return err } httpReq.Header.Set("Content-Type", "application/json") + setAgentAuth(httpReq) resp, err := http.DefaultClient.Do(httpReq) if err != nil { lastErr = err @@ -220,6 +224,7 @@ func startWithRetry(agentURL string, req prepRequest) error { func getStatus(agentURL, podUID string) (*prepStatus, error) { httpReq, err := http.NewRequestWithContext(context.Background(), http.MethodGet, agentURL+"/v1/restore/prep/"+podUID, http.NoBody) + setAgentAuth(httpReq) if err != nil { return nil, err } @@ -244,3 +249,15 @@ func getStatus(agentURL, podUID string) (*prepStatus, error) { } return &s, nil } + +// setAgentAuth attaches the agent API bearer token when one is configured. +// Empty is the normal state until the operator turns auth on, and sending no +// header is exactly what a disabled or permissive agent expects. See GH #486. +func setAgentAuth(r *http.Request) { + if r == nil { + return + } + if tok := os.Getenv("NVSNAP_AGENT_TOKEN"); tok != "" { + r.Header.Set("Authorization", "Bearer "+tok) + } +} diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-server/main.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-server/main.go index 4c89a802b..878c636c0 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-server/main.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-server/main.go @@ -106,6 +106,11 @@ func run(cmd *cobra.Command, args []string) error { Address: address, AgentPort: agentPort, BlobstoreURL: blobstoreURL, + // Env rather than a flag: the chart projects it straight from the + // nvsnap-agent-token Secret, and a flag would put the credential in + // the process argv where any pod-reading client can see it. + // Empty when auth is off, which sends no header (nvsnap#736). + AgentToken: os.Getenv("NVSNAP_AGENT_TOKEN"), }, kubeClient, dynClient, catalog) // Embed React UI — serves at / with SPA fallback diff --git a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml index cb00d22f8..b4b4dfc5d 100644 --- a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml @@ -42,9 +42,9 @@ spec: {{- toYaml .Values.agent.tolerations | nindent 8 }} hostPID: {{ .Values.agent.hostPID }} hostNetwork: {{ .Values.agent.hostNetwork }} - # ClusterFirstWithHostNet so Service DNS still resolves under - # hostNetwork (nvsnap-server..svc, nvsnap-blobstore..svc). - dnsPolicy: ClusterFirstWithHostNet + # ClusterFirstWithHostNet is only correct under hostNetwork; with pod + # networking it is wrong (it points resolution at the node's resolv.conf). + dnsPolicy: {{ if .Values.agent.hostNetwork }}ClusterFirstWithHostNet{{ else }}ClusterFirst{{ end }} serviceAccountName: nvsnap-agent {{- include "nvsnap.imagePullSecrets" . | nindent 6 }} initContainers: @@ -82,6 +82,18 @@ spec: image: {{ include "nvsnap.agent.image" . }} imagePullPolicy: {{ .Values.agent.image.pullPolicy }} args: + {{- if .Values.agent.auth.enabled }} + # permissive counts and logs unauthenticated callers but still + # serves them; required returns 401. Roll out on permissive until + # nvsnap_agent_auth_total{result="missing"} is zero. + - --auth-mode={{ .Values.agent.auth.mode }} + {{- end }} + {{- if not .Values.agent.hostNetwork }} + # Pod networking: the init container reaches its node-local agent + # through the internalTrafficPolicy:Local Service instead of the + # node IP, so no hostPort is needed (GH #490). + - --webhook-agent-base-url=http://nvsnap-agent-local.{{ .Release.Namespace }}.svc.cluster.local:8081 + {{- end }} - --cuda-checkpoint-path=/criu-bundle/cuda-checkpoint - --criu-path=/criu-bundle/criu # Translate in-container --checkpoint-dir to the host path @@ -168,6 +180,24 @@ spec: valueFrom: fieldRef: fieldPath: status.hostIP + # POD_IP is what peers dial (--advertise-ip). Under hostNetwork + # kubelet reports status.podIP as the node IP, so this is correct + # in both network modes and needs no conditional (GH #490). + - name: POD_IP + valueFrom: + fieldRef: + fieldPath: status.podIP + {{- if .Values.agent.auth.enabled }} + # Shared bearer token for the agent API (GH #486). Env rather than + # a flag: flag values are visible in the pod spec and in `ps`. + # The agent uses it both to verify inbound requests and to sign + # its own peer calls. + - name: NVSNAP_AGENT_TOKEN + valueFrom: + secretKeyRef: + name: nvsnap-agent-token + key: token + {{- end }} {{- if .Values.server.enabled }} - name: NVSNAP_CATALOG_URL value: "http://nvsnap-server.{{ .Release.Namespace }}.svc.cluster.local:{{ .Values.server.service.port }}" @@ -225,7 +255,13 @@ spec: {{- end }} ports: - containerPort: 8081 + {{- if .Values.agent.hostNetwork }} + # Binds the API to every node's IP. Only declared under + # hostNetwork; with pod networking peers dial the pod IP and + # same-node callers use the internalTrafficPolicy:Local + # Service, so no node-wide listener is needed (GH #490). hostPort: 8081 + {{- end }} name: http-api {{- if .Values.webhook.enabled }} - containerPort: 8443 @@ -386,3 +422,33 @@ spec: name: http-api clusterIP: None {{- end }} + +{{- if not .Values.agent.hostNetwork }} +--- +# Node-local Service: the pod-network replacement for hostPort (GH #490). +# +# Callers that must reach the agent on THEIR OWN node -- the nvsnap-mount-prep +# init container is the one that matters -- used to do it via +# http://$(status.hostIP):8081, which requires the API to be bound to every +# node's IP. internalTrafficPolicy:Local is the Kubernetes-native way to say +# the same thing: this ClusterIP only ever routes to the endpoint on the +# calling node, and has no node-IP listener at all. +# +# The headless nvsnap-agent Service above stays for tools that want to address +# a specific agent; this one is for "whichever agent is on my node". +apiVersion: v1 +kind: Service +metadata: + name: nvsnap-agent-local + namespace: {{ .Release.Namespace }} + labels: + {{- include "nvsnap.agent.labels" . | nindent 4 }} +spec: + selector: + {{- include "nvsnap.agent.selectorLabels" . | nindent 4 }} + internalTrafficPolicy: Local + ports: + - port: 8081 + targetPort: 8081 + name: http-api +{{- end }} diff --git a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml new file mode 100644 index 000000000..1503576b9 --- /dev/null +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml @@ -0,0 +1,88 @@ +{{- if .Values.agent.auth.enabled }} +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Shared bearer token for the agent HTTP API (GH #486). +# +# The agent API is the control surface of a privileged process and the +# DaemonSet binds it to every node's IP, so it needs authentication in the +# request path. This Secret holds the token both the agent (to verify) and its +# callers (to present) read. +# +# Generated once and preserved across upgrades: `helm upgrade` re-renders every +# template, so a freshly random token on each upgrade would rotate the +# credential out from under running callers and cause a self-inflicted outage +# mid-rollout. The lookup below reuses the existing value when the Secret is +# already present. Set agent.auth.token explicitly to manage it yourself (or to +# rotate deliberately). +{{- $ns := .Release.Namespace }} +{{- $name := "nvsnap-agent-token" }} +{{- $existing := lookup "v1" "Secret" $ns $name }} +{{- $token := "" }} +{{- if .Values.agent.auth.token }} +{{- $token = .Values.agent.auth.token | b64enc }} +{{- else if and $existing $existing.data $existing.data.token }} +{{- $token = $existing.data.token }} +{{- else }} +# Reached on a first install, and on any client-side render -- `lookup` returns +# empty without cluster access, so an existing Secret is invisible here and +# indistinguishable from a missing one. +# +# That makes `helm template` non-deterministic: every run mints a different +# token, and `helm template | kubectl apply` would rotate the credential out +# from under running agents, which keep the old value in their environment +# while fresh mount-prep pods get the new one and 401 until the DaemonSet +# rolls. Set agent.auth.token to render offline safely. +# +# Not guarded with `fail`, because `lookup` is equally empty under +# `helm install --dry-run` -- which scripts/install-nvsnap.sh --dry-run +# relies on, and which applies nothing. helm install/upgrade is the supported +# path and does see the existing Secret, so the token survives upgrades. +{{- $token = randAlphaNum 48 | b64enc }} +{{- end }} +apiVersion: v1 +kind: Secret +metadata: + name: {{ $name }} + namespace: {{ $ns }} + labels: + app.kubernetes.io/name: nvsnap + app.kubernetes.io/part-of: nvsnap + annotations: + # helm.sh/resource-policy keeps the Secret if the release is removed with + # --keep-history style workflows; without it a delete/reinstall cycle + # silently rotates the token. + helm.sh/resource-policy: keep +type: Opaque +data: + token: {{ $token }} +{{- /* + Replicate into every restore namespace. A SecretKeyRef resolves only in + the pod's own namespace, and the mount-prep init container the webhook + injects runs in the restore pod's namespace, not the release namespace. + Without a copy there its reference (marked optional, so the pod still + starts) silently resolves to nothing -- mount-prep then sends no header + and the agent 401s every restore once auth.mode=required. + + Same list the restore-pod NetworkPolicy uses, so a namespace already + declared for egress is covered here too with no new configuration. +*/}} +{{- range $rns := .Values.agent.l2.restoreNamespaces }} +{{- if ne $rns $ns }} +--- +apiVersion: v1 +kind: Secret +metadata: + name: {{ $name }} + namespace: {{ $rns }} + labels: + app.kubernetes.io/name: nvsnap + app.kubernetes.io/part-of: nvsnap + annotations: + helm.sh/resource-policy: keep +type: Opaque +data: + token: {{ $token }} +{{- end }} +{{- end }} +{{- end }} diff --git a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml index 49a0cb767..f2e14a306 100644 --- a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml @@ -108,7 +108,7 @@ Only rendered when the init-container strategy is selected AND agentHostCIDR is set. The default inline strategy does the mount inside the webhook and needs no pod->agent egress. */ -}} -{{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") .Values.webhook.agentHostCIDR .Values.agent.l2.restoreNamespaces }} +{{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) .Values.agent.l2.restoreNamespaces }} {{- range $ns := .Values.agent.l2.restoreNamespaces }} --- apiVersion: networking.k8s.io/v1 @@ -130,8 +130,25 @@ spec: - Egress egress: - to: + {{- if $.Values.agent.hostNetwork }} + # hostNetwork: the agent carries NODE identity, so a podSelector never + # matches it (verified on GKE Dataplane V2 / Cilium) and the rule has + # to name the whole node CIDR -- every node, on this port, for every + # pod in the namespace. - ipBlock: cidr: {{ $.Values.webhook.agentHostCIDR }} + {{- else }} + # Pod networking: the agent has a pod identity again, so the rule can + # name exactly the agent pods and nothing else. This is the concrete + # payoff of GH #490 -- no operator-supplied CIDR, and the grant shrinks + # from "the node network" to "these pods". + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: {{ $.Release.Namespace }} + podSelector: + matchLabels: + {{- include "nvsnap.agent.selectorLabels" $ | nindent 14 }} + {{- end }} ports: - protocol: TCP port: {{ $.Values.webhook.agentHostPort | default 8081 }} diff --git a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/server.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/server.yaml index 404315093..bda5ecea1 100644 --- a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/server.yaml +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/server.yaml @@ -137,8 +137,9 @@ spec: - --agent-port=8081 - --log-level=info - --db-path=/data/nvsnap.db - {{- if .Values.jaeger.enabled }} + {{- if or .Values.jaeger.enabled .Values.agent.auth.enabled }} env: + {{- if .Values.jaeger.enabled }} # OTLP/gRPC to the bundled Jaeger collector. nvsnap-server reads # this via internal/tracing.Init — unset means no-op. otelhttp # then emits one server span per API request. @@ -146,6 +147,19 @@ spec: value: "nvsnap-jaeger-collector.{{ .Release.Namespace }}.svc.cluster.local:4317" - name: NVSNAP_VERSION value: {{ .Values.server.image.tag | quote }} + {{- end }} + {{- if .Values.agent.auth.enabled }} + # The server is an agent client: it drops L1 dumps during + # cascade delete, dispatches captures, and polls the checkpoint + # list. Without the token those all 401 once the agent runs with + # --auth-mode=required, and the delete path fails silently — + # 204 returned, catalog row gone, dump orphaned (nvsnap#736). + - name: NVSNAP_AGENT_TOKEN + valueFrom: + secretKeyRef: + name: nvsnap-agent-token + key: token + {{- end }} {{- end }} ports: - containerPort: {{ .Values.server.service.port }} diff --git a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml index 9bfb7a2bb..d9ac3952c 100644 --- a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml @@ -176,13 +176,54 @@ agent: effect: NoSchedule # The agent runs privileged with hostPID/hostNetwork — required to - # see host processes (for CRIU) and to expose hostPort 8081 reliably. - # Don't disable unless you know what you're trading away. + # hostPID is required: CRIU and cuda-checkpoint address target processes by + # host PID, and the pre-checkpoint socket sweep opens /proc//ns/net. hostPID: true + + # hostNetwork is NOT required by any agent capability (GH #490). Everything + # that looked like it needed the host netns actually enters the TARGET pod's + # namespace: external_tcp.go setns's via /proc//ns/net, and CRIU + # dump/restore nsenter into the container's netns. The apiserver reaches the + # webhook through a Service, not the node IP. + # + # What it does carry is hostPort 8081 -- which is what binds the agent's + # privileged API to every node's IP and makes NetworkPolicy unable to fence + # it, since a hostNetwork pod has node identity rather than pod identity. + # + # Setting this false switches to: peers dial the pod IP (--advertise-ip from + # status.podIP), same-node callers use the internalTrafficPolicy:Local + # Service, no hostPort is declared, and the restore-pod egress policy + # tightens from a node CIDR to a podSelector. + # + # Still true by default because the flip is a network topology change that + # has not been validated on a cluster yet. Do that before flipping. hostNetwork: true # Host paths the agent bind-mounts. Override if your nodes use # non-standard layouts (e.g. K3s on a single laptop). + + # Authentication for the agent HTTP API (GH #486). + # + # The agent API restores and deletes checkpoints, serves any file inside a + # checkpoint, and exposes pprof, on a process running privileged with + # /var/lib and the containerd root bind-mounted. The DaemonSet binds it to + # every node's IP (hostNetwork + hostPort), and NetworkPolicy cannot fence a + # hostNetwork pod, so access control has to live in the request path. + # + # Rollout: enable with mode=permissive first. The agent then counts and logs + # unauthenticated callers via nvsnap_agent_auth_total{result="missing"} but + # still serves them, so nothing breaks while callers pick up the token. + # Switch to required once that series is flat at zero. + auth: + # Off by default so an upgrade does not lock out callers that have not + # been given the token yet. The agent logs a warning while it is off. + enabled: false + # permissive | required. Ignored when enabled=false. + mode: permissive + # Leave empty to have the chart generate one and preserve it across + # upgrades. Set explicitly to manage or rotate the credential yourself. + token: "" + hostPaths: checkpoints: /var/lib/containerd/nvsnap-checkpoints containerdSock: /run/containerd/containerd.sock diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index f98b80baf..8f763b850 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "agent", srcs = [ "agent.go", + "auth.go", "blob_uploader.go", "capture_cascade.go", "capture_peer.go", @@ -81,6 +82,8 @@ go_library( go_test( name = "agent_test", srcs = [ + "advertise_test.go", + "auth_test.go", "blob_uploader_test.go", "cascade_fetch_test.go", "catalog_test.go", diff --git a/src/compute-plane-services/nvsnap/internal/agent/advertise_test.go b/src/compute-plane-services/nvsnap/internal/agent/advertise_test.go new file mode 100644 index 000000000..b21ed680a --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/advertise_test.go @@ -0,0 +1,63 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/ + +package agent + +import "testing" + +// selfAgentURL decides what peers dial. Getting it wrong does not fail +// loudly -- it registers an unreachable address in the catalog and the +// cascade silently degrades to blobstore-only, so the fallback order is +// pinned here. See GH #490. +func TestSelfAgentURL(t *testing.T) { + cases := []struct { + name string + advertiseIP string + nodeIP string + listen string + want string + }{ + { + // Pod networking: peers must dial the pod IP; the node IP would + // only reach us via hostPort. + name: "advertise wins", advertiseIP: "10.1.2.3", nodeIP: "192.168.0.5", + listen: ":8081", want: "http://10.1.2.3:8081", + }, + { + // hostNetwork: kubelet reports status.podIP as the node IP, so + // both fields hold the same value and the URL is what it always + // was. This is why enabling the new field changes nothing at the + // default settings. + name: "hostNetwork parity", advertiseIP: "192.168.0.5", nodeIP: "192.168.0.5", + listen: ":8081", want: "http://192.168.0.5:8081", + }, + { + // An older deployment that sets only --node-ip keeps working. + name: "falls back to node IP", advertiseIP: "", nodeIP: "192.168.0.5", + listen: ":8081", want: "http://192.168.0.5:8081", + }, + { + // Neither known: return empty so the caller skips peer + // registration rather than advertising a bogus endpoint. + name: "no address", advertiseIP: "", nodeIP: "", listen: ":8081", want: "", + }, + { + name: "non-default port", advertiseIP: "10.1.2.3", nodeIP: "", + listen: ":9090", want: "http://10.1.2.3:9090", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + a := &Agent{config: Config{ + AdvertiseIP: c.advertiseIP, + NodeIP: c.nodeIP, + ListenAddr: c.listen, + }} + if got := a.selfAgentURL(); got != c.want { + t.Errorf("selfAgentURL() = %q, want %q", got, c.want) + } + }) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/agent.go b/src/compute-plane-services/nvsnap/internal/agent/agent.go index ca43cd766..b46ca1b28 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -67,7 +67,15 @@ type Config struct { CRIUPath string NodeName string LogLevel string - UseNsenter bool // Run CRIU/cuda-checkpoint in host mount namespace (for containerized agents) + + // AuthToken is the shared bearer token callers must present on the + // agent API. Sourced from a Secret rather than a flag so it does not + // land in the pod spec or in `ps` output. Empty disables the check. + AuthToken string + // AuthMode is disabled (default), permissive, or required. See auth.go + // for why the rollout needs a permissive state. + AuthMode AuthMode + UseNsenter bool // Run CRIU/cuda-checkpoint in host mount namespace (for containerized agents) // Prewarm enables agent-side page-cache prewarm of the rox-backed // overlay lowerdir on restore (--prewarm, default true). Reads the @@ -113,6 +121,17 @@ type Config struct { // when registering as a peer in the catalog. NodeIP string + // AdvertiseIP overrides NodeIP as the address peers dial (GH #490). + // + // Under hostNetwork the two are the same, because kubelet reports a + // hostNetwork pod's status.podIP as the node IP. Under pod networking + // they differ, and peers must dial the pod IP -- the node IP would only + // work via hostPort, which is exactly the node-wide exposure we are + // trying not to require. The chart sets this from status.podIP, which + // is correct in both modes; NodeIP stays available for the places that + // genuinely mean "this node". + AdvertiseIP string + // BlobStoreURL is the base URL of the cluster's nvsnap-blobstore // (Phase 5d.2 durable backstop). Empty disables capture-side // upload AND cascade tier-3 fallback — agents fall back to @@ -437,6 +456,30 @@ func (a *Agent) Run(ctx context.Context) error { router := mux.NewRouter() + // RED metrics for every route: rate and status via APIRequestsTotal, + // duration via APIRequestDuration, keyed on the route TEMPLATE rather + // than the concrete path so checkpoint IDs never become label values. + // Registered outermost so it also observes requests the auth guard + // rejects -- a spike of 401s is exactly what the rollout needs to see. + router.Use(metrics.InstrumentRoute()) + + // Present the token on our own peer calls too. Set unconditionally: an + // agent in permissive mode still has peers that may already require it. + SetOutboundToken(a.config.AuthToken) + + // Order matters: gorilla/mux runs middleware in registration order, so + // auth is registered FIRST. Otherwise pathVarGuard answers a malformed + // {id} with 400 before the caller is authenticated, telling an + // unauthenticated client which routes exist and how their variables are + // shaped. Authenticate, then validate. + if guard := tokenGuard(a.config.AuthMode, a.config.AuthToken, a.log); guard != nil { + router.Use(guard) + a.log.WithField("mode", a.config.AuthMode).Info("Agent API authentication enabled") + } else { + a.log.Warn("Agent API is UNAUTHENTICATED: set NVSNAP_AGENT_TOKEN and " + + "--auth-mode to require a bearer token (GH #486)") + } + // Every {id}/{hash}/{pod-uid} below names a directory under a hostPath // mount. Validate them in one place so a route added later is covered // without remembering to. See pathVarGuard in pathsafe.go. diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth.go b/src/compute-plane-services/nvsnap/internal/agent/auth.go new file mode 100644 index 000000000..57373332d --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/auth.go @@ -0,0 +1,224 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package agent + +import ( + "crypto/subtle" + "fmt" + "net/http" + "strings" + "sync/atomic" + + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/metrics" + "github.com/sirupsen/logrus" +) + +// Authentication for the agent's HTTP API. +// +// The API is the control surface of a privileged process: it restores and +// deletes checkpoints, serves any file inside a checkpoint, and exposes pprof. +// The DaemonSet binds it to the node's IP (hostNetwork + hostPort 8081), and +// NetworkPolicy cannot fence it -- a hostNetwork pod carries node identity, so +// podSelector ingress rules do not match it. Access control therefore has to +// live in the request path. +// +// A shared bearer token rather than mTLS: this same router serves the peer +// fan-out endpoints that move multi-GB checkpoints, and TLS handshakes amortize +// with connection reuse but per-byte encryption does not. A header comparison +// costs nothing on the transfer path. See GH #486. + +const authHeader = "Authorization" + +// AuthMode selects what happens to a request that does not present a valid +// token. +type AuthMode string + +const ( + // AuthDisabled skips the check. The default, so an upgrade that has not + // yet been given a token behaves exactly as before. + AuthDisabled AuthMode = "disabled" + + // AuthPermissive checks the token, logs and counts failures, and serves + // the request anyway. This is the rollout state: agents and callers + // cannot be updated in the same instant, so operators run permissive + // until nvsnap_agent_auth_total{result="missing|invalid"} reaches zero, + // then switch to required. + AuthPermissive AuthMode = "permissive" + + // AuthRequired rejects with 401. + AuthRequired AuthMode = "required" +) + +// ParseAuthMode validates operator input. An unrecognized mode is refused at +// startup rather than silently treated as disabled, since "we set the flag and +// assumed it was on" is the failure this whole change exists to prevent. +func ParseAuthMode(s string) (AuthMode, error) { + switch AuthMode(s) { + case AuthDisabled, AuthPermissive, AuthRequired: + return AuthMode(s), nil + case "": + return AuthDisabled, nil + default: + return "", fmt.Errorf("auth mode %q is not one of disabled|permissive|required", s) + } +} + +// unauthenticatedPaths bypass the token check. +// +// Probes and scraping must keep working without distributing the token to the +// kubelet and to Prometheus. Both are information-free: /health reports +// liveness, /metrics reports counters. Everything else, pprof included, is +// gated -- profiles leak memory contents and goroutine state, so an endpoint +// that is merely inconvenient to exploit is still not one to leave open. +var unauthenticatedPaths = map[string]bool{ + "/health": true, + "/metrics": true, +} + +// tokenGuard returns middleware enforcing mode against token. +// +// Returns nil only for AuthDisabled, where there is genuinely nothing to +// enforce and the caller can skip installing a no-op on every request. +// +// AuthRequired with an empty token returns a deny-all guard rather than nil. +// Startup already rejects that combination, but a security primitive that +// silently becomes a no-op when misconfigured is the wrong shape: any future +// caller that builds a guard without going through main() would open the API +// and nothing would say so. Fail closed, and say why in the log. +func tokenGuard(mode AuthMode, token string, log *logrus.Logger) func(http.Handler) http.Handler { + if mode == AuthDisabled { + return nil + } + if token == "" { + if mode == AuthRequired { + log.Error("Agent API auth is required but no token is configured; denying all requests") + return denyAll + } + // Permissive with no token can only ever log every request as + // unauthenticated; that is noise, not signal. + return nil + } + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if unauthenticatedPaths[r.URL.Path] { + next.ServeHTTP(w, r) + return + } + result := checkToken(r, token) + metrics.AgentAuthTotal.WithLabelValues(result).Inc() + if result == authOK { + next.ServeHTTP(w, r) + return + } + // RemoteAddr and path only: no header values, since the thing + // being logged is a credential. + entry := log.WithFields(logrus.Fields{ + "remote": r.RemoteAddr, + "path": r.URL.Path, + "result": result, + }) + if mode == AuthPermissive { + entry.Warn("Unauthenticated request served (auth mode is permissive)") + next.ServeHTTP(w, r) + return + } + entry.Warn("Rejected unauthenticated request") + w.Header().Set("WWW-Authenticate", "Bearer") + http.Error(w, "unauthorized", http.StatusUnauthorized) + }) + } +} + +// denyAll is the fail-closed fallback: everything except the probe endpoints +// gets a 401, so a misconfigured agent is loudly broken rather than quietly +// open. +func denyAll(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if unauthenticatedPaths[r.URL.Path] { + next.ServeHTTP(w, r) + return + } + metrics.AgentAuthTotal.WithLabelValues(authMissing).Inc() + w.Header().Set("WWW-Authenticate", "Bearer") + http.Error(w, "unauthorized: agent has no token configured", http.StatusUnauthorized) + }) +} + +const ( + authOK = "ok" + authMissing = "missing" + authInvalid = "invalid" +) + +// outboundToken is the token this agent presents when it calls a peer. +// +// Package-level and atomic because peerHTTPClient is constructed at import +// time, long before flags are parsed, while the token only exists after +// startup. The alternative -- threading a client through every cascade call +// site -- would put the same header logic in a dozen places and leave the +// next call site free to forget it. +var outboundToken atomic.Pointer[string] + +// SetOutboundToken records the token used on agent-to-agent requests. Safe to +// call before any request is issued; a nil/empty token sends no header, which +// is what keeps a disabled deployment working unchanged. +func SetOutboundToken(tok string) { + outboundToken.Store(&tok) +} + +// authTransport adds the bearer token to every outbound request. +// +// Wrapping the transport rather than editing call sites means a peer endpoint +// added later is authenticated without anyone remembering to do it -- the same +// reasoning as pathVarGuard on the inbound side. +type authTransport struct{ base http.RoundTripper } + +func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) { + tok := outboundToken.Load() + if tok != nil && *tok != "" && r.Header.Get(authHeader) == "" { + // RoundTrip must not modify the request it is given. + r = r.Clone(r.Context()) + r.Header.Set(authHeader, "Bearer "+*tok) + } + base := t.base + if base == nil { + base = http.DefaultTransport + } + return base.RoundTrip(r) +} + +// checkToken compares the request's bearer token against the expected value in +// constant time, so a caller cannot recover the token byte by byte from +// response timing. +func checkToken(r *http.Request, want string) string { + h := r.Header.Get(authHeader) + if h == "" { + return authMissing + } + // RFC 7235 makes the auth scheme case-insensitive, so "bearer " is a + // valid credential a conforming client may send. Compare the scheme with + // EqualFold; the token itself stays a byte-exact constant-time compare. + scheme, got, ok := strings.Cut(h, " ") + if !ok || !strings.EqualFold(scheme, "Bearer") { + return authInvalid + } + if subtle.ConstantTimeCompare([]byte(got), []byte(want)) != 1 { + return authInvalid + } + return authOK +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth_test.go b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go new file mode 100644 index 000000000..44e8f6018 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go @@ -0,0 +1,244 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/ + +package agent + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/sirupsen/logrus" +) + +func quietLog() *logrus.Logger { + l := logrus.New() + l.SetOutput(io.Discard) + return l +} + +// served reports whether the guarded handler ran, and the status returned. +func served(t *testing.T, mode AuthMode, token, header, path string) (bool, int) { + t.Helper() + ran := false + guard := tokenGuard(mode, token, quietLog()) + var h http.Handler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + }) + if guard != nil { + h = guard(h) + } + r := httptest.NewRequest(http.MethodGet, path, http.NoBody) + if header != "" { + r.Header.Set(authHeader, header) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return ran, w.Code +} + +func TestTokenGuardRequired(t *testing.T) { + const tok = "s3cret-token" + + cases := []struct { + name string + header string + wantRan bool + wantCode int + }{ + {"valid token", "Bearer " + tok, true, http.StatusOK}, + {"no header", "", false, http.StatusUnauthorized}, + {"wrong token", "Bearer nope", false, http.StatusUnauthorized}, + {"missing Bearer prefix", tok, false, http.StatusUnauthorized}, + {"empty bearer", "Bearer ", false, http.StatusUnauthorized}, + // A prefix of the real token must not pass: constant-time compare + // returns 0 on a length mismatch, but assert it rather than trust it. + {"token prefix", "Bearer " + tok[:5], false, http.StatusUnauthorized}, + {"wrong scheme", "Basic " + tok, false, http.StatusUnauthorized}, + // RFC 7235: the scheme is case-insensitive, so a conforming client + // may legitimately send these and must not be turned away. + {"lowercase scheme", "bearer " + tok, true, http.StatusOK}, + {"mixed-case scheme", "BeArEr " + tok, true, http.StatusOK}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ran, code := served(t, AuthRequired, tok, c.header, "/v1/checkpoints") + if ran != c.wantRan || code != c.wantCode { + t.Errorf("ran=%v code=%d, want ran=%v code=%d", ran, code, c.wantRan, c.wantCode) + } + }) + } +} + +// The rollout depends on permissive serving the request while still counting +// the failure -- if it rejected, enabling it would be the same outage as +// switching straight to required. +func TestTokenGuardPermissiveServesButCounts(t *testing.T) { + ran, code := served(t, AuthPermissive, "tok", "", "/v1/restore") + if !ran || code != http.StatusOK { + t.Errorf("permissive rejected an unauthenticated request: ran=%v code=%d", ran, code) + } +} + +func TestTokenGuardDisabledInstallsNothing(t *testing.T) { + if g := tokenGuard(AuthDisabled, "tok", quietLog()); g != nil { + t.Error("mode=disabled returned a middleware; caller should skip installing one") + } + // Permissive with no token could only log every request as + // unauthenticated, which is noise; skipping it is correct. + if g := tokenGuard(AuthPermissive, "", quietLog()); g != nil { + t.Error("permissive with no token returned a middleware") + } +} + +// AuthRequired with no token must FAIL CLOSED. Returning nil here would make +// Agent.Run skip the middleware entirely and serve the privileged API +// unauthenticated -- a misconfiguration silently becoming an open API is the +// exact failure this feature exists to prevent. +func TestTokenGuardRequiredWithoutTokenDeniesAll(t *testing.T) { + g := tokenGuard(AuthRequired, "", quietLog()) + if g == nil { + t.Fatal("mode=required with no token returned nil; the API would be served unauthenticated") + } + ran := false + h := g(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + })) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/v1/restore", http.NoBody)) + if ran || w.Code != http.StatusUnauthorized { + t.Errorf("privileged route: ran=%v code=%d, want ran=false code=401", ran, w.Code) + } + // Even a well-formed token cannot help: there is nothing to compare to. + ran = false + w = httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/v1/restore", http.NoBody) + r.Header.Set(authHeader, "Bearer anything") + h.ServeHTTP(w, r) + if ran || w.Code != http.StatusUnauthorized { + t.Errorf("with a token: ran=%v code=%d, want ran=false code=401", ran, w.Code) + } + // Probes must still work, or the pod fails its liveness check and the + // operator sees a crashloop instead of the actual misconfiguration. + ran = false + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/health", http.NoBody)) + if !ran || w.Code != http.StatusOK { + t.Errorf("/health: ran=%v code=%d, want ran=true code=200", ran, w.Code) + } +} + +// Probes and scraping must not need the token, or enabling auth breaks +// liveness and Prometheus. pprof deliberately is NOT exempt: profiles expose +// memory contents and goroutine state. +func TestTokenGuardExemptPaths(t *testing.T) { + for _, p := range []string{"/health", "/metrics"} { + if ran, code := served(t, AuthRequired, "tok", "", p); !ran || code != http.StatusOK { + t.Errorf("%s required a token: ran=%v code=%d", p, ran, code) + } + } + for _, p := range []string{"/debug/pprof/", "/debug/pprof/heap", "/debug/pprof/profile"} { + if ran, _ := served(t, AuthRequired, "tok", "", p); ran { + t.Errorf("%s served without a token", p) + } + } +} + +func TestParseAuthMode(t *testing.T) { + for in, want := range map[string]AuthMode{ + "": AuthDisabled, + "disabled": AuthDisabled, + "permissive": AuthPermissive, + "required": AuthRequired, + } { + got, err := ParseAuthMode(in) + if err != nil || got != want { + t.Errorf("ParseAuthMode(%q) = %q, %v; want %q, nil", in, got, err, want) + } + } + // A typo must be an error, not a silent fallback to disabled. + for _, in := range []string{"Required", "enabled", "on", "true", "requird"} { + if _, err := ParseAuthMode(in); err == nil { + t.Errorf("ParseAuthMode(%q) = nil error; want a failure so a typo cannot silently leave the API open", in) + } + } +} + +// recordingRT captures what authTransport handed to the base transport. +type recordingRT struct{ got *http.Request } + +func (r *recordingRT) RoundTrip(req *http.Request) (*http.Response, error) { + r.got = req + return &http.Response{StatusCode: 200, Body: http.NoBody, Header: http.Header{}}, nil +} + +func TestAuthTransportSignsOutbound(t *testing.T) { + t.Cleanup(func() { SetOutboundToken("") }) + + base := &recordingRT{} + c := &http.Client{Transport: &authTransport{base: base}} + + // No token configured: no header, so a cluster running with auth off is + // byte-for-byte unchanged on the wire. + SetOutboundToken("") + req, _ := http.NewRequest(http.MethodGet, "http://peer/v1/checkpoints/x/manifest", http.NoBody) + if _, err := c.Do(req); err != nil { + t.Fatal(err) + } + if h := base.got.Header.Get(authHeader); h != "" { + t.Errorf("sent %q with no token configured", h) + } + + SetOutboundToken("peer-token") + req, _ = http.NewRequest(http.MethodGet, "http://peer/v1/checkpoints/x/manifest", http.NoBody) + if _, err := c.Do(req); err != nil { + t.Fatal(err) + } + if got, want := base.got.Header.Get(authHeader), "Bearer peer-token"; got != want { + t.Errorf("Authorization = %q, want %q", got, want) + } + // RoundTrip must not mutate the caller's request. + if h := req.Header.Get(authHeader); h != "" { + t.Errorf("caller's request was mutated: %q", h) + } +} + +// The signed request must actually satisfy the guard. Testing the two halves +// separately would not catch a format mismatch between them. +func TestOutboundTokenSatisfiesGuard(t *testing.T) { + t.Cleanup(func() { SetOutboundToken("") }) + const tok = "round-trip-token" + SetOutboundToken(tok) + + base := &recordingRT{} + c := &http.Client{Transport: &authTransport{base: base}} + req, _ := http.NewRequest(http.MethodGet, "http://peer/v1/checkpoints/x/manifest", http.NoBody) + if _, err := c.Do(req); err != nil { + t.Fatal(err) + } + if got := checkToken(base.got, tok); got != authOK { + t.Errorf("guard rejected our own signed request: %s", got) + } +} + +// Same leak as the server side (internal/server/agent_auth_test.go): the peer +// client must refuse redirects, because authTransport re-adds the bearer token +// on the redirected request after net/http strips it for a cross-origin hop. +func TestPeerClientDoesNotFollowRedirects(t *testing.T) { + if peerHTTPClient.CheckRedirect == nil { + t.Fatal("peerHTTPClient follows redirects; a peer 302 would leak the token") + } + req, err := http.NewRequest(http.MethodGet, "http://evil.example/steal", nil) + if err != nil { + t.Fatal(err) + } + if err := peerHTTPClient.CheckRedirect(req, nil); err != http.ErrUseLastResponse { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse", err) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go b/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go index c10bacb57..b62c47203 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go +++ b/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go @@ -95,7 +95,11 @@ const peerFetchTimeoutPerFile = 5 * time.Minute // All cascade-fetch call sites go through this client; downloadToFile // receives it as an explicit argument so tests can substitute. var peerHTTPClient = &http.Client{ - Transport: &http.Transport{ + // authTransport wraps the tuned transport rather than replacing it: every + // agent-to-agent request carries the bearer token (a no-op until one is + // configured) without any cascade call site knowing about auth. See + // auth.go and GH #486. + Transport: &authTransport{base: &http.Transport{ MaxIdleConns: peerFetchConcurrency * 2, MaxIdleConnsPerHost: peerFetchConcurrency * 2, IdleConnTimeout: 90 * time.Second, @@ -103,6 +107,14 @@ var peerHTTPClient = &http.Client{ // can reason about TCP stream count for the Cilium-multi-stream // hypothesis. Re-enable explicitly if/when we switch to h2c. ForceAttemptHTTP2: false, + }}, + // Do not follow redirects. net/http drops Authorization when a redirect + // crosses origins, but authTransport runs on the redirected request too + // and re-adds it -- so a peer that answers a fetch with a 302 to any host + // would be handed the shared token. No peer endpoint redirects, so + // surfacing the 3xx to the caller loses nothing. + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse }, } @@ -572,14 +584,22 @@ func (a *Agent) registerAsPeer(ctx context.Context, checkpointID string) error { // agent's peer-server endpoints. Empty string if we don't have // enough config to construct it (NodeIP missing). func (a *Agent) selfAgentURL() string { - if a.config.NodeIP == "" { + // AdvertiseIP first: under pod networking peers must dial the pod IP, + // since the node IP only resolves to us via hostPort (GH #490). Falls + // back to NodeIP so a deployment that sets neither, or only the older + // value, keeps working. + ip := a.config.AdvertiseIP + if ip == "" { + ip = a.config.NodeIP + } + if ip == "" { return "" } port := "8081" if addr := a.config.ListenAddr; len(addr) > 1 && addr[0] == ':' { port = addr[1:] } - return fmt.Sprintf("http://%s:%s", a.config.NodeIP, port) + return fmt.Sprintf("http://%s:%s", ip, port) } // bytesReader returns an io.Reader for a byte slice. Tiny helper to diff --git a/src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go b/src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go index 8402c08af..273ae84f7 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go +++ b/src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go @@ -87,6 +87,12 @@ type WebhookConfig struct { // reaches the agent on (status.hostIP:AgentHostPort). Defaults to // 8081 (matches the agent's --listen=:8081 default). AgentHostPort int + + // AgentBaseURL overrides the host-IP form the mount-prep init container + // uses to reach its node-local agent. Empty keeps + // http://$(NVSNAP_HOST_IP):, which needs hostPort. See + // GH #490 and internal/webhook/mount_prep_init.go. + AgentBaseURL string } // startWebhook starts the agent's mutating-admission TLS server in a @@ -174,6 +180,7 @@ func (a *Agent) startWebhook(ctx context.Context, cfg WebhookConfig, backend che RestorePrepStrategy: cfg.RestorePrepStrategy, MountPrepInitImage: cfg.MountPrepInitImage, AgentHostPort: cfg.AgentHostPort, + AgentBaseURL: cfg.AgentBaseURL, } handler := &webhook.Handler{ Mutator: mut, diff --git a/src/compute-plane-services/nvsnap/internal/metrics/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/metrics/BUILD.bazel index 933a25014..802da0ec4 100644 --- a/src/compute-plane-services/nvsnap/internal/metrics/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/metrics/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "metrics", @@ -11,3 +11,9 @@ go_library( "@com_github_prometheus_client_golang//prometheus/promhttp", ], ) + +go_test( + name = "metrics_test", + srcs = ["register_test.go"], + embed = [":metrics"], +) diff --git a/src/compute-plane-services/nvsnap/internal/metrics/metrics.go b/src/compute-plane-services/nvsnap/internal/metrics/metrics.go index 555852b20..8cf090bec 100644 --- a/src/compute-plane-services/nvsnap/internal/metrics/metrics.go +++ b/src/compute-plane-services/nvsnap/internal/metrics/metrics.go @@ -87,6 +87,18 @@ var ( Name: "gpu_processes_discovered", Help: "Number of GPU processes discovered on this node.", }) + + // AgentAuthTotal counts requests to the agent API by authentication + // outcome. The point of the "missing" and "invalid" series is the + // rollout: operators run auth in permissive mode until both reach zero, + // which proves every caller now sends a token, and only then switch to + // required. Pre-initialized below so the series exist on the first + // scrape and an alert on them does not misfire as absent. See GH #486. + AgentAuthTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: namespace, + Name: "agent_auth_total", + Help: "Agent API requests by authentication result (ok, missing, invalid).", + }, []string{"result"}) ) // Server metrics — API and cluster-wide. @@ -120,8 +132,20 @@ var ( var ( agentOnce sync.Once serverOnce sync.Once + // apiOnce guards the RED pair, which BOTH the agent and the server + // register. Without its own Once, a process starting both would panic in + // MustRegister on the second call. + apiOnce sync.Once ) +// registerAPIMetrics registers the shared request rate/duration pair used by +// InstrumentRoute on any router. +func registerAPIMetrics() { + apiOnce.Do(func() { + prometheus.MustRegister(APIRequestsTotal, APIRequestDuration) + }) +} + // RegisterAgent registers agent-side metrics with the default Prometheus registry. func RegisterAgent() { agentOnce.Do(func() { @@ -134,7 +158,16 @@ func RegisterAgent() { ActiveOperations, CRIUDumpDuration, GPUProcessesDiscovered, + AgentAuthTotal, ) + // The agent serves an HTTP API too, so it needs the same RED metrics + // the server has. See InstrumentRoute. + registerAPIMetrics() + // Counters must exist before the first scrape or rate() gaps and + // absent() alerts misfire. + for _, r := range []string{"ok", "missing", "invalid"} { + AgentAuthTotal.WithLabelValues(r) + } }) } @@ -142,11 +175,10 @@ func RegisterAgent() { func RegisterServer() { serverOnce.Do(func() { prometheus.MustRegister( - APIRequestsTotal, - APIRequestDuration, CheckpointsStored, WebSocketConnections, ) + registerAPIMetrics() }) } diff --git a/src/compute-plane-services/nvsnap/internal/metrics/register_test.go b/src/compute-plane-services/nvsnap/internal/metrics/register_test.go new file mode 100644 index 000000000..c31b15459 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/metrics/register_test.go @@ -0,0 +1,24 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/ + +package metrics + +import "testing" + +// The agent and the server both register the shared API rate/duration pair. +// prometheus.MustRegister panics on a duplicate, so a process starting both -- +// or either one twice -- must not blow up at startup. Guarded by apiOnce; +// this pins that. +func TestRegisterAgentAndServerDoNotPanic(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("duplicate metric registration panicked: %v", r) + } + }() + RegisterAgent() + RegisterServer() + RegisterAgent() + RegisterServer() +} diff --git a/src/compute-plane-services/nvsnap/internal/server/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/server/BUILD.bazel index a5fbd11b3..74d28ce7a 100644 --- a/src/compute-plane-services/nvsnap/internal/server/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/server/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "server", srcs = [ + "agent_auth.go", "demo.go", "lookup.go", "manifests.go", @@ -39,6 +40,7 @@ go_library( go_test( name = "server_test", srcs = [ + "agent_auth_test.go", "checkpoint_status_test.go", "delete_cascade_integration_test.go", "delete_checkpoint_test.go", diff --git a/src/compute-plane-services/nvsnap/internal/server/agent_auth.go b/src/compute-plane-services/nvsnap/internal/server/agent_auth.go new file mode 100644 index 000000000..cc7b9803a --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/server/agent_auth.go @@ -0,0 +1,80 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package server + +import "net/http" + +// agentAuthTransport adds the shared bearer token to every request the server +// sends to an agent. +// +// The server is an agent client: cascadeDeleteCheckpoint drops the L1 dump, +// dispatch posts captures, and the checkpoint poller lists them. Once the agent +// runs with --auth-mode=required it rejects all of those, and the delete path +// fails in the worst possible way -- it returns 204, deletes the catalog row, +// and orphans the on-disk dump with nothing left pointing at it (nvsnap#736). +// +// Wrapping the transport rather than editing call sites means an agent endpoint +// added later is authenticated without anyone remembering to do it, matching +// the agent's own outbound wrapper in internal/agent/auth.go. +// +// This is deliberately simpler than the agent's version, which stores the token +// in an atomic pointer because its peer client is built at import time, before +// the token is known. The server builds its client in New() with config already +// parsed, so a plain field is enough. +// +// An empty token installs no wrapper at all (see New), so a deployment without +// agent.auth.enabled behaves exactly as before. +type agentAuthTransport struct { + base http.RoundTripper + token string +} + +func (t *agentAuthTransport) RoundTrip(r *http.Request) (*http.Response, error) { + // RoundTrip must not modify the request it is given. + if t.token != "" && r.Header.Get("Authorization") == "" { + r = r.Clone(r.Context()) + r.Header.Set("Authorization", "Bearer "+t.token) + } + base := t.base + if base == nil { + base = http.DefaultTransport + } + return base.RoundTrip(r) +} + +// withAgentAuth wraps c so its requests carry the token. Returns c untouched +// when there is no token, keeping the no-auth deployment byte-identical. +// +// It also stops the client following redirects. net/http strips Authorization +// when a redirect crosses origins, but a header-adding RoundTripper runs on the +// redirected request too and puts it straight back -- so a compromised or +// spoofed agent could bounce the server at any host and harvest the token. No +// agent endpoint redirects, so refusing outright costs nothing. +func withAgentAuth(c *http.Client, token string) *http.Client { + if token == "" { + return c + } + c.Transport = &agentAuthTransport{base: c.Transport, token: token} + c.CheckRedirect = refuseRedirect + return c +} + +// refuseRedirect surfaces the 3xx to the caller instead of following it. +func refuseRedirect(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse +} diff --git a/src/compute-plane-services/nvsnap/internal/server/agent_auth_test.go b/src/compute-plane-services/nvsnap/internal/server/agent_auth_test.go new file mode 100644 index 000000000..defa1fa8c --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/server/agent_auth_test.go @@ -0,0 +1,127 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package server + +import ( + "net/http" + "testing" +) + +// captureRT records the request it is handed and returns a canned response. +// A fake round tripper rather than httptest: this exercises the header logic +// with no listener, which also keeps the test runnable in sandboxes that +// block loopback TCP. +type captureRT struct{ got *http.Request } + +func (c *captureRT) RoundTrip(r *http.Request) (*http.Response, error) { + c.got = r + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: r}, nil +} + +func TestWithAgentAuth(t *testing.T) { + tests := []struct { + name string + token string + preset string // Authorization already on the request + wantAuth string + }{ + { + name: "token is sent as a bearer credential", + token: "s3cret", + wantAuth: "Bearer s3cret", + }, + { + // The no-auth deployment must behave exactly as before, so an + // empty token has to mean "no header", not "Bearer ". + name: "empty token sends no header", + token: "", + wantAuth: "", + }, + { + // Preserves an explicit credential a caller already chose. + name: "existing Authorization is not overwritten", + token: "s3cret", + preset: "Bearer caller-supplied", + wantAuth: "Bearer caller-supplied", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rt := &captureRT{} + c := withAgentAuth(&http.Client{Transport: rt}, tt.token) + + req, err := http.NewRequest(http.MethodGet, "http://agent.invalid:8081/v1/checkpoints", http.NoBody) + if err != nil { + t.Fatalf("new request: %v", err) + } + if tt.preset != "" { + req.Header.Set("Authorization", tt.preset) + } + if _, err := c.Do(req); err != nil { + t.Fatalf("do: %v", err) + } + + if got := rt.got.Header.Get("Authorization"); got != tt.wantAuth { + t.Errorf("Authorization = %q, want %q", got, tt.wantAuth) + } + // RoundTrip must not mutate the caller's request. + if tt.preset == "" && req.Header.Get("Authorization") != "" { + t.Errorf("caller request was mutated: %q", req.Header.Get("Authorization")) + } + }) + } +} + +// An empty token must leave the client untouched, so a no-auth deployment +// keeps whatever transport it was constructed with. +func TestWithAgentAuthEmptyTokenLeavesTransportAlone(t *testing.T) { + rt := &captureRT{} + in := &http.Client{Transport: rt} + out := withAgentAuth(in, "") + + if out != in { + t.Errorf("client was replaced for an empty token") + } + if out.Transport != http.RoundTripper(rt) { + t.Errorf("transport was wrapped for an empty token: %T", out.Transport) + } + if out.CheckRedirect != nil { + t.Error("CheckRedirect was set for an empty token") + } +} + +// net/http strips Authorization when a redirect crosses origins, but a +// header-adding RoundTripper runs on the redirected request too and puts it +// back. Without CheckRedirect a compromised agent could answer any call with a +// 302 to a host it controls and be handed the shared token. The client must +// stop at the 3xx and hand it to the caller, who treats a non-200 as an error. +func TestWithAgentAuthDoesNotFollowRedirects(t *testing.T) { + c := withAgentAuth(&http.Client{Transport: &captureRT{}}, "tok") + + if c.CheckRedirect == nil { + t.Fatal("CheckRedirect not set; redirects would be followed with the token attached") + } + req, err := http.NewRequest(http.MethodGet, "http://evil.example/steal", nil) + if err != nil { + t.Fatal(err) + } + if err := c.CheckRedirect(req, nil); err != http.ErrUseLastResponse { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse", err) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/server/server.go b/src/compute-plane-services/nvsnap/internal/server/server.go index 36959c7ee..234870cef 100644 --- a/src/compute-plane-services/nvsnap/internal/server/server.go +++ b/src/compute-plane-services/nvsnap/internal/server/server.go @@ -79,6 +79,12 @@ type Config struct { // and every NVCA rootfs capture timed out at 15m → Failed → no // pvc_promote_state=ready → no restore. GCP-H100-a 2026-06-10.) ManifestNamespace string + // AgentToken is the shared bearer token the agent expects on its API + // (nvsnap#486). Empty when the deployment runs without + // agent.auth.enabled, in which case no header is sent and behaviour is + // unchanged. Read from NVSNAP_AGENT_TOKEN, which the chart projects + // from the nvsnap-agent-token Secret. + AgentToken string } // Server is the K8s-aware NVSNAP API server. @@ -114,7 +120,7 @@ func New(cfg Config, kubeClient kubernetes.Interface, dynClient dynamic.Interfac config: cfg, kubeClient: kubeClient, dynClient: dynClient, - httpClient: &http.Client{Timeout: 10 * time.Minute}, + httpClient: withAgentAuth(&http.Client{Timeout: 10 * time.Minute}, cfg.AgentToken), log: log, demo: newDemoSession(), hub: newHub(log), @@ -1116,7 +1122,10 @@ func (r *cascadeDeleteResult) Summary() string { func (s *Server) cascadeDeleteCheckpoint(ctx context.Context, id, agentID string, row *db.Checkpoint) cascadeDeleteResult { var result cascadeDeleteResult - client := &http.Client{Timeout: 10 * time.Second} + // Own client rather than s.httpClient (much shorter timeout), so it needs + // the token wrapper too. Without it the L1 delete 401s, the row is dropped + // anyway, and the dump is orphaned (nvsnap#736). + client := withAgentAuth(&http.Client{Timeout: 10 * time.Second}, s.config.AgentToken) // Targeted row, synthesizing missing fields from (id, agentID) // so the per-row helper has what it needs even when row is nil @@ -1421,7 +1430,7 @@ func (s *Server) proxyToAgentCheckpoint(w http.ResponseWriter, r *http.Request, continue } url := fmt.Sprintf("http://%s:%d%s?%s", ip, s.config.AgentPort, agentPath, r.URL.RawQuery) - client := &http.Client{Timeout: 10 * time.Second} + client := withAgentAuth(&http.Client{Timeout: 10 * time.Second}, s.config.AgentToken) req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, url, http.NoBody) if err != nil { s.writeError(w, http.StatusInternalServerError, err.Error()) @@ -1844,7 +1853,7 @@ func (s *Server) listAgentCheckpoints(ctx context.Context) []map[string]interfac wg sync.WaitGroup ) - client := &http.Client{Timeout: 5 * time.Second} + client := withAgentAuth(&http.Client{Timeout: 5 * time.Second}, s.config.AgentToken) for i := range nodes.Items { node := &nodes.Items[i] @@ -1907,7 +1916,7 @@ func (s *Server) checkAgentHealth(ctx context.Context, nodeIP string) bool { if nodeIP == "" { return false } - client := &http.Client{Timeout: 3 * time.Second} + client := withAgentAuth(&http.Client{Timeout: 3 * time.Second}, s.config.AgentToken) req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s:%d/health", nodeIP, s.config.AgentPort), http.NoBody) if err != nil { diff --git a/src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go b/src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go index b642ed3c6..2a6b13eef 100644 --- a/src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go +++ b/src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go @@ -23,6 +23,7 @@ package webhook import ( "encoding/json" "fmt" + "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -30,6 +31,15 @@ import ( "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/checkpointstore" ) +// AgentTokenSecretName / AgentTokenSecretKey locate the shared agent API +// bearer token (GH #486). The chart creates this Secret only when auth is +// enabled, so every reference to it is marked optional -- a pod admitted +// before the operator turns auth on must still start. +const ( + AgentTokenSecretName = "nvsnap-agent-token" + AgentTokenSecretKey = "token" +) + const ( // MountPrepContainerName is the canonical name of the injected // init container; surfaces in `kubectl describe pod` and logs. @@ -79,12 +89,23 @@ func (m *Mutator) emitMountPrepInitContainer( if agentPort == 0 { agentPort = MountPrepDefaultAgentPort } - // NVSNAP_AGENT_URL points at the host IP via downward API - // (status.hostIP), so the init container always hits the agent - // on its OWN node — same trust boundary as today's hostNetwork - // agent endpoints. Cross-node peer routing is the agent's job, - // driven by captureNode in the POST body. + // Both forms reach the agent on the pod's OWN node; cross-node peer + // routing is the agent's job, driven by captureNode in the POST body. + // + // Default is the downward-API host IP, which depends on the agent + // binding hostPort. AgentBaseURL replaces it with the + // internalTrafficPolicy:Local Service under pod networking, which routes + // to the node-local endpoint with no node-wide listener (GH #490). + // Addressable true for the Secret's Optional field. Inlined rather than + // pulling in k8s.io/utils/ptr for a single call: bazel enforces strict + // deps, so one import here means a new external dependency in the build + // graph for something the language expresses in a line. + secretOptional := true + agentURL := fmt.Sprintf("http://$(NVSNAP_HOST_IP):%d", agentPort) + if m.AgentBaseURL != "" { + agentURL = strings.TrimRight(m.AgentBaseURL, "/") + } c := corev1.Container{ Name: MountPrepContainerName, @@ -104,6 +125,16 @@ func (m *Mutator) emitMountPrepInitContainer( {Name: "NVSNAP_CAPTURE_NODE", Value: captureNode}, {Name: "NVSNAP_PREP_MOUNTS", Value: string(mountsJSON)}, {Name: "NVSNAP_PREP_DEADLINE", Value: MountPrepDeadline}, + // Optional: the Secret only exists once the operator enables + // auth, so the reference is marked optional and the init + // container simply sends no header until then (GH #486). + {Name: "NVSNAP_AGENT_TOKEN", ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: AgentTokenSecretName}, + Key: AgentTokenSecretKey, + Optional: &secretOptional, + }, + }}, }, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ diff --git a/src/compute-plane-services/nvsnap/internal/webhook/mutate.go b/src/compute-plane-services/nvsnap/internal/webhook/mutate.go index 297fc6c8b..889254fc3 100644 --- a/src/compute-plane-services/nvsnap/internal/webhook/mutate.go +++ b/src/compute-plane-services/nvsnap/internal/webhook/mutate.go @@ -351,6 +351,14 @@ type Mutator struct { // = http://:; host-ip is plumbed via // downward API in the patched pod spec. AgentHostPort int + + // AgentBaseURL overrides how the injected init container addresses the + // agent (GH #490). Empty keeps the hostPort form, + // http://$(NVSNAP_HOST_IP):, which requires the API to be + // bound to every node's IP. Under pod networking the chart sets this to + // the internalTrafficPolicy:Local Service instead, which reaches the + // agent on the caller's own node without any node-wide listener. + AgentBaseURL string } // OverlayPreparer is implemented by *agent.Agent via PrepareOverlay. diff --git a/src/compute-plane-services/nvsnap/scripts/bench-cross-node-restore.sh b/src/compute-plane-services/nvsnap/scripts/bench-cross-node-restore.sh index 5d1bc3bf2..428e016ff 100755 --- a/src/compute-plane-services/nvsnap/scripts/bench-cross-node-restore.sh +++ b/src/compute-plane-services/nvsnap/scripts/bench-cross-node-restore.sh @@ -81,7 +81,11 @@ for r in "${RECEIVERS[@]}"; do # default run lightweight. if [ "${BENCH_PROFILE:-0}" = "1" ]; then log "Starting CPU pprof on agent (30s)…" - kubectl -n $NS exec "$agent" -- sh -c "curl -sS 'http://localhost:8081/debug/pprof/profile?seconds=30' -o /tmp/cpu.pb && echo PROFILE_DONE" >/tmp/pprof-$short.log 2>&1 & + # pprof is gated by the agent's auth middleware (profiles leak memory + # contents), so send the token the chart injects into this container. + # --oauth2-bearer, not -H: the header form word-splits when expanded + # unquoted through ${:+}. + kubectl -n $NS exec "$agent" -- sh -c "curl -sS \${NVSNAP_AGENT_TOKEN:+--oauth2-bearer \$NVSNAP_AGENT_TOKEN} 'http://localhost:8081/debug/pprof/profile?seconds=30' -o /tmp/cpu.pb && echo PROFILE_DONE" >/tmp/pprof-$short.log 2>&1 & PPROF_PID=$! fi diff --git a/src/compute-plane-services/nvsnap/scripts/checkpoint.sh b/src/compute-plane-services/nvsnap/scripts/checkpoint.sh index a4aa50839..6c0e2f3fc 100755 --- a/src/compute-plane-services/nvsnap/scripts/checkpoint.sh +++ b/src/compute-plane-services/nvsnap/scripts/checkpoint.sh @@ -20,6 +20,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "${SCRIPT_DIR}/config.sh" 2>/dev/null || true +source "${SCRIPT_DIR}/lib/agent-auth.sh" # Agent API port - IMPORTANT: Agent listens on 8081, not 8080! AGENT_PORT="${AGENT_PORT:-8081}" @@ -156,9 +157,19 @@ call_agent_api() { local method="$2" local endpoint="$3" local data="${4:-}" - + # Callers resolve $agent_pod in the namespace they were given, so the + # port-forward and the token Secret have to be read from that same + # namespace -- pinning them to $NAMESPACE made a non-default namespace + # port-forward to a pod that isn't there and authenticate with the wrong + # token. + local namespace="${5:-$NAMESPACE}" + + # Empty unless the chart was installed with agent.auth.enabled; without it + # every call here returns "unauthorized" under --auth-mode=required. + nvsnap_agent_auth_args "$namespace" + # Start port-forward in background (redirect output to avoid contaminating API response) - kubectl port-forward -n "$NAMESPACE" "$agent_pod" "${AGENT_PORT}:${AGENT_PORT}" >/dev/null 2>&1 & + kubectl port-forward -n "$namespace" "$agent_pod" "${AGENT_PORT}:${AGENT_PORT}" >/dev/null 2>&1 & local pf_pid=$! trap "kill $pf_pid 2>/dev/null || true" EXIT @@ -166,7 +177,9 @@ call_agent_api() { # Port-forward can take time to establish, especially on remote clusters local ready=false for i in {1..15}; do - if timeout 5 curl -s http://localhost:${AGENT_PORT}/v1/checkpoints >/dev/null 2>&1; then + # /health, not an API route: it stays unauthenticated in every auth + # mode, so this probes the port-forward rather than the credential. + if timeout 5 curl -sf http://localhost:${AGENT_PORT}/health >/dev/null 2>&1; then ready=true break fi @@ -182,10 +195,12 @@ call_agent_api() { local max_time="${CHECKPOINT_TIMEOUT:-600}" if [[ -n "$data" ]]; then curl -s --max-time "$max_time" -X "$method" "http://localhost:${AGENT_PORT}${endpoint}" \ + "${NVSNAP_AUTH_ARGS[@]}" \ -H "Content-Type: application/json" \ -d "$data" else - curl -s --max-time "$max_time" -X "$method" "http://localhost:${AGENT_PORT}${endpoint}" + curl -s --max-time "$max_time" -X "$method" "http://localhost:${AGENT_PORT}${endpoint}" \ + "${NVSNAP_AUTH_ARGS[@]}" fi # Cleanup @@ -240,7 +255,7 @@ $capture_path_line EOF ) - local response=$(call_agent_api "$agent" "POST" "/v1/checkpoint" "$payload") + local response=$(call_agent_api "$agent" "POST" "/v1/checkpoint" "$payload" "$namespace") # Agent may return a structured 422-style redirect when the workload's # backend (Riva or Triton) requires the rootfs capture path. The JSON @@ -295,7 +310,7 @@ cmd_list() { fi echo "Listing checkpoints via agent $agent..." - call_agent_api "$agent" "GET" "/v1/checkpoints" + call_agent_api "$agent" "GET" "/v1/checkpoints" "" "$namespace" echo "" } diff --git a/src/compute-plane-services/nvsnap/scripts/lib/agent-auth.sh b/src/compute-plane-services/nvsnap/scripts/lib/agent-auth.sh new file mode 100644 index 000000000..1167e49e9 --- /dev/null +++ b/src/compute-plane-services/nvsnap/scripts/lib/agent-auth.sh @@ -0,0 +1,45 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Shared bearer-token helper for scripts that call the agent HTTP API. +# +# Once the agent runs with --auth-mode=required it gates every route except +# /health and /metrics (nvsnap#486). A caller that omits the header gets back a +# bare "unauthorized" with no indication of which credential is missing, so +# every script that talks to the API has to send the token (nvsnap#734). +# +# An absent Secret is the normal case, not an error: the chart only creates it +# when agent.auth.enabled is set, and these scripts must keep working against a +# cluster installed without auth. Callers therefore get an empty header list +# rather than a failure, and the same command works in both configurations. +# +# Sourced, not executed — no shebang, no set -e (that would leak into callers). +# +# Usage: +# source "$(dirname "${BASH_SOURCE[0]}")/lib/agent-auth.sh" +# nvsnap_agent_auth_args # populates NVSNAP_AUTH_ARGS +# curl -s "${NVSNAP_AUTH_ARGS[@]}" "$url" +# +# For calls made *inside* the agent container via kubectl exec, prefer the +# NVSNAP_AGENT_TOKEN env var the chart already injects there over piping the +# token in from outside. + +# Print the agent API token, or nothing when auth is not configured. +# Namespace: explicit argument, else $NAMESPACE, else nvsnap-system. +nvsnap_agent_token() { + local ns="${1:-${NAMESPACE:-nvsnap-system}}" + kubectl get secret nvsnap-agent-token -n "$ns" \ + -o jsonpath='{.data.token}' 2>/dev/null | base64 -d 2>/dev/null || true +} + +# Populate NVSNAP_AUTH_ARGS with the curl flags carrying the token, or leave it +# empty when there is no token to send. Kept as an array so the header survives +# word-splitting intact. +nvsnap_agent_auth_args() { + local token + token=$(nvsnap_agent_token "$@") + NVSNAP_AUTH_ARGS=() + if [ -n "$token" ]; then + NVSNAP_AUTH_ARGS=(-H "Authorization: Bearer $token") + fi +} diff --git a/src/compute-plane-services/nvsnap/scripts/test-agent-driven-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-agent-driven-e2e.sh index ade52c9a8..ee748a7c6 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-agent-driven-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-agent-driven-e2e.sh @@ -221,6 +221,7 @@ step_begin RESTORE_RESP=$(kubectl -n "$NS" exec "$AGENT_POD" -- sh -c " rm -f /var/lib/nvsnap/checkpoints/${CKPT_ID}/restore.pid /var/lib/nvsnap/checkpoints/${CKPT_ID}/restore.log curl -sS -m 600 -X POST 'http://localhost:8081/v1/restore' \ + \${NVSNAP_AGENT_TOKEN:+--oauth2-bearer \$NVSNAP_AGENT_TOKEN} \ -H 'Content-Type: application/json' \ -d '{\"checkpointId\":\"${CKPT_ID}\",\"placeholderPodName\":\"${PLACEHOLDER_NAME}\",\"placeholderNamespace\":\"${NS}\"}'") if ! echo "$RESTORE_RESP" | grep -q '"restoredPid"'; then diff --git a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh index 0ef40263f..afb54ee14 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh @@ -31,6 +31,7 @@ fi # Verify deployed agent matches expected version source "$SCRIPT_DIR/versions.sh" +source "$SCRIPT_DIR/lib/agent-auth.sh" DEPLOYED=$(kubectl get ds nvsnap-agent -n nvsnap-system -o jsonpath='{.spec.template.spec.containers[0].image}' 2>/dev/null) EXPECTED="${NVSNAP_REGISTRY}/nvsnap-agent:${NVSNAP_APP_VERSION}" if [ "$DEPLOYED" != "$EXPECTED" ]; then @@ -799,8 +800,13 @@ if [ "$CAPTURE_PATH" = "criu-v2" ]; then --field-selector "spec.nodeName=$POD_NODE" -o jsonpath='{.items[0].metadata.name}') [ -n "$AGENT_POD" ] || fail "criu-v2 restore (no agent pod on $POD_NODE)" log_info "criu-v2: agent-driven restore via $AGENT_POD (synchronous, up to 21min)..." + # curl runs in the pod, but these args are expanded by the local shell + # (no sh -c), so the token comes from the Secret rather than the + # container's NVSNAP_AGENT_TOKEN. Empty unless auth is enabled. + nvsnap_agent_auth_args "$NAMESPACE" RESTORE_RESP=$(kubectl exec -n $NAMESPACE "$AGENT_POD" -c agent -- \ curl -s --max-time 1260 -X POST "http://localhost:8081/v1/restore" \ + "${NVSNAP_AUTH_ARGS[@]}" \ -H 'Content-Type: application/json' \ -d "{\"checkpointId\":\"$CHECKPOINT_ID\",\"placeholderPodName\":\"$RESTORE_POD_NAME\",\"placeholderNamespace\":\"$NAMESPACE\"}") || true if ! printf '%s' "$RESTORE_RESP" | grep -q '"newContainerId"'; then