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: add fallback logic if no ip address references sandboxed interface #9895

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

nickethier
Copy link
Member

This PR adds a branch of logic to how the alloc's IP address is discovered for CNI networks. Some CNI plugins don't reference an interface index in the result response. This leads to go-cni, the library we use for CNI, to associate the address with the default interface.

This logic will attempt to cycle through the interfaces and find the first address if there is no address found for the sandboxed interface.

Some additional debug log statements are also included to aid future CNI debugging.

@nickethier nickethier self-assigned this Jan 26, 2021
Base automatically changed from master to main March 8, 2021 19:25
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Is it possible to unit test this? Maybe split the cni.Setup code and the address-finding-code into separate funcs as the latter seems unittestable. I'm actually less worried about correctness than I am about having a test as a sort of documentation for intended behavior. The "use first IP address we find" seems like strange behavior to me as someone who is not familiar with CNI plugin behavior.

client/allocrunner/networking_cni.go Show resolved Hide resolved
client/allocrunner/networking_cni.go Outdated Show resolved Hide resolved
The goal is to always find an interface with an address, preferring
sandbox interfaces, but falling back to the first address found.

A test was added against a known CNI plugin output that was not handled
correctly before.
@schmichael
Copy link
Member

schmichael commented Apr 8, 2021

I refactored the CNIResult -> AllocNetworkStatus conversion into a new func for unit testing. I'd love to test any more configurations if you have them handy!

Sadly its still possible to get nondeterministic results from this new func, but I think the interface precedence logic is correct.

I think I may error in more cases than before.

Before we would only error if CNIResults.Interfaces = map[string]*Interface{"some name": nil}.

Now we also error if there are no interfaces with an IP address set. I think that was the intention of the original code, but not the actual behavior.

@nickethier
Copy link
Member Author

Thanks for digging into this @schmichael I approve of these changes (even though I can't approve my own PR).

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Approving my own changes since @nickethier can't approve his own PR. 😅

@schmichael schmichael merged commit a4622e6 into main Apr 9, 2021
@schmichael schmichael deleted the b-cni-ipaddr branch April 9, 2021 15:58
schmichael added a commit that referenced this pull request Apr 15, 2021
schmichael added a commit that referenced this pull request Apr 30, 2021
schmichael added a commit that referenced this pull request Apr 30, 2021
schmichael added a commit that referenced this pull request May 14, 2021
CNI: add fallback logic if no ip address references sandboxed interface
@tgross tgross added this to the 1.1.0 milestone May 17, 2021
@github-actions
Copy link

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 Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants