Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic at gosnappi.(*deviceBgpRouter).RouterId if BGP is not present in the configuration #213

Closed
bortok opened this issue Oct 22, 2023 · 2 comments

Comments

@bortok
Copy link

bortok commented Oct 22, 2023

After upgrading to gosnappi:0.13.0 the following code of otgen (run.go) causes panic:

		for _, d := range config.Devices().Items() {
			if d.Bgp().RouterId() != "" {}

encountered with the following OTG configuration otg.panic.yml.txt – it doesn't have BGP configuration, and the code above is trying to detect if BGP is configured

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa8e868]

goroutine 1 [running]:
github.com/open-traffic-generator/snappi/gosnappi.(*deviceBgpRouter).RouterId(0xc0006a2bb0?)
	/home/parallels/go/pkg/mod/github.com/open-traffic-generator/snappi/gosnappi@v0.13.0/gosnappi.go:26718 +0x8
github.com/open-traffic-generator/otgen/cmd.startProtocols({0x15ecc00, 0xc00009c900}, {0x15df7b0, 0xc000259d40})
	/home/parallels/ix/otg/code/otgen/cmd/run.go:228 +0x342
github.com/open-traffic-generator/otgen/cmd.glob..func13(0xc0001cec00?, {0x1435733?, 0x4?, 0x1435737?})
	/home/parallels/ix/otg/code/otgen/cmd/run.go:70 +0x53
github.com/spf13/cobra.(*Command).execute(0x21e6180, {0xc0000a6780, 0x5, 0x5})
	/home/parallels/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:944 +0x863
github.com/spf13/cobra.(*Command).ExecuteC(0x21e5ea0)
	/home/parallels/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
	/home/parallels/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:992
github.com/open-traffic-generator/otgen/cmd.Execute()
	/home/parallels/ix/otg/code/otgen/cmd/root.go:57 +0x1a
main.main()
	/home/parallels/ix/otg/code/otgen/main.go:27 +0xf

See the CI test run for details https://github.com/open-traffic-generator/otgen/actions/runs/6601539315/job/17933084738

@bortok bortok changed the title Panic at gosnappi.(*deviceBgpRouter).RouterId Panic at gosnappi.(*deviceBgpRouter).RouterId when location of the PE is not specified Oct 22, 2023
@bortok bortok changed the title Panic at gosnappi.(*deviceBgpRouter).RouterId when location of the PE is not specified Panic at gosnappi.(*deviceBgpRouter).RouterId Oct 22, 2023
@bortok bortok changed the title Panic at gosnappi.(*deviceBgpRouter).RouterId Panic at gosnappi.(*deviceBgpRouter).RouterId if BGP is not present in the configuration Oct 22, 2023
bortok added a commit to open-traffic-generator/otgen that referenced this issue Oct 23, 2023
@Vibaswan
Copy link
Contributor

Vibaswan commented Dec 14, 2023

Hi @bortok
basically we made a change in openapiart that all the fields in proto will be made optional,
#212
This helps in validating required fields more efficiently as earlier required fields for primitive types were difficult to check
for example lets a and required field of integer
as we know by default the integer in gosnappi is assigned a value of zero now in order for us to check if user has set a value zero to this field it becomes difficult for us to know whether the required integer field was set by user or not

That's why accessing router id without setting it results in an error as accessing nil pointer, User get to know this cause SDK throws an error if it is not set.

I guess a better way of checking would be this

         for _, d := range config.Devices().Items() {
   		if d.HasBgp() {}
         }

we were also thinking to introduce has method for every field
i.e to have HasRouterId() for which we have raised a PR (currently in review with @ashutshkumr) but we have not decided on it yet open-traffic-generator/openapiart#454

         for _, d := range config.Devices().Items() {
   		if d.Bgp().HasRouterId() != nil {} // as router id is now a string pointer and not a string
          }

@bortok
Copy link
Author

bortok commented Dec 19, 2023

Thanks, confirming the fix works

@bortok bortok closed this as completed Dec 19, 2023
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

No branches or pull requests

2 participants