Remove child group existence validation for ClusterGroup

[Dyanngg]

This is the first of two PRs that relaxes admission validations for ClusterGroup references. This PR removes the restriction that a child ClusterGroup must exist before it can be referred to as childGroup in other ClusterGroups. With this change, the effective member of a nested ClusterGroup will be calculated as the union of its childGroup members, for each childGroup that exist in the cluster.

Note that this PR does not change the max nested level of ClusterGroups, which is 1 at the moment. This means that any ClusterGroup that has a parent cannot be a nested ClusterGroup itself; and reversely, a ClusterGroup cannot select another nested ClusterGroup as its childGroup. This is validated upon creation/update of a ClusterGroup. The order of events matter in this case, as any update which would cause a ClusterGroup nesting over the order of 1 will be rejected.

i.e. if we have 3 ClusterGroups: cg1; cg2: child=cg1; cg3: child=cg2

1. Creation of cg1, cg2 in whichever order will be allowed. After that, creation of cg3 will not be allowed.
2. Creation of cg1, cg3 in whichever order will be allowed. After that, creation of cg2 will not be allowed.
3. Creation of cg2, cg3 in whichever order will be allowed. After that, creation of cg1 will not be allowed.

Tests done:

1. Creation of CG validation, as specified above
2. ClusterGroup membership for nested CG whose childGroups all exist, partially exist and none exist.
3. ACNP with ClusterGroup reference, whose childGroup is then created/updated/deleted.

[codecov-commenter]

Codecov Report

Merging #2443 (d0b5321) into main (365623f) will increase coverage by 5.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main    #2443      +/-   ##
==========================================
+ Coverage   59.82%   64.84%   +5.01%     
==========================================
  Files         284      284              
  Lines       22170    25524    +3354     
==========================================
+ Hits        13264    16551    +3287     
+ Misses       7487     7418      -69     
- Partials     1419     1555     +136     

Flag	Coverage Δ
e2e-tests	55.79% <77.77%> (?)
kind-e2e-tests	46.87% <0.00%> (-0.21%)	⬇️
unit-tests	42.21% <0.00%> (+0.20%)	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/networkpolicy/validate.go	74.06% <77.77%> (+54.28%)	⬆️
pkg/agent/agent_linux.go	80.00% <0.00%> (-20.00%)	⬇️
pkg/controller/egress/ipallocator/allocator.go	67.82% <0.00%> (-15.16%)	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	77.64% <0.00%> (-13.79%)	⬇️
pkg/apis/controlplane/v1beta1/conversion.go	72.44% <0.00%> (-11.89%)	⬇️
pkg/util/ip/ip.go	68.67% <0.00%> (-11.63%)	⬇️
pkg/legacyapis/core/v1alpha2/register.go	69.23% <0.00%> (-10.77%)	⬇️
pkg/controller/egress/controller.go	76.76% <0.00%> (-10.44%)	⬇️
pkg/apis/stats/register.go	71.42% <0.00%> (-10.39%)	⬇️
pkg/legacyapis/stats/register.go	71.42% <0.00%> (-10.39%)	⬇️
... and 269 more

[abhiraut]

the following cases are most likely handled by the controllers but would like to verify:

• create CNP (appTo: cgA) first; then create cgA; ensure cgA is reflected in CNP AppliedToGroup
  • Similarly ensure cgA referenced in source/dest groups is reflected in AddressGroups
• Delete cgA and ensure cgA is removed from CNP AppliedToGroup/AddressGroups

maybe we can add e2e tests for above too

[Dyanngg]

the following cases are most likely handled by the controllers but would like to verify:

• create CNP (appTo: cgA) first; then create cgA; ensure cgA is reflected in CNP AppliedToGroup

  • Similarly ensure cgA referenced in source/dest groups is reflected in AddressGroups

• Delete cgA and ensure cgA is removed from CNP AppliedToGroup/AddressGroups

maybe we can add e2e tests for above too

Yes those cases are covered and I have added E2E tests for those

[jianjuns]

Question - we do not need NetworkPolicyController changes to support child group creation after parent?

[Dyanngg]

Question - we do not need NetworkPolicyController changes to support child group creation after parent?

We don't. Before this change, any updates in the childGroup selectors also need to propagate to the parent ClusterGroups, and if those ClusterGroups are used in ACNPs, the effective appliedTo/addressGroups will be updated. Hence the logic is already there.

[jianjuns]

Great!

We don't. Before this change, any updates in the childGroup selectors also need to propagate to the parent ClusterGroups, and if those ClusterGroups are used in ACNPs, the effective appliedTo/addressGroups will be updated. Hence the logic is already there.

[Dyanngg]

/test-all

[antoninbas]

/test-e2e

[antoninbas]

@Dyanngg the e2e test failure actually seems to be because of this PR

FAIL: TestLegacyClusterGroup/TestLegacyGroupClusterGroupValidate/Case=LegacyInvalidChildGroupName

[Dyanngg]

@Dyanngg the e2e test failure actually seems to be because of this PR

FAIL: TestLegacyClusterGroup/TestLegacyGroupClusterGroupValidate/Case=LegacyInvalidChildGroupName

Thanks for the reminder. Yes this is an out of date testcase since we do not validate childGroup existence anymore. Forgot to update the testcase for legacy testgroup. Fixed now. /test-all

[Dyanngg]

/test-all

[tnqn]

LGTM

[tnqn]

i.e. if we have 3 ClusterGroups: cg1; cg2: child=cg1; cg3: child=cg2

3. Creation of cg2, cg3 in whichever order will be allowed. After that, creation of cg1 will not be allowed.

I think this is not true? Creating cg3 after cg2 or creating cg2 after cg3 will fail?