Add validation and documentation

docs/antrea-network-policy.md

@@ -605,16 +605,16 @@ be referenced in ACNP in place of stand-alone `podSelector` and/or `namespaceSel
 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.
+
 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.
+ClusterGroup to share IPBlocks between ACNPs. Furthermore, the `childGroups` field
+allows a ClusterGroup to select other ClusterGroups by name. The effective members
+of the 'parent' ClusterGrup will be the union of its childGroups' members.
 
 ### The ClusterGroup resource
 
@@ -643,9 +643,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 +657,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 +666,34 @@ 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 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 CGs.
+- 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 CGs (Pod/Service/CIDRs), but as mentioned above, it cannot select a CG 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.
 

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
 }

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) })