Multi policies ipamless ipblock

[maiqueb]

- What this PR does and why is it needed

This PR adds support for multi-network policies for secondary networks whose peers are of IPBlock type.

- Special notes for reviewers

- How to verify it

Run the multi-net tests.

- Description for the changelog

Add support for multi-network policies for secondary networks whose peers are of IPBlock type.

[maiqueb]

/cc @npinaeva

Don't bother looking yet, the production code implementation will change - I don't like the code I've added.

But still, this has an e2e test to assert the feature, and right now, I'm just pushing the simplest thing that works.

[maiqueb]

/cc @cathy-zhou

For your awareness. I plan on re-writing the code I'm adding to make it more readable, but for now I just want you to be able to track this work.

I'll let you know once this is ready for review.

Thanks in advance.

[maiqueb]

FWIW, the e2e test I've added to check multi-network policies for IPAM less networks using IPBlock peers is working.

[npinaeva]

some initial suggestions

[maiqueb]

0_o waaaat.

I guess I broke the PR when I tried to push a rebase + new code separately :S

[coveralls]

Coverage: 53.518% (+0.009%) from 53.51% when pulling 9cd62fb on maiqueb:multi-policies-ipamless-ipblock into bbb9d7a on ovn-org:master.

[jcaamano]

I think there is some of warnings we might want to get rid of.

Not sure we even want GetPodIPsOfNetwork to return an error, given that IPs are not mandatory:

ovn-kubernetes/go-controller/pkg/ovn/base_network_controller_namespace.go

Lines 362 to 366 in 0e395d5

	podIPs, err := util.GetPodIPsOfNetwork(pod, bnc.NetInfo)
	if err != nil {
	klog.Warningf(err.Error())
	continue
	}

Here neither the comment nor the returned error are very precise:

ovn-kubernetes/go-controller/pkg/util/pod_annotation.go

Lines 302 to 316 in 0e395d5

	// An OVN annotation should never have empty IPs, but just in case
	if len(ips) == 0 {
	klog.Warningf("No IPs found in existing OVN annotation for NAD %s! Pod Name: %s, Annotation: %#v",
	nadName, pod.Name, annotation)
	}
	}
	}
	if len(ips) != 0 {
	return ips, nil
	}
	if nInfo.IsSecondary() {
	return []net.IP{}, fmt.Errorf("no pod annotation of pod %s/%s found for network %s",
	pod.Namespace, pod.Name, nInfo.GetNetworkName())
	}

Again, here:

ovn-kubernetes/go-controller/pkg/util/pod_annotation.go

Lines 264 to 268 in 0e395d5

	if err != nil {
	// this should not happen;
	klog.Warningf("Failed to parse pod IP %v err: %v", podIP, err)
	continue
	}

[maiqueb]

I think there is some of warnings we might want to get rid of.

Not sure we even want GetPodIPsOfNetwork to return an error, given that IPs are not mandatory:

ovn-kubernetes/go-controller/pkg/ovn/base_network_controller_namespace.go

Lines 362 to 366 in 0e395d5

	podIPs, err := util.GetPodIPsOfNetwork(pod, bnc.NetInfo)
	if err != nil {
	klog.Warningf(err.Error())
	continue
	}

Here neither the comment nor the returned error are very precise:

ovn-kubernetes/go-controller/pkg/util/pod_annotation.go

Lines 302 to 316 in 0e395d5

	// An OVN annotation should never have empty IPs, but just in case
	if len(ips) == 0 {
	klog.Warningf("No IPs found in existing OVN annotation for NAD %s! Pod Name: %s, Annotation: %#v",
	nadName, pod.Name, annotation)
	}
	}
	}
	if len(ips) != 0 {
	return ips, nil
	}
	if nInfo.IsSecondary() {
	return []net.IP{}, fmt.Errorf("no pod annotation of pod %s/%s found for network %s",
	pod.Namespace, pod.Name, nInfo.GetNetworkName())
	}

Again, here:

ovn-kubernetes/go-controller/pkg/util/pod_annotation.go

Lines 264 to 268 in 0e395d5

	if err != nil {
	// this should not happen;
	klog.Warningf("Failed to parse pod IP %v err: %v", podIP, err)
	continue
	}

What if I do not even invoke util.GetPodIPsOfNetwork if the network does not have IPAM ?

It would reduce the code changes, while preventing all those misleading warnings from being printed.
@jcaamano

[maiqueb]

Added the simplest way out of this @jcaamano .

I can push a follow-up PR to fix the 3 utility funcs you have pointed out.

[npinaeva]

Very nice PR, thanks @maiqueb !

[maiqueb]

/retest-required

[jcaamano]

/retest-failed

[ovn-robot]

Oops, something went wrong:

This workflow run cannot be retried

[jcaamano]

/retest

[maiqueb]

rebasing in this forced push since it appears the bot is a bit incoherent today.

Hope now it goes back to work.

[maiqueb]

/retest-failed

[maiqueb]

/retest-failed

[maiqueb]

/retest-failed