Skip to content

Panic on GNMI delete with union leafs#167

Merged
bstoll merged 7 commits intoopenconfig:mainfrom
bstoll:panic
Oct 16, 2025
Merged

Panic on GNMI delete with union leafs#167
bstoll merged 7 commits intoopenconfig:mainfrom
bstoll:panic

Conversation

@bstoll
Copy link
Copy Markdown
Contributor

@bstoll bstoll commented Oct 1, 2025

We have seen some Ondatra tests failing with a backtrace roughly similar to the test case being added to ygnmi/ygnmi_test.go:

panic: reflect: call of reflect.Value.IsZero on zero Value

goroutine 3310 [running]:
reflect.Value.IsZero({0x0?, 0x0?, 0x0?})
	/usr/local/google/home/bstoll/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.1.linux-amd64/src/reflect/value.go:1708 +0x66b
github.com/openconfig/ygnmi/internal/exampleocconfig/exampleocconfigpath.(*Component_TypePathAny).State.func1({0x11a16e0?, 0xc000bbbe00?})
	/usr/local/google/home/bstoll/Projects/ygnmi/internal/exampleocconfig/exampleocconfigpath/exampleocconfigpath.go:2708 +0x97
github.com/openconfig/ygnmi/ygnmi.(*baseQuery[...]).extract(0x1033f9b?, {0x11a16e0?, 0xc000bbbe00?})
	/usr/local/google/home/bstoll/Projects/ygnmi/ygnmi/types.go:227 +0xca
github.com/openconfig/ygnmi/ygnmi.unmarshalAndExtract[...]({0xc0009a5328, 0x1, 0x1}, {0x7f37ac0cb600, 0xc000d08bd0}, {0x11a16e0, 0xc000bbbe00}, 0xc000812c80)
	/usr/local/google/home/bstoll/Projects/ygnmi/ygnmi/unmarshal.go:210 +0x7f0
github.com/openconfig/ygnmi/ygnmi.WatchAll[...].func1()
	/usr/local/google/home/bstoll/Projects/ygnmi/ygnmi/ygnmi.go:660 +0x534
created by github.com/openconfig/ygnmi/ygnmi.WatchAll[...] in goroutine 3307
	/usr/local/google/home/bstoll/Projects/ygnmi/ygnmi/ygnmi.go:638 +0x3db
FAIL	github.com/openconfig/ygnmi/ygnmi	0.214s
FAIL

This PR modifies pathgen/gnmigen.go to check if the reflect value is valid before calling IsZero(). In the case of YANG union values, an uninitialized interface can be passed to reflect.ValueOf() which is one of the few cases it can return a zero value. This condition seems to exist only when a GNMI message is received that contains enough for a struct to be allocated but not enough for values to be populated (DELETE on a YANG keyed list).

This PR contains a lot of changes because I have included a test case in ygnmi/ygnmi_test.go that reproduces the panic condition. e2ba447 contains just the changes to trigger the panic on the existing code and 77928a6 includes the fix.

I am not sure if there is a better way to approach amending the test cases: I added the YANG modeling required to pathgen/testdata/yang/openconfig-unione.yang and added this YANG file to all of the gen.sh scripts. The remainder of the changes in the PR are generated code changes as a result of modifying the YANG testdata to support the conditions for this panic.

@bstoll bstoll requested a review from robshakir October 2, 2025 18:05
@robshakir
Copy link
Copy Markdown
Member

One way to avoid so much code change is to add a new test case based on a synthetic schema that does the minimum that you need (such that it can be added to a subset of the code generation). I think this then allows you to just regenerate the specific schema that you need, but might need an additional test.

Copy link
Copy Markdown
Member

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change -- I agree that there's a lot of code here :-). I added a comment that I think minimises this, but this looks a reasonable change.

I looked to see whether there was a useful helper function in ygot/util/reflect.go here -- since we have a number of cases where we have helpers that are structured to avoid panics, but I don't see a specific one for this case, which is essentially IsValidAndNotZero. If we expect that this is something that we'll check elsewhere, we could add it to that library (optional).

@bstoll bstoll merged commit b249dbf into openconfig:main Oct 16, 2025
12 checks passed
@bstoll bstoll deleted the panic branch October 17, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants