-
Notifications
You must be signed in to change notification settings - Fork 38
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
Adds CreateNetworkStatuses for CNI results with multiple pod interfaces #68
Adds CreateNetworkStatuses for CNI results with multiple pod interfaces #68
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits.
Thanks for the unit tests; they are very readable.
pkg/utils/net-attach-def.go
Outdated
} | ||
|
||
// Discover default routes upfront and reuse them if necessary. | ||
var usegateway []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe name this defaultRoutes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, cool, I like it, I'm going with useDefaultRoute
as I'm setting it to be used in the loop following it (instead of calculating it there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the reasoning :)
Sorry. But again, this is just a nit you can ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm thinking it's because you "use" the same default route across interfaces in these CNI results, they apply to all the interfaces in the result, so you wind up re-using them. So, I'm going with the half/half, half of what I started with, half what you suggested haha
pkg/utils/net-attach-def.go
Outdated
} | ||
|
||
// Map IPs to network interface based on index | ||
for _, ipconfig := range result.IPs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's be consistent and use camel case everywhere
for _, ipconfig := range result.IPs { | |
for _, ipConfig := range result.IPs { |
pkg/utils/net-attach-def.go
Outdated
v1dns := convertDNS(result.DNS) | ||
|
||
// Initialize NetworkStatus for each container interface (e.g. with sandbox present) | ||
keepidx := 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we must rename this variable. I have no clue what is it supposed to track.
Is it supposed to track the number of interfaces on the pod ? (i.e. the ones w/ sandbox defined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good suggestion -- this is the trickiest bit of code, I renamed it to indexOfFoundPodInterface
because it's this thing where we have to map the interfaces that we keep (pod interfaces) to the index as in the CNI result
…multiple pod interfaces
1204ccc
to
38ceddc
Compare
Review super appreciated! New commit up, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Essentially, the gist is that the current singular
CreateNetworkStatus
method isn't aware of CNI results that have multiple pod interfaces returned.This approach adds a new method (to not break current implementations, as it needs to return a list instead of a single Network Status)
With the existing method, this is somewhat buggy. If multiple pod interfaces are present, it returns the last one -- and groups all of the IP addresses to that address.
This method returns a list, still limited to interface results that have the sandbox present, and it maps the IPs to the interface using the
ips.interface
index property in the CNI result.From the spec (in ADD Success):
For replication of the error yourself, and to see the fix in action, see this gist: https://gist.github.com/dougbtv/1eb8ac2d61d494b56d65a6b236a86e61
For the related suggested update to the NPWG net-attach-def specification, see this proposal @ https://docs.google.com/document/d/1DUTV-o6E6zlRTKZkxeDhAeyGrmq03qPgPbU580Rm7-g/edit