[gnoi] file: implement Stat in-house using /mnt/host#697
Open
hdwhdw wants to merge 6 commits into
Open
Conversation
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 <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 updates the gNOI File.Stat implementation to remove the D-Bus dependency and instead serve Stat requests directly from the gNMI binary by reading host filesystem metadata via the existing /mnt/host bind-mount translation logic.
Changes:
- Added a pure-Go
pkg/gnoi/file.HandleStatthat usesos.Stat/os.ReadDirand emitsStatInfo(including octal-as-decimal permissions and a constant umask). - Updated
gnmi_server.FileServer.Statto authenticate and delegate to the new handler; removed the legacy D-Bus-based Stat path. - Added/updated unit tests to cover Stat behavior for files and directories without D-Bus mocking.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/gnoi/file/file.go | Adds HandleStat, statInfoFromFileInfo, and defaultUmask; switches Stat to host filesystem reads via translated paths. |
| pkg/gnoi/file/stat_test.go | Adds new unit tests for HandleStat and permissions encoding. |
| gnmi_server/gnoi_file.go | Replaces the old D-Bus Stat logic with a thin wrapper delegating to gnoifile.HandleStat. |
| gnmi_server/gnoi_file_test.go | Reworks Stat tests to exercise the new handler behavior (real temp files/dirs). |
| gnmi_server/server_test.go | Updates the gNOI Stat integration-style tests to validate the new filesystem-based behavior. |
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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
6 tasks
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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hdwhdw
added a commit
to hdwhdw/sonic-gnmi
that referenced
this pull request
Jun 4, 2026
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>
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.Statcurrently delegates to a SONiC host service over D-Bus(
sonic_service_client.GetFileStat). That introduces a runtimedependency on the host service for what is fundamentally a metadata
read against the host filesystem — metadata that is already reachable
from inside the gNMI container through the existing
/mnt/hostbindmount used by
File.Put,File.Remove, andFile.TransferToRemote.Bringing Stat in-house removes the D-Bus hop, lets the RPC be served
entirely from the gNMI binary, and aligns Stat with how the rest of
the file RPCs already access the host filesystem. It also unblocks
directory Stat: per the gNOI proto comment ("Stat will list files at
the provided path") a directory input should return one entry per
child, which the D-Bus path did not implement.
How I did it
pkg/gnoi/file.HandleStatthat readsmetadata via
os.Stat/os.ReadDirand reuses the existingtranslatePathForContainerhelper to bridge container-vs-hostpaths.
StatInfofor that fileStatInfoper immediate child (non-recursive)gnmi_server.FileServer.Statis now a thin wrapper: authenticate,then delegate to
HandleStat. ThereadFileStathelper and theD-Bus dependency for Stat are removed.
(e.g. mode
0644→Permissions=644).umaskis reported as a constant0022(defaultUmask).Rationale: the proto field is "default file creation mask" — a
process-level attribute, not a per-file one — and it can't be
derived from
os.Stat.syscall.Umaskis racy across goroutines(it both reads and writes in a single call). The previous D-Bus
implementation also returned a process umask, so callers that
treated this as informational see no change.
Corner cases worth flagging
Not blockers, but reviewers may want to confirm these match intent:
Stat. Cleaned paths like/tmp/../etc/hostnameresolve to/etc/hostnameand Stat returnsit. This matches the previous D-Bus implementation (which also
had no whitelist) and is consistent with
Statbeing a read-onlymetadata RPC. The write/remove RPCs (
Put,TransferToRemote,Remove) continue to enforce the existing/tmp/,/var/tmp/,/host/whitelist. Easy to add a Stat whitelist later if securityreview wants one.
os.Statfollows symlinks; if the link target is host-absolute,the kernel resolves it against the gNMI container's filesystem
root, not the host's, and Stat returns
NotFound. This is apre-existing limitation of every
/mnt/host-based code path(
internal/diskspace,pkg/gnoi/file.Put,Remove,TransferToRemote); it is not introduced by this change.Relative symlinks resolve correctly.
Stat. Stat on a singlesymlink uses
os.Stat(follows). Listing a directory usesos.ReadDir+entry.Info(), which islstat-equivalent (doesnot follow). The proto does not specify either way;
ls -ldoesnot follow either.
PermissionDeniedis not exercised by the live test. ThegNMI container in a vanilla KVM testbed runs as
uid=0withCAP_DAC_OVERRIDE, so no filesystem permission can deny it.The branch is exercised by the unit tests; it will fire in
hardened production deployments where the container is
unprivileged.
How to verify it
Unit tests
pkg/gnoi/file/stat_test.gocovering nil request,empty path, relative path, NotFound, regular-file success,
directory listing (with a subdir), empty directory, and the
octal-as-decimal permissions encoding for several modes.
gnmi_serverStat tests rewritten to drive the newhandler against real temp files (no D-Bus mocks).
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.Statover the gNMI container's localUnix-domain socket.
Reproducible example:
Real output:
{ "stats": [ { "path": "/tmp/stat-smoke/empty.bin", "lastModified": "1780514631773086705", "permissions": 664, "umask": 18 }, { "path": "/tmp/stat-smoke/file.txt", "lastModified": "1780514631769086705", "permissions": 664, "size": "10", "umask": 18 }, { "path": "/tmp/stat-smoke/sub", "lastModified": "1780514631773086705", "permissions": 775, "umask": 18 } ] }Notes on the example:
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 is the practical way toexercise the server interactively on a test DUT.
sudois required because the socket is mode0660 root:root.permissions: 664is octal0664encoded as decimal per thegNOI proto.
umask: 18is decimal for0022octal — theconstant
defaultUmaskdiscussed above.sizeis omitted (proto default0) for directories.Test matrix (all exercised against the deployed binary using the
same
grpcurl/ UDS flow):/tmp/stat-smoke/file.txtStatInfowithsize=10,permissions=664/tmp/stat-smokeStatInfoentries, dirs reported with nosize/tmp/this-does-not-exist-xyzNotFound""InvalidArgument: path cannot be emptyrelative/fooInvalidArgument: path must be absolute/etc/hostname/tmp/mnt/hosttranslation is general/var/log/tmpdirectory/tmp/stat-extra/../stat-extra/target.txtfilepath.Clean/host/reboot-causehistory/,platform/,reboot-cause.txt, and theprevious-reboot-cause.jsonsymlinkWhich release branch to backport (provide reason below if selected)
Description for the changelog
[gnoi] file: implement Stat in-house using /mnt/host; add directory support
Link to config_db schema for YANG module changes
N/A — no YANG changes.