From 63ad8e86a7bb1e134a44a703196c9b15a762f2c5 Mon Sep 17 00:00:00 2001 From: Yang Ding Date: Tue, 30 Mar 2021 15:18:00 -0700 Subject: [PATCH] Add validation and documentation --- docs/antrea-network-policy.md | 64 ++++++++++++++++---- pkg/controller/networkpolicy/clustergroup.go | 4 +- pkg/controller/networkpolicy/validate.go | 16 +++-- test/e2e/clustergroup_test.go | 23 +++++++ 4 files changed, 86 insertions(+), 21 deletions(-) diff --git a/docs/antrea-network-policy.md b/docs/antrea-network-policy.md index f7836e063de..50ee1927e75 100644 --- a/docs/antrea-network-policy.md +++ b/docs/antrea-network-policy.md @@ -602,19 +602,18 @@ order in which they are enforced. A ClusterGroup (CG) CRD is a specification of how workloads are grouped together. It allows admins to group Pods using traditional label selectors, which can then be referenced in ACNP in place of stand-alone `podSelector` and/or `namespaceSelector`. -In addition, ClusterGroup also supports Pod grouping by `serviceReference`. ClusterGroup -specified by `serviceReference` will contain the same Pod members that are currently -selected by the Service's selector. +In addition to `podSelector` and `namespaceSelector`, ClusterGroup also supports the +following ways to select endpoints: + +- Pod grouping by `serviceReference`. ClusterGroup specified by `serviceReference` will +contain the same Pod members that are currently selected by the Service's selector. +- `ipBlock` or `ipBlocks` to share IPBlocks between ACNPs. +- `childGroups` to select other ClusterGroups by name. + ClusterGroups allow admins to separate the concern of grouping of workloads from the security aspect of Antrea-native policies. It adds another level of indirection allowing users to update group membership without having to update individual policy rules. -In addition to specifying label selectors to group workloads, admins can create a -ClusterGroup to share IPBlocks. -An `ipBlock` selector may not be specified with a `podSelector` and `namespaceSelector`, -i.e. a single ClusterGroup can either group workloads or share IPBlocks. -A ClusterGroup is cluster scoped resource and therefore can only be set in an Antrea -ClusterNetworkPolicy's `appliedTo` and `to`/`from` peers. ### The ClusterGroup resource @@ -643,9 +642,9 @@ kind: ClusterGroup metadata: name: test-cg-ip-block spec: - # IPBlock cannot be set along with PodSelector, NamespaceSelector or serviceReference. - ipBlock: - cidr: 10.0.10.0/24 + # IPBlocks cannot be set along with PodSelector, NamespaceSelector or serviceReference. + ipBlocks: + - cidr: 10.0.10.0/24 status: conditions: - type: "GroupMembersComputed" @@ -657,7 +656,7 @@ kind: ClusterGroup metadata: name: test-cg-svc-ref spec: - # ServiceReference cannot be set along with PodSelector, NamespaceSelector or ipBlock. + # ServiceReference cannot be set along with PodSelector, NamespaceSelector or ipBlocks. serviceReference: name: test-service namespace: default @@ -666,8 +665,35 @@ status: - type: "GroupMembersComputed" status: "True" lastTransitionTime: "2021-01-29T20:21:46Z" +--- +apiVersion: core.antrea.tanzu.vmware.com/v1alpha2 +kind: ClusterGroup +metadata: + name: test-cg-nested +spec: + childGroups: [test-cg-sel, test-cg-ip-blocks, test-cg-svc-ref] +status: + conditions: + - type: "GroupMembersComputed" + status: "True" + lastTransitionTime: "2021-01-29T20:21:48Z" ``` +There are a few __restrictions__ on how ClusterGroups can be configured: + +- A ClusterGroup is a cluster-scoped resource and therefore can only be set in an Antrea +ClusterNetworkPolicy's `appliedTo` and `to`/`from` peers. +- For the `childGroup` field, currently only one level of nesting is supported: +If a ClusterGroup has childGroups, it cannot be selected as a childGroup by other ClusterGroups. +- ClusterGroup must exist before another ClusterGroup can select it by name as its childGroup. +A ClusterGroup cannot be deleted if it is referred to by other ClusterGroup as childGroup. +This restriction may be lifted in future releases. +- At most one of `podSelector`, `serviceReference`, `ipBlock`, `ipBlocks` or `childGroups` +can be set for a ClusterGroup, i.e. a single ClusterGroup can either group workloads, +represent IP CIDRs or select other ClusterGroups. A parent ClusterGroup can select different +types of ClusterGroups (Pod/Service/CIDRs), but as mentioned above, it cannot select a +ClusterGroup that has childGroups itself. + **spec**: The ClusterGroup `spec` has all the information needed to define a cluster-wide group. @@ -684,6 +710,14 @@ If set with a `podSelector`, all matching Pods from Namespaces selected by the "sources" or `egress` "destinations". A ClusterGroup with `ipBlock` referenced in an ACNP's `appliedTo` field will be ignored, and the policy will have no effect. +For a same ClusterGroup, `ipBlock` and `ipBlocks` cannot be set concurrently. +ipBlock will be deprecated for ipBlocks in future versions of ClusterGroup. + +**ipBlocks**: This selects a list of IP CIDR ranges to allow as `ingress` +"sources" or `egress` "destinations". +A ClusterGroup with `ipBlocks` referenced in an ACNP's `appliedTo` field will be +ignored, and the policy will have no effect. +For a same ClusterGroup, `ipBlock` and `ipBlocks` cannot be set concurrently. **serviceReference**: Pods that serve as the backend for the specified Service will be grouped. Services without selectors are currently not supported, and will @@ -694,6 +728,10 @@ traffic enforcement. `ServiceReference` is merely a mechanism to group Pods and ensure that a ClusterGroup stays in sync with the set of Pods selected by a given Service. +**childGroups**: This selects existing ClusterGroups by name. The effective members +of the "parent" ClusterGrup will be the union of all its childGroups' members. +See the section above for restrictions. + **status**: The ClusterGroup `status` field determines the overall realization status of the group. diff --git a/pkg/controller/networkpolicy/clustergroup.go b/pkg/controller/networkpolicy/clustergroup.go index 4dbe07030e5..bb822a55519 100644 --- a/pkg/controller/networkpolicy/clustergroup.go +++ b/pkg/controller/networkpolicy/clustergroup.go @@ -63,7 +63,7 @@ func (n *NetworkPolicyController) updateClusterGroup(oldObj, curObj interface{}) } return true } - ipBlockUpdated := func() bool { + ipBlocksUpdated := func() bool { oldIPBs, newIPBs := sets.String{}, sets.String{} for _, ipb := range oldGroup.IPBlocks { oldIPBs.Insert(ipNetToCIDRStr(ipb.CIDR)) @@ -83,7 +83,7 @@ func (n *NetworkPolicyController) updateClusterGroup(oldObj, curObj interface{}) } return !oldChildGroups.Equal(newChildGroups) } - if !ipBlockUpdated() && !svcRefUpdated() && !selectorUpdated() && !childGroupsUpdated() { + if !ipBlocksUpdated() && !svcRefUpdated() && !selectorUpdated() && !childGroupsUpdated() { // No change in the contents of the ClusterGroup. No need to enqueue for further sync. return } diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go index ae219dd1373..ae5b2472f1f 100644 --- a/pkg/controller/networkpolicy/validate.go +++ b/pkg/controller/networkpolicy/validate.go @@ -635,24 +635,28 @@ func (t *tierValidator) deleteValidate(oldObj interface{}, userInfo authenticati // validateAntreaGroupSpec ensures that an IPBlock is not set along with namespaceSelector and/or a // podSelector. Similarly, ExternalEntitySelector cannot be set with PodSelector. func validateAntreaGroupSpec(s corev1a2.GroupSpec) (string, bool) { - selector, serviceRef, ipBlock, childGroups := 0, 0, 0, 0 + errMsg := "At most one of podSelector, externalEntitySelector, serviceReference, ipBlock, ipBlocks or childGroups can be set for a ClusterGroup" + if s.PodSelector != nil && s.ExternalEntitySelector != nil { + return errMsg, false + } + selector, serviceRef, ipBlock, ipBlocks, childGroups := 0, 0, 0, 0, 0 if s.NamespaceSelector != nil || s.ExternalEntitySelector != nil || s.PodSelector != nil { selector = 1 } - if s.PodSelector != nil && s.ExternalEntitySelector != nil { - selector = 2 - } if s.IPBlock != nil { ipBlock = 1 } + if len(s.IPBlocks) > 0 { + ipBlocks = 1 + } if s.ServiceReference != nil { serviceRef = 1 } if len(s.ChildGroups) > 0 { childGroups = 1 } - if selector+serviceRef+ipBlock+childGroups > 1 { - return fmt.Sprint("At most one of podSelector/namespaceSelector, externalEntitySelector/namespaceSelector, serviceReference, ipBlock or childGroups can be set for a ClusterGroup"), false + if selector+serviceRef+ipBlock+ipBlocks+childGroups > 1 { + return errMsg, false } return "", true } diff --git a/test/e2e/clustergroup_test.go b/test/e2e/clustergroup_test.go index a2dd3aca0a9..0b96eabaeda 100644 --- a/test/e2e/clustergroup_test.go +++ b/test/e2e/clustergroup_test.go @@ -66,6 +66,28 @@ func testInvalidCGIPBlockWithNSSelector(t *testing.T) { } } +func testInvalidCGIPBlockWithIPBlocks(t *testing.T) { + invalidErr := fmt.Errorf("clustergroup created with ipBlock and ipBlocks") + cgName := "ipb-ipbs" + cidr := "10.0.0.10/32" + cidr2 := "10.0.0.20/32" + ipb := &secv1alpha1.IPBlock{CIDR: cidr} + ipbs := []secv1alpha1.IPBlock{{CIDR: cidr2}} + cg := &corev1a1.ClusterGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: cgName, + }, + Spec: corev1a1.GroupSpec{ + IPBlocks: ipbs, + IPBlock: ipb, + }, + } + if _, err := k8sUtils.CreateOrUpdateCG(cg); err == nil { + // Above creation of CG must fail as it is an invalid spec. + failOnError(invalidErr, t) + } +} + func testInvalidCGServiceRefWithPodSelector(t *testing.T) { invalidErr := fmt.Errorf("clustergroup created with serviceReference and podSelector") cgName := "svcref-pod-selector" @@ -257,6 +279,7 @@ func TestClusterGroup(t *testing.T) { t.Run("TestGroupClusterGroupValidate", func(t *testing.T) { t.Run("Case=IPBlockWithPodSelectorDenied", func(t *testing.T) { testInvalidCGIPBlockWithPodSelector(t) }) t.Run("Case=IPBlockWithNamespaceSelectorDenied", func(t *testing.T) { testInvalidCGIPBlockWithNSSelector(t) }) + t.Run("Case=IPBlockWithIPBlocksDenied", func(t *testing.T) { testInvalidCGIPBlockWithIPBlocks(t) }) t.Run("Case=ServiceRefWithPodSelectorDenied", func(t *testing.T) { testInvalidCGServiceRefWithPodSelector(t) }) t.Run("Case=ServiceRefWithNamespaceSelectorDenied", func(t *testing.T) { testInvalidCGServiceRefWithNSSelector(t) }) t.Run("Case=ServiceRefWithIPBlockDenied", func(t *testing.T) { testInvalidCGServiceRefWithIPBlock(t) })