[gnoi] file: implement Get in-house using /mnt/host#698
Open
hdwhdw wants to merge 2 commits into
Open
Conversation
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 sonic-net#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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the gNOI File.Get RPC in-process for sonic-gnmi by reading from the host filesystem through the existing /mnt/host bind-mount translation layer, enabling clients to retrieve files via gRPC where the server previously returned Unimplemented.
Changes:
- Added
pkg/gnoi/file.HandleGetto validate requests, translate paths via/mnt/hostwhen applicable, stream file contents in 64 KiB chunks, and emit a final MD5 hash trailer. - Updated
gnmi_server.FileServer.Getto authenticate and delegate to the new handler. - Added focused unit tests for
HandleGetand updated the server-level Get test to assert delegation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/gnoi/file/get_test.go | Adds unit tests covering HandleGet validation, streaming chunking behavior, hashing, send failures, and cancellation. |
| pkg/gnoi/file/file.go | Introduces HandleGet implementing chunked streaming + MD5 hash trailer with /mnt/host path translation. |
| gnmi_server/gnoi_file.go | Replaces the Unimplemented Get stub with delegation to gnoifile.HandleGet after auth. |
| gnmi_server/gnoi_file_test.go | Updates Get test to validate handler delegation and propagated status codes. |
Comment on lines
+640
to
+664
| 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) | ||
| } | ||
| } |
…anches
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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
File.Getwas previously a stub that always returnedUnimplemented, which meant the gNOI client could not pull files offthe switch via gRPC at all. Bringing it in-house now (a) lets us read
host files through the existing gnmi container bind mount without
adding a new D-Bus dependency, and (b) follows the exact same
host-filesystem path pattern File.Put, File.TransferToRemote,
File.Remove, and (in #697) File.Stat already use, so there is one
mental model for the whole RPC family.
How I did it
pkg/gnoi/file.HandleGetthat reads thehost file via
os.Openand reuses the existingtranslatePathForContainerhelper to bridge container-vs-hostpaths.
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"), the handler:
/mnt/host(the same rule HandleStat enforces).FailedPrecondition.maxFileSize(4 GiB).HashType{MD5, sum}message.aborts promptly.
gnmi_server.FileServer.Getis a thin wrapper: authenticate, thendelegate to
HandleGet. The previous "log warning + returnUnimplemented" body is gone.
Hash choice
MD5. It matches what
HandlePutandHandleTransferToRemotealready use in this package, so callers and tests have a single
expectation across the File RPCs. The proto allows MD5/SHA256/SHA512;
this is integrity, not security, so MD5 is fine. Trivial follow-up
if a caller needs SHA256.
Corner cases worth flagging
/mnt/hostsymlink limitation as Stat / Put / Remove.Inside the gnmi container,
os.Openfollows symlinks via thekernel, and a host-absolute symlink target resolves against the
container's filesystem root, not
/mnt/host. This is not newto Get — every
/mnt/host-based code path in this repo behavesthat way.
PermissionDeniedis not exercised live. The gnmi containeron a vanilla KVM testbed runs as
uid=0withCAP_DAC_OVERRIDE,so the OS won't deny it. The branch is exercised via the unit
test that constructs a non-readable file; it will fire in
hardened production deployments where the container is
unprivileged.
How to verify it
Unit tests
pkg/gnoi/file/get_test.gocovers:/mnt/host-prefixrejection — all
InvalidArgument.FailedPrecondition.both verified.
round-trip verified; chunk-size invariant checked (every chunk
but the last is exactly 64 KiB).
Internal.Canceled/DeadlineExceeded.The fake
File_GetServercopies theContentsslice onSendtomirror real gRPC's synchronous-marshal contract — without that copy,
the handler's reused 64 KiB read buffer aliases all captured
responses.
Local pure-test run: 628 → 639 (+11). No regressions.
Live end-to-end test on a sonic-mgmt KVM testbed (vlab-01)
Built the deb from the branch, installed it inside the running gNMI
container via the host bind mount, restarted the supervised gNMI
process, and exercised
File.Getover the gNMI container's localUnix-domain socket.
Reproducible example
Real input/output:
{ "contents": "aGVsbG8gc29uaWMgZ05PSSBGaWxlLkdldCBvdmVyIFVEUwo=" } { "hash": { "method": "MD5", "hash": "zwUK80epXAYX6+yPphKCcw==" } }Verifying:
— matches the local
md5sumexactly.For a 200 KiB random file the server emits 4 Contents messages plus
the final Hash, and the MD5 of the reassembled payload matches the
server-reported hash bit-for-bit:
Test matrix
All cases below were exercised against the deployed binary using the
same
grpcurl/ UDS flow.remote_file/tmp/get-smoke/hello.txt(35 B)md5sum/tmp/get-smoke/big.bin(200 KiB random)/etc/hostname/mnt/hosttranslation)/host/reboot-cause/reboot-cause.txt/tmp/get-smoke(a directory)FailedPrecondition/tmp/no-such-zzzNotFound""InvalidArgument: remote_file cannot be emptyrelative/fooInvalidArgument: must be absolute/mnt/host/etc/hostnameInvalidArgument: drop the /mnt/host prefixNotes:
unix:///var/run/gnmi/gnmi.sockis the gNMI container's localUDS. It bypasses the TLS / mTLS chain that the TCP listeners on
:50051/:50052require, so it's the practical way toexercise the server interactively on a test DUT.
sudois required because the socket is mode0660 root:root.Which release branch to backport (provide reason below if selected)
Description for the changelog
[gnoi] file: implement Get in-house via /mnt/host with 64 KiB
chunked stream and MD5 hash trailer
Link to config_db schema for YANG module changes
N/A — no YANG changes.