Add nested ClusterGroup support #1920

Dyanngg · 2021-02-26T23:50:32Z

This PR adds a new childGroups field for ClusterGroups, which allows a ClusterGroup to select other ClusterGroups by name. The effective members of the 'parent' CG will be the union of its childGroups' members. Restrictions do apply:

childGroups cannot be used concurrently with selectors/ipBlock/serviceReference. A ClusterGroup can either select other CGs, or select workloads/ipBlock/Services, but not both.
Currently only one level of 'nested-ness' is supported: If a ClusterGroup has childGroups, it cannot be selected as a childGroup by other CGs.
ClusterGroup must exist before another ClusterGroup can select it by name as its childGroup. A ClusterGroup cannot be deleted if it is referred by other ClusterGroup as childGroup.

codecov-io · 2021-02-27T02:03:05Z

Codecov Report

Merging #1920 (5472973) into main (6be27c7) will increase coverage by 17.87%.
The diff coverage is 48.81%.

@@             Coverage Diff             @@
##             main    #1920       +/-   ##
===========================================
+ Coverage   41.88%   59.75%   +17.87%     
===========================================
  Files         124      197       +73     
  Lines       15218    17313     +2095     
===========================================
+ Hits         6374    10346     +3972     
+ Misses       8319     5862     -2457     
- Partials      525     1105      +580

Flag	Coverage Δ
kind-e2e-tests	`45.01% <6.29%> (?)`
unit-tests	`41.87% <48.81%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/controller/networkpolicy/store/group.go	`5.00% <0.00%> (+5.00%)`	⬆️
pkg/controller/networkpolicy/validate.go	`0.00% <0.00%> (ø)`
pkg/controller/types/group.go	`0.00% <ø> (ø)`
pkg/controller/networkpolicy/clustergroup.go	`68.47% <66.66%> (+0.44%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`69.23% <87.17%> (+12.70%)`	⬆️
pkg/apiserver/certificate/certificate.go	`69.86% <0.00%> (-6.85%)`	⬇️
pkg/apiserver/handlers/webhook/mutation_labels.go	`24.71% <0.00%> (ø)`
pkg/apiserver/registry/networkpolicy/util.go	`100.00% <0.00%> (ø)`
pkg/apis/networking/v1beta1/register.go	`92.30% <0.00%> (ø)`
pkg/agent/cniserver/ipam/ipam_delegator.go	`48.83% <0.00%> (ø)`
... and 148 more

pkg/controller/networkpolicy/clustergroup.go

jianjuns · 2021-03-11T19:35:13Z

pkg/apis/core/v1alpha2/types.go

@@ -129,6 +132,11 @@ type GroupSpec struct {
 	// Cannot be set with any other selector except NamespaceSelector.
 	// +optional
 	ExternalEntitySelector *metav1.LabelSelector `json:"externalEntitySelector,omitempty"`
+	// Select other ClusterGroups by name. The ClusterGroups must already
+	// exist and must not contain ChildGroups themselves.


Question - if I apply a single yaml that creates a group and its child groups together, could the creation fail due to ordering?

Good point, the order do matter in this case, but this brings up another problem: if child group comes before the parent group, then create can succeed but deletion with the same yaml would fail... I remember @antoninbas has the same concerns regarding this kind of constraints, since it exists in ACNP today as well (a CG can only be referenced in ACNP if it exists). Maybe we should considering lifting it for nested groups?

The 'gotcha' I can think of is, for example, if we have CG1 which has CG2 as child, and CG2 which has CG3 as child. It used to be we can only create CG3, CG2, CG1 in that order and CG3 creation will fail. Now, CG3 can be created first, and CG2 creation will fail. I suppose that is acceptable....

Yeah, I always feel we should allow dangling references. Do not mean we must fix it in your PR.

If everyone agrees and does not see any risks with dangling reference (for child groups), I can definitely remove the constraint in this PR. @antoninbas @tnqn

How about let us do that together with NetworkPolicy, once we agree that is the right approach to go.

works for me

tnqn

LGTM overall

pkg/controller/networkpolicy/clustergroup.go

pkg/controller/networkpolicy/networkpolicy_controller.go

pkg/controller/types/group.go

pkg/controller/networkpolicy/networkpolicy_controller.go

pkg/controller/networkpolicy/clustergroup.go

jianjuns · 2021-03-24T00:19:23Z

pkg/controller/networkpolicy/networkpolicy_controller.go

+
+// getClusterGroupMemberSet knows how to construct a GroupMemberSet that contains
+// all the entities selected by a ClusterGroup. For ClusterGroup that has childGroups,
+// the members are computed as the union of all its childGroup's members.


So we flatten nested groups in controller and send both the flattened parent groups and child groups (?) to agent?
This can be a simpler strategy, but meanwhile have we thought about the possibility of let agent flatten the groups?
@tnqn

We don't send the parent Groups to the agent... whenever a parent Group is referenced in policies, the its corresponding appliedToGroup/addressGroup gets the flattened addresses, and it's the ATGrp/AddrGrp we send to the agent

If letting agent flatten the groups, we need to have a controlplane clustergroup API to send their members and childgroups to agents. Maybe this could be discussed separately when we make group API generic for features to consume?

Yes, we can think about CHILD group flattening in agent later.

Back to the current implementation, so when two NPs share the same ClusterGroup in the Peers, we create a single AppliedToGroup/AddressGroup? And we still use the normalized ID for the AppliedTo/Address groups?
This way might be ok for now, but when we support multiple groups in Peer or the new AppliedTo, it won't help to send a single copy of Group members to agent. We need to think about a solution for that.

I think the problem of 1:1 mapping between AppliedTo/AddressGroup and CG is independent of nested groups: if we support multiple CGs in a single peer, even w/o nested group there will be this problem. And if that is the case, there's two level of flattening: 1. flattening nested group 2. flattening groups in a single peer for AppliedTo/AddressGroup. I still think 1 makes sense to be done in controller, even 2. The complexity of flattening can be taken care of by the controller and agent only needs to know about the result: it doesn't care about the nesting, does it?

If we just have a single level of nesting, it is not complex to flatten, and in that case I would at least handle your #2 (flattening groups in a single peer for AppliedTo/AddressGroup) in agent, at least for AddressGroup (as AppliedTo group only includes local members so should be much smaller).
When a group is used by multiple policies/rules (that is the reason we introduce group right), sharing the group member message can reduce data on-wire a lot.

Got it. Makes sense to me.

Add E2E tests

jianjuns · 2021-03-24T20:28:56Z

pkg/controller/networkpolicy/clustergroup.go

+		}
+		return !oldChildGroups.Equal(newChildGroups)
+	}
+	if !selectorUpdated && !ipBlockUpdated && !svcRefUpdated && !childGroupsUpdated(oldGroup, newGroup) {


Do you think we can do the same for other conditions, like

if newGroup.IPBlock == oldGroup.IPBlock && newGroup.ServiceReference == oldGroup.ServiceReference && getNormalizedNameForSelector(newGroup.Selector) == getNormalizedNameForSelector(oldGroup.Selector) && !childGroupsUpdated(oldGroup, newGroup)

I saw your comments that these should not be expensive, but considering we might need to process a large number of groups, I still think the optimization deserves the extra code complexity.

Thanks for the suggestion @jianjuns. After a closer look, I realized that the == checks we had in place was actually wrong: the address of the ipBlocks cannot be identical. Fixed this logic and changed the code as you suggested

Dyanngg · 2021-03-25T17:16:25Z

/test-all

tnqn

LGTM

Dyanngg requested review from edwardbadboy, antoninbas and jianjuns February 26, 2021 23:50

vmwclabot added the cla-not-required label Feb 26, 2021

Dyanngg force-pushed the nested-group branch 2 times, most recently from 138772c to db94c67 Compare February 27, 2021 02:01

Dyanngg force-pushed the nested-group branch 2 times, most recently from c3db0ae to 958d60e Compare March 2, 2021 00:54

Dyanngg changed the title ~~[WIP] Add nested group support~~ Add nested ClusterGroup support Mar 2, 2021

Dyanngg force-pushed the nested-group branch 2 times, most recently from 12d4734 to 2e83cae Compare March 4, 2021 01:15

Dyanngg requested review from tnqn and antoninbas and removed request for antoninbas March 4, 2021 01:16

tnqn reviewed Mar 11, 2021

View reviewed changes

pkg/controller/networkpolicy/clustergroup.go Outdated Show resolved Hide resolved

jianjuns reviewed Mar 11, 2021

View reviewed changes

Dyanngg force-pushed the nested-group branch 3 times, most recently from bcb33d0 to ff28087 Compare March 12, 2021 22:43

Dyanngg requested a review from tnqn March 12, 2021 22:49

Dyanngg force-pushed the nested-group branch from ff28087 to b987c9c Compare March 12, 2021 23:13

Dyanngg added this to the Antrea v0.14.0 release milestone Mar 16, 2021

Dyanngg self-assigned this Mar 16, 2021

Dyanngg added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 16, 2021

tnqn mentioned this pull request Mar 18, 2021

Add Egress CRD types #1433

Merged

Dyanngg force-pushed the nested-group branch 2 times, most recently from b1099fd to 079cb7f Compare March 18, 2021 22:07

tnqn reviewed Mar 19, 2021

View reviewed changes

Dyanngg force-pushed the nested-group branch from 079cb7f to 5472973 Compare March 19, 2021 18:08

tnqn reviewed Mar 23, 2021

View reviewed changes

pkg/controller/networkpolicy/networkpolicy_controller.go Outdated Show resolved Hide resolved

jianjuns reviewed Mar 24, 2021

View reviewed changes

Dyanngg added 2 commits March 23, 2021 21:01

Add nested ClusterGroup support and E2E tests

8f3f121

Add E2E tests

Address comments

4833610

Dyanngg force-pushed the nested-group branch from 5472973 to 4833610 Compare March 24, 2021 04:01

Dyanngg requested review from tnqn and jianjuns March 24, 2021 04:04

make group member and association functions non-recursive

b3199d2

jianjuns reviewed Mar 24, 2021

View reviewed changes

Fix CG update check function

f4fe08d

Dyanngg force-pushed the nested-group branch from c0e7078 to f4fe08d Compare March 24, 2021 21:53

Dyanngg requested a review from jianjuns March 24, 2021 22:24

tnqn approved these changes Mar 26, 2021

View reviewed changes

Dyanngg merged commit aaaea4f into antrea-io:main Mar 27, 2021

Dyanngg deleted the nested-group branch March 27, 2021 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nested ClusterGroup support #1920

Add nested ClusterGroup support #1920

Dyanngg commented Feb 26, 2021 •

edited

Loading

codecov-io commented Feb 27, 2021 •

edited

Loading

jianjuns Mar 11, 2021

Dyanngg Mar 11, 2021

jianjuns Mar 11, 2021

Dyanngg Mar 12, 2021 •

edited

Loading

jianjuns Mar 12, 2021

Dyanngg Mar 13, 2021

tnqn left a comment

jianjuns Mar 24, 2021

Dyanngg Mar 24, 2021

tnqn Mar 24, 2021

jianjuns Mar 24, 2021

Dyanngg Mar 24, 2021

jianjuns Mar 24, 2021

Dyanngg Mar 24, 2021

jianjuns Mar 24, 2021

Dyanngg Mar 24, 2021

Dyanngg commented Mar 25, 2021

tnqn left a comment

Add nested ClusterGroup support #1920

Add nested ClusterGroup support #1920

Conversation

Dyanngg commented Feb 26, 2021 • edited Loading

codecov-io commented Feb 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg commented Mar 25, 2021

tnqn left a comment

Choose a reason for hiding this comment

Dyanngg commented Feb 26, 2021 •

edited

Loading

codecov-io commented Feb 27, 2021 •

edited

Loading

Dyanngg Mar 12, 2021 •

edited

Loading