-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add nested ClusterGroup support #1920
Conversation
138772c
to
db94c67
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c3db0ae
to
958d60e
Compare
12d4734
to
2e83cae
Compare
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question - if I apply a single yaml that creates a group and its child groups together, could the creation fail due to ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I always feel we should allow dangling references. Do not mean we must fix it in your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about let us do that together with NetworkPolicy, once we agree that is the right approach to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me
bcb33d0
to
ff28087
Compare
b1099fd
to
079cb7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
|
||
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Makes sense to me.
} | ||
return !oldChildGroups.Equal(newChildGroups) | ||
} | ||
if !selectorUpdated && !ipBlockUpdated && !svcRefUpdated && !childGroupsUpdated(oldGroup, newGroup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 itschildGroups
' 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.childGroups
, it cannot be selected as achildGroup
by other CGs.childGroup
. A ClusterGroup cannot be deleted if it is referred by other ClusterGroup aschildGroup
.