Merging latest upstream sonic-gnmi master#223
Merged
Conversation
When FIPS mode is enabled (sonic_fips=1), the Go FIPS runtime calls EVP_default_properties_enable_fips(NULL, 1), which constrains all EVP_CIPHER_fetch calls to FIPS-approved ciphers only. The SymCrypt OpenSSL provider does not implement AES-128-CTR, so EVP_CIPHER_fetch returns NULL. The Go FIPS cipher cache stores this NULL and panics with 'unsupported cipher: AES-128' when TLS 1.3 tries to encrypt session tickets (which use AES-128-CTR). Disable session tickets in the gNMI TLS config to avoid the AES-128-CTR code path entirely. TLS 1.3 connections still work normally; only session resumption is affected, which is not needed for long-lived gRPC connections. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* gnmi_server: do not fail server when one listener fails Previously, if either the TCP or UDS listener failed during initialization, NewServer() would tear down any already-created listener and return an error, preventing the server from starting entirely. Similarly, Serve() returned on the first listener error, effectively orphaning the remaining listener goroutine. Change NewServer() to log a warning and disable the failed listener instead of cascading the failure. The existing 'at least one listener' check ensures the server still fails when no listener is available. Change Serve() to use a WaitGroup so it blocks until all listener goroutines complete, rather than returning on the first error. Individual listener failures are logged but do not affect other listeners. Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: add tests for listener resilience Test that NewServer() continues when one listener fails: - TestTCPListenerFailsUDSContinues: TCP port in use, UDS still works - TestUDSListenerFailsTCPContinues: UDS path invalid, TCP still works - TestBothListenersFail: both fail, server returns error Test that Serve() blocks until all listeners stop: - TestServeBlocksWhenOneListenerCloses: closing one listener at runtime does not cause Serve() to return early Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: cover remaining listener resilience paths Add tests targeting the three uncovered code paths: - TestUDSBindFailsTCPContinues: socket path exceeds unix max length, directory creation succeeds but bind fails (lines 582-584) - TestUDSChmodFailsTCPContinues: uses gomonkey to simulate chmod failure after socket creation (lines 588-593) - TestServeUDSErrorDoesNotStopTCP: closes UDS listener at runtime to trigger UDS server error path in Serve() (lines 645-648) Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: drop unreliable gomonkey chmod test The TestUDSChmodFailsTCPContinues test used gomonkey.ApplyFunc to patch os.Chmod, which is unreliable under -race (used in CI). Remove it; the remaining tests cover 85% of the diff, above the 80% threshold. Signed-off-by: Dawei Huang <daweihuang@microsoft.com> --------- Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…C Rotate streams (#618) Signed-off-by: Pattela JAYARAGINI <pattelaj@google.com> Co-authored-by: Neha Das <nehadas@google.com>
…646) The fix in PR #618 (bumping parent context timeout to 10s) did not resolve the underlying race. The internal 5s context.WithTimeout still causes stream2.Recv() to return DeadlineExceeded before the server returns Aborted under CI load. Re-skip the test until the concurrency logic is fixed with explicit synchronization. Tracking: sonic-net/sonic-gnmi#616 Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…(#645)
* Upgrade go directive to 1.24.4
Bump go.mod from go 1.19 to go 1.24.4 to match the Go version
in the Trixie container (sonic-buildimage#26499).
Go 1.24 module graph pruning only resolves modules actually in the
import graph, so packages that don't transitively depend on
sonic-mgmt-common no longer fail when the replace target is absent.
This fixes silent build failures in 7 pure-CI packages.
Also fix non-constant format string in pkg/gnoi/system/system.go
that go vet now rejects under Go 1.24.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Migrate Azure pipeline from Bookworm to Trixie
Update container images from sonic-slave-bookworm to sonic-slave-trixie.
Update artifact paths from bookworm to trixie for libyang, libnl,
sonic-yang-models, and sonic-swss-common packages.
Update .NET repository from debian/12 to debian/13.
Update PureCIJob Go version from 1.19.8 to 1.24.4.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add libpcre3 to trixie artifact download patterns
Trixie does not ship libpcre3, but libyang 1.0.73 depends on it.
The common_libs pipeline builds libpcre3 debs under target/debs/trixie/,
so download and install them alongside libyang.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add missing libpcre3 transitive dependencies for trixie
libpcre3-dev depends on libpcre16-3, libpcre32-3, and libpcrecpp0v5,
which are not available on Trixie. Download them from common_libs
alongside libpcre3 and libpcre3-dev.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix .NET SDK install for Trixie
apt-key was removed in Debian Trixie. Use signed-by in the sources
list with the key stored in /etc/apt/keyrings/ instead.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Dearmor Microsoft GPG key for Trixie sqv compatibility
Trixie uses sqv (Sequoia) for APT signature verification which
requires binary (dearmored) GPG keys. The previous commit stored the
key in ASCII-armored format (.asc) which sqv cannot parse, resulting
in 'Missing key EE4D7792F748182B' errors.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove unused .NET SDK install from CI pipeline
The .NET SDK was never used by sonic-gnmi (pure Go project). It was
a leftover from a shared pipeline template. Removing it also avoids
the sqv GPG verification issue on Trixie.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix non-constant format string in dbus_client for Go 1.24 vet
Go 1.24 vet rejects variable strings as format arguments. Change
fmt.Errorf(msg) to fmt.Errorf("%s", msg) for two call sites in
dbus_client.go. This also fixes sonic_data_client which imports
sonic_service_client transitively.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix additional non-constant format strings for Go 1.24 vet
Go 1.24 vet rejects variable strings as format arguments in
printf-style functions. Fix log.V(1).Infof(msg) and
status.Errorf(code, errStr) patterns in sonic_data_client and
gnmi_server packages.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Disable inlining in tests for gomonkey compatibility with Go 1.24
gomonkey patches functions by rewriting machine code at runtime,
which requires inlining to be disabled. Go 1.24 inlines more
aggressively than 1.19, causing gomonkey's ApplyMethod to fail
silently (the real method executes instead of the mock). Add
-gcflags=all=-l via TEST_FLAGS to all test invocations.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix test timeouts and skip hanging CGO test for Trixie
- Increase inotify cert monitoring test timeouts from 10s to 30s to
account for slower execution with -gcflags=all=-l
- Skip TestNewEventClient: event_set_global_options blocks indefinitely
on Trixie, leaking a goroutine that causes the test binary to exceed
its timeout
- Add -timeout 20m to basic integration tests for safety margin
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Deinit event subscriber in TestDummyEventClient to prevent leaked C thread
C_init_subs creates a ZMQ subscriber background thread. Without
calling C_deinit_subs, this thread keeps the test binary alive until
the go test timeout kills it.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Retrigger CI
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove C_init_subs from TestDummyEventClient
The events CGO subscriber creates a background C thread that blocks
indefinitely on Trixie, preventing the test binary from exiting.
Both C_init_subs and C_deinit_subs block on Trixie's swss-common.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix non-constant format strings in gnmi_server for Go 1.24 vet
Go 1.24 vet rejects non-constant format strings in printf-style
functions. Fix all status.Errorf(code, err.Error()) and
status.Errorf(code, fmt.Sprintf(...)) calls to use explicit %s
format in gnmi_server package.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Debug: use standard-verbose format for env integration tests
Temporarily use standard-verbose gotestsum format to see the actual
gnmi_server build error that testname format is hiding.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add pre-compile check to surface gnmi_server build errors
gotestsum --format testname swallows build/vet errors. Add an
explicit go test -run=^$ (no tests, just compile) before running
gotestsum so compiler errors appear in the log.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix t.Logf vet errors in gnmi_server/server_test.go
Go 1.24 vet rejects non-constant format strings in printf-style
functions. Fix t.Logf(string(data)) to t.Logf("%s", string(data)).
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Increase timeout for GenerateCSR certz tests
GenerateCsrRSA, GenerateCsrECDSA, and GenerateCsrAttest all need
more than the default 1s timeout for key generation on Trixie. Set
timeout to 10s matching other CSR-related test cases.
The 1s default caused GenerateCsrRSA to timeout, which left the
Rotate RPC in progress, cascading to concurrent-RPC errors in
subsequent subtests.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Disable both optimizations and inlining for gomonkey tests
Use -gcflags='all=-N -l' instead of just -gcflags=all=-l to also
disable compiler optimizations. Go 1.24 may optimize time.Sleep
in ways that prevent gomonkey from patching it at runtime.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Replace time.Sleep mock with injectable variable for Go 1.24
Go 1.24 makes time.Sleep unpatchable by gomonkey even with
-gcflags=all=-l. Replace the direct time.Sleep call with an
exported SleepFunc variable that tests can reliably replace
via gomonkey.ApplyGlobalVar.
Revert -gcflags back to 'all=-l' (inlining only) since -N
causes tests to run too slowly and hit the 20m timeout.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Increase test timeouts from 20m to 40m
With -gcflags=all=-l disabling inlining for gomonkey compatibility,
the gnmi_server test suite takes over 20 minutes. The top tests are
TestGnmiSubscribeMultiNs (188s), TestGnmiSubscribe (180s),
TestGNMINative (95s), TestSystem (73s), totaling well over 20m.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add unit tests for diff-coverage on vet-fix error paths
Add tests covering error return paths that were changed for Go 1.24
vet compliance (non-constant format strings in status.Errorf).
gnoi_reset_test.go:
- Marshal error (protojson.Marshal failure)
- Backend error (FactoryReset returns error)
- Unmarshal error (invalid JSON response)
gnoi_os_test.go:
- Enhanced fakeInstallServer with Recv() support
- RecvError on TransferRequest
- SendError on TransferReady response
- RecvError during TransferContent loop
- SendError on TransferEnd response
gnsi_authz_test.go:
- writeCredentialsMetadataToDB failure on version field
- writeCredentialsMetadataToDB failure on created_on field
- saveToAuthzFile failure
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix gofmt formatting in gnoi_os_test.go
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Trigger CI rebuild
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add unit tests for remaining diff-coverage gaps
Cover additional error paths in:
- jwtAuth.go: invalid JWT token parsing error (line 80)
- gnoi_os.go: Send failure after processTransferContent (line 353)
- gnoi_system.go: GetRedisDBClient failures in Reboot, RebootStatus,
CancelReboot (lines 219, 251, 279)
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix TransferContent type in OS install test
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix OS install test: use temp dir instead of gomonkey for unexported methods
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix Send-error test coverage and add RebootStatus unmarshal test
- Set sendErr in TestInstall_SendErrorAfterTransferContent so the
fakeInstallServer.Send actually returns a non-nil error, covering
gnoi_os.go line 353 (previously the test hit the EOF path instead).
- Add assert.Contains to verify the error message, ensuring the test
exercises the intended code path.
- Add TestRebootStatus_UnmarshalError to cover gnoi_system.go line 263
(protojson.Unmarshal failure in RebootStatus). Patches
sendRebootReqOnNotifCh to return invalid JSON data.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Generate diff-cover JSON for pipeline coverage decorator
The [Auto] Generate/Update build coverage properties tasks are
injected by a pipeline decorator after PublishCodeCoverageResults@2.
They expect .diff-coverage/diff-cover.json to exist in the Build
job workspace, but diff-cover only ran in the DiffCoverageCheck job.
Add a diff-cover --json-report step in the Build job before publishing
coverage so the decorator can read the JSON and report the
coverage.sonic-net.sonic-gnmi.build status check to GitHub.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use --break-system-packages for pip on Trixie
Trixie's system Python has Pygments installed via apt without a
RECORD file. pip refuses to upgrade it when installing diff-cover,
causing 'diff-cover: command not found' in the auto-injected
pipeline decorator step. This is why .diff-coverage/diff-cover.json
was never generated and the coverage status check got stuck.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove redundant DiffCoverageCheck job and unused threshold variable
The diff-coverage check is handled by the org-level pipeline decorator
that auto-injects after PublishCodeCoverageResults@2. The separate
DiffCoverageCheck job is redundant.
Also remove the now-unused DIFF_COVER_THRESHOLD variable and add
--ignore-installed to pip install diff-cover to work around Trixie's
Pygments packaging (dist-info without RECORD file blocks uninstall).
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix coverage decorator by setting working directory and pre-installing diff-cover
The org-level pipeline decorator (code-diff-coverage) cd's to
DIFF_COVER_WORKING_DIRECTORY, defaulting to System.DefaultWorkingDirectory.
For sonic-gnmi, coverage.xml lives in the sonic-gnmi subdirectory, so
the decorator couldn't find it.
On Trixie, the decorator's pip install diff-cover also fails because
Debian ships Pygments via dist-info without a RECORD file, causing pip
to refuse the upgrade. Pre-installing with --ignore-installed before
the decorator runs makes the decorator's subsequent pip install a no-op.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use sudo for diff-cover pre-install to match decorator
Pipeline runs as azureuser_azpcontainer, not root. Without sudo, pip
installs to ~/.local/bin which is not on PATH. The decorator then runs
sudo pip3 install as root and still hits the Pygments RECORD issue.
Using sudo ensures diff-cover is installed system-wide where both the
decorator and subsequent steps can find it.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Signed-off-by: Spandan Chowdhury <spandan@nexthop.ai>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
* telemetry: fix auth bypass in noTLS code path
Two fixes to close unauthenticated gNMI access vulnerability:
1. Move cfg.UserAuth assignment above the TLS setup block so
application-layer authentication is enforced regardless of
whether TLS transport is active.
2. Fatal error instead of silently unsetting cert auth when
--client_auth cert is requested without --cacert. Prevents
server starting with no authentication configured.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: add --bind_address flag to restrict TCP listener
When --noTLS is active (no certs configured), the gRPC server runs
cleartext. Add --bind_address flag so operators can restrict the
listener to localhost only, preventing remote access to the cleartext
endpoint:
telemetry --noTLS --bind_address 127.0.0.1 --port 8080
When bind_address is empty (default), behavior is unchanged: binds all
interfaces. VRF-aware listener ignores bind_address (VRF binding is
handled by createVrfListener).
gnmi_server/server.go: add BindAddress to Config struct
telemetry/telemetry.go: add --bind_address flag and pass to Config
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: reject --noTLS without --bind_address 127.0.0.1
--noTLS disables TLS, exposing cleartext gRPC. Require --bind_address 127.0.0.1
to ensure the cleartext endpoint is only accessible from localhost.
Also update existing test cases that use --noTLS to include --bind_address,
and add TestNoTLSRequiresBindAddress to verify the enforcement.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: relax --noTLS validation to require any loopback address
Use net.ParseIP + IsLoopback() instead of exact string match for 127.0.0.1.
This accepts the full 127.0.0.0/8 range and ::1 (IPv6 loopback) while
rejecting empty/non-loopback addresses.
Also update test cases to include --bind_address 127.0.0.1 where needed,
and add TestNoTLSRequiresLoopbackAddress covering valid and invalid cases.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: fix syntax error and gofmt issues in loopback validation
- Remove extra closing brace introduced during conflict resolution
- Fix 'net' import to correct alphabetical position
- Fix string continuation indentation in fmt.Errorf
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* gnmi_server,telemetry: fix extra closing brace from conflict resolution
Remove stray } that was left in both server.go and telemetry.go
during manual conflict resolution. Fixes compilation errors.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: add --bind_address 127.0.0.1 to noTLS auth tests
TestStartGNMIServerNoTLS and TestNoTLSAuthNotBypassed were missing
--bind_address which is now required when using --noTLS.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
---------
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
Co-authored-by: xq9mend <xq9mend@users.noreply.github.com>
* Move diff-cover enforcement from build job to DiffCoverageCheck
Remove DIFF_COVER_* variables from the build job so it can be renamed
without breaking the wrapper's GitHub check name coupling. The
DiffCoverageCheck job now sets DIFF_COVER_ENABLE and related variables,
making it the sole hook point for the sonic-build-web coverage wrapper.
The native diff-cover --fail-under step is removed since the wrapper
handles threshold enforcement via GitHub checks.
Also remove the now-unused global DIFF_COVER_THRESHOLD variable.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Rename build job to IntegrationTest
Now that diff-cover enforcement is on DiffCoverageCheck, the build job
name is no longer coupled to the wrapper's GitHub check name. Rename it
to IntegrationTest to better describe what it does.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use 'build' as job id for DiffCoverageCheck
Reuse the 'build' job id for the diff-coverage job so the wrapper's
GitHub check name remains coverage.{DefName}.build, matching existing
branch protection rules. No admin changes needed.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Set displayName to 'build' for diff-cover job
Azure Pipelines uses displayName (not job id) for the status context
reported to GitHub. The wrapper also uses Agent.JobName which resolves
to displayName. Set it back to 'build' so both the Azure Pipelines
check (sonic-net.sonic-gnmi (Build build)) and the wrapper check
(coverage.sonic-net.sonic-gnmi.build) match existing branch protection
rules.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
* Support maximum gnmi message size of 125k Signed-off-by: Connor Roos <croos@nvidia.com> * Correctly set default to 4MB Signed-off-by: Connor Roos <croos@nvidia.com> * Run gofmt on telemetry.go Signed-off-by: Connor Roos <croos@nvidia.com> --------- Signed-off-by: Connor Roos <croos@nvidia.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
The SmartSwitch upgrade workflow needs to verify the running OS version on individual DPUs after upgrade. OS.Verify and OS.Activate are already implemented in sonic-gnmi but not registered in the DPU proxy's forwardable methods list, so requests with DPU metadata headers are rejected. Add both RPCs as ForwardToDPU entries so external clients can call OS.Verify and OS.Activate targeting specific DPUs via the NPU proxy. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
Register OS.Verify and OS.Activate as ForwardToDPU methods in the DPU proxy, and implement the unary forwarding handlers so requests are actually proxied to the target DPU. Previously, only System.Time had a forwarding handler. Other unary methods registered as ForwardToDPU hit the default switch case and returned Unimplemented. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
Signed-off-by: Neha Das nehadas@google.com Signed-off-by: Neha Das nehadas@google.com
…(#673) * Revert "telemetry: add --bind_address flag to restrict TCP listener (#652)" This reverts commit 4cc592b. * telemetry: add test for cert auth fallback when ca_crt is empty Signed-off-by: Dawei Huang <daweihuang@microsoft.com> --------- Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…e.xml (#656)
* ci: surface gofmt failures instead of masking them as missing coverage.xml
A recent build produced a confusing "Path does not exist: coverage.xml"
failure whose real root cause was a gofmt violation. Three separate
issues combined to hide it:
1. In the integration and memory-leak jobs the bash step was written as:
pushd sonic-gnmi
make all && ... make check_*_junit
popd
bash's 'set -e' is explicitly disabled for commands that are not the
final one in an '&&' list, so a failing 'make all' did not abort the
step. The final command (popd) returned 0, so the task was reported
as succeeded even though nothing built. coverage.xml was never
generated, which in turn failed the downstream publish step with a
misleading error.
2. The 'Publish integration coverage artifact' step used
condition: always(), which guaranteed a secondary failure on top of
any real earlier failure and made the real error harder to find.
3. The .formatcheck Makefile target printed the entire list of
~170 unformatted files on a single line with no diff, which is hard
to read in the Azure DevOps log viewer.
Changes:
- Replace 'pushd; cmd1 && cmd2; popd' with plain 'cd; cmd1; cmd2' in
the integration and memory-leak bash steps so 'set -e' actually
aborts the step on first failure.
- Switch the integration coverage artifact publish step to
condition: succeeded() so it no longer emits a confusing secondary
error when tests have already failed.
- Add a 'Go format check (gofmt)' step to the fast PureCIJob (no SONiC
dependencies) so formatting issues fail the build in seconds instead
of being buried inside a long integration test step.
- Improve the .formatcheck target: print each offending file on its
own line, include the first 200 lines of 'gofmt -d' output, and
point users at a new top-level 'make fmt' target to auto-fix.
- Add convenience 'fmt-check' and 'fmt' phony targets.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: add StaticChecks stage that gates Build, BuildAmd64, BuildArm64
Before this change, a formatting violation still burned a full set of
integration-test and deb-build runs on the self-hosted amd64/arm64
pools (~30-40 min each, running in parallel) before the pipeline went
red. That is wasted agent time on a constrained pool.
Introduce a dedicated StaticChecks stage running on a hosted
ubuntu-22.04 image with no SONiC dependencies. It runs gofmt via the
'make fmt-check' target in about a minute from a cold agent. The
three downstream stages (Build, BuildAmd64, BuildArm64) now declare
'dependsOn: [StaticChecks]' so they do not even start if static
checks fail.
Remove the duplicate gofmt step that was added to PureCIJob in the
previous commit; the StaticChecks stage supersedes it.
The StaticChecks stage is intentionally scoped to be the home for
future cheap, dependency-free Go checks (vet, go mod tidy drift,
golangci-lint) so they can be added as additional steps in the same
job without restructuring the pipeline again.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* make: share one fmt-check/fmt implementation between Makefile and pure.mk
Previously the pure.mk `fmt-check` target loop-called `gofmt -l` over
PACKAGES (narrow scope, per-package output, no diff) while the
Makefile's `.formatcheck` stamp-file target used a different shell
block with a temp log file. Keeping two implementations in sync by
hand is a bug waiting to happen.
Make pure.mk the single source of truth:
- pure.mk defines GO_FILES with the same find invocation as the
Makefile and runs gofmt -l on that full set, printing the per-file
list plus the first 200 lines of gofmt -d.
- pure.mk 'fmt' runs 'gofmt -w' on the same GO_FILES set.
- The Makefile's phony 'fmt-check'/'fmt' targets now delegate to
'make -f pure.mk fmt-check' / 'make -f pure.mk fmt'.
- The Makefile's incremental $(FORMAT_CHECK) stamp-file target (still
a dependency of the main 'sonic-gnmi' build target) uses the same
shell block as pure.mk, so running the full build or the standalone
target produces identical output.
- Drop the unused FORMAT_LOG variable.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* make: revert cross-makefile delegation for fmt-check
Previous commit made the Makefile's fmt-check delegate to pure.mk.
That is wrong: Makefile and pure.mk own different scopes (full repo
vs. pure packages) by design, and neither should depend on the other.
Revert to self-contained targets in each file:
- Makefile's fmt-check/fmt target scope is the full-repo GO_FILES list
via the existing $(FORMAT_CHECK) stamp-file target.
- pure.mk's fmt-check/fmt target scope is the PACKAGES list only.
Both targets use the same shell idiom and produce the same output
format (per-file list followed by the first 200 lines of 'gofmt -d'
plus a fix-it hint), but they are independent.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: drop Makefile/pure.mk changes, inline gofmt in StaticChecks stage
The format check is a single 'gofmt -l' invocation; there is no need
for a new make target to wrap it. Revert Makefile and pure.mk to
their state on master and run gofmt inline from the StaticChecks
stage. This keeps build-system changes out of a CI-only fix.
The stage prints the list of offending files followed by the first
200 lines of 'gofmt -d' so a PR author can see exactly what needs
to change.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: build sonic-mgmt-common in MemoryLeakJob before 'make all'
Build 1094963 failed in the Memory Leak Tests step with:
No such file or directory: '.../sonic-mgmt-common/build/yang/*.yang'
Root cause: MemoryLeakJob checks out sonic-mgmt-common but never
builds it. sonic-gnmi's 'make all' invokes a pyang proto-gen target
(Makefile:178) that globs yang files from
$(MGMT_COMMON_DIR)/build/yang — a directory that only exists after
dpkg-buildpackage has run in sonic-mgmt-common. IntegrationTest
already has an explicit 'Build sonic-mgmt-common' step; MemoryLeakJob
was missing it.
This bug was previously hidden by the same 'make all && ... popd'
pattern this PR fixed: 'make all' failed, && short-circuited, popd
returned 0, the task was reported succeeded, and check_memleak_junit
silently never ran. Memory leak tests have been a no-op on CI. With
'set -e' now honored correctly, the missing build step became
visible.
Add the 'Build sonic-mgmt-common' step to MemoryLeakJob, identical
to the one already present in IntegrationTest.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: restructure azure-pipelines layout and factor shared templates
Rename and regroup stages/jobs so the layout matches what each piece
actually does, and extract the two biggest copy-paste blocks (Go install,
test-env preamble) into templates.
Stages:
Build -> Test (it runs tests, not a build)
BuildAmd64 -> Package (now a single stage with amd64 and arm64 jobs)
BuildArm64 -> Package
Jobs:
GoStaticChecks -> go_static_checks
PureCIJob -> pure_tests
MemoryLeakJob -> memleak_tests
IntegrationTest -> integration_tests
The 'build' job inside the Test stage is intentionally left with both
'job: build' and 'displayName: "build"'. The pipeline decorator that
publishes the diff-coverage check derives its GitHub name as
coverage.{BuildDefinitionName}.{Agent.JobName}, and Agent.JobName
resolves to the displayName. Renaming either would break the
coverage.sonic-net.sonic-gnmi.build check. Documented in the file
header.
Also:
* Drop the redundant 'Run Pure Package Tests (coverage only)' step
from the integration job; pure coverage is already produced by
pure_tests and consumed via the coverage-pure artifact.
* Hoist GO_VERSION to a pipeline-level variable.
* Factor .azure/templates/install-go.yml (was duplicated in two jobs).
* Factor .azure/templates/setup-test-env.yml (checkout x3 +
install-dependencies + build sonic-mgmt-common) used by both
memleak_tests and integration_tests.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add OS.Verify and OS.Activate DPU proxy forwarding Register OS.Verify and OS.Activate as ForwardToDPU methods in the DPU proxy, and implement the unary forwarding handlers so requests are actually proxied to the target DPU. Previously, only System.Time had a forwarding handler. Other unary methods registered as ForwardToDPU hit the default switch case and returned Unimplemented. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> * Add completeness test for DPU proxy ForwardToDPU handlers Add TestAllForwardToDPUMethodsHaveHandlers which validates that every method registered as ForwardToDPU in defaultForwardableMethods has a corresponding forwarding handler. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> --------- Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
…ters (#665)
Add special GET handling for two virtual path types in DPU_COUNTERS_DB:
ENI Counters (DPU_COUNTERS_DB/COUNTERS/ENI/<name>)
Maps ENI friendly names to OIDs via COUNTERS_ENI_NAME_MAP, following the existing COUNTERS_DB/COUNTERS/Ethernet* convention. Supports wildcard (all ENIs) and specific ENI name lookups.
DASH_METER (DPU_COUNTERS_DB/DASH_METER/<eni>/<class>)
Maps to JSON-formatted Redis keys: COUNTERS:{"eni_id":"<OID>","meter_class":"<class>","switch_id":"<switch_oid>"}
Supported operations:
GET on specific ENI + metering class ID
GET on specific ENI (returns all metering class IDs)
GET with wildcard ENI (returns all metering class IDs for all ENIs)
GET with wildcard ENI + specific class (returns that class for all ENIs)
Signed-off-by: Lawrence Lee <lawlee@microsoft.com>
Two related changes to make File.Put usable for SONiC upgrade staging
into the next-image overlay.
1) validatePath whitelist expansion (/host/ allowed)
The current allowedPrefixes = ["/tmp/", "/var/tmp/"] blocks any write
under /host/, including the legitimate next-image overlay staging path
/host/image-*/rw/etc/sonic/... The SONiC upgrade agent needs to put
configs/certs into that directory between InstallImage and Reboot, so
this commit appends "/host/" to the whitelist and updates the rejection
error message accordingly.
The whitelist is broad on purpose for the PoC: it also accepts
/host/grub/grub.cfg and /host/machine.conf. A follow-up will tighten the
prefix to /host/image-*/rw/ once callers stabilize. Mounted security is
still enforced at the FS layer — without the companion sonic-buildimage
PR ("[gnmi] mount /host rw into gnmi container") the gnmi container only
sees /host as read-only and writes return EROFS regardless of this
whitelist.
The leading rationale comment block is rewritten to match the new
behavior (the old block listed /host/ as forbidden which now contradicts
the code).
2) HandlePut mkdir-p parent dirs
HandlePut previously assumed the parent directory of the target path
already existed; otherwise os.OpenFile(tempPath, ...) failed with
Internal. That forced every gNOI client to pre-create parent dirs via
an out-of-band SSH mkdir, which defeats the purpose of File.Put as a
self-contained upload primitive — especially painful for the upgrade
agent because /host/image-{next}/rw/etc/sonic/credentials/ usually
doesn't exist yet on a freshly-installed next image.
This commit inserts os.MkdirAll(filepath.Dir(tempPath), 0755) right
before the os.OpenFile call, after validatePath (Step 2) and
translatePathForContainer (Step 3), so no new path can be reached that
validatePath wouldn't already accept. Test coverage:
- TestHandlePut_CreatesParentDir asserts mkdir-p happens on a fresh
deeply-nested /tmp/<uniq>/sub/dir/file.txt path.
- TestHandlePut_ErrorPaths/"parent dir creation failure" was rewritten
to provoke an actual MkdirAll failure (a regular file planted where
a parent dir would need to be), replacing the previous test which
relied on the now-eliminated missing-parent failure path.
Whitelist tests updated: /host/grub/grub.cfg, /host/machine.conf and
/host/image-master/rw/usr/bin/test moved from the rejection table to
the allowed-paths table (with an explicit NOTE about the broad prefix);
a new positive case covers /host/image-internal.164866913-.../rw/etc/sonic/minigraph.xml;
a /host traversal case (/host/image-foo/../../etc/passwd) verifies
filepath.Clean still resolves the traversal before the whitelist check.
Signed-off-by: Dawei Huang <hdwhdw@users.noreply.github.com>
Co-authored-by: Dawei Huang <hdwhdw@users.noreply.github.com>
…(#679) * [gnoi] SetPackage: auto-resolve version from image when not provided When the caller does not provide the version (to_version) field in the SetPackage request, instead of failing with an error, the API now automatically resolves the version by running 'sonic-installer binary_version <filename>' on the installed image. This allows hwproxy and other callers to omit the version field while still getting proper activation (set-default) behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ryanzhu <ryanzhu@microsoft.com> * [gnoi] SetPackage: resolve version before install, fix test coverage Restructure HandleSetPackage to validate and resolve the version from the image binary BEFORE performing install. This ensures we fail early without side effects if version resolution fails. Order is now: validate inputs -> resolve version -> install -> activate Also addresses PR review comments: - Fix args bounds check (len >= 2) before accessing args[1] - Add test for Version='' + Activate=false (install-only) - Update test names to accurately reflect scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ryanzhu <ryanzhu@microsoft.com> --------- Signed-off-by: ryanzhu <ryanzhu@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Leyza <wangnina@microsoft.com>
Signed-off-by: Pattela JAYARAGINI <pattelaj@google.com> Signed-off-by: Niranjani Vivek <niranjaniv@google.com> Co-authored-by: Neha Das <nehadas@google.com>
Signed-off-by: Feng Pan <fenpan@microsoft.com>
…ENI coun…" (#686) This reverts commit 4cd908e. Reverts #665 to unblock sonic-gnmi submodule advance PR
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // RotateAccountCredentials implements corresponding RPC | ||
| func (srv *GNSICredentialzServer) RotateAccountCredentials(stream credz.Credentialz_RotateAccountCredentialsServer) error { | ||
| ctx := stream.Context() | ||
| ctx, err := authenticate(srv.config, ctx, "gnsi", false) |
Sreevanich16
changed the title
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…(#675)
* MixedDbClient: batch ZMQ Set/Del for multi-key gNMI updates
Why:
The current handleTableData loops DbSetTable/DbDelTable per key, so a
single gNMI Set carrying N keys to a DASH_* table generates N ZMQ
round-trips even though the messages all target the same producer.
At the scale DASH route programming requires (10k-100k entries per
update), the per-key overhead dominates wall-clock time.
What:
* Add DbBatchSetTable(table, []tableBatchEntry) and DbBatchDelTable(
table, []string) on MixedDbClient. When the table is ZMQ-backed
these collapse the batch into a single ZmqProducerBatchedSet /
ZmqProducerBatchedDel call (one ZMQ message). For plain Table or
non-ZMQ ProducerStateTable backends — where no batched C++ API
exists — they fall back to the existing per-entry loop, so behavior
for DASH_HA_* tables and ordinary CONFIG_DB writes is unchanged.
* Wire the multi-key code paths in handleTableData to the new
helpers: the opRemove branch and the multi-key JSON opAdd branch
now build a slice and make one batched call. Single-key paths are
untouched (batching one entry is pointless).
* Wrap the new SWIG entry points with ZmqProducerBatched{Set,Del}Wrapper
to convert C++ exceptions into Go errors, mirroring the existing
ProducerStateTable wrappers.
Tests:
client_test.go adds:
* TestZmqBatchedSetEndToEnd / TestZmqBatchedDelEndToEnd — round-trip
N entries through a real ZmqProducerStateTable + ZmqConsumerStateTable
and verify the consumer drains them all.
* TestDbBatchSetTableFallsBackForPlainTable / ...EmptyAndSingle —
verify the plainTableMap fallback path and empty/single-entry
shortcuts.
* TestHandleTableDataBatchesMultiKeyJSON / ...OpRemove — gomonkey-
patched proof that handleTableData routes through DbBatchSetTable /
DbBatchDelTable exactly once for multi-key payloads instead of N
per-key calls.
Related:
* sonic-net/sonic-buildimage#27250 — tracking issue for the
optimization.
* Depends on sonic-net/sonic-swss-common#1188, which exposes
ZmqProducerBatchedSet/Del/Send to Go via SWIG %inline helpers.
Until that PR lands and a fresh libswsscommon artifact is
published, sonic-gnmi CI on this branch will fail to build — the
Go bindings for the new symbols do not exist in the current
master swsscommon artifact.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* MixedDbClient tests: harden batch tests against global-state ordering
Why:
Copilot review on PR #675 flagged four genuine test-hygiene issues in
the new batched-Set/Del tests. All four come from the tests touching
package-level globals (RedisDbMap, zmqClientMap) without going through
the existing helpers, which makes them order-dependent and racy with
other tests that explicitly nil out RedisDbMap.
What:
* TestZmqBatchedSetEndToEnd / TestZmqBatchedDelEndToEnd: the cleanup
loop freed every SWIG handle in zmqClientMap but left the (now
dangling) entries in the map. A later test that ranges zmqClientMap
would double-DeleteZmqClient the same pointer, which on CGO is a
real crash. Delete the map entry alongside the handle.
* TestHandleTableDataBatchesMultiKeyJSON /
TestHandleTableDataBatchesOpRemove: replace the ad-hoc
RedisDbMap[...] = redis.NewClient(...) with the existing
setupMixedDbRedis(t, mapkey) helper, which save/restores RedisDbMap
via Target2RedisDb. This stops the tests from panicking with
'assignment to entry in nil map' when they run after any test that
sets RedisDbMap = nil (e.g. the TestUseRedis* family at lines
~490/521/537/553).
No production code changes; behavior of the batched Set/Del path is
unchanged.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: install libyang3 runtime alongside libyang 1.x
sonic-swss-common master switched to libyang3 (>= 3.12.2) in commit
1431c3d3 ('port to libyang3 with libyang1 fallback') and the published
libswsscommon Debian artifact now hard-depends on the libyang3 package.
The sonic-gnmi pipeline only downloads libyang 1.0 from common_libs, so
'dpkg -i libswsscommon_1.0.0_amd64.deb' fails with:
dpkg: dependency problems prevent configuration of libswsscommon:
libswsscommon depends on libyang3 (>= 3.12.2); however:
Package libyang3 is not installed.
This breaks every sonic-gnmi PR build from #1188 onward.
Add libyang3_*.deb to the download pattern. libyang 1.x is still needed
because sonic-mgmt-common/cvl/internal/util/util.go cgo-links '-lyang'
against the v1 headers; the two runtimes coexist as distinct SONAMEs.
Carried in this PR because it is also the unblock for #675's own CI.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* MixedDbClient tests: drop bogus zmqClientMap nuke loop from new batch tests
The new TestZmqBatched{Set,Del}EndToEnd tests construct their ZmqClient
directly with swsscommon.NewZmqClient(zmqAddr), so they do not register
anything in the package-level zmqClientMap (only getZmqClientByAddress
does). The cleanup loop that ranged over zmqClientMap and called
DeleteZmqClient was therefore freeing pointers that belong to *other*
tests.
In source order, TestZmqReconnect (~line 657) and TestRemoveZmqClient
(~line 823) run first; both already do the same broken
'for _, c := range zmqClientMap { DeleteZmqClient(c) }' pattern but
without deleting from the map. By the time the new batch tests run,
zmqClientMap still holds entries that point at already-freed C++
objects. Iterating those entries triggered:
double free or corruption (out)
SIGABRT: abort
signal arrived during cgo execution
...DeleteZmqClient...
client_test.go:2173
in CI as soon as the build actually linked successfully (build 1125898).
The previous commit (5b770fb) added delete(zmqClientMap, k) on Copilot's
advice, which fixes the in-loop iterator-mutation hazard but does not
help here because the bad entries are inserted by prior tests, not by
ours. The right fix for the new tests is to not touch zmqClientMap at
all — the explicit DeleteZmqClient(zmqClient) is sufficient.
Pre-existing tests are left alone; their map-nuke pattern is latent
and only matters if any later test also iterates zmqClientMap. After
this change none do.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* MixedDbClient: log batch size when sending batched ZMQ messages
Adds a V(2) log line in DbBatchSetTable and DbBatchDelTable on the
ZMQ-backed path so the actual batch size landing in a single ZMQ
message is visible when debugging. Matches the verbosity used by the
surrounding 'Create ZmqProducerStateTable' logs and stays quiet at
default verbosity.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sharmavijay-ms
approved these changes
Jun 3, 2026
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
16 participants
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
To bring latest public sonic-gnmi master changes into Azure/sonic-gnmi.msft:kubesonic
How I did it
Merged all the latest commits from public gnmi repo into sonic-gnmi.msft and resolved merge conflicts in the below files manually.
.azure/templates/build-deb.yml
Makefile
azure-pipelines.yml
gnmi_server/cli_helpers_test.go
gnmi_server/interface_cli_test.go
gnmi_server/poll_mode_test.go
gnmi_server/server.go
gnmi_server/virtual_db_test.go
go.mod
go.sum
show_client/interface_cli.go
sonic_data_client/virtual_db.go
sonic_data_client/virtual_db_test.go
How to verify it
Build succeeded with the latest commits.
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)