Enhance ExternalIPPool validation

[aroradaman]

• Validate if IPRange.Start <= IPRange.End
• Validate if IPRange.Start and IPRange.End belongs to the same IP family
• Validate if IPRanges don't overlap with each other

Fixes: #5842

[aroradaman]

I initially planned to use netip.Addr and netip.Prefix instead of net.IP and net.IPNet as netip.Addr is directly comparable.
In general netip is more efficient than net but it would require to rewriteIPNetContains for netip.Prefix. Maybe in future, we can consider this 🤔

antrea/pkg/util/ip/ip.go

Line 231 in ba57a7f

func IPNetContains(ipNet1, ipNet2 *net.IPNet) bool {

[aroradaman]

cc @tnqn

[tnqn]

The 3rd item of the issue originally means "All IPRanges of ExternalIPPools must not overlap", not only IP ranges of a single ExternalIPPool, because the datapath wouldn't be working if it's configured so. I guess it's hard to strictly prevent it because externalIPPoolLister may be not in sync with actual objects in kube-apiserver, but it should be able to prevent most configuration errors and still better than not checking at all, so could you also checking overlap with ranges in ExternalIPPoolController.externalIPPoolLister?

[tnqn]

3 e2e tests failed at TestEgress. There seemed to be a panic when calling the changed webhook:
https://github.com/antrea-io/antrea/actions/runs/7687867552/job/20948603505?pr=5898

[tnqn]

3 e2e tests failed at TestEgress. There seemed to be a panic when calling the changed webhook:
https://github.com/antrea-io/antrea/actions/runs/7687867552/job/20948603505?pr=5898

It was because c.externalIPPoolController below is nil. It worked previously because it never relies on any member variables to do the validation. You need to change NewConfig to pass the externalIPPoolController.

antrea/pkg/apiserver/apiserver.go

Line 330 in 025c3f6

s.Handler.NonGoRestfulMux.HandleFunc("/validate/externalippool", webhook.HandlerForValidateFunc(c.externalIPPoolController.ValidateExternalIPPool))

[aroradaman]

3 e2e tests failed at TestEgress. There seemed to be a panic when calling the changed webhook:
https://github.com/antrea-io/antrea/actions/runs/7687867552/job/20948603505?pr=5898

It was because c.externalIPPoolController below is nil. It worked previously because it never relies on any member variables to do the validation. You need to change NewConfig to pass the externalIPPoolController.

antrea/pkg/apiserver/apiserver.go

Line 330 in 025c3f6

s.Handler.NonGoRestfulMux.HandleFunc("/validate/externalippool", webhook.HandlerForValidateFunc(c.externalIPPoolController.ValidateExternalIPPool))

yeah, figured that. I was just about to push the commit fixing that.

[tnqn]

LGTM, thanks @aroradaman

[tnqn]

/skip-all

[tnqn]

/test-ipv6-e2e
/test-ipv6-only-e2e

[tnqn]

@aroradaman e2e test failed when it tried to update an externalIPPool. The combinedRanges should exclude the existing ip ranges of the pool being update.

[aroradaman]

@aroradaman e2e test failed when it tried to update an externalIPPool. The combinedRanges should exclude the existing ip ranges of the pool being update.

I added a unit test to update 10.10.10.0/24 range with 10.10.0.0/16 to check the same but it failed. Right now getIPRangeSet makes a string/set comparison, should I enhance this validation as well?

Assuming an update from 10.10.10.0/24 to 10.10.0.0/16 should be allowed as no range is deleted.

[tnqn]

@aroradaman e2e test failed when it tried to update an externalIPPool. The combinedRanges should exclude the existing ip ranges of the pool being update.

I added a unit test to update 10.10.10.0/24 range with 10.10.0.0/16 to check the same but it failed. Right now getIPRangeSet makes a string/set comparison, should I enhance this validation as well?

Assuming an update from 10.10.10.0/24 to 10.10.0.0/16 should be allowed as no range is deleted.

That's another story and may require more changes in ip allocators as well. It's better to focus on the 3 validations in this PR.

[aroradaman]

/test-ipv6-e2e
/test-ipv6-only-e2e

[tnqn]

LGTM

[tnqn]

/skip-all