From a01ec4bbef481c64caabc873171b5721977b6543 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 14:32:01 -0500 Subject: [PATCH 1/6] [gnoi] file: implement Stat in-house using /mnt/host Replace the DBus-backed File.Stat handler with a pure-Go implementation that reads metadata directly via os.Stat / os.ReadDir on the host filesystem, bind-mounted into the gnmi container at /mnt/host. The existing translatePathForContainer helper (already used by Put, TransferToRemote, and Remove) handles the host-vs-container path prefix. Per the gNOI proto ("Stat will list files at the provided path") the handler now supports both cases: - regular file -> single StatInfo for that file - directory -> one StatInfo per immediate child (non-recursive) Permissions are encoded octal-as-decimal as the proto requires (e.g. 0644 -> Permissions=644). The umask field is reported as a constant 0022 (defaultUmask); per-file umask is not derivable from os.Stat and the previous DBus implementation also returned a process umask, so behavior is unchanged for callers that don't rely on the exact value. Tests: - 8 new pure-Go unit tests in pkg/gnoi/file/stat_test.go covering nil/empty/relative path, NotFound, regular file, directory listing (incl. subdirs), empty directory, and the octal-as-decimal permissions encoding. - Existing gnmi_server Stat tests rewritten to drive the new handler (no DBus mock); removes the dependency on sonic_service_client.GetFileStat for Stat. - Local pure-test run: 636 tests pass (was 628; +8 new). - Live verified end-to-end on a sonic-mgmt KVM testbed against the gnmi container's /var/run/gnmi/gnmi.sock UDS for /tmp/, /etc/, /var/log/, and /host/reboot-cause; see PR description for the full test matrix and a reproducible grpcurl example. Signed-off-by: Dawei Huang --- gnmi_server/gnoi_file.go | 71 +------------ gnmi_server/gnoi_file_test.go | 190 ++++++++++++++++------------------ gnmi_server/server_test.go | 63 +++++------ pkg/gnoi/file/file.go | 125 ++++++++++++++++++++++ pkg/gnoi/file/stat_test.go | 188 +++++++++++++++++++++++++++++++++ 5 files changed, 433 insertions(+), 204 deletions(-) create mode 100644 pkg/gnoi/file/stat_test.go diff --git a/gnmi_server/gnoi_file.go b/gnmi_server/gnoi_file.go index c286a0eff..3df6a5b8f 100644 --- a/gnmi_server/gnoi_file.go +++ b/gnmi_server/gnoi_file.go @@ -2,16 +2,10 @@ package gnmi import ( "context" - "strconv" - "strings" log "github.com/golang/glog" gnoi_file_pb "github.com/openconfig/gnoi/file" gnoifile "github.com/sonic-net/sonic-gnmi/pkg/gnoi/file" - ssc "github.com/sonic-net/sonic-gnmi/sonic_service_client" - - // Renamed local package alias to avoid redeclaration / collision. - filepkg "github.com/sonic-net/sonic-gnmi/pkg/gnoi/file" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -24,68 +18,7 @@ func (srv *FileServer) Stat(ctx context.Context, req *gnoi_file_pb.StatRequest) log.Errorf("authentication failed in Stat RPC: %v", err) return nil, err } - path := req.GetPath() - log.V(1).Info("Request: ", req) - statInfo, err := readFileStat(path) - if err != nil { - log.Errorf("readFileStat error: %v", err) - return nil, err - } - resp := &gnoi_file_pb.StatResponse{ - Stats: []*gnoi_file_pb.StatInfo{statInfo}, - } - return resp, nil -} - -func readFileStat(path string) (*gnoi_file_pb.StatInfo, error) { - sc, err := ssc.NewDbusClient() - if err != nil { - log.Errorf("DbusClient init failed: %v", err) - return nil, status.Errorf(codes.Internal, "DBus client init failed: %v", err) - } - defer sc.Close() - data, err := sc.GetFileStat(path) - if err != nil { - log.V(2).Infof("Failed to read file stat at path %s: %v. Error ", path, err) - return nil, err - } - // Parse the data and populate StatInfo - lastModified, err := strconv.ParseUint(data["last_modified"], 10, 64) - if err != nil { - log.Errorf("Stat Fails on Invalid last_modified %v", err) - return nil, err - } - - permissions, err := strconv.ParseUint(data["permissions"], 8, 32) - if err != nil { - log.Errorf("Stat Fails on Invalid permissions: %v", err) - return nil, err - } - - size, err := strconv.ParseUint(data["size"], 10, 64) - if err != nil { - log.Errorf("Stat Fails on Invalid size: %v", err) - return nil, err - } - - umaskStr := data["umask"] - if strings.HasPrefix(umaskStr, "o") { - umaskStr = umaskStr[1:] // Remove leading "o" - } - umask, err := strconv.ParseUint(umaskStr, 8, 32) - if err != nil { - log.Errorf("Stat Fails on Invalid umaskStr: %v", err) - return nil, err - } - - statInfo := &gnoi_file_pb.StatInfo{ - Path: data["path"], - LastModified: lastModified, - Permissions: uint32(permissions), - Size: size, - Umask: uint32(umask), - } - return statInfo, nil + return gnoifile.HandleStat(ctx, req) } // Get RPC is unimplemented. @@ -139,5 +72,5 @@ func (srv *FileServer) Remove(ctx context.Context, req *gnoi_file_pb.RemoveReque return nil, err } // Delegate to handler (all logic except authentication is in the handler) - return filepkg.HandleFileRemove(ctx, req) + return gnoifile.HandleFileRemove(ctx, req) } diff --git a/gnmi_server/gnoi_file_test.go b/gnmi_server/gnoi_file_test.go index 18da7174d..c2f548cfc 100644 --- a/gnmi_server/gnoi_file_test.go +++ b/gnmi_server/gnoi_file_test.go @@ -5,7 +5,7 @@ import ( "fmt" "net" "os" - "reflect" + "path/filepath" "testing" "github.com/agiledragon/gomonkey/v2" @@ -73,130 +73,124 @@ func TestGnoiFileServer(t *testing.T) { t.Run("Stat Success", func(t *testing.T) { patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - patch2 := gomonkey.ApplyFuncReturn(ssc.NewDbusClient, &ssc.FakeClient{}, nil) defer patch1.Reset() - defer patch2.Reset() - req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} + // Create a real temp file so HandleStat can stat it. + // HandleStat uses translatePathForContainer, which prepends /mnt/host + // only if /mnt/host exists on the test machine. Build the request path + // so it points at the temp file regardless of which branch is taken. + tmpDir := t.TempDir() + f, err := os.CreateTemp(tmpDir, "stat-success-*.txt") + if err != nil { + t.Fatalf("failed to create temp file: %v", err) + } + realPath := f.Name() + if _, err := f.WriteString("hello"); err != nil { + t.Fatalf("write: %v", err) + } + f.Close() + + // If /mnt/host exists, translatePathForContainer will prepend it, + // so feed it the un-prefixed path. Otherwise pass realPath as-is. + reqPath := realPath + if _, err := os.Stat("/mnt/host"); err == nil { + // Build reverse: strip /mnt/host if temp file already lives under it, + // or skip the test when the temp dir is not reachable from /mnt/host. + t.Skip("test relies on plain /tmp; /mnt/host present on host") + } + + req := &gnoi_file_pb.StatRequest{Path: reqPath} resp, err := client.Stat(context.Background(), req) if err != nil { t.Fatalf("Expected success, got error: %v", err) } - if len(resp.GetStats()) == 0 || resp.Stats[0].Path != "/tmp/test.txt" { - t.Fatalf("Unexpected Stat response: %+v", resp) + if len(resp.GetStats()) != 1 { + t.Fatalf("Expected 1 stat entry, got %d: %+v", len(resp.GetStats()), resp) + } + got := resp.Stats[0] + if got.Path != reqPath { + t.Errorf("Path = %q, want %q", got.Path, reqPath) + } + if got.Size != 5 { + t.Errorf("Size = %d, want 5", got.Size) } }) - t.Run("Stat Fails with Auth Error", func(t *testing.T) { - patch := gomonkey.ApplyFuncReturn(authenticate, nil, status.Error(codes.Unauthenticated, "unauth")) - defer patch.Reset() + t.Run("Stat Directory lists children", func(t *testing.T) { + patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) + defer patch1.Reset() - req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} - _, err := client.Stat(context.Background(), req) - if err == nil || status.Code(err) != codes.Unauthenticated { - t.Fatalf("Expected unauthenticated error, got: %v", err) + if _, err := os.Stat("/mnt/host"); err == nil { + t.Skip("test relies on plain /tmp; /mnt/host present on host") + } + + dir := t.TempDir() + for _, name := range []string{"a.txt", "b.txt", "c.txt"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte("x"), 0644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + + req := &gnoi_file_pb.StatRequest{Path: dir} + resp, err := client.Stat(context.Background(), req) + if err != nil { + t.Fatalf("Expected success, got error: %v", err) + } + if len(resp.GetStats()) != 3 { + t.Fatalf("Expected 3 stat entries, got %d: %+v", len(resp.GetStats()), resp) + } + for _, s := range resp.GetStats() { + if filepath.Dir(s.Path) != dir { + t.Errorf("entry %q not under dir %q", s.Path, dir) + } + if s.Size != 1 { + t.Errorf("entry %q size = %d, want 1", s.Path, s.Size) + } } }) - t.Run("Stat Fails with Dbus Error", func(t *testing.T) { + t.Run("Stat NotFound", func(t *testing.T) { patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - patch2 := gomonkey.ApplyFuncReturn(ssc.NewDbusClient, nil, fmt.Errorf("dbus failure")) defer patch1.Reset() - defer patch2.Reset() - req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} + req := &gnoi_file_pb.StatRequest{Path: "/tmp/definitely-does-not-exist-xyz-12345"} _, err := client.Stat(context.Background(), req) - if err == nil || status.Code(err) != codes.Internal { - t.Fatalf("Expected internal error, got: %v", err) + if err == nil || status.Code(err) != codes.NotFound { + t.Fatalf("Expected NotFound error, got: %v", err) } }) - t.Run("Stat Fails on Invalid last_modified", func(t *testing.T) { - patches := gomonkey.NewPatches() - defer patches.Reset() - - badClient := &ssc.FakeClient{} - patches.ApplyFuncReturn(authenticate, nil, nil) - patches.ApplyFuncReturn(ssc.NewDbusClient, badClient, nil) - patches.ApplyMethod(reflect.TypeOf(badClient), "GetFileStat", func(_ *ssc.FakeClient, path string) (map[string]string, error) { - return map[string]string{ - "path": path, - "last_modified": "not_a_number", - "permissions": "644", - "size": "100", - "umask": "022", - }, nil - }) - - _, err := readFileStat("/path/to/file") - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid syntax") - }) - - t.Run("Stat Fails on Invalid permissions", func(t *testing.T) { - patches := gomonkey.NewPatches() - defer patches.Reset() - - badClient := &ssc.FakeClient{} - patches.ApplyFuncReturn(authenticate, nil, nil) - patches.ApplyFuncReturn(ssc.NewDbusClient, badClient, nil) - patches.ApplyMethod(reflect.TypeOf(badClient), "GetFileStat", func(_ *ssc.FakeClient, path string) (map[string]string, error) { - return map[string]string{ - "path": path, - "last_modified": "1686999999", - "permissions": "xyz", - "size": "100", - "umask": "022", - }, nil - }) + t.Run("Stat InvalidArgument empty path", func(t *testing.T) { + patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) + defer patch1.Reset() - _, err := readFileStat("/path/to/file") - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid syntax") + req := &gnoi_file_pb.StatRequest{Path: ""} + _, err := client.Stat(context.Background(), req) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("Expected InvalidArgument error, got: %v", err) + } }) - t.Run("Stat Fails on Invalid size", func(t *testing.T) { - patches := gomonkey.NewPatches() - defer patches.Reset() - - badClient := &ssc.FakeClient{} - patches.ApplyFuncReturn(authenticate, nil, nil) - patches.ApplyFuncReturn(ssc.NewDbusClient, badClient, nil) - patches.ApplyMethod(reflect.TypeOf(badClient), "GetFileStat", func(_ *ssc.FakeClient, path string) (map[string]string, error) { - return map[string]string{ - "path": path, - "last_modified": "1686999999", - "permissions": "644", - "size": "abc", - "umask": "022", - }, nil - }) + t.Run("Stat InvalidArgument relative path", func(t *testing.T) { + patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) + defer patch1.Reset() - _, err := readFileStat("/path/to/file") - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid syntax") + req := &gnoi_file_pb.StatRequest{Path: "relative/path"} + _, err := client.Stat(context.Background(), req) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("Expected InvalidArgument error, got: %v", err) + } }) - t.Run("Stat Fails on Invalid umask", func(t *testing.T) { - patches := gomonkey.NewPatches() - defer patches.Reset() - - badClient := &ssc.FakeClient{} - patches.ApplyFuncReturn(authenticate, nil, nil) - patches.ApplyFuncReturn(ssc.NewDbusClient, badClient, nil) - patches.ApplyMethod(reflect.TypeOf(badClient), "GetFileStat", func(_ *ssc.FakeClient, path string) (map[string]string, error) { - return map[string]string{ - "path": path, - "last_modified": "1686999999", - "permissions": "644", - "size": "100", - "umask": "oXYZ", - }, nil - }) + t.Run("Stat Fails with Auth Error", func(t *testing.T) { + patch := gomonkey.ApplyFuncReturn(authenticate, nil, status.Error(codes.Unauthenticated, "unauth")) + defer patch.Reset() - _, err := readFileStat("/path/to/file") - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid syntax") + req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} + _, err := client.Stat(context.Background(), req) + if err == nil || status.Code(err) != codes.Unauthenticated { + t.Fatalf("Expected unauthenticated error, got: %v", err) + } }) t.Run("Put Fails with Auth Error", func(t *testing.T) { diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index e50a17d85..8e738929c 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -4267,60 +4267,50 @@ func TestGNOI(t *testing.T) { }) t.Run("FileStatSuccess", func(t *testing.T) { - mockClient := &ssc.DbusClient{} - expectedResult := map[string]string{ - "last_modified": "1609459200000000000", - "permissions": "644", - "size": "1024", - "umask": "o022", + // Stat now uses the host filesystem directly via /mnt/host (or + // directly when not in a container) instead of DBus. Build a real + // temp file and validate the round-trip; skip if /mnt/host exists + // because the temp file location may not be reachable through it. + if _, err := os.Stat("/mnt/host"); err == nil { + t.Skip("Stat exercised against /mnt/host elsewhere; skipping in this environment") + } + + dir := t.TempDir() + path := filepath.Join(dir, "config_db.json") + if err := os.WriteFile(path, make([]byte, 1024), 0644); err != nil { + t.Fatalf("write file: %v", err) } - mock := gomonkey.ApplyMethod(reflect.TypeOf(mockClient), "GetFileStat", func(_ *ssc.DbusClient, path string) (map[string]string, error) { - return expectedResult, nil - }) - defer mock.Reset() - // Prepare context and request ctx := context.Background() - req := &gnoi_file_pb.StatRequest{Path: "/etc/sonic/config_db.json"} + req := &gnoi_file_pb.StatRequest{Path: path} fc := gnoi_file_pb.NewFileClient(conn) resp, err := fc.Stat(ctx, req) if err != nil { t.Fatalf("FileStat failed: %v", err) } - // Validate the response - if len(resp.Stats) == 0 { - t.Fatalf("Expected at least one StatInfo in response") + if len(resp.Stats) != 1 { + t.Fatalf("Expected 1 StatInfo, got %d", len(resp.Stats)) } - statInfo := resp.Stats[0] - - if statInfo.LastModified != 1609459200000000000 { - t.Errorf("Expected last_modified %d but got %d", 1609459200000000000, statInfo.LastModified) + if statInfo.Path != path { + t.Errorf("Expected path %q, got %q", path, statInfo.Path) } - if statInfo.Permissions != 420 { - t.Errorf("Expected permissions 420 but got %d", statInfo.Permissions) + if statInfo.Permissions != 644 { + t.Errorf("Expected permissions 644, got %d", statInfo.Permissions) } if statInfo.Size != 1024 { - t.Errorf("Expected size 1024 but got %d", statInfo.Size) + t.Errorf("Expected size 1024, got %d", statInfo.Size) } - if statInfo.Umask != 18 { - t.Errorf("Expected umask 18 but got %d", statInfo.Umask) + if statInfo.LastModified == 0 { + t.Errorf("Expected non-zero last_modified") } }) t.Run("FileStatFailure", func(t *testing.T) { - mockClient := &ssc.DbusClient{} - expectedError := fmt.Errorf("failed to get file stats") - - mock := gomonkey.ApplyMethod(reflect.TypeOf(mockClient), "GetFileStat", func(_ *ssc.DbusClient, path string) (map[string]string, error) { - return nil, expectedError - }) - defer mock.Reset() - - // Prepare context and request + // Non-existent path now returns NotFound directly from the OS. ctx := context.Background() - req := &gnoi_file_pb.StatRequest{Path: "/etc/sonic/config_db.json"} + req := &gnoi_file_pb.StatRequest{Path: "/tmp/non-existent-file-xyz-12345"} fc := gnoi_file_pb.NewFileClient(conn) resp, err := fc.Stat(ctx, req) @@ -4330,9 +4320,8 @@ func TestGNOI(t *testing.T) { if resp != nil { t.Fatalf("Expected nil response but got: %v", resp) } - - if !strings.Contains(err.Error(), expectedError.Error()) { - t.Errorf("Expected error to contain '%v' but got '%v'", expectedError, err) + if status.Code(err) != codes.NotFound { + t.Errorf("Expected NotFound, got %v", err) } }) diff --git a/pkg/gnoi/file/file.go b/pkg/gnoi/file/file.go index 769d93713..ed573998b 100644 --- a/pkg/gnoi/file/file.go +++ b/pkg/gnoi/file/file.go @@ -12,6 +12,7 @@ import ( "io" "os" "path/filepath" + "strconv" "strings" "time" @@ -569,3 +570,127 @@ func HandleFileRemove(ctx context.Context, req *gnoi_file_pb.RemoveRequest) (*gn log.Infof("Successfully removed file: %s", remoteFile) return &gnoi_file_pb.RemoveResponse{}, nil } + +// defaultUmask is the assumed process file-creation mask reported in StatInfo. +// +// gNOI's StatInfo.umask is "Default file creation mask" — a process-level +// attribute, not a per-file one. We can't query it per-file via os.Stat, and +// reading the running process's umask via syscall.Umask is racy (it both gets +// AND sets in a single call, briefly clobbering the real value for any +// concurrent goroutine that creates files). SONiC services run with the +// distro default of 0022, so report that. +const defaultUmask = 0022 + +// statInfoFromFileInfo builds a StatInfo for `info` at absolute path `reportPath`. +// +// `reportPath` is the path as the client should see it (i.e. with any +// /mnt/host container-translation prefix already stripped) — gNOI clients +// expect the StatInfo.path to round-trip the host-side path they asked for. +// +// Permissions field follows the gNOI proto: octal mode formatted as decimal +// digits. e.g. mode 0755 → 755 (not 493). +func statInfoFromFileInfo(reportPath string, info os.FileInfo) (*gnoi_file_pb.StatInfo, error) { + permsOctalStr := strconv.FormatUint(uint64(info.Mode().Perm()), 8) + perms, err := strconv.ParseUint(permsOctalStr, 10, 32) + if err != nil { + return nil, fmt.Errorf("failed to encode permissions for %s: %v", reportPath, err) + } + + var size uint64 + if !info.IsDir() { + // Reporting fs-block size for a directory is not portable and + // gNOI consumers don't rely on it. Leave size=0 for dirs. + if s := info.Size(); s > 0 { + size = uint64(s) + } + } + + return &gnoi_file_pb.StatInfo{ + Path: reportPath, + LastModified: uint64(info.ModTime().UnixNano()), + Permissions: uint32(perms), + Size: size, + Umask: defaultUmask, + }, nil +} + +// HandleStat implements gNOI File.Stat using direct host-filesystem access +// via the /mnt/host bind mount, replacing the legacy DBus host-service path. +// +// Per the gNOI proto: "Stat will list files at the provided path." So: +// - If the path is a regular file, return one StatInfo for it. +// - If the path is a directory, return one StatInfo for each immediate +// child (non-recursive), matching the documented "list" semantics. +// The directory itself is intentionally not included; clients that want +// metadata about the directory can stat its parent. +// +// The path field in each StatInfo is the host-visible path (the /mnt/host +// container prefix is stripped on output) so clients see paths consistent +// with what they requested. +func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_pb.StatResponse, error) { + if req == nil { + return nil, status.Error(codes.InvalidArgument, "request cannot be nil") + } + + reqPath := req.GetPath() + if reqPath == "" { + return nil, status.Error(codes.InvalidArgument, "path cannot be empty") + } + if !filepath.IsAbs(reqPath) { + return nil, status.Errorf(codes.InvalidArgument, "path must be absolute, got: %s", reqPath) + } + + cleanReqPath := filepath.Clean(reqPath) + translatedPath := translatePathForContainer(cleanReqPath) + + info, err := os.Stat(translatedPath) + if err != nil { + if os.IsNotExist(err) { + return nil, status.Errorf(codes.NotFound, "path not found: %s", reqPath) + } + if os.IsPermission(err) { + return nil, status.Errorf(codes.PermissionDenied, "permission denied: %s", reqPath) + } + return nil, status.Errorf(codes.Internal, "failed to stat %s: %v", reqPath, err) + } + + resp := &gnoi_file_pb.StatResponse{} + + if !info.IsDir() { + statInfo, err := statInfoFromFileInfo(cleanReqPath, info) + if err != nil { + return nil, status.Errorf(codes.Internal, "%v", err) + } + resp.Stats = []*gnoi_file_pb.StatInfo{statInfo} + return resp, nil + } + + // Directory: list immediate children (non-recursive). + entries, err := os.ReadDir(translatedPath) + if err != nil { + if os.IsPermission(err) { + return nil, status.Errorf(codes.PermissionDenied, "permission denied reading directory: %s", reqPath) + } + return nil, status.Errorf(codes.Internal, "failed to read directory %s: %v", reqPath, err) + } + + stats := make([]*gnoi_file_pb.StatInfo, 0, len(entries)) + for _, entry := range entries { + entryInfo, err := entry.Info() + if err != nil { + // Entry vanished between ReadDir and Info (race): skip it + // rather than failing the whole listing. + log.Warningf("HandleStat: skipping %s/%s: %v", reqPath, entry.Name(), err) + continue + } + childReportPath := filepath.Join(cleanReqPath, entry.Name()) + statInfo, err := statInfoFromFileInfo(childReportPath, entryInfo) + if err != nil { + log.Warningf("HandleStat: skipping %s: %v", childReportPath, err) + continue + } + stats = append(stats, statInfo) + } + resp.Stats = stats + return resp, nil +} diff --git a/pkg/gnoi/file/stat_test.go b/pkg/gnoi/file/stat_test.go new file mode 100644 index 000000000..d0b23bb35 --- /dev/null +++ b/pkg/gnoi/file/stat_test.go @@ -0,0 +1,188 @@ +package file + +import ( + "context" + "os" + "path/filepath" + "testing" + + gnoi_file_pb "github.com/openconfig/gnoi/file" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// skipIfMntHost skips the test when /mnt/host exists on the host running the +// tests. translatePathForContainer prepends /mnt/host in that case, which +// breaks tests that point HandleStat at a t.TempDir() under /tmp because the +// real path lives at /tmp/..., not /mnt/host/tmp/.... +func skipIfMntHost(t *testing.T) { + t.Helper() + if _, err := os.Stat("/mnt/host"); err == nil { + t.Skip("/mnt/host exists; HandleStat path translation makes tempdirs unreachable") + } +} + +func TestHandleStat_NilRequest(t *testing.T) { + _, err := HandleStat(context.Background(), nil) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleStat_EmptyPath(t *testing.T) { + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: ""}) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleStat_RelativePath(t *testing.T) { + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "relative/foo"}) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleStat_NotFound(t *testing.T) { + skipIfMntHost(t) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{ + Path: "/tmp/definitely-does-not-exist-zzzz-9876543", + }) + if err == nil || status.Code(err) != codes.NotFound { + t.Fatalf("expected NotFound, got %v", err) + } +} + +func TestHandleStat_RegularFile(t *testing.T) { + skipIfMntHost(t) + dir := t.TempDir() + path := filepath.Join(dir, "f.bin") + if err := os.WriteFile(path, []byte("hello world"), 0640); err != nil { + t.Fatal(err) + } + + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: path}) + if err != nil { + t.Fatalf("HandleStat: %v", err) + } + if len(resp.Stats) != 1 { + t.Fatalf("got %d entries, want 1", len(resp.Stats)) + } + got := resp.Stats[0] + if got.Path != path { + t.Errorf("Path = %q, want %q", got.Path, path) + } + if got.Size != 11 { + t.Errorf("Size = %d, want 11", got.Size) + } + // Permissions: 0640 → octal-as-decimal = 640. + if got.Permissions != 640 { + t.Errorf("Permissions = %d, want 640", got.Permissions) + } + if got.LastModified == 0 { + t.Error("LastModified must be non-zero") + } + if got.Umask != defaultUmask { + t.Errorf("Umask = %d, want %d", got.Umask, defaultUmask) + } +} + +func TestHandleStat_Directory(t *testing.T) { + skipIfMntHost(t) + dir := t.TempDir() + names := []string{"alpha.txt", "beta.txt", "gamma.txt"} + for _, n := range names { + if err := os.WriteFile(filepath.Join(dir, n), []byte("x"), 0644); err != nil { + t.Fatal(err) + } + } + // Add a subdirectory to make sure directories are reported too. + subdir := filepath.Join(dir, "subdir") + if err := os.Mkdir(subdir, 0755); err != nil { + t.Fatal(err) + } + + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: dir}) + if err != nil { + t.Fatalf("HandleStat: %v", err) + } + if len(resp.Stats) != len(names)+1 { + t.Fatalf("got %d entries, want %d", len(resp.Stats), len(names)+1) + } + + seen := map[string]*gnoi_file_pb.StatInfo{} + for _, s := range resp.Stats { + if filepath.Dir(s.Path) != dir { + t.Errorf("entry %q not under dir %q", s.Path, dir) + } + seen[filepath.Base(s.Path)] = s + } + for _, n := range names { + s, ok := seen[n] + if !ok { + t.Errorf("missing entry %q", n) + continue + } + if s.Size != 1 { + t.Errorf("file %q size = %d, want 1", n, s.Size) + } + } + if s, ok := seen["subdir"]; !ok { + t.Error("missing subdir entry") + } else if s.Size != 0 { + // Per statInfoFromFileInfo, dirs report Size=0. + t.Errorf("subdir size = %d, want 0", s.Size) + } +} + +func TestHandleStat_EmptyDirectory(t *testing.T) { + skipIfMntHost(t) + dir := t.TempDir() + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: dir}) + if err != nil { + t.Fatalf("HandleStat: %v", err) + } + if len(resp.Stats) != 0 { + t.Fatalf("got %d entries, want 0", len(resp.Stats)) + } +} + +func TestStatInfoFromFileInfo_PermissionsOctalAsDecimal(t *testing.T) { + skipIfMntHost(t) + dir := t.TempDir() + path := filepath.Join(dir, "f") + if err := os.WriteFile(path, []byte{}, 0); err != nil { + t.Fatal(err) + } + for _, mode := range []os.FileMode{0644, 0755, 0600, 0777, 0444} { + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + st, err := statInfoFromFileInfo(path, info) + if err != nil { + t.Fatal(err) + } + // Build expected by formatting mode in octal, then reading as + // decimal — same as the production code. + want := uint32(0) + switch mode { + case 0644: + want = 644 + case 0755: + want = 755 + case 0600: + want = 600 + case 0777: + want = 777 + case 0444: + want = 444 + } + if st.Permissions != want { + t.Errorf("mode %o → Permissions=%d, want %d", mode, st.Permissions, want) + } + } +} From 8b66d3aa32a228bd07063589726a7197c7e20ed1 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 14:50:00 -0500 Subject: [PATCH 2/6] [gnoi] file: trim gnmi_server Stat tests to wiring-only Behavior coverage for File.Stat lives in pkg/gnoi/file/stat_test.go (NotFound, regular file, directory listing, empty directory, the octal-as-decimal permissions encoding, nil/empty/relative request validation). Re-asserting all of that through the gnmi_server gRPC client just duplicated the pure-package coverage and forced fragile /mnt/host skip guards into the gnmi_server suite. Reduce gnmi_server's TestGnoiFile Stat coverage to two minimal sub-tests that only verify server wiring: - the authenticate hook runs before the handler (Unauthenticated surfaces as gRPC Unauthenticated) - an authenticated request reaches HandleStat and a handler-level error (empty path -> InvalidArgument) propagates out as the matching gRPC status code Drop the FileStatSuccess and FileStatFailure sub-tests from gnmi_server/server_test.go for the same reason; nothing they asserted was specific to the gnmi_server stack. Signed-off-by: Dawei Huang --- gnmi_server/gnoi_file_test.go | 127 +++++----------------------------- gnmi_server/server_test.go | 61 +--------------- 2 files changed, 20 insertions(+), 168 deletions(-) diff --git a/gnmi_server/gnoi_file_test.go b/gnmi_server/gnoi_file_test.go index c2f548cfc..d6d77bed9 100644 --- a/gnmi_server/gnoi_file_test.go +++ b/gnmi_server/gnoi_file_test.go @@ -71,125 +71,32 @@ func TestGnoiFileServer(t *testing.T) { client := gnoi_file_pb.NewFileClient(conn) - t.Run("Stat Success", func(t *testing.T) { - patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - defer patch1.Reset() - - // Create a real temp file so HandleStat can stat it. - // HandleStat uses translatePathForContainer, which prepends /mnt/host - // only if /mnt/host exists on the test machine. Build the request path - // so it points at the temp file regardless of which branch is taken. - tmpDir := t.TempDir() - f, err := os.CreateTemp(tmpDir, "stat-success-*.txt") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - realPath := f.Name() - if _, err := f.WriteString("hello"); err != nil { - t.Fatalf("write: %v", err) - } - f.Close() - - // If /mnt/host exists, translatePathForContainer will prepend it, - // so feed it the un-prefixed path. Otherwise pass realPath as-is. - reqPath := realPath - if _, err := os.Stat("/mnt/host"); err == nil { - // Build reverse: strip /mnt/host if temp file already lives under it, - // or skip the test when the temp dir is not reachable from /mnt/host. - t.Skip("test relies on plain /tmp; /mnt/host present on host") - } - - req := &gnoi_file_pb.StatRequest{Path: reqPath} - resp, err := client.Stat(context.Background(), req) - if err != nil { - t.Fatalf("Expected success, got error: %v", err) - } - if len(resp.GetStats()) != 1 { - t.Fatalf("Expected 1 stat entry, got %d: %+v", len(resp.GetStats()), resp) - } - got := resp.Stats[0] - if got.Path != reqPath { - t.Errorf("Path = %q, want %q", got.Path, reqPath) - } - if got.Size != 5 { - t.Errorf("Size = %d, want 5", got.Size) - } - }) - - t.Run("Stat Directory lists children", func(t *testing.T) { - patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - defer patch1.Reset() - - if _, err := os.Stat("/mnt/host"); err == nil { - t.Skip("test relies on plain /tmp; /mnt/host present on host") - } - - dir := t.TempDir() - for _, name := range []string{"a.txt", "b.txt", "c.txt"} { - if err := os.WriteFile(filepath.Join(dir, name), []byte("x"), 0644); err != nil { - t.Fatalf("write %s: %v", name, err) - } - } - - req := &gnoi_file_pb.StatRequest{Path: dir} - resp, err := client.Stat(context.Background(), req) - if err != nil { - t.Fatalf("Expected success, got error: %v", err) - } - if len(resp.GetStats()) != 3 { - t.Fatalf("Expected 3 stat entries, got %d: %+v", len(resp.GetStats()), resp) - } - for _, s := range resp.GetStats() { - if filepath.Dir(s.Path) != dir { - t.Errorf("entry %q not under dir %q", s.Path, dir) - } - if s.Size != 1 { - t.Errorf("entry %q size = %d, want 1", s.Path, s.Size) - } - } - }) - - t.Run("Stat NotFound", func(t *testing.T) { - patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - defer patch1.Reset() + // Behavior coverage for HandleStat lives in pkg/gnoi/file/stat_test.go. + // The gnmi_server tests below only verify the server wiring: that the + // authenticate hook fires before the handler, and that handler errors + // surface as gRPC status codes through the server stack. + t.Run("Stat Fails with Auth Error", func(t *testing.T) { + patch := gomonkey.ApplyFuncReturn(authenticate, nil, status.Error(codes.Unauthenticated, "unauth")) + defer patch.Reset() - req := &gnoi_file_pb.StatRequest{Path: "/tmp/definitely-does-not-exist-xyz-12345"} + req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} _, err := client.Stat(context.Background(), req) - if err == nil || status.Code(err) != codes.NotFound { - t.Fatalf("Expected NotFound error, got: %v", err) + if err == nil || status.Code(err) != codes.Unauthenticated { + t.Fatalf("Expected unauthenticated error, got: %v", err) } }) - t.Run("Stat InvalidArgument empty path", func(t *testing.T) { - patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - defer patch1.Reset() + t.Run("Stat Delegates to Handler", func(t *testing.T) { + // Smoke test: an authenticated request reaches HandleStat and a + // handler-level error (empty path -> InvalidArgument) propagates + // through the server stack as the matching gRPC status code. + patch := gomonkey.ApplyFuncReturn(authenticate, nil, nil) + defer patch.Reset() req := &gnoi_file_pb.StatRequest{Path: ""} _, err := client.Stat(context.Background(), req) if err == nil || status.Code(err) != codes.InvalidArgument { - t.Fatalf("Expected InvalidArgument error, got: %v", err) - } - }) - - t.Run("Stat InvalidArgument relative path", func(t *testing.T) { - patch1 := gomonkey.ApplyFuncReturn(authenticate, nil, nil) - defer patch1.Reset() - - req := &gnoi_file_pb.StatRequest{Path: "relative/path"} - _, err := client.Stat(context.Background(), req) - if err == nil || status.Code(err) != codes.InvalidArgument { - t.Fatalf("Expected InvalidArgument error, got: %v", err) - } - }) - - t.Run("Stat Fails with Auth Error", func(t *testing.T) { - patch := gomonkey.ApplyFuncReturn(authenticate, nil, status.Error(codes.Unauthenticated, "unauth")) - defer patch.Reset() - - req := &gnoi_file_pb.StatRequest{Path: "/tmp/test.txt"} - _, err := client.Stat(context.Background(), req) - if err == nil || status.Code(err) != codes.Unauthenticated { - t.Fatalf("Expected unauthenticated error, got: %v", err) + t.Fatalf("Expected InvalidArgument from handler, got: %v", err) } }) diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index 8e738929c..d115f7b2e 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -4266,64 +4266,9 @@ func TestGNOI(t *testing.T) { } }) - t.Run("FileStatSuccess", func(t *testing.T) { - // Stat now uses the host filesystem directly via /mnt/host (or - // directly when not in a container) instead of DBus. Build a real - // temp file and validate the round-trip; skip if /mnt/host exists - // because the temp file location may not be reachable through it. - if _, err := os.Stat("/mnt/host"); err == nil { - t.Skip("Stat exercised against /mnt/host elsewhere; skipping in this environment") - } - - dir := t.TempDir() - path := filepath.Join(dir, "config_db.json") - if err := os.WriteFile(path, make([]byte, 1024), 0644); err != nil { - t.Fatalf("write file: %v", err) - } - - ctx := context.Background() - req := &gnoi_file_pb.StatRequest{Path: path} - fc := gnoi_file_pb.NewFileClient(conn) - - resp, err := fc.Stat(ctx, req) - if err != nil { - t.Fatalf("FileStat failed: %v", err) - } - if len(resp.Stats) != 1 { - t.Fatalf("Expected 1 StatInfo, got %d", len(resp.Stats)) - } - statInfo := resp.Stats[0] - if statInfo.Path != path { - t.Errorf("Expected path %q, got %q", path, statInfo.Path) - } - if statInfo.Permissions != 644 { - t.Errorf("Expected permissions 644, got %d", statInfo.Permissions) - } - if statInfo.Size != 1024 { - t.Errorf("Expected size 1024, got %d", statInfo.Size) - } - if statInfo.LastModified == 0 { - t.Errorf("Expected non-zero last_modified") - } - }) - - t.Run("FileStatFailure", func(t *testing.T) { - // Non-existent path now returns NotFound directly from the OS. - ctx := context.Background() - req := &gnoi_file_pb.StatRequest{Path: "/tmp/non-existent-file-xyz-12345"} - fc := gnoi_file_pb.NewFileClient(conn) - - resp, err := fc.Stat(ctx, req) - if err == nil { - t.Fatalf("Expected error but got none") - } - if resp != nil { - t.Fatalf("Expected nil response but got: %v", resp) - } - if status.Code(err) != codes.NotFound { - t.Errorf("Expected NotFound, got %v", err) - } - }) + // Stat behavior is covered by pkg/gnoi/file/stat_test.go; the + // gnmi_server-level wiring (auth + delegation) is covered by + // TestGnoiFile in gnoi_file_test.go. No FileStat sub-test here. t.Run("OSVerifySuccess", func(t *testing.T) { mockClient := &ssc.DbusClient{} From e9cd4ecb48ac1600541034aa88d879b06c11219c Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 14:59:21 -0500 Subject: [PATCH 3/6] [gnoi] file: address copilot review on Stat handler Three fixes to the new HandleStat from PR review: 1. Map os.ReadDir NotExist to NotFound. The directory may be removed between the os.Stat above the dir branch and the ReadDir inside it; that race used to surface as Internal, hiding a benign condition behind a server-error code. 2. Reject /mnt/host-prefixed input paths with InvalidArgument. translatePathForContainer unconditionally prepends /mnt/host when that directory exists, so a client that already sent a container-internal path would otherwise hit /mnt/host/mnt/host/... and get a confusing NotFound. Tell the client to drop the prefix instead. 3. Replace the test-level skipIfMntHost helper with a statTestRoot helper that creates the fixture under the *physical* root (/mnt/host/tmp/... when /mnt/host is present) while feeding the matching *logical* path to HandleStat. This restores Stat test coverage on containerized hosts where /mnt/host exists. Also adds TestHandleStat_RejectsMntHostPrefix to lock in the new InvalidArgument path. Signed-off-by: Dawei Huang --- pkg/gnoi/file/file.go | 13 +++++ pkg/gnoi/file/stat_test.go | 98 ++++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 35 deletions(-) diff --git a/pkg/gnoi/file/file.go b/pkg/gnoi/file/file.go index ed573998b..9d9e8b94f 100644 --- a/pkg/gnoi/file/file.go +++ b/pkg/gnoi/file/file.go @@ -641,6 +641,13 @@ func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_ } cleanReqPath := filepath.Clean(reqPath) + // Reject /mnt/host-prefixed inputs to avoid double-prefixing in + // translatePathForContainer (e.g. "/mnt/host/tmp/x" → "/mnt/host/mnt/host/tmp/x"). + // Clients should pass host-visible paths like /tmp/..., /etc/..., /host/... + if cleanReqPath == "/mnt/host" || strings.HasPrefix(cleanReqPath, "/mnt/host/") { + return nil, status.Errorf(codes.InvalidArgument, + "path must be host-visible, not container-internal: %s (drop the /mnt/host prefix)", reqPath) + } translatedPath := translatePathForContainer(cleanReqPath) info, err := os.Stat(translatedPath) @@ -668,6 +675,12 @@ func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_ // Directory: list immediate children (non-recursive). entries, err := os.ReadDir(translatedPath) if err != nil { + // The directory may have been removed between the os.Stat above + // and this ReadDir; surface that as NotFound, not Internal, so + // transient races don't look like server errors. + if os.IsNotExist(err) { + return nil, status.Errorf(codes.NotFound, "path not found: %s", reqPath) + } if os.IsPermission(err) { return nil, status.Errorf(codes.PermissionDenied, "permission denied reading directory: %s", reqPath) } diff --git a/pkg/gnoi/file/stat_test.go b/pkg/gnoi/file/stat_test.go index d0b23bb35..87121a9cb 100644 --- a/pkg/gnoi/file/stat_test.go +++ b/pkg/gnoi/file/stat_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" gnoi_file_pb "github.com/openconfig/gnoi/file" @@ -11,15 +12,42 @@ import ( "google.golang.org/grpc/status" ) -// skipIfMntHost skips the test when /mnt/host exists on the host running the -// tests. translatePathForContainer prepends /mnt/host in that case, which -// breaks tests that point HandleStat at a t.TempDir() under /tmp because the -// real path lives at /tmp/..., not /mnt/host/tmp/.... -func skipIfMntHost(t *testing.T) { +// statTestRoot returns a (logicalRoot, physicalRoot) pair that the test +// can use to build fixtures. +// +// When /mnt/host exists (containerized / SONiC-style host), HandleStat +// translates an incoming logical path P into /mnt/host+P before touching +// the filesystem. So a test that wants HandleStat to actually find its +// fixture must: +// - put files at /mnt/host+P (the *physical* root), and +// - send P (the *logical* root) in the StatRequest. +// +// When /mnt/host is absent the two roots coincide. +// +// The helper builds a unique subdirectory under /tmp via os.MkdirTemp on +// the physical root, registers cleanup, and returns: +// - logical: what the test should put in StatRequest.Path +// - physical: where the test should actually create files/dirs +func statTestRoot(t *testing.T) (logical, physical string) { t.Helper() + physBase := "/tmp" if _, err := os.Stat("/mnt/host"); err == nil { - t.Skip("/mnt/host exists; HandleStat path translation makes tempdirs unreachable") + physBase = "/mnt/host/tmp" + if err := os.MkdirAll(physBase, 0755); err != nil { + t.Fatalf("ensure /mnt/host/tmp: %v", err) + } + } + phys, err := os.MkdirTemp(physBase, "stat-test-*") + if err != nil { + t.Fatalf("mkdir temp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(phys) }) + + logi := phys + if strings.HasPrefix(phys, "/mnt/host") { + logi = strings.TrimPrefix(phys, "/mnt/host") } + return logi, phys } func TestHandleStat_NilRequest(t *testing.T) { @@ -43,10 +71,19 @@ func TestHandleStat_RelativePath(t *testing.T) { } } +func TestHandleStat_RejectsMntHostPrefix(t *testing.T) { + for _, p := range []string{"/mnt/host", "/mnt/host/tmp/foo"} { + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: p}) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Errorf("path %q: expected InvalidArgument, got %v", p, err) + } + } +} + func TestHandleStat_NotFound(t *testing.T) { - skipIfMntHost(t) + logi, _ := statTestRoot(t) _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{ - Path: "/tmp/definitely-does-not-exist-zzzz-9876543", + Path: filepath.Join(logi, "definitely-does-not-exist-zzzz-9876543"), }) if err == nil || status.Code(err) != codes.NotFound { t.Fatalf("expected NotFound, got %v", err) @@ -54,14 +91,14 @@ func TestHandleStat_NotFound(t *testing.T) { } func TestHandleStat_RegularFile(t *testing.T) { - skipIfMntHost(t) - dir := t.TempDir() - path := filepath.Join(dir, "f.bin") - if err := os.WriteFile(path, []byte("hello world"), 0640); err != nil { + logi, phys := statTestRoot(t) + physPath := filepath.Join(phys, "f.bin") + if err := os.WriteFile(physPath, []byte("hello world"), 0640); err != nil { t.Fatal(err) } + logiPath := filepath.Join(logi, "f.bin") - resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: path}) + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: logiPath}) if err != nil { t.Fatalf("HandleStat: %v", err) } @@ -69,13 +106,12 @@ func TestHandleStat_RegularFile(t *testing.T) { t.Fatalf("got %d entries, want 1", len(resp.Stats)) } got := resp.Stats[0] - if got.Path != path { - t.Errorf("Path = %q, want %q", got.Path, path) + if got.Path != logiPath { + t.Errorf("Path = %q, want %q", got.Path, logiPath) } if got.Size != 11 { t.Errorf("Size = %d, want 11", got.Size) } - // Permissions: 0640 → octal-as-decimal = 640. if got.Permissions != 640 { t.Errorf("Permissions = %d, want 640", got.Permissions) } @@ -88,21 +124,18 @@ func TestHandleStat_RegularFile(t *testing.T) { } func TestHandleStat_Directory(t *testing.T) { - skipIfMntHost(t) - dir := t.TempDir() + logi, phys := statTestRoot(t) names := []string{"alpha.txt", "beta.txt", "gamma.txt"} for _, n := range names { - if err := os.WriteFile(filepath.Join(dir, n), []byte("x"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(phys, n), []byte("x"), 0644); err != nil { t.Fatal(err) } } - // Add a subdirectory to make sure directories are reported too. - subdir := filepath.Join(dir, "subdir") - if err := os.Mkdir(subdir, 0755); err != nil { + if err := os.Mkdir(filepath.Join(phys, "subdir"), 0755); err != nil { t.Fatal(err) } - resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: dir}) + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: logi}) if err != nil { t.Fatalf("HandleStat: %v", err) } @@ -112,8 +145,8 @@ func TestHandleStat_Directory(t *testing.T) { seen := map[string]*gnoi_file_pb.StatInfo{} for _, s := range resp.Stats { - if filepath.Dir(s.Path) != dir { - t.Errorf("entry %q not under dir %q", s.Path, dir) + if filepath.Dir(s.Path) != logi { + t.Errorf("entry %q not under dir %q", s.Path, logi) } seen[filepath.Base(s.Path)] = s } @@ -130,15 +163,13 @@ func TestHandleStat_Directory(t *testing.T) { if s, ok := seen["subdir"]; !ok { t.Error("missing subdir entry") } else if s.Size != 0 { - // Per statInfoFromFileInfo, dirs report Size=0. t.Errorf("subdir size = %d, want 0", s.Size) } } func TestHandleStat_EmptyDirectory(t *testing.T) { - skipIfMntHost(t) - dir := t.TempDir() - resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: dir}) + logi, _ := statTestRoot(t) + resp, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: logi}) if err != nil { t.Fatalf("HandleStat: %v", err) } @@ -148,9 +179,8 @@ func TestHandleStat_EmptyDirectory(t *testing.T) { } func TestStatInfoFromFileInfo_PermissionsOctalAsDecimal(t *testing.T) { - skipIfMntHost(t) - dir := t.TempDir() - path := filepath.Join(dir, "f") + _, phys := statTestRoot(t) + path := filepath.Join(phys, "f") if err := os.WriteFile(path, []byte{}, 0); err != nil { t.Fatal(err) } @@ -166,9 +196,7 @@ func TestStatInfoFromFileInfo_PermissionsOctalAsDecimal(t *testing.T) { if err != nil { t.Fatal(err) } - // Build expected by formatting mode in octal, then reading as - // decimal — same as the production code. - want := uint32(0) + var want uint32 switch mode { case 0644: want = 644 From 50917d55728327eafa2d3cf2cab57efc309b6bf9 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 16:37:04 -0500 Subject: [PATCH 4/6] [gnoi] file: drop unused test imports Integration build (Test stage on the ADO pipeline) failed with: gnmi_server/gnoi_file_test.go:8:2: "path/filepath" imported and not used gnmi_server/server_test.go:68:2: "github.com/openconfig/gnoi/file" imported as gnoi_file_pb and not used Pure tests didn't catch this because they don't compile gnmi_server. Remove the now-orphaned imports left over after the test trim in 8b66d3a. Signed-off-by: Dawei Huang --- gnmi_server/gnoi_file_test.go | 1 - gnmi_server/server_test.go | 1 - 2 files changed, 2 deletions(-) diff --git a/gnmi_server/gnoi_file_test.go b/gnmi_server/gnoi_file_test.go index d6d77bed9..cc214b4fe 100644 --- a/gnmi_server/gnoi_file_test.go +++ b/gnmi_server/gnoi_file_test.go @@ -5,7 +5,6 @@ import ( "fmt" "net" "os" - "path/filepath" "testing" "github.com/agiledragon/gomonkey/v2" diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index d115f7b2e..c7a35b98c 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -65,7 +65,6 @@ import ( cacheclient "github.com/openconfig/gnmi/client" gclient "github.com/openconfig/gnmi/client/gnmi" gnmipb "github.com/openconfig/gnmi/proto/gnmi" - gnoi_file_pb "github.com/openconfig/gnoi/file" gnoi_os_pb "github.com/openconfig/gnoi/os" gnoi_system_pb "github.com/openconfig/gnoi/system" ) From d826a6cd31e1211c742c9369b8d608787f9889a8 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 17:31:30 -0500 Subject: [PATCH 5/6] [gnoi] file: cover HandleStat error branches via fs seam Diff coverage on the PR was 75% (43/57), below the 80% threshold. The uncovered lines were all error paths in HandleStat that the test container can't reach with real os.* calls: as root on tmpfs we don't hit PermissionDenied (DAC bypassed), and we can't race a TOCTOU between os.Stat and os.ReadDir. Avoid gomonkey by introducing a tiny package-internal seam: var ( fsStat = os.Stat fsReadDir = os.ReadDir ) HandleStat calls those instead of os.Stat / os.ReadDir directly. Tests swap them in for the duration of a single test (see withFsStat / withFsReadDir helpers in stat_test.go) to inject specific errors: - TestHandleStat_StatPermissionDenied (line 660-661) - TestHandleStat_StatGenericError (line 663) - TestHandleStat_ReadDirNotExistRace (line 683-684) - TestHandleStat_ReadDirPermissionDenied(line 686-687) - TestHandleStat_ReadDirGenericError (line 689) Production behavior is unchanged: fsStat and fsReadDir keep their os.* defaults, the original os.Is{NotExist,Permission} mapping logic is the same, and no new package is introduced. HandleStat statement coverage: ~78% -> 89.1%; diff coverage clears the 80% gate. Signed-off-by: Dawei Huang --- pkg/gnoi/file/file.go | 14 +++++- pkg/gnoi/file/stat_test.go | 89 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/pkg/gnoi/file/file.go b/pkg/gnoi/file/file.go index 9d9e8b94f..fc0407561 100644 --- a/pkg/gnoi/file/file.go +++ b/pkg/gnoi/file/file.go @@ -40,6 +40,16 @@ const ( // (the generated function is tiny and gets inlined, defeating gomonkey). var newFileClient = gnoi_file_pb.NewFileClient +// fsStat / fsReadDir are package-level seams over os.Stat and os.ReadDir. +// Tests override them to exercise error branches that the root-on-tmpfs +// test environment can't otherwise reach: permission-denied (root bypasses +// DAC) and TOCTOU races (path removed between Stat and ReadDir). Production +// callers go through os.* unchanged. +var ( + fsStat = os.Stat + fsReadDir = os.ReadDir +) + // HandleTransferToRemote implements the complete logic for the TransferToRemote RPC. // It validates the request, checks for DPU metadata, and routes accordingly. // @@ -650,7 +660,7 @@ func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_ } translatedPath := translatePathForContainer(cleanReqPath) - info, err := os.Stat(translatedPath) + info, err := fsStat(translatedPath) if err != nil { if os.IsNotExist(err) { return nil, status.Errorf(codes.NotFound, "path not found: %s", reqPath) @@ -673,7 +683,7 @@ func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_ } // Directory: list immediate children (non-recursive). - entries, err := os.ReadDir(translatedPath) + entries, err := fsReadDir(translatedPath) if err != nil { // The directory may have been removed between the os.Stat above // and this ReadDir; surface that as NotFound, not Internal, so diff --git a/pkg/gnoi/file/stat_test.go b/pkg/gnoi/file/stat_test.go index 87121a9cb..9233a51f2 100644 --- a/pkg/gnoi/file/stat_test.go +++ b/pkg/gnoi/file/stat_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" gnoi_file_pb "github.com/openconfig/gnoi/file" "google.golang.org/grpc/codes" @@ -214,3 +215,91 @@ func TestStatInfoFromFileInfo_PermissionsOctalAsDecimal(t *testing.T) { } } } + +// withFsStat installs a fake fsStat for the duration of the test. Tests +// use this to cover error branches (permission denied, generic Internal) +// that the root-on-tmpfs environment can't trigger via real os.Stat. +func withFsStat(t *testing.T, fake func(string) (os.FileInfo, error)) { +t.Helper() +prev := fsStat +fsStat = fake +t.Cleanup(func() { fsStat = prev }) +} + +// withFsReadDir is the os.ReadDir equivalent of withFsStat. +func withFsReadDir(t *testing.T, fake func(string) ([]os.DirEntry, error)) { +t.Helper() +prev := fsReadDir +fsReadDir = fake +t.Cleanup(func() { fsReadDir = prev }) +} + +func TestHandleStat_StatPermissionDenied(t *testing.T) { +withFsStat(t, func(path string) (os.FileInfo, error) { +return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrPermission} +}) +_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) +if err == nil || status.Code(err) != codes.PermissionDenied { +t.Fatalf("expected PermissionDenied, got %v", err) +} +} + +func TestHandleStat_StatGenericError(t *testing.T) { +// e.g. ELOOP from a symlink loop is neither IsNotExist nor IsPermission; +// HandleStat must surface it as Internal. +withFsStat(t, func(path string) (os.FileInfo, error) { +return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrInvalid} +}) +_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) +if err == nil || status.Code(err) != codes.Internal { +t.Fatalf("expected Internal, got %v", err) +} +} + +// fakeDirInfo is a minimal os.FileInfo for a directory; the only fields +// HandleStat looks at on the parent are IsDir() and Mode(), so the rest +// can be zero-valued. +type fakeDirInfo struct{} + +func (fakeDirInfo) Name() string { return "x" } +func (fakeDirInfo) Size() int64 { return 0 } +func (fakeDirInfo) Mode() os.FileMode { return os.ModeDir | 0755 } +func (fakeDirInfo) ModTime() time.Time { return time.Time{} } +func (fakeDirInfo) IsDir() bool { return true } +func (fakeDirInfo) Sys() interface{} { return nil } + +func TestHandleStat_ReadDirNotExistRace(t *testing.T) { +// Stat says directory exists; ReadDir then races with a removal. +// HandleStat must downgrade Internal to NotFound to keep transient +// races from looking like server errors. +withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) +withFsReadDir(t, func(path string) ([]os.DirEntry, error) { +return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrNotExist} +}) +_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) +if err == nil || status.Code(err) != codes.NotFound { +t.Fatalf("expected NotFound on race, got %v", err) +} +} + +func TestHandleStat_ReadDirPermissionDenied(t *testing.T) { +withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) +withFsReadDir(t, func(path string) ([]os.DirEntry, error) { +return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrPermission} +}) +_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) +if err == nil || status.Code(err) != codes.PermissionDenied { +t.Fatalf("expected PermissionDenied, got %v", err) +} +} + +func TestHandleStat_ReadDirGenericError(t *testing.T) { +withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) +withFsReadDir(t, func(path string) ([]os.DirEntry, error) { +return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrInvalid} +}) +_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) +if err == nil || status.Code(err) != codes.Internal { +t.Fatalf("expected Internal, got %v", err) +} +} From 0b20ae6f7c24e196e5520786491cffcea16f556b Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 18:28:27 -0500 Subject: [PATCH 6/6] [gnoi] file: gofmt stat_test.go Signed-off-by: Dawei Huang --- pkg/gnoi/file/stat_test.go | 102 ++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/pkg/gnoi/file/stat_test.go b/pkg/gnoi/file/stat_test.go index 9233a51f2..a36acf586 100644 --- a/pkg/gnoi/file/stat_test.go +++ b/pkg/gnoi/file/stat_test.go @@ -220,40 +220,40 @@ func TestStatInfoFromFileInfo_PermissionsOctalAsDecimal(t *testing.T) { // use this to cover error branches (permission denied, generic Internal) // that the root-on-tmpfs environment can't trigger via real os.Stat. func withFsStat(t *testing.T, fake func(string) (os.FileInfo, error)) { -t.Helper() -prev := fsStat -fsStat = fake -t.Cleanup(func() { fsStat = prev }) + t.Helper() + prev := fsStat + fsStat = fake + t.Cleanup(func() { fsStat = prev }) } // withFsReadDir is the os.ReadDir equivalent of withFsStat. func withFsReadDir(t *testing.T, fake func(string) ([]os.DirEntry, error)) { -t.Helper() -prev := fsReadDir -fsReadDir = fake -t.Cleanup(func() { fsReadDir = prev }) + t.Helper() + prev := fsReadDir + fsReadDir = fake + t.Cleanup(func() { fsReadDir = prev }) } func TestHandleStat_StatPermissionDenied(t *testing.T) { -withFsStat(t, func(path string) (os.FileInfo, error) { -return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrPermission} -}) -_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) -if err == nil || status.Code(err) != codes.PermissionDenied { -t.Fatalf("expected PermissionDenied, got %v", err) -} + withFsStat(t, func(path string) (os.FileInfo, error) { + return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrPermission} + }) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) + if err == nil || status.Code(err) != codes.PermissionDenied { + t.Fatalf("expected PermissionDenied, got %v", err) + } } func TestHandleStat_StatGenericError(t *testing.T) { -// e.g. ELOOP from a symlink loop is neither IsNotExist nor IsPermission; -// HandleStat must surface it as Internal. -withFsStat(t, func(path string) (os.FileInfo, error) { -return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrInvalid} -}) -_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) -if err == nil || status.Code(err) != codes.Internal { -t.Fatalf("expected Internal, got %v", err) -} + // e.g. ELOOP from a symlink loop is neither IsNotExist nor IsPermission; + // HandleStat must surface it as Internal. + withFsStat(t, func(path string) (os.FileInfo, error) { + return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrInvalid} + }) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) + if err == nil || status.Code(err) != codes.Internal { + t.Fatalf("expected Internal, got %v", err) + } } // fakeDirInfo is a minimal os.FileInfo for a directory; the only fields @@ -269,37 +269,37 @@ func (fakeDirInfo) IsDir() bool { return true } func (fakeDirInfo) Sys() interface{} { return nil } func TestHandleStat_ReadDirNotExistRace(t *testing.T) { -// Stat says directory exists; ReadDir then races with a removal. -// HandleStat must downgrade Internal to NotFound to keep transient -// races from looking like server errors. -withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) -withFsReadDir(t, func(path string) ([]os.DirEntry, error) { -return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrNotExist} -}) -_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) -if err == nil || status.Code(err) != codes.NotFound { -t.Fatalf("expected NotFound on race, got %v", err) -} + // Stat says directory exists; ReadDir then races with a removal. + // HandleStat must downgrade Internal to NotFound to keep transient + // races from looking like server errors. + withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) + withFsReadDir(t, func(path string) ([]os.DirEntry, error) { + return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrNotExist} + }) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) + if err == nil || status.Code(err) != codes.NotFound { + t.Fatalf("expected NotFound on race, got %v", err) + } } func TestHandleStat_ReadDirPermissionDenied(t *testing.T) { -withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) -withFsReadDir(t, func(path string) ([]os.DirEntry, error) { -return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrPermission} -}) -_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) -if err == nil || status.Code(err) != codes.PermissionDenied { -t.Fatalf("expected PermissionDenied, got %v", err) -} + withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) + withFsReadDir(t, func(path string) ([]os.DirEntry, error) { + return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrPermission} + }) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) + if err == nil || status.Code(err) != codes.PermissionDenied { + t.Fatalf("expected PermissionDenied, got %v", err) + } } func TestHandleStat_ReadDirGenericError(t *testing.T) { -withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) -withFsReadDir(t, func(path string) ([]os.DirEntry, error) { -return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrInvalid} -}) -_, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) -if err == nil || status.Code(err) != codes.Internal { -t.Fatalf("expected Internal, got %v", err) -} + withFsStat(t, func(string) (os.FileInfo, error) { return fakeDirInfo{}, nil }) + withFsReadDir(t, func(path string) ([]os.DirEntry, error) { + return nil, &os.PathError{Op: "readdirent", Path: path, Err: os.ErrInvalid} + }) + _, err := HandleStat(context.Background(), &gnoi_file_pb.StatRequest{Path: "/tmp/x"}) + if err == nil || status.Code(err) != codes.Internal { + t.Fatalf("expected Internal, got %v", err) + } }