Skip to content

Inttest#330

Draft
thesrinath wants to merge 6 commits into
openconfig:mainfrom
thesrinath:inttest
Draft

Inttest#330
thesrinath wants to merge 6 commits into
openconfig:mainfrom
thesrinath:inttest

Conversation

@thesrinath

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a monolithic SUT emulator for Bootz, integrating DHCP, HTTP, and Bootz controller services along with Dockerfiles, Kubernetes manifests, and Monax-based integration tests. Key feedback points identify critical issues including a potential channel-closing panic during controller shutdown, a path traversal vulnerability in the HTTP SUT image upload path, a data race when starting the Bootz server before linking its notification function, a potential nil-pointer panic with invalid CIDR mask lengths in the DHCP SUT, and a line-continuation syntax error in the Kubernetes test script.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +67 to +72
s.mu.Lock()
defer s.mu.Unlock()
for _, ch := range s.subscribers {
close(ch)
}
s.subscribers = nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

In Stop(), the server manually closes all subscriber channels. However, the Subscribe() function also has a deferred cleanup block that closes the same channel ch. In Go, closing an already closed channel causes a panic (panic: close of closed channel).

Since GracefulStop() will automatically cancel the contexts of all active streams (which triggers the stream.Context().Done() case in Subscribe() and runs the deferred cleanup), there is no need to manually close the channels here. Removing this block prevents a critical panic during shutdown.

Suggested change
s.mu.Lock()
defer s.mu.Unlock()
for _, ch := range s.subscribers {
close(ch)
}
s.subscribers = nil
// GracefulStop will cancel the contexts of all active streams,
// which triggers the cleanup defer in Subscribe() to safely close each channel.

Comment on lines +84 to +85
filename := fmt.Sprintf("%s-%s", img.GetName(), img.GetVersion())
localPath := filepath.Join(s.folder, filename)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

The Upload function constructs filename directly from img.GetName() and img.GetVersion(), and then joins it with s.folder using filepath.Join. If the name or version contains path traversal sequences (e.g., ../), this can lead to an arbitrary file write (Path Traversal) vulnerability outside of the intended directory.

Using filepath.Base on the constructed filename ensures that any directory traversal components are stripped, keeping the file write safely within s.folder.

Suggested change
filename := fmt.Sprintf("%s-%s", img.GetName(), img.GetVersion())
localPath := filepath.Join(s.folder, filename)
filename := filepath.Base(fmt.Sprintf("%s-%s", img.GetName(), img.GetVersion()))
localPath := filepath.Join(s.folder, filename)

Comment thread server/sut/main.go
Comment on lines +110 to +114
go func() {
if err := bootzServer.Start(); err != nil {
log.Errorf("Bootz server failed: %v", err)
}
}()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Starting the bootzServer in a background goroutine here creates a data race on s.NotifyFn because SetNotifyFn is called later on line 130. To prevent this race condition, the server should be started only after all configuration and linking (such as SetNotifyFn) have been completed.

	// Start the Bootz server later, after linking it with the controller to avoid a data race.

Comment thread server/sut/main.go
Comment on lines +130 to +131
bootzServer.SetNotifyFn(controllerServer.NotifyEvent)
log.Infof("Linked Bootz server and Controller")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

To resolve the data race on s.NotifyFn, start the bootzServer here, immediately after linking it with the controller's notification function.

Suggested change
bootzServer.SetNotifyFn(controllerServer.NotifyEvent)
log.Infof("Linked Bootz server and Controller")
bootzServer.SetNotifyFn(controllerServer.NotifyEvent)
log.Infof("Linked Bootz server and Controller")
go func() {
if err := bootzServer.Start(); err != nil {
log.Errorf("Bootz server failed: %v", err)
}
}()
log.Infof("Bootz server started on %s", *bootzAddr)

Comment on lines +75 to +78
mask := net.CIDRMask(int(req.GetMaskLen()), ip.BitLen())

for _, mac := range req.GetMacAddresses() {
slease.AddRecord4(mac, net.ParseIP(req.GetIpAddress()), mask, gw)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

net.CIDRMask returns nil if the mask length is invalid (e.g., negative or greater than the bit length). If mask is nil, passing it to slease.AddRecord4 can cause panics or unexpected behavior in the DHCP server. Please validate that mask is not nil before proceeding.

	mask := net.CIDRMask(int(req.GetMaskLen()), ip.BitLen())
	if mask == nil {
		return nil, fmt.Errorf("invalid mask length %d for IP bit length %d", req.GetMaskLen(), ip.BitLen())
	}

	for _, mac := range req.GetMacAddresses() {
		slease.AddRecord4(mac, net.ParseIP(req.GetIpAddress()), mask, gw)

Comment on lines +49 to +50
--runtime_parameters="${BOOTZ_DIR}/server/tests/monax_integration/kubernetes_runtime_parame
ters.txtpb" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The --runtime_parameters argument is split across two lines without a backslash or proper continuation. Because the double quote is not closed on line 49, Bash will treat the newline as part of the string, resulting in a literal newline in the filename (e.g., kubernetes_runtime_parame\nters.txtpb), which will cause the script to fail to find the file. Please join these lines.

Suggested change
--runtime_parameters="${BOOTZ_DIR}/server/tests/monax_integration/kubernetes_runtime_parame
ters.txtpb" \
--runtime_parameters="${BOOTZ_DIR}/server/tests/monax_integration/kubernetes_runtime_parameters.txtpb" \

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.

1 participant