From 744b617ea3e39e2181dc35b70a4b5e6403df3a88 Mon Sep 17 00:00:00 2001 From: balaji Date: Mon, 27 Jul 2026 15:58:45 -0700 Subject: [PATCH 1/7] fix(nvsnap): confine agent path construction to its own directories Code scanning flagged 39 go/path-injection alerts across the agent. They are not 39 defects: every one is a filesystem call downstream of one of two identifiers the agent joins onto a host directory without checking. checkpointDir := filepath.Join(a.config.CheckpointDir, req.CheckpointID) req.CheckpointID is decoded straight from an HTTP request body, and the relative paths driving cascade fetch come from a manifest another agent serves over HTTP. The agent runs privileged with hostPath mounts covering /var/lib and the containerd root, so a "../" in either is a read or write anywhere on the node as root, not a contained bug. Closed at the two entry points rather than the 39 sinks: - validPathSegment rejects an identifier that is not a single, benign path component. Applied to Restore, TriggerRestore, EnsureLocal and the gpuRestore handler, and to every {id}/{hash}/{pod-uid} route through pathVarGuard router middleware -- a per-handler check is one forgotten line away from reopening the hole on the next route added. - joinWithinRoot is the write-side counterpart to the existing resolveWithinRoot: it confines a peer-supplied relative path to the destination directory without requiring the file to exist yet. It also closes the two-step variant a lexical check alone misses, where the peer sends a symlink out of the tree and then a file underneath it. The shape check is deliberately looser than what buildCheckpointID emits so checkpoints written by older agents stay readable; the property being enforced is "cannot leave the parent directory", not "matches today's generator". Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/agent.go | 10 ++ .../nvsnap/internal/agent/cascade_fetch.go | 22 +++- .../nvsnap/internal/agent/pathsafe.go | 99 ++++++++++++++ .../internal/agent/pathsafe_write_test.go | 124 ++++++++++++++++++ .../nvsnap/internal/agent/restore.go | 11 ++ 5 files changed, 261 insertions(+), 5 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go diff --git a/src/compute-plane-services/nvsnap/internal/agent/agent.go b/src/compute-plane-services/nvsnap/internal/agent/agent.go index 73c38c9b3..ca43cd766 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -437,6 +437,11 @@ func (a *Agent) Run(ctx context.Context) error { router := mux.NewRouter() + // 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. + router.Use(pathVarGuard) + // Metrics endpoint router.Handle("/metrics", metrics.Handler()).Methods("GET") @@ -835,6 +840,11 @@ func (a *Agent) gpuRestoreHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, fmt.Sprintf("invalid request: %v", err), http.StatusBadRequest) return } + // Body-supplied, unlike the {id} routes the middleware covers. + if err := validPathSegment("checkpointId", req.CheckpointID); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } log := a.log.WithFields(logrus.Fields{ "checkpointId": req.CheckpointID, 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 9786ec69f..c10bacb57 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go +++ b/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go @@ -118,9 +118,11 @@ func (a *Agent) EnsureLocal(ctx context.Context, checkpointID string) error { defer span.End() span.SetAttributes(attribute.String("nvsnap.checkpoint_id", checkpointID)) - if checkpointID == "" { - span.SetStatus(codes.Error, "checkpoint id required") - return errors.New("checkpoint id required") + // This ID becomes a directory that the cascade below creates, deletes + // and writes files into. Everything downstream trusts it. + if err := validPathSegment("checkpoint id", checkpointID); err != nil { + span.SetStatus(codes.Error, err.Error()) + return err } localDir := filepath.Join(a.config.CheckpointDir, checkpointID) @@ -377,7 +379,12 @@ func (a *Agent) fetchOneBlob(ctx context.Context, blobBaseURL, sha string, expec blobURL := fmt.Sprintf("%s/v1/blob/%s", blobBaseURL, sha) fileCtx, cancel := context.WithTimeout(ctx, peerFetchTimeoutPerFile) defer cancel() - dst := filepath.Join(destDir, filepath.FromSlash(relPath)) + // Same as the peer path: relPath comes from a manifest the blob store + // served, not from us. + dst, err := joinWithinRoot(destDir, filepath.FromSlash(relPath)) + if err != nil { + return fmt.Errorf("manifest entry %q: %w", relPath, err) + } return downloadToFile(fileCtx, peerHTTPClient, []string{blobURL}, expectedSize, dst) } @@ -515,7 +522,12 @@ func (a *Agent) fetchOneFile(ctx context.Context, peerURL string, alternateURLs } fileCtx, cancel := context.WithTimeout(ctx, peerFetchTimeoutPerFile) defer cancel() - dst := filepath.Join(destDir, relPath) + // relPath is an entry in the manifest the peer served, so it decides + // where we write. Confine it to destDir. + dst, err := joinWithinRoot(destDir, relPath) + if err != nil { + return fmt.Errorf("manifest entry %q: %w", relPath, err) + } return downloadToFile(fileCtx, peerHTTPClient, urls, expectedSize, dst) } diff --git a/src/compute-plane-services/nvsnap/internal/agent/pathsafe.go b/src/compute-plane-services/nvsnap/internal/agent/pathsafe.go index 30ef5d53c..392229fa9 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/pathsafe.go +++ b/src/compute-plane-services/nvsnap/internal/agent/pathsafe.go @@ -19,9 +19,13 @@ package agent import ( "fmt" + "net/http" "os" "path/filepath" + "regexp" "strings" + + "github.com/gorilla/mux" ) // resolveWithinRoot validates a caller-supplied relative path against a @@ -65,3 +69,98 @@ func resolveWithinRoot(root, relPath string) (string, error) { } return realTarget, nil } + +// pathSegment is the shape every identifier that becomes a directory name +// must have: one benign path component. Deliberately a shape check rather +// than the exact "__" buildCheckpointID emits, so +// checkpoints written by older agents stay readable; the security property +// is "cannot leave the parent directory", not "matches today's generator". +var pathSegment = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$`) + +// validPathSegment rejects an identifier that would not be a single, benign +// path component. +// +// Checkpoint IDs, capture hashes and pod UIDs all arrive over the agent's +// HTTP API and are joined onto a host directory with no further checking: +// +// checkpointDir := filepath.Join(a.config.CheckpointDir, req.CheckpointID) +// +// The agent runs privileged with hostPath mounts covering /var/lib and the +// containerd root, so a "../" here is not a contained bug -- it is a read or +// write anywhere on the node as root. Leading dots are refused too, since a +// bare ".." is otherwise a legal segment. +func validPathSegment(kind, s string) error { + if s == "" { + return fmt.Errorf("%s is required", kind) + } + if !pathSegment.MatchString(s) { + return fmt.Errorf("%s %q is not a valid identifier", kind, s) + } + return nil +} + +// pathVarGuard validates the route variables that name a directory before +// any handler runs. +// +// Applied as router middleware rather than at each call site: the agent has +// a dozen {id}/{hash}/{pod-uid} routes today, and a per-handler check is one +// forgotten line away from reopening the hole on the next route added. Here +// a new route is covered by construction. +func pathVarGuard(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for _, k := range []string{"id", "hash", "pod-uid"} { + v, ok := mux.Vars(r)[k] + if !ok { + continue + } + if err := validPathSegment(k, v); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + } + next.ServeHTTP(w, r) + }) +} + +// joinWithinRoot is the write-side counterpart to resolveWithinRoot: it +// returns where a file named by an untrusted relative path may be created +// under root, without requiring that the file already exist. +// +// The relative paths driving cascade fetch come from a manifest served by +// another agent over HTTP, and are joined straight onto the local +// destination directory. Two escapes are possible and both are closed here: +// "../" in the manifest entry (handled by anchoring the clean at "/"), and a +// symlink the peer planted earlier in the same transfer -- send "cache" as a +// link to /etc, then "cache/cron.d/x" as a regular file. Since the target +// itself does not exist yet, the deepest ancestor that does exist is the one +// resolved and boundary-checked. +func joinWithinRoot(root, relPath string) (string, error) { + cleaned := filepath.Clean("/" + relPath) + if cleaned == "/" { + return "", fmt.Errorf("empty path") + } + target := filepath.Join(root, cleaned) + + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return "", fmt.Errorf("resolve root: %w", err) + } + + // Walk up to the deepest component that exists; everything below it is + // ours to create, so it cannot be a symlink we did not just make. + probe := target + for { + resolved, rErr := filepath.EvalSymlinks(probe) + if rErr == nil { + if resolved != realRoot && !strings.HasPrefix(resolved, realRoot+string(os.PathSeparator)) { + return "", fmt.Errorf("path escapes destination root") + } + return target, nil + } + parent := filepath.Dir(probe) + if parent == probe { + return "", fmt.Errorf("resolve path: %w", rErr) + } + probe = parent + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go b/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go new file mode 100644 index 000000000..f6c7f9d0c --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go @@ -0,0 +1,124 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/ + +package agent + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gorilla/mux" +) + +func TestValidPathSegment(t *testing.T) { + ok := []string{ + "a1b2c3d4__20260724-120000", // what buildCheckpointID emits + "deadbeef", // capture hash + "3f2a1c9e-0b45-4d8f-9a11-77c0de1234ab", // pod UID + "v0.2.2", + } + for _, s := range ok { + if err := validPathSegment("id", s); err != nil { + t.Errorf("validPathSegment(%q) = %v, want nil", s, err) + } + } + + // Each of these reaches filepath.Join on a hostPath directory in an + // agent that runs privileged, so each is a read or write outside the + // checkpoint tree as root on the node. + bad := []string{ + "", + "..", + "../../etc", + "../../../var/lib/kubelet", + "a/b", + "/etc/passwd", + ".hidden", + "id\x00truncate", + "id with spaces", + "id;rm -rf /", + strings.Repeat("a", 200), + } + for _, s := range bad { + if err := validPathSegment("id", s); err == nil { + t.Errorf("validPathSegment(%q) = nil, want error", s) + } + } +} + +func TestPathVarGuardRejectsTraversal(t *testing.T) { + r := mux.NewRouter() + r.Use(pathVarGuard) + reached := false + r.HandleFunc("/v1/checkpoints/{id}/manifest", func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + // Drive the guard with the vars already bound. Going through a URL + // would only prove mux's own path normalization works: it 301s + // "/v1/checkpoints/../x" before any middleware runs. That redirect is + // not what protects us -- it is route-matching behavior we do not + // control, and the same handlers are reachable with vars set from + // other sources. Assert the guard rejects a bad var on its own. + guarded := pathVarGuard(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + for _, id := range []string{"..", "../../etc", "a/b", ""} { + reached = false + w := httptest.NewRecorder() + req := mux.SetURLVars(httptest.NewRequest(http.MethodGet, "/v1/checkpoints/x/manifest", http.NoBody), + map[string]string{"id": id}) + guarded.ServeHTTP(w, req) + if reached { + t.Errorf("id=%q: handler ran; guard did not reject it", id) + } + if w.Code != http.StatusBadRequest { + t.Errorf("id=%q: status %d, want 400", id, w.Code) + } + } + + reached = false + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/v1/checkpoints/abc__20260724-120000/manifest", http.NoBody)) + if !reached || w.Code != http.StatusOK { + t.Errorf("legitimate id rejected: reached=%v status=%d", reached, w.Code) + } +} + +func TestJoinWithinRoot(t *testing.T) { + root := t.TempDir() + + got, err := joinWithinRoot(root, "sub/dir/pages-1.img") + if err != nil { + t.Fatalf("legitimate relative path rejected: %v", err) + } + if want := filepath.Join(root, "sub", "dir", "pages-1.img"); got != want { + t.Errorf("got %q, want %q", got, want) + } + + // A peer manifest entry that climbs out of the destination. + for _, rel := range []string{"../escape", "../../etc/cron.d/x", "/etc/passwd"} { + got, err := joinWithinRoot(root, rel) + if err == nil && !strings.HasPrefix(got, root+string(os.PathSeparator)) { + t.Errorf("joinWithinRoot(%q) = %q, escaped root", rel, got) + } + } + + // The two-step attack the lexical check alone misses: the peer sends a + // symlink first, then a file "under" it. + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(root, "cache")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + if _, err := joinWithinRoot(root, "cache/payload"); err == nil { + t.Error("write through a symlink pointing outside the root was allowed") + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/restore.go b/src/compute-plane-services/nvsnap/internal/agent/restore.go index c61977ffb..320ab6dca 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/restore.go +++ b/src/compute-plane-services/nvsnap/internal/agent/restore.go @@ -215,6 +215,10 @@ type PlaceholderManifestRequest struct { // TriggerRestore writes a trigger file for a placeholder pod to start restoring func (a *Agent) TriggerRestore(ctx context.Context, req TriggerRestoreRequest) (*TriggerRestoreResult, error) { + if err := validPathSegment("checkpointId", req.CheckpointID); err != nil { + return nil, err + } + log := a.log.WithFields(logrus.Fields{ "checkpointId": req.CheckpointID, "placeholderId": req.PlaceholderContainerID, @@ -390,6 +394,13 @@ spec: // Restore restores a checkpointed process using CRIU directly. // This uses the host's CRIU binary to restore the process. func (a *Agent) Restore(ctx context.Context, req RestoreRequest) (*RestoreResult, error) { + // The ID is joined onto CheckpointDir below and reaches os.Stat, + // os.ReadFile and os.WriteFile from there. Reject anything that is not + // a single path component before it becomes a path. + if err := validPathSegment("checkpointId", req.CheckpointID); err != nil { + return nil, err + } + ctx, span := tracing.Tracer().Start(ctx, "restore.full") defer span.End() span.SetAttributes( From 7a59a4d81234c879ef0ed6267ce131e86b6cac76 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 29 Jul 2026 09:15:58 -0700 Subject: [PATCH 2/7] test(nvsnap): fold path-safety tests into pathsafe_test.go CI's BUILD-file check (#491) failed: pathsafe_write_test.go was a new file absent from internal/agent/BUILD.bazel srcs. Adding it to BUILD would work, but the package already pairs one test file per source file and pathsafe_test.go was the obvious home. Same tests, no BUILD churn, and one fewer place to look for coverage of pathsafe.go. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/pathsafe_test.go | 112 ++++++++++++++++ .../internal/agent/pathsafe_write_test.go | 124 ------------------ 2 files changed, 112 insertions(+), 124 deletions(-) delete mode 100644 src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go diff --git a/src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go b/src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go index 191ffa1e5..b4cda7771 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go @@ -19,9 +19,14 @@ package agent import ( "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" "testing" + + "github.com/gorilla/mux" ) func TestResolveWithinRoot(t *testing.T) { @@ -98,3 +103,110 @@ func TestResolveWithinRoot(t *testing.T) { } }) } + +func TestValidPathSegment(t *testing.T) { + ok := []string{ + "a1b2c3d4__20260724-120000", // what buildCheckpointID emits + "deadbeef", // capture hash + "3f2a1c9e-0b45-4d8f-9a11-77c0de1234ab", // pod UID + "v0.2.2", + } + for _, s := range ok { + if err := validPathSegment("id", s); err != nil { + t.Errorf("validPathSegment(%q) = %v, want nil", s, err) + } + } + + // Each of these reaches filepath.Join on a hostPath directory in an + // agent that runs privileged, so each is a read or write outside the + // checkpoint tree as root on the node. + bad := []string{ + "", + "..", + "../../etc", + "../../../var/lib/kubelet", + "a/b", + "/etc/passwd", + ".hidden", + "id\x00truncate", + "id with spaces", + "id;rm -rf /", + strings.Repeat("a", 200), + } + for _, s := range bad { + if err := validPathSegment("id", s); err == nil { + t.Errorf("validPathSegment(%q) = nil, want error", s) + } + } +} + +func TestPathVarGuardRejectsTraversal(t *testing.T) { + r := mux.NewRouter() + r.Use(pathVarGuard) + reached := false + r.HandleFunc("/v1/checkpoints/{id}/manifest", func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + // Drive the guard with the vars already bound. Going through a URL + // would only prove mux's own path normalization works: it 301s + // "/v1/checkpoints/../x" before any middleware runs. That redirect is + // not what protects us -- it is route-matching behavior we do not + // control, and the same handlers are reachable with vars set from + // other sources. Assert the guard rejects a bad var on its own. + guarded := pathVarGuard(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + for _, id := range []string{"..", "../../etc", "a/b", ""} { + reached = false + w := httptest.NewRecorder() + req := mux.SetURLVars(httptest.NewRequest(http.MethodGet, "/v1/checkpoints/x/manifest", http.NoBody), + map[string]string{"id": id}) + guarded.ServeHTTP(w, req) + if reached { + t.Errorf("id=%q: handler ran; guard did not reject it", id) + } + if w.Code != http.StatusBadRequest { + t.Errorf("id=%q: status %d, want 400", id, w.Code) + } + } + + reached = false + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/v1/checkpoints/abc__20260724-120000/manifest", http.NoBody)) + if !reached || w.Code != http.StatusOK { + t.Errorf("legitimate id rejected: reached=%v status=%d", reached, w.Code) + } +} + +func TestJoinWithinRoot(t *testing.T) { + root := t.TempDir() + + got, err := joinWithinRoot(root, "sub/dir/pages-1.img") + if err != nil { + t.Fatalf("legitimate relative path rejected: %v", err) + } + if want := filepath.Join(root, "sub", "dir", "pages-1.img"); got != want { + t.Errorf("got %q, want %q", got, want) + } + + // A peer manifest entry that climbs out of the destination. + for _, rel := range []string{"../escape", "../../etc/cron.d/x", "/etc/passwd"} { + got, err := joinWithinRoot(root, rel) + if err == nil && !strings.HasPrefix(got, root+string(os.PathSeparator)) { + t.Errorf("joinWithinRoot(%q) = %q, escaped root", rel, got) + } + } + + // The two-step attack the lexical check alone misses: the peer sends a + // symlink first, then a file "under" it. + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(root, "cache")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + if _, err := joinWithinRoot(root, "cache/payload"); err == nil { + t.Error("write through a symlink pointing outside the root was allowed") + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go b/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go deleted file mode 100644 index f6c7f9d0c..000000000 --- a/src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go +++ /dev/null @@ -1,124 +0,0 @@ -/* -SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -SPDX-License-Identifier: Apache-2.0 -*/ - -package agent - -import ( - "net/http" - "net/http/httptest" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/gorilla/mux" -) - -func TestValidPathSegment(t *testing.T) { - ok := []string{ - "a1b2c3d4__20260724-120000", // what buildCheckpointID emits - "deadbeef", // capture hash - "3f2a1c9e-0b45-4d8f-9a11-77c0de1234ab", // pod UID - "v0.2.2", - } - for _, s := range ok { - if err := validPathSegment("id", s); err != nil { - t.Errorf("validPathSegment(%q) = %v, want nil", s, err) - } - } - - // Each of these reaches filepath.Join on a hostPath directory in an - // agent that runs privileged, so each is a read or write outside the - // checkpoint tree as root on the node. - bad := []string{ - "", - "..", - "../../etc", - "../../../var/lib/kubelet", - "a/b", - "/etc/passwd", - ".hidden", - "id\x00truncate", - "id with spaces", - "id;rm -rf /", - strings.Repeat("a", 200), - } - for _, s := range bad { - if err := validPathSegment("id", s); err == nil { - t.Errorf("validPathSegment(%q) = nil, want error", s) - } - } -} - -func TestPathVarGuardRejectsTraversal(t *testing.T) { - r := mux.NewRouter() - r.Use(pathVarGuard) - reached := false - r.HandleFunc("/v1/checkpoints/{id}/manifest", func(w http.ResponseWriter, _ *http.Request) { - reached = true - w.WriteHeader(http.StatusOK) - }) - - // Drive the guard with the vars already bound. Going through a URL - // would only prove mux's own path normalization works: it 301s - // "/v1/checkpoints/../x" before any middleware runs. That redirect is - // not what protects us -- it is route-matching behavior we do not - // control, and the same handlers are reachable with vars set from - // other sources. Assert the guard rejects a bad var on its own. - guarded := pathVarGuard(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - reached = true - w.WriteHeader(http.StatusOK) - })) - for _, id := range []string{"..", "../../etc", "a/b", ""} { - reached = false - w := httptest.NewRecorder() - req := mux.SetURLVars(httptest.NewRequest(http.MethodGet, "/v1/checkpoints/x/manifest", http.NoBody), - map[string]string{"id": id}) - guarded.ServeHTTP(w, req) - if reached { - t.Errorf("id=%q: handler ran; guard did not reject it", id) - } - if w.Code != http.StatusBadRequest { - t.Errorf("id=%q: status %d, want 400", id, w.Code) - } - } - - reached = false - w := httptest.NewRecorder() - r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/v1/checkpoints/abc__20260724-120000/manifest", http.NoBody)) - if !reached || w.Code != http.StatusOK { - t.Errorf("legitimate id rejected: reached=%v status=%d", reached, w.Code) - } -} - -func TestJoinWithinRoot(t *testing.T) { - root := t.TempDir() - - got, err := joinWithinRoot(root, "sub/dir/pages-1.img") - if err != nil { - t.Fatalf("legitimate relative path rejected: %v", err) - } - if want := filepath.Join(root, "sub", "dir", "pages-1.img"); got != want { - t.Errorf("got %q, want %q", got, want) - } - - // A peer manifest entry that climbs out of the destination. - for _, rel := range []string{"../escape", "../../etc/cron.d/x", "/etc/passwd"} { - got, err := joinWithinRoot(root, rel) - if err == nil && !strings.HasPrefix(got, root+string(os.PathSeparator)) { - t.Errorf("joinWithinRoot(%q) = %q, escaped root", rel, got) - } - } - - // The two-step attack the lexical check alone misses: the peer sends a - // symlink first, then a file "under" it. - outside := t.TempDir() - if err := os.Symlink(outside, filepath.Join(root, "cache")); err != nil { - t.Skipf("symlink unsupported: %v", err) - } - if _, err := joinWithinRoot(root, "cache/payload"); err == nil { - t.Error("write through a symlink pointing outside the root was allowed") - } -} From c49d23c36d87243515c4a4feaf723262679efca9 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 29 Jul 2026 09:37:27 -0700 Subject: [PATCH 3/7] feat(nvsnap): shared-token authentication for the agent API (#486) The agent API had no authentication. Any caller reaching the port got the full control surface of a privileged process: POST /v1/restore, DELETE /v1/checkpoints/{id}, GET /v1/checkpoints/{id}/file, and /debug/pprof/*, on a process running privileged with /var/lib and the containerd root bind-mounted. That is wider than it looks, because the DaemonSet binds it to each node's IP (hostNetwork + hostPort 8081) rather than a cluster-internal Service, and NetworkPolicy cannot fence it -- a hostNetwork pod carries node identity, so a podSelector ingress rule does not match it. Access control 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, which is the path range chunking and parallel multi-source fetch exist to speed up. TLS handshakes amortize with connection reuse; per-byte encryption on the bulk stream does not. A header comparison costs nothing on either control or transfer requests. Three modes, because agents and their callers cannot be updated in the same instant: disabled (default) no check, so an upgrade without a token behaves exactly as before -- but the agent logs a warning saying so permissive check, count and log failures, serve anyway. Operators run here until nvsnap_agent_auth_total{result="missing|invalid"} reaches zero, which proves every caller sends a token required 401 Details worth noting: - The token is env-only (NVSNAP_AGENT_TOKEN), never a flag: flag values appear in the pod spec and in `ps`, and this is a credential. - Comparison is constant-time, so the token cannot be recovered byte by byte from response timing. - /health and /metrics are exempt so probes and scraping keep working without distributing the token to kubelet and Prometheus. /debug/pprof/* is NOT exempt: profiles expose memory contents and goroutine state. - An unrecognized --auth-mode fails startup rather than falling back to disabled. An operator who typos the flag should hear about it immediately, not discover months later that the API was open. - nvsnap_agent_auth_total is pre-initialized to zero for all three results so the series exist on the first scrape and absent() alerts do not misfire. This is the server half. Callers (nvsnap-server, restore-entrypoint, webhook, peer agents) do not send the header yet, which is why the default is disabled and why permissive exists. Chart wiring and the client half follow. Co-Authored-By: Balaji Ganesan --- .../nvsnap/cmd/agent/main.go | 18 +++ .../nvsnap/internal/agent/BUILD.bazel | 2 + .../nvsnap/internal/agent/agent.go | 20 +++ .../nvsnap/internal/agent/auth.go | 153 ++++++++++++++++++ .../nvsnap/internal/agent/auth_test.go | 127 +++++++++++++++ .../nvsnap/internal/metrics/metrics.go | 18 +++ 6 files changed, 338 insertions(+) create mode 100644 src/compute-plane-services/nvsnap/internal/agent/auth.go create mode 100644 src/compute-plane-services/nvsnap/internal/agent/auth_test.go diff --git a/src/compute-plane-services/nvsnap/cmd/agent/main.go b/src/compute-plane-services/nvsnap/cmd/agent/main.go index 8ecc62a44..cb50d0a58 100644 --- a/src/compute-plane-services/nvsnap/cmd/agent/main.go +++ b/src/compute-plane-services/nvsnap/cmd/agent/main.go @@ -90,6 +90,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)") @@ -204,6 +209,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/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index f98b80baf..128a15832 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "agent.go", "blob_uploader.go", + "auth.go", "capture_cascade.go", "capture_peer.go", "cascade_fetch.go", @@ -90,6 +91,7 @@ go_test( "l2_promote_async_test.go", "l2_writer_test.go", "nim_backend_test.go", + "auth_test.go", "pathsafe_test.go", "peer_fanout_test.go", "peer_load_test.go", diff --git a/src/compute-plane-services/nvsnap/internal/agent/agent.go b/src/compute-plane-services/nvsnap/internal/agent/agent.go index ca43cd766..c13ebe664 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -67,6 +67,14 @@ type Config struct { CRIUPath string NodeName string LogLevel string + + // 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 @@ -442,6 +450,18 @@ func (a *Agent) Run(ctx context.Context) error { // without remembering to. See pathVarGuard in pathsafe.go. router.Use(pathVarGuard) + // Authenticate before anything else runs. Installed only when a token is + // configured, so an upgrade that has not been given one behaves exactly + // as before -- but say so loudly, since an agent silently serving an + // unauthenticated privileged API is the state this guards against. + 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)") + } + // Metrics endpoint router.Handle("/metrics", metrics.Handler()).Methods("GET") 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..cfce728a1 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/auth.go @@ -0,0 +1,153 @@ +/* +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" + + "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 when there is nothing to enforce, so the caller can skip +// installing it entirely rather than paying for a no-op on every request. +func tokenGuard(mode AuthMode, token string, log *logrus.Logger) func(http.Handler) http.Handler { + if mode == AuthDisabled || token == "" { + 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) + }) + } +} + +const ( + authOK = "ok" + authMissing = "missing" + authInvalid = "invalid" +) + +// 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 + } + got, ok := strings.CutPrefix(h, "Bearer ") + if !ok { + 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..adf63a248 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/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 +*/ + +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}, + } + 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") + } + // An empty token is the same situation: nothing to enforce against. + if g := tokenGuard(AuthRequired, "", quietLog()); g != nil { + t.Error("empty token returned a middleware") + } +} + +// 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) + } + } +} diff --git a/src/compute-plane-services/nvsnap/internal/metrics/metrics.go b/src/compute-plane-services/nvsnap/internal/metrics/metrics.go index 555852b20..6b5c55a64 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. @@ -134,7 +146,13 @@ func RegisterAgent() { ActiveOperations, CRIUDumpDuration, GPUProcessesDiscovered, + AgentAuthTotal, ) + // Counters must exist before the first scrape or rate() gaps and + // absent() alerts misfire. + for _, r := range []string{"ok", "missing", "invalid"} { + AgentAuthTotal.WithLabelValues(r) + } }) } From 2dfd2d760154e3dfb85bbeec7f1b69786e2dfbd3 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 29 Jul 2026 16:17:46 -0700 Subject: [PATCH 4/7] feat(nvsnap): send and require the agent token end to end (#486) Completes the auth work: callers now present the token and the chart distributes it, so the feature is usable rather than just implemented. Outbound, agent to peer: authTransport wraps peerHTTPClient's tuned transport instead of editing call sites. Every cascade and capture-fanout request is signed, and a peer endpoint added later is authenticated without anyone remembering to do it -- the same reasoning as pathVarGuard inbound. The token lives in an atomic package var because peerHTTPClient is built at import time, long before flags are parsed. An unset token sends no header, so a cluster running with auth off is byte-for-byte unchanged on the wire. Outbound, init container to agent: nvsnap-mount-prep reads NVSNAP_AGENT_TOKEN and attaches it to both the POST and the status poll. The webhook injects that env from the Secret with optional: true, so a pod admitted before the operator enables auth still starts. Chart: agent.auth.{enabled,mode,token}, default off. When enabled the chart renders a Secret, sets NVSNAP_AGENT_TOKEN on the DaemonSet, and passes --auth-mode. Two details that matter more than they look: - The generated token is preserved across upgrades via a lookup of the existing Secret. `helm upgrade` re-renders every template, so a fresh randAlphaNum on each upgrade would rotate the credential out from under running callers and cause a self-inflicted outage mid-rollout. - helm.sh/resource-policy: keep, so a delete/reinstall cycle does not silently rotate it either. Verified by rendering the chart both ways: default produces zero occurrences of the Secret, the env, or the flag; enabled produces all four wiring points, a 48-character generated token, and honors an explicitly set one. helm lint passes. Tests cover the transport signing (and not mutating the caller's request), the no-token case sending nothing, and a round trip asserting that a request signed by authTransport is accepted by tokenGuard -- testing the two halves separately would not catch a format mismatch between them. Co-Authored-By: Balaji Ganesan --- .../nvsnap/cmd/nvsnap-mount-prep/main.go | 17 ++++++ .../nvsnap/templates/agent-daemonset.yaml | 17 ++++++ .../nvsnap/templates/agent-token-secret.yaml | 45 +++++++++++++++ .../nvsnap/deploy/helm/nvsnap/values.yaml | 23 ++++++++ .../nvsnap/internal/agent/agent.go | 4 ++ .../nvsnap/internal/agent/auth.go | 38 +++++++++++++ .../nvsnap/internal/agent/auth_test.go | 57 +++++++++++++++++++ .../nvsnap/internal/agent/cascade_fetch.go | 8 ++- .../internal/webhook/mount_prep_init.go | 20 +++++++ 9 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml 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 6848d8497..a60673b8c 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 @@ -82,6 +82,12 @@ 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 }} - --cuda-checkpoint-path=/criu-bundle/cuda-checkpoint - --criu-path=/criu-bundle/criu # Translate in-container --checkpoint-dir to the host path @@ -168,6 +174,17 @@ spec: valueFrom: fieldRef: fieldPath: status.hostIP + {{- 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 }}" 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/values.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml index 9bfb7a2bb..4679fd911 100644 --- a/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml +++ b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml @@ -183,6 +183,29 @@ agent: # 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/agent.go b/src/compute-plane-services/nvsnap/internal/agent/agent.go index c13ebe664..5646fdfd7 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -454,6 +454,10 @@ func (a *Agent) Run(ctx context.Context) error { // configured, so an upgrade that has not been given one behaves exactly // as before -- but say so loudly, since an agent silently serving an // unauthenticated privileged API is the state this guards against. + // 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) + 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") diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth.go b/src/compute-plane-services/nvsnap/internal/agent/auth.go index cfce728a1..01399937b 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/auth.go +++ b/src/compute-plane-services/nvsnap/internal/agent/auth.go @@ -22,6 +22,7 @@ import ( "fmt" "net/http" "strings" + "sync/atomic" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/metrics" "github.com/sirupsen/logrus" @@ -134,6 +135,43 @@ const ( 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. diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth_test.go b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go index adf63a248..048929f24 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/auth_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go @@ -125,3 +125,60 @@ func TestParseAuthMode(t *testing.T) { } } } + +// 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..eddbc30e3 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// 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..de474674e 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 @@ -26,10 +26,20 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/utils/ptr" "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. @@ -104,6 +114,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: ptr.To(true), + }, + }}, }, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ From 28ea9be195184e35f15955fd3f0d7a023a5796de Mon Sep 17 00:00:00 2001 From: balaji Date: Fri, 31 Jul 2026 08:09:17 -0700 Subject: [PATCH 5/7] fix(nvsnap): address review on agent authentication (#486) Four findings from CodeRabbit. Fail closed when required has no token. tokenGuard returned nil for AuthRequired with an empty token, so Agent.Run skipped the middleware and served the privileged API unauthenticated. Startup already rejected that combination, but a security primitive that silently becomes a no-op when misconfigured is the wrong shape: any future caller building a guard without going through main() would open the API and nothing would say so. Now returns a deny-all guard, logs why, and keeps /health and /metrics working so the operator sees the misconfiguration rather than a crashloop. Authenticate before validating. gorilla/mux runs middleware in registration order, and pathVarGuard was registered first, so a malformed {id} got a 400 before the caller was authenticated -- telling an unauthenticated client which routes exist and how their variables are shaped. Swapped. Accept the Bearer scheme case-insensitively. RFC 7235 makes the scheme case-insensitive, so "bearer " is a valid credential a conforming client may send and we were rejecting it. Compared with EqualFold; the token itself stays a byte-exact constant-time compare. RED metrics on the agent API. metrics.InstrumentRoute already existed and was wired only to nvsnap-server, so the agent's API had no rate, error or duration series at all. Now registered outermost on the agent router, so it also observes requests the auth guard rejects -- a spike of 401s is exactly what the permissive-to-required rollout needs to watch. Keyed on the route template, not the concrete path, so checkpoint IDs never become label values. The shared rate/duration pair is now registered through its own sync.Once, since both RegisterAgent and RegisterServer reference it and prometheus.MustRegister panics on a duplicate. A test pins that calling both, twice, does not panic. Not adopting the per-request OpenTelemetry span from the same comment. This router serves the peer file-transfer endpoints, where a large fetch is hundreds of parallel range requests; a span each would add real overhead and cardinality to the exact path we chose a header check over mTLS to keep fast. The route-level RED metrics give the rate, error and duration signal, and the existing operation spans still cover checkpoint and restore. Happy to add inbound spans scoped to the control endpoints if reviewers want them. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/agent.go | 28 +++++++---- .../nvsnap/internal/agent/auth.go | 43 ++++++++++++++-- .../nvsnap/internal/agent/auth_test.go | 50 +++++++++++++++++-- .../nvsnap/internal/metrics/BUILD.bazel | 8 ++- .../nvsnap/internal/metrics/metrics.go | 18 ++++++- .../nvsnap/internal/metrics/register_test.go | 24 +++++++++ 6 files changed, 150 insertions(+), 21 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/internal/metrics/register_test.go diff --git a/src/compute-plane-services/nvsnap/internal/agent/agent.go b/src/compute-plane-services/nvsnap/internal/agent/agent.go index 5646fdfd7..d788f60f0 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -74,8 +74,8 @@ type Config struct { 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) + 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 @@ -445,19 +445,22 @@ func (a *Agent) Run(ctx context.Context) error { router := mux.NewRouter() - // 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. - router.Use(pathVarGuard) + // 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()) - // Authenticate before anything else runs. Installed only when a token is - // configured, so an upgrade that has not been given one behaves exactly - // as before -- but say so loudly, since an agent silently serving an - // unauthenticated privileged API is the state this guards against. // 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") @@ -466,6 +469,11 @@ func (a *Agent) Run(ctx context.Context) error { "--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. + router.Use(pathVarGuard) + // Metrics endpoint router.Handle("/metrics", metrics.Handler()).Methods("GET") diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth.go b/src/compute-plane-services/nvsnap/internal/agent/auth.go index 01399937b..57373332d 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/auth.go +++ b/src/compute-plane-services/nvsnap/internal/agent/auth.go @@ -92,10 +92,25 @@ var unauthenticatedPaths = map[string]bool{ // tokenGuard returns middleware enforcing mode against token. // -// Returns nil when there is nothing to enforce, so the caller can skip -// installing it entirely rather than paying for a no-op on every request. +// 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 || token == "" { + 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 { @@ -129,6 +144,21 @@ func tokenGuard(mode AuthMode, token string, log *logrus.Logger) func(http.Handl } } +// 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" @@ -180,8 +210,11 @@ func checkToken(r *http.Request, want string) string { if h == "" { return authMissing } - got, ok := strings.CutPrefix(h, "Bearer ") - if !ok { + // 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 { diff --git a/src/compute-plane-services/nvsnap/internal/agent/auth_test.go b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go index 048929f24..924ec1977 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/auth_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/auth_test.go @@ -59,6 +59,10 @@ func TestTokenGuardRequired(t *testing.T) { // 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) { @@ -84,9 +88,49 @@ 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") } - // An empty token is the same situation: nothing to enforce against. - if g := tokenGuard(AuthRequired, "", quietLog()); g != nil { - t.Error("empty token returned a middleware") + // 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) } } 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 6b5c55a64..8cf090bec 100644 --- a/src/compute-plane-services/nvsnap/internal/metrics/metrics.go +++ b/src/compute-plane-services/nvsnap/internal/metrics/metrics.go @@ -132,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() { @@ -148,6 +160,9 @@ func RegisterAgent() { 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"} { @@ -160,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() +} From 0f368ffe7ce6d99943058f0f6e4436f93d1ff9f7 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 4 Aug 2026 10:51:23 -0700 Subject: [PATCH 6/7] feat(nvsnap): make pod networking a supported mode for the agent (#561) Co-authored-by: balaji --- .../nvsnap/cmd/agent/main.go | 4 ++ .../nvsnap/templates/agent-daemonset.yaml | 55 +++++++++++++++- .../network-policy-restore-pods.yaml | 19 +++++- .../nvsnap/deploy/helm/nvsnap/values.yaml | 22 ++++++- .../nvsnap/internal/agent/BUILD.bazel | 1 + .../nvsnap/internal/agent/advertise_test.go | 63 +++++++++++++++++++ .../nvsnap/internal/agent/agent.go | 11 ++++ .../nvsnap/internal/agent/cascade_fetch.go | 12 +++- .../internal/agent/webhook_integration.go | 7 +++ .../internal/webhook/mount_prep_init.go | 16 +++-- .../nvsnap/internal/webhook/mutate.go | 8 +++ 11 files changed, 205 insertions(+), 13 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/internal/agent/advertise_test.go diff --git a/src/compute-plane-services/nvsnap/cmd/agent/main.go b/src/compute-plane-services/nvsnap/cmd/agent/main.go index cb50d0a58..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). @@ -188,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).") 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 a60673b8c..192dad720 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: @@ -88,6 +88,12 @@ spec: # 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 @@ -174,6 +180,13 @@ 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`. @@ -242,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 @@ -403,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/network-policy-restore-pods.yaml b/src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml index 2829436cb..1780c62bd 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 @@ -107,7 +107,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 @@ -129,8 +129,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 4679fd911..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,9 +176,27 @@ 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 diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 128a15832..7f927d264 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -91,6 +91,7 @@ go_test( "l2_promote_async_test.go", "l2_writer_test.go", "nim_backend_test.go", + "advertise_test.go", "auth_test.go", "pathsafe_test.go", "peer_fanout_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 d788f60f0..b46ca1b28 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/agent.go +++ b/src/compute-plane-services/nvsnap/internal/agent/agent.go @@ -121,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 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 eddbc30e3..2de53ca4c 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go +++ b/src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go @@ -576,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/webhook/mount_prep_init.go b/src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go index de474674e..2b9843710 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" @@ -89,12 +90,17 @@ 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). agentURL := fmt.Sprintf("http://$(NVSNAP_HOST_IP):%d", agentPort) + if m.AgentBaseURL != "" { + agentURL = strings.TrimRight(m.AgentBaseURL, "/") + } c := corev1.Container{ Name: MountPrepContainerName, 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. From 6c34a38a8740d93e415d40f67b095e4d4edc67ce Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 4 Aug 2026 21:29:21 -0700 Subject: [PATCH 7/7] build(nvsnap): drop the k8s.io/utils/ptr import from the webhook CI's bazel build failed on internal/webhook with "missing strict dependencies: import of k8s.io/utils/ptr". My doing: I reached for ptr.To(true) when adding the optional Secret reference for the agent token, and never declared the dependency in the BUILD file. `go test` resolves it from the module graph, so it passed locally and only bazel's strict-deps enforcement caught it. Inlined an addressable bool instead of declaring the dep. One call site, one file, and nothing else in nvsnap imports it -- AGENTS.md says not to add a library for something that can be safely expressed in existing code, and under bazel that import is a new external node in the build graph for a line the language already has. Also picks up gazelle's reordering of auth_test.go and advertise_test.go in the agent BUILD after the main merge. Confirmed both stayed in go_test rather than drifting into go_library, along with pathsafe_test.go. Unrelated BUILD churn gazelle produced under src/libraries/{java,rust} was reverted; it is outside this PR and outside check-gazelle's Go-root scope. Verified: build clean; 13 internal packages pass, 0 fail; gofmt clean; helm lint clean; check-gazelle reports BUILD files up to date. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/BUILD.bazel | 6 +++--- .../nvsnap/internal/webhook/mount_prep_init.go | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 7f927d264..8f763b850 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -4,8 +4,8 @@ go_library( name = "agent", srcs = [ "agent.go", - "blob_uploader.go", "auth.go", + "blob_uploader.go", "capture_cascade.go", "capture_peer.go", "cascade_fetch.go", @@ -82,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", @@ -91,8 +93,6 @@ go_test( "l2_promote_async_test.go", "l2_writer_test.go", "nim_backend_test.go", - "advertise_test.go", - "auth_test.go", "pathsafe_test.go", "peer_fanout_test.go", "peer_load_test.go", 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 2b9843710..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 @@ -27,7 +27,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/utils/ptr" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/checkpointstore" ) @@ -97,6 +96,12 @@ func (m *Mutator) emitMountPrepInitContainer( // 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, "/") @@ -127,7 +132,7 @@ func (m *Mutator) emitMountPrepInitContainer( SecretKeyRef: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{Name: AgentTokenSecretName}, Key: AgentTokenSecretKey, - Optional: ptr.To(true), + Optional: &secretOptional, }, }}, },