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/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..541f4673b --- /dev/null +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml @@ -0,0 +1,45 @@ +{{- 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 }} +{{- $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 }} +{{- 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/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..924ec1977 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go @@ -0,0 +1,228 @@ +/* +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) + } +} 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..2de53ca4c 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,7 +107,7 @@ 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, - }, + }}, } // EnsureLocal guarantees that /var/lib/nvsnap/checkpoints// @@ -572,14 +576,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/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.