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

cni: prevent NPE if no interface has sandbox field set #9648

Merged
merged 3 commits into from
Dec 16, 2020
Merged

Conversation

tgross
Copy link
Member

@tgross tgross commented Dec 16, 2020

Fixes #9647

When we iterate over the interfaces returned from CNI setup, we filter for one
with the Sandbox field set. Ensure that if none of the interfaces has that
field set that we still return an available interface.

When we iterate over the interfaces returned from CNI setup, we filter for one
with the `Sandbox` field set. Ensure that if none of the interfaces has that
field set that we still return an available interface.
@tgross tgross added this to the 1.0.1 milestone Dec 16, 2020
client/allocrunner/networking_cni.go Outdated Show resolved Hide resolved
// interface has it set
iface, name := func(r *cni.CNIResult) (iface *cni.Config, name string) {
for name, iface = range r.Interfaces {
if iface.Sandbox != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Questions to clarify later: should we aim for a deterministic arbitration when multiple interfaces are present? Also, Is it possible to have multiple interfaces with Sandboxes set?

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 16, 2020 14:16 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 16, 2020 14:16 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 16, 2020 14:32 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 16, 2020 14:32 Inactive
@tgross
Copy link
Member Author

tgross commented Dec 16, 2020

Added changelog entry.

return nil, ""
}(res)
}
if iface == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would iface be nil here? My impression is the original bug is from iface never being set; here it is being set in the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this isn't for protecting from the original bug, but for hopefully-unlikely situations from third-party plugins like:

res.Interfaces = map[string]*cni.Config{"foo": nil}

Copy link
Member

Choose a reason for hiding this comment

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

Oooh I see, makes sense with the your comment above, thanks!

@tgross tgross merged commit 463c304 into master Dec 16, 2020
@tgross tgross deleted the b-cni-setup-npe branch December 16, 2020 15:36
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic on nil pointer dereference using CNI with Calico
3 participants