Add ability to specify a list of ipBlocks in crd/v1alpha2 ClusterGroup

[Dyanngg]

This PR adds a new ipBlocks field for the ClusterGroup resource. It allows users to group a list of CIDRs in a ClusterGroup, instead of a single CIDR in the ipBlock field. For a single ClusterGroup, ipBlock and ipBlocks should not be set at the same time. Users can continue to create ClusterGroups with ipBlock. This field however will be deprecated in the next API upgrade for ClusterGroup. See #2008.

[codecov-io]

Codecov Report

Merging #1993 (d501b59) into main (117cd94) will increase coverage by 1.37%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   55.97%   57.35%   +1.37%     
==========================================
  Files         262      262              
  Lines       19468    19473       +5     
==========================================
+ Hits        10898    11168     +270     
+ Misses       7407     7126     -281     
- Partials     1163     1179      +16     

Flag	Coverage Δ
e2e-tests	25.39% <0.00%> (?)
kind-e2e-tests	42.54% <0.00%> (-0.03%)	⬇️
unit-tests	41.84% <42.30%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/networkpolicy/validate.go	0.00% <0.00%> (ø)
pkg/controller/types/group.go	0.00% <ø> (ø)
pkg/controller/networkpolicy/crd_utils.go	88.60% <33.33%> (ø)
pkg/controller/networkpolicy/clustergroup.go	65.13% <54.54%> (-1.07%)	⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go	72.14% <100.00%> (ø)
...ver/registry/controlplane/nodestatssummary/rest.go	50.00% <0.00%> (-50.00%)	⬇️
pkg/agent/stats/collector.go	94.61% <0.00%> (-3.85%)	⬇️
pkg/monitor/controller.go	35.07% <0.00%> (-2.99%)	⬇️
pkg/agent/controller/traceflow/packetin.go	54.35% <0.00%> (-1.03%)	⬇️
pkg/agent/cniserver/server.go	68.58% <0.00%> (+0.32%)	⬆️
... and 20 more

[antoninbas]

Typically with an API change such as this one, the server needs to ensure that the first member of the new ipBlocks field matches ipBlock. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility. The rationale for this is that a CG created by a new client that includes a single IPBlock (using ipBlocks) can still be accessed by older clients. Similarly, CG objects created by an older client (with ipBlock) can be accessed by a new client which only looks at the new ipBlocks field. However, given that this is a CRD and not an aggregated API, I am not sure there is a good way of doing it automatically. I am not sure what the best way to handle this is (cc @tnqn):

• considering this is an alpha API, it may be acceptable to ignore this altogether
• or instead of ipBlock and ipBlocks, we can have ipBlock and extraIPBlocks, and mandate that ipBlock is always set first
• or we can have a mutating webhook, but the cost seems high

[Dyanngg]

Typically with an API change such as this one, the server needs to ensure that the first member of the new ipBlocks field matches ipBlock. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility. The rationale for this is that a CG created by a new client that includes a single IPBlock (using ipBlocks) can still be accessed by older clients. Similarly, CG objects created by an older client (with ipBlock) can be accessed by a new client which only looks at the new ipBlocks field. However, given that this is a CRD and not an aggregated API, I am not sure there is a good way of doing it automatically. I am not sure what the best way to handle this is (cc @tnqn):

• considering this is an alpha API, it may be acceptable to ignore this altogether
• or instead of ipBlock and ipBlocks, we can have ipBlock and extraIPBlocks, and mandate that ipBlock is always set first
• or we can have a mutating webhook, but the cost seems high

I'm trying to understand the notion of client here. I thought this was okay because, as you mentioned, this is a CRD and not an aggregated API, so the only client in our context would be the Antrea controller. Is it really possible that we will have a situation where the CG CRD is updated in a cluster but not the Antrea controller (considering they will be patched with the same yaml, or maybe I am making too much assumptions here)? I also don't think we can handle this case because if the user specify ipBlocks > 1, then the old client wouldn't be able to process it anyways. And I personally don't think setting the ClusterGroup to contain only the first CIDR for old clients is right...

[antoninbas]

so the only client in our context would be the Antrea controller

The clients are the Antrea Controller and the application / user creating policies and CGs
The Controller is updated "at the same time" as the CRD (most of the time). Applications that read / write CG CRs can be updated asynchronously with Antrea.

And I personally don't think setting the ClusterGroup to contain only the first CIDR for old clients is right...

I would tend to agree, since it changes the semantics of the CG. But then again, reading a CG and seeing an empty ipBlock field is not great either.

I would say the use case I was looking at was a new client being able to handle previously created CG CRs by only considering the new ipBlocks field. However, I believe the only way to handle this would be to introduce a new API version and use a CRD conversion webhook, which we are trying to avoid here.

Given that, and given that I believe I am starting to overthink this, I think the current solution is fine. A client should only need to look at its own CG CRs, and once upgraded should consider both the ipBlock and ipBlocks field. We (or rather I) will document this well in the release notes. I do feel like if this was a beta API we would need to give it more thought.

[antoninbas]

LGTM

[Dyanngg]

/test-all

[jianjuns]

LGTM. See if @tnqn wants to review.

[Dyanngg]

rebased on main /test-all

[Dyanngg]

/test-all rebased to resolve merge conflict

[tnqn]

Code LGTM. However there are some compilation issues.

[Dyanngg]

/test-all

[antoninbas]

LGTM