Assigns default=true on a multiple interface return for first interface with a gateway

[dougbtv]

This is necessary especially under crio, as you may not want to present the default gateway interfaces IP address as the first CNI response, as crio utilizes that IP address as the PodIP, so you may need flexibility in expression of which interface should be presented first, vs. another interface which should be the default gateway (which we mark as default=true in the network-status)

[trozet]

/lgtm

[maiqueb]

Some nits ...

I am a bit concerned about scenarios where CNIs are not setting the Interface attribute in their result.IPs, but nothing we can do about that, other than push for them to comply with the CNI spec.

[maiqueb]

nit: maybe, for readability, you should export this into an helper ?

Something like:

Suggested change

	// Check for a gateway-associated interface, we'll use this later if we did to mark as the default.
	gatewayInterfaceIndex := -1
	if defaultNetwork {
	for _, ipConfig := range result.IPs {
	if ipConfig.Gateway != nil && ipConfig.Interface != nil {
	// Keep the index of the first interface that has a gateway
	gatewayInterfaceIndex = *ipConfig.Interface
	break
	}
	}
	}
	if defaultNetwork {
	gatewayIfaceIndex := gatewayInterfaceIndex(result.IPs)
	}

And the helper would be something like:

func gatewayInterfaceIndex(ips IPs) int {
        for _, ipConfig := range ips {
			if ipConfig.Gateway != nil && ipConfig.Interface != nil {
				return *ipConfig.Interface
			}
		}
	return -1
}

I do wonder about CNIs that don't bother setting the Interface attribute for IPs in the CNI result ...

We would discard them; is that OK ? I think so - we can always open a bug on them to honor the CNI spec.

[dougbtv]

re:

Totally hear that concern, I think that we were kind of in that situation previously too with the existing:

for i, iface := range result.Interfaces {

Buuuut... You're right. There's a sticky situation here with those in that if you don't set any interfaces, you don't have anything to index in the IPs either, so we can't determine if there's a gateway, and we can't determine if it's the first sandbox interface either. Sooo... I will try to do my best to explain what needs to be done in the release note too, and potentially update the language in the spec update, too.

Helper function++, can't hurt to break it out, makes this block easier to read for the main functionality.

[maiqueb]

nit: I feel the comment is a bit redundant

[dougbtv]

+1

[maiqueb]

shouldn't we also enable the foundFirstSandboxIface flag ?

I know we're doing it later on, in line 202, but I think it would be more readable if we did it in this code block.

[dougbtv]

Yeah this got spread out huh, good eye, agreed.

[maiqueb]

Thanks !