From 123f13f0a09c896648bb8f2d9f1686db4bdd573b Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 15:25:26 -0500 Subject: [PATCH 1/2] [gnoi] file: implement Get in-house using /mnt/host Replace the Unimplemented stub for File.Get with a pure-Go handler that streams a host file directly to the caller via the existing /mnt/host bind mount, the same translation pattern File.Put, File.TransferToRemote, File.Remove, and (after #697) File.Stat already use. No D-Bus hop, no host service. Per the gNOI proto: "Get reads and streams the contents of a file from the target. The file is streamed by sequential messages, each containing up to 64KB of data. A final message is sent prior to closing the stream that contains the hash of the data sent." HandleGet implements that: - validate request (non-nil, absolute path, not /mnt/host-prefixed) - reject directories and non-regular files with FailedPrecondition - cap file size at the package-wide maxFileSize (4 GiB) - stream 64 KiB chunks while updating a running MD5 - send a final HashType{MD5, sum} message - check stream context between chunks so cancelled clients abort promptly MD5 matches the convention HandlePut and HandleTransferToRemote already use in this package; it's integrity-only, not security critical. Tests: - pkg/gnoi/file/get_test.go covers nil request, empty path, relative path, /mnt/host prefix rejection, NotFound, directory rejection, empty file (just the hash), small file (single chunk + hash), 200 KiB file (forces 4 chunks + hash, validates payload and MD5 round-trip), Send error propagation, and cancelled context. Uses a fake File_GetServer that copies Contents on Send to mirror real gRPC's synchronous-marshal contract. - gnmi_server/gnoi_file_test.go: replace the old Get_Fails_With_Unimplemented_Error sub-test with a Get_Delegates_To_Handler smoke test that confirms an authenticated request reaches HandleGet and a handler-level error (empty remote_file -> InvalidArgument) propagates out through the server stack as the matching gRPC status code. Auth-error sub-test kept as-is. - Local pure suite: 628 -> 639 (+11). Live verified end-to-end on a sonic-mgmt KVM testbed via grpcurl over /var/run/gnmi/gnmi.sock for /tmp/, /etc/, and /host/ paths; see PR description. Signed-off-by: Dawei Huang --- gnmi_server/gnoi_file.go | 3 +- gnmi_server/gnoi_file_test.go | 12 +- pkg/gnoi/file/file.go | 107 +++++++++++++ pkg/gnoi/file/get_test.go | 277 ++++++++++++++++++++++++++++++++++ 4 files changed, 393 insertions(+), 6 deletions(-) create mode 100644 pkg/gnoi/file/get_test.go diff --git a/gnmi_server/gnoi_file.go b/gnmi_server/gnoi_file.go index 3df6a5b8f..266c69f87 100644 --- a/gnmi_server/gnoi_file.go +++ b/gnmi_server/gnoi_file.go @@ -29,8 +29,7 @@ func (srv *FileServer) Get(req *gnoi_file_pb.GetRequest, stream gnoi_file_pb.Fil log.Errorf("authentication failed in Get RPC: %v", err) return err } - log.Warning("file.Get RPC is unimplemented") - return status.Errorf(codes.Unimplemented, "Method file.Get is unimplemented.") + return gnoifile.HandleGet(req, stream) } // TransferToRemote downloads a file from a remote URL. diff --git a/gnmi_server/gnoi_file_test.go b/gnmi_server/gnoi_file_test.go index cc214b4fe..fe69d668d 100644 --- a/gnmi_server/gnoi_file_test.go +++ b/gnmi_server/gnoi_file_test.go @@ -333,7 +333,6 @@ func TestGnoiFileServer(t *testing.T) { stream, err := client.Get(context.Background(), &gnoi_file_pb.GetRequest{}) if err == nil { - // since Get returns Unimplemented after auth, this should be hit _, err = stream.Recv() } @@ -342,7 +341,12 @@ func TestGnoiFileServer(t *testing.T) { } }) - t.Run("Get_Fails_With_Unimplemented_Error", func(t *testing.T) { + t.Run("Get_Delegates_To_Handler", func(t *testing.T) { + // Smoke test: an authenticated request reaches HandleGet and a + // handler-level error (empty remote_file -> InvalidArgument) + // propagates through the server stack as the matching gRPC + // status code. Behavior coverage for HandleGet lives in + // pkg/gnoi/file/get_test.go. patch := gomonkey.ApplyFuncReturn(authenticate, nil, nil) defer patch.Reset() @@ -351,8 +355,8 @@ func TestGnoiFileServer(t *testing.T) { _, err = stream.Recv() } - if err == nil || status.Code(err) != codes.Unimplemented { - t.Fatalf("Expected Unimplemented error, got: %v", err) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("Expected InvalidArgument from handler, got: %v", err) } }) diff --git a/pkg/gnoi/file/file.go b/pkg/gnoi/file/file.go index fc0407561..1e4427658 100644 --- a/pkg/gnoi/file/file.go +++ b/pkg/gnoi/file/file.go @@ -717,3 +717,110 @@ func HandleStat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_ resp.Stats = stats return resp, nil } + +// HandleGet implements the complete logic for the gNOI File.Get RPC. +// +// Per the gNOI proto: "Get reads and streams the contents of a file from +// the target. The file is streamed by sequential messages, each +// containing up to 64KB of data. A final message is sent prior to +// closing the stream that contains the hash of the data sent." +// +// Behavior: +// - Validates the request: non-nil, non-empty absolute path, and not +// prefixed with /mnt/host (clients pass host-visible paths). +// - Translates the path through translatePathForContainer so it works +// both inside the gnmi container and on a bare host. +// - Rejects directories with FailedPrecondition (the proto requires +// a file). +// - Streams the file in 64 KiB chunks while updating a running MD5, +// then sends a final HashType message. MD5 matches the convention +// used by HandlePut and HandleTransferToRemote in this package; it +// is integrity-only, not security-critical. +func HandleGet(req *gnoi_file_pb.GetRequest, stream gnoi_file_pb.File_GetServer) error { + if req == nil { + return status.Error(codes.InvalidArgument, "request cannot be nil") + } + remoteFile := req.GetRemoteFile() + if remoteFile == "" { + return status.Error(codes.InvalidArgument, "remote_file cannot be empty") + } + if !filepath.IsAbs(remoteFile) { + return status.Errorf(codes.InvalidArgument, "remote_file must be absolute, got: %s", remoteFile) + } + + cleanPath := filepath.Clean(remoteFile) + if cleanPath == "/mnt/host" || strings.HasPrefix(cleanPath, "/mnt/host/") { + return status.Errorf(codes.InvalidArgument, + "remote_file must be host-visible, not container-internal: %s (drop the /mnt/host prefix)", remoteFile) + } + + translatedPath := translatePathForContainer(cleanPath) + + info, err := os.Stat(translatedPath) + if err != nil { + if os.IsNotExist(err) { + return status.Errorf(codes.NotFound, "file not found: %s", remoteFile) + } + if os.IsPermission(err) { + return status.Errorf(codes.PermissionDenied, "permission denied: %s", remoteFile) + } + return status.Errorf(codes.Internal, "failed to stat %s: %v", remoteFile, err) + } + if info.IsDir() { + return status.Errorf(codes.FailedPrecondition, "remote_file is a directory: %s", remoteFile) + } + if !info.Mode().IsRegular() { + return status.Errorf(codes.FailedPrecondition, "remote_file is not a regular file: %s", remoteFile) + } + if info.Size() > maxFileSize { + return status.Errorf(codes.FailedPrecondition, "file %s exceeds maximum size of %d bytes", remoteFile, maxFileSize) + } + + f, err := os.Open(translatedPath) + if err != nil { + if os.IsPermission(err) { + return status.Errorf(codes.PermissionDenied, "permission denied opening %s: %v", remoteFile, err) + } + return status.Errorf(codes.Internal, "failed to open %s: %v", remoteFile, err) + } + defer f.Close() + + hashCalc := hash.NewStreamingMD5Calculator() + buf := make([]byte, 64*1024) // 64 KiB chunks per gNOI proto. + for { + if err := stream.Context().Err(); err != nil { + return status.FromContextError(err).Err() + } + n, readErr := f.Read(buf) + if n > 0 { + chunk := buf[:n] + if _, werr := hashCalc.Write(chunk); werr != nil { + return status.Errorf(codes.Internal, "hash update failed: %v", werr) + } + if serr := stream.Send(&gnoi_file_pb.GetResponse{ + Response: &gnoi_file_pb.GetResponse_Contents{Contents: chunk}, + }); serr != nil { + return status.Errorf(codes.Internal, "failed to send chunk: %v", serr) + } + } + if readErr == io.EOF { + break + } + if readErr != nil { + return status.Errorf(codes.Internal, "failed to read %s: %v", remoteFile, readErr) + } + } + + if err := stream.Send(&gnoi_file_pb.GetResponse{ + Response: &gnoi_file_pb.GetResponse_Hash{ + Hash: &types.HashType{ + Method: types.HashType_MD5, + Hash: hashCalc.Sum(), + }, + }, + }); err != nil { + return status.Errorf(codes.Internal, "failed to send hash: %v", err) + } + log.Infof("Successfully streamed %d bytes from %s", info.Size(), remoteFile) + return nil +} diff --git a/pkg/gnoi/file/get_test.go b/pkg/gnoi/file/get_test.go new file mode 100644 index 000000000..5314f7823 --- /dev/null +++ b/pkg/gnoi/file/get_test.go @@ -0,0 +1,277 @@ +package file + +import ( + "context" + "crypto/md5" + "io" + "os" + "path/filepath" + "strings" + "testing" + + gnoi_file_pb "github.com/openconfig/gnoi/file" + "github.com/openconfig/gnoi/types" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" +) + +// fakeGetServer captures GetResponse messages emitted by HandleGet. It +// satisfies gnoi_file_pb.File_GetServer (grpc.ServerStream + Send). +type fakeGetServer struct { + grpc.ServerStream + ctx context.Context + responses []*gnoi_file_pb.GetResponse + sendErr error +} + +func newFakeGetServer() *fakeGetServer { + return &fakeGetServer{ctx: context.Background()} +} + +func (s *fakeGetServer) Send(r *gnoi_file_pb.GetResponse) error { + if s.sendErr != nil { + return s.sendErr + } + // Real gRPC Send marshals the message synchronously, so the + // handler is allowed to reuse its buffer after Send returns. For + // the fake, copy any Contents to mimic that contract. + if c := r.GetContents(); c != nil { + dup := make([]byte, len(c)) + copy(dup, c) + r = &gnoi_file_pb.GetResponse{ + Response: &gnoi_file_pb.GetResponse_Contents{Contents: dup}, + } + } + s.responses = append(s.responses, r) + return nil +} +func (s *fakeGetServer) Context() context.Context { return s.ctx } +func (s *fakeGetServer) SetHeader(metadata.MD) error { return nil } +func (s *fakeGetServer) SendHeader(metadata.MD) error { return nil } +func (s *fakeGetServer) SetTrailer(metadata.MD) {} + +// getTestRoot mirrors statTestRoot from stat_test.go: builds fixtures +// under the *physical* root (/mnt/host/tmp/... when /mnt/host is +// present) and returns the matching *logical* root that should be sent +// in the request. +func getTestRoot(t *testing.T) (logical, physical string) { + t.Helper() + physBase := "/tmp" + if _, err := os.Stat("/mnt/host"); err == nil { + 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, "get-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 +} + +// collectStream concatenates the contents from every Contents message +// and returns (data, finalHash). Asserts that the last message is a +// HashType message. +func collectStream(t *testing.T, srv *fakeGetServer) ([]byte, *types.HashType) { + t.Helper() + if len(srv.responses) == 0 { + t.Fatal("no responses captured") + } + var data []byte + for i, r := range srv.responses[:len(srv.responses)-1] { + c := r.GetContents() + if c == nil { + t.Fatalf("response %d: expected Contents, got %T", i, r.GetResponse()) + } + data = append(data, c...) + } + last := srv.responses[len(srv.responses)-1] + h := last.GetHash() + if h == nil { + t.Fatalf("final response is not a Hash, got %T", last.GetResponse()) + } + return data, h +} + +func TestHandleGet_NilRequest(t *testing.T) { + err := HandleGet(nil, newFakeGetServer()) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleGet_EmptyPath(t *testing.T) { + err := HandleGet(&gnoi_file_pb.GetRequest{RemoteFile: ""}, newFakeGetServer()) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleGet_RelativePath(t *testing.T) { + err := HandleGet(&gnoi_file_pb.GetRequest{RemoteFile: "rel/path"}, newFakeGetServer()) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } +} + +func TestHandleGet_RejectsMntHostPrefix(t *testing.T) { + for _, p := range []string{"/mnt/host", "/mnt/host/tmp/foo"} { + err := HandleGet(&gnoi_file_pb.GetRequest{RemoteFile: p}, newFakeGetServer()) + if err == nil || status.Code(err) != codes.InvalidArgument { + t.Errorf("path %q: expected InvalidArgument, got %v", p, err) + } + } +} + +func TestHandleGet_NotFound(t *testing.T) { + logi, _ := getTestRoot(t) + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "no-such-file"), + }, newFakeGetServer()) + if err == nil || status.Code(err) != codes.NotFound { + t.Fatalf("expected NotFound, got %v", err) + } +} + +func TestHandleGet_DirectoryRejected(t *testing.T) { + logi, _ := getTestRoot(t) + err := HandleGet(&gnoi_file_pb.GetRequest{RemoteFile: logi}, newFakeGetServer()) + if err == nil || status.Code(err) != codes.FailedPrecondition { + t.Fatalf("expected FailedPrecondition, got %v", err) + } +} + +func TestHandleGet_EmptyFile(t *testing.T) { + logi, phys := getTestRoot(t) + if err := os.WriteFile(filepath.Join(phys, "empty.bin"), nil, 0644); err != nil { + t.Fatal(err) + } + srv := newFakeGetServer() + if err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "empty.bin"), + }, srv); err != nil { + t.Fatalf("HandleGet: %v", err) + } + // Empty file: no Contents messages, only the final Hash. + if len(srv.responses) != 1 { + t.Fatalf("expected 1 response, got %d", len(srv.responses)) + } + h := srv.responses[0].GetHash() + if h == nil { + t.Fatalf("expected Hash, got %T", srv.responses[0].GetResponse()) + } + if h.Method != types.HashType_MD5 { + t.Errorf("hash method = %v, want MD5", h.Method) + } + want := md5.Sum(nil) + if string(h.Hash) != string(want[:]) { + t.Errorf("hash mismatch for empty file") + } +} + +func TestHandleGet_SmallFile(t *testing.T) { + logi, phys := getTestRoot(t) + payload := []byte("hello sonic gnmi") + if err := os.WriteFile(filepath.Join(phys, "small.bin"), payload, 0644); err != nil { + t.Fatal(err) + } + srv := newFakeGetServer() + if err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "small.bin"), + }, srv); err != nil { + t.Fatalf("HandleGet: %v", err) + } + got, h := collectStream(t, srv) + if string(got) != string(payload) { + t.Errorf("payload mismatch: got %q want %q", got, payload) + } + want := md5.Sum(payload) + if string(h.Hash) != string(want[:]) { + t.Errorf("hash mismatch") + } + // Small file should fit in a single chunk. + if n := len(srv.responses); n != 2 { + t.Errorf("expected 2 responses (1 chunk + hash), got %d", n) + } +} + +func TestHandleGet_LargeFileChunks(t *testing.T) { + // 200 KiB file forces >1 chunk (chunk size is 64 KiB). + logi, phys := getTestRoot(t) + payload := make([]byte, 200*1024) + for i := range payload { + payload[i] = byte(i % 251) + } + if err := os.WriteFile(filepath.Join(phys, "large.bin"), payload, 0644); err != nil { + t.Fatal(err) + } + srv := newFakeGetServer() + if err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "large.bin"), + }, srv); err != nil { + t.Fatalf("HandleGet: %v", err) + } + got, h := collectStream(t, srv) + if string(got) != string(payload) { + t.Errorf("payload mismatch (lengths got=%d want=%d)", len(got), len(payload)) + } + want := md5.Sum(payload) + if string(h.Hash) != string(want[:]) { + t.Errorf("hash mismatch on chunked stream") + } + // 200 KiB / 64 KiB = 4 chunks (3 full + 1 partial), plus the final hash. + if n := len(srv.responses); n != 5 { + t.Errorf("expected 5 responses (4 chunks + hash), got %d", n) + } + // Every Contents message except possibly the last must be exactly 64 KiB. + for i := 0; i < len(srv.responses)-2; i++ { + c := srv.responses[i].GetContents() + if len(c) != 64*1024 { + t.Errorf("response %d size = %d, want %d", i, len(c), 64*1024) + } + } +} + +func TestHandleGet_StreamSendError(t *testing.T) { + logi, phys := getTestRoot(t) + if err := os.WriteFile(filepath.Join(phys, "x"), []byte("data"), 0644); err != nil { + t.Fatal(err) + } + srv := newFakeGetServer() + srv.sendErr = io.ErrClosedPipe + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "x"), + }, srv) + if err == nil || status.Code(err) != codes.Internal { + t.Fatalf("expected Internal on send failure, got %v", err) + } +} + +func TestHandleGet_ContextCancelled(t *testing.T) { + logi, phys := getTestRoot(t) + if err := os.WriteFile(filepath.Join(phys, "x"), []byte("data"), 0644); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + srv := newFakeGetServer() + srv.ctx = ctx + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "x"), + }, srv) + if err == nil { + t.Fatal("expected error from cancelled context, got nil") + } + if c := status.Code(err); c != codes.Canceled && c != codes.DeadlineExceeded { + t.Errorf("expected Canceled/DeadlineExceeded, got %v", err) + } +} From 36aade44f86304036417a7a09215693c1e3bd560 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Wed, 3 Jun 2026 16:55:16 -0500 Subject: [PATCH 2/2] [gnoi] file: make hostRoot/maxFileSize injectable, cover Get error branches Diff coverage on the original PR was 78%, below the 80% threshold. The uncovered lines were all error paths in HandleGet that os.Stat / os.Open basically never trigger on the test container (root-bypasses-mode-bits, no >4 GiB files, MD5 never errors, etc.). Rather than reach for gomonkey, expose two knobs that already wanted to be injectable for cleaner test setup: - hostRoot (was a hardcoded "/mnt/host" inside translatePathForContainer) is now a package var. Tests can point it at a t.TempDir() and build real fixtures (regular files, fifos, oversize sparse files) without needing /mnt/host on the host or relying on dual-path probing. The helper useTempHostRoot(t) wraps the swap+restore. - maxFileSize is now a var so tests can lower it to 16 bytes and exercise the oversize branch with a 32-byte file instead of a 4 GiB one. Production behavior is unchanged: hostRoot defaults to "/mnt/host" and maxFileSize to 4 GiB. Three new tests cover the previously-missed branches: - TestHandleGet_NotRegularFile (line 625, syscall.Mkfifo) - TestHandleGet_OversizeFile (line 628, lowered maxFileSize) - TestHandleGet_HashSendError (line 674, fakeGetServer.failOnHash) Existing tests are migrated to the new helper, removing the /mnt/host-detection fallback in get_test.go. HandleGet line coverage: 64% -> 84%; diff coverage clears the 80% gate. Signed-off-by: Dawei Huang --- pkg/gnoi/file/file.go | 27 +++++-- pkg/gnoi/file/get_test.go | 158 +++++++++++++++++++++++++++++++------- 2 files changed, 152 insertions(+), 33 deletions(-) diff --git a/pkg/gnoi/file/file.go b/pkg/gnoi/file/file.go index 1e4427658..f9ec7a7f2 100644 --- a/pkg/gnoi/file/file.go +++ b/pkg/gnoi/file/file.go @@ -32,10 +32,26 @@ const ( // Maximum time allowed for downloading a file (5 minutes for large firmware images) downloadTimeout = 5 * time.Minute - // Maximum file size allowed (4GB - typical maximum firmware size) - maxFileSize = 4 * 1024 * 1024 * 1024 // 4GB in bytes + // defaultMaxFileSize is the default cap for File.Get / File.Put / TransferToRemote + // (4 GiB — typical maximum firmware size). Exposed as a var below so tests can + // lower it without producing actual 4 GiB files. + defaultMaxFileSize = 4 * 1024 * 1024 * 1024 ) +// hostRoot is the path prefix that maps the *container* view onto the *host* +// filesystem. In production it is "/mnt/host" (the bind mount the gnmi +// container ships with). Tests set it to a t.TempDir() so they can build real +// fixtures (regular files, fifos, oversize sparse files, broken perms, ...) +// without touching the actual /mnt/host on the test machine. +// +// translatePathForContainer is the only consumer; it prepends hostRoot to the +// caller-supplied logical path when hostRoot exists on disk. +var hostRoot = "/mnt/host" + +// maxFileSize is the per-RPC size cap. var (not const) so tests can lower it +// to exercise the over-size branch without producing 4 GiB files. +var maxFileSize int64 = defaultMaxFileSize + // newFileClient wraps gnoi_file_pb.NewFileClient to allow test patching // (the generated function is tiny and gets inlined, defeating gomonkey). var newFileClient = gnoi_file_pb.NewFileClient @@ -171,9 +187,10 @@ func translatePathForContainer(path string) string { // Clean the path first cleanPath := filepath.Clean(path) - // Check if /mnt/host exists (indicates we're running in a container) - if _, err := os.Stat("/mnt/host"); err == nil { - return "/mnt/host" + cleanPath + // hostRoot exists on disk → we're running in a container with the host + // filesystem bind-mounted (or in a test that injected a fake root). + if _, err := os.Stat(hostRoot); err == nil { + return hostRoot + cleanPath } // Not in container, return original path diff --git a/pkg/gnoi/file/get_test.go b/pkg/gnoi/file/get_test.go index 5314f7823..d4e912d02 100644 --- a/pkg/gnoi/file/get_test.go +++ b/pkg/gnoi/file/get_test.go @@ -1,12 +1,14 @@ package file import ( + "bytes" "context" "crypto/md5" "io" "os" "path/filepath" "strings" + "syscall" "testing" gnoi_file_pb "github.com/openconfig/gnoi/file" @@ -23,7 +25,12 @@ type fakeGetServer struct { grpc.ServerStream ctx context.Context responses []*gnoi_file_pb.GetResponse - sendErr error + // sendErr fails *every* Send call when set. + sendErr error + // failOnHash, when set, fails only the trailing Hash message. Lets + // tests exercise the post-stream send-error branch independently of + // the per-chunk send-error branch. + failOnHash error } func newFakeGetServer() *fakeGetServer { @@ -34,6 +41,9 @@ func (s *fakeGetServer) Send(r *gnoi_file_pb.GetResponse) error { if s.sendErr != nil { return s.sendErr } + if s.failOnHash != nil && r.GetHash() != nil { + return s.failOnHash + } // Real gRPC Send marshals the message synchronously, so the // handler is allowed to reuse its buffer after Send returns. For // the fake, copy any Contents to mimic that contract. @@ -52,29 +62,41 @@ func (s *fakeGetServer) SetHeader(metadata.MD) error { return nil } func (s *fakeGetServer) SendHeader(metadata.MD) error { return nil } func (s *fakeGetServer) SetTrailer(metadata.MD) {} -// getTestRoot mirrors statTestRoot from stat_test.go: builds fixtures -// under the *physical* root (/mnt/host/tmp/... when /mnt/host is -// present) and returns the matching *logical* root that should be sent -// in the request. -func getTestRoot(t *testing.T) (logical, physical string) { +// useTempHostRoot points hostRoot at a fresh t.TempDir() and restores it +// when the test ends. Returns (logical, physical): +// +// - logical is the path to use in RemoteFile / Path requests; it has +// the hostRoot prefix stripped, so it looks like a normal SONiC path +// (e.g. "/tmp/get-test-123"). +// - physical is the on-disk path under the injected hostRoot where the +// test should create fixtures (regular files, fifos, sparse files, +// ...). +// +// Behavior matches translatePathForContainer: hostRoot is the prefix the +// handler will glue back on. So a fixture written to +// `/tmp/foo` is reached by RemoteFile=`/tmp/foo`. +// +// The injected root is /tmp-rooted (`/tmp/...` is in validatePath's +// whitelist so the handler accepts the logical path). +func useTempHostRoot(t *testing.T) (logical, physical string) { t.Helper() - physBase := "/tmp" - if _, err := os.Stat("/mnt/host"); err == nil { - 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, "get-test-*") - if err != nil { - t.Fatalf("mkdir temp: %v", err) - } - t.Cleanup(func() { _ = os.RemoveAll(phys) }) + root := t.TempDir() + prevRoot := hostRoot + hostRoot = root + t.Cleanup(func() { hostRoot = prevRoot }) - logi := phys - if strings.HasPrefix(phys, "/mnt/host") { - logi = strings.TrimPrefix(phys, "/mnt/host") + // Build /tmp/get-test-XXXX so the handler-visible logical path + // (after stripping ) starts with /tmp/, which validatePath + // accepts. + physTmp := filepath.Join(root, "tmp") + if err := os.MkdirAll(physTmp, 0755); err != nil { + t.Fatalf("mkdir %s: %v", physTmp, err) + } + phys, err := os.MkdirTemp(physTmp, "get-test-*") + if err != nil { + t.Fatalf("mkdir temp under %s: %v", physTmp, err) } + logi := strings.TrimPrefix(phys, root) return logi, phys } @@ -133,7 +155,7 @@ func TestHandleGet_RejectsMntHostPrefix(t *testing.T) { } func TestHandleGet_NotFound(t *testing.T) { - logi, _ := getTestRoot(t) + logi, _ := useTempHostRoot(t) err := HandleGet(&gnoi_file_pb.GetRequest{ RemoteFile: filepath.Join(logi, "no-such-file"), }, newFakeGetServer()) @@ -143,7 +165,7 @@ func TestHandleGet_NotFound(t *testing.T) { } func TestHandleGet_DirectoryRejected(t *testing.T) { - logi, _ := getTestRoot(t) + logi, _ := useTempHostRoot(t) err := HandleGet(&gnoi_file_pb.GetRequest{RemoteFile: logi}, newFakeGetServer()) if err == nil || status.Code(err) != codes.FailedPrecondition { t.Fatalf("expected FailedPrecondition, got %v", err) @@ -151,7 +173,7 @@ func TestHandleGet_DirectoryRejected(t *testing.T) { } func TestHandleGet_EmptyFile(t *testing.T) { - logi, phys := getTestRoot(t) + logi, phys := useTempHostRoot(t) if err := os.WriteFile(filepath.Join(phys, "empty.bin"), nil, 0644); err != nil { t.Fatal(err) } @@ -179,7 +201,7 @@ func TestHandleGet_EmptyFile(t *testing.T) { } func TestHandleGet_SmallFile(t *testing.T) { - logi, phys := getTestRoot(t) + logi, phys := useTempHostRoot(t) payload := []byte("hello sonic gnmi") if err := os.WriteFile(filepath.Join(phys, "small.bin"), payload, 0644); err != nil { t.Fatal(err) @@ -206,7 +228,7 @@ func TestHandleGet_SmallFile(t *testing.T) { func TestHandleGet_LargeFileChunks(t *testing.T) { // 200 KiB file forces >1 chunk (chunk size is 64 KiB). - logi, phys := getTestRoot(t) + logi, phys := useTempHostRoot(t) payload := make([]byte, 200*1024) for i := range payload { payload[i] = byte(i % 251) @@ -242,7 +264,7 @@ func TestHandleGet_LargeFileChunks(t *testing.T) { } func TestHandleGet_StreamSendError(t *testing.T) { - logi, phys := getTestRoot(t) + logi, phys := useTempHostRoot(t) if err := os.WriteFile(filepath.Join(phys, "x"), []byte("data"), 0644); err != nil { t.Fatal(err) } @@ -257,7 +279,7 @@ func TestHandleGet_StreamSendError(t *testing.T) { } func TestHandleGet_ContextCancelled(t *testing.T) { - logi, phys := getTestRoot(t) + logi, phys := useTempHostRoot(t) if err := os.WriteFile(filepath.Join(phys, "x"), []byte("data"), 0644); err != nil { t.Fatal(err) } @@ -275,3 +297,83 @@ func TestHandleGet_ContextCancelled(t *testing.T) { t.Errorf("expected Canceled/DeadlineExceeded, got %v", err) } } + +// TestHandleGet_NotRegularFile covers the "not a regular file" branch. +// We create a fifo (named pipe) inside the injected hostRoot — the real +// os.Stat returns Mode().IsRegular()==false on it and HandleGet must +// reject with FailedPrecondition without ever opening it. +func TestHandleGet_NotRegularFile(t *testing.T) { + logi, phys := useTempHostRoot(t) + fifo := filepath.Join(phys, "pipe") + if err := syscall.Mkfifo(fifo, 0644); err != nil { + t.Skipf("mkfifo unsupported on this platform: %v", err) + } + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "pipe"), + }, newFakeGetServer()) + if err == nil || status.Code(err) != codes.FailedPrecondition { + t.Fatalf("expected FailedPrecondition for fifo, got %v", err) + } + if !strings.Contains(err.Error(), "not a regular file") { + t.Errorf("expected 'not a regular file' in error, got %v", err) + } +} + +// TestHandleGet_OversizeFile covers the size-cap branch. We lower +// maxFileSize to a tiny value and write a file slightly over it — the +// real os.Stat reports the actual size and HandleGet must reject with +// FailedPrecondition before opening the file. +func TestHandleGet_OversizeFile(t *testing.T) { + logi, phys := useTempHostRoot(t) + + prevMax := maxFileSize + maxFileSize = 16 + t.Cleanup(func() { maxFileSize = prevMax }) + + fname := "huge.bin" + if err := os.WriteFile(filepath.Join(phys, fname), bytes.Repeat([]byte{0xab}, 32), 0644); err != nil { + t.Fatal(err) + } + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, fname), + }, newFakeGetServer()) + if err == nil || status.Code(err) != codes.FailedPrecondition { + t.Fatalf("expected FailedPrecondition for oversize file, got %v", err) + } + if !strings.Contains(err.Error(), "exceeds maximum size") { + t.Errorf("expected 'exceeds maximum size' in error, got %v", err) + } +} + +// TestHandleGet_HashSendError covers the post-stream "failed to send +// hash" branch. failOnHash on the fake server fails *only* the trailing +// HashType message, leaving Contents sends to succeed normally. +func TestHandleGet_HashSendError(t *testing.T) { + logi, phys := useTempHostRoot(t) + if err := os.WriteFile(filepath.Join(phys, "x"), []byte("payload"), 0644); err != nil { + t.Fatal(err) + } + srv := newFakeGetServer() + srv.failOnHash = io.ErrShortWrite + err := HandleGet(&gnoi_file_pb.GetRequest{ + RemoteFile: filepath.Join(logi, "x"), + }, srv) + if err == nil || status.Code(err) != codes.Internal { + t.Fatalf("expected Internal on hash send failure, got %v", err) + } + if !strings.Contains(err.Error(), "failed to send hash") { + t.Errorf("expected 'failed to send hash' in error, got %v", err) + } + // All Contents were sent successfully; the failure is on the trailer. + // At least one Contents response must have been captured. + gotContents := false + for _, r := range srv.responses { + if r.GetContents() != nil { + gotContents = true + break + } + } + if !gotContents { + t.Errorf("expected at least one Contents response captured before hash send failure") + } +}