From 58658406351b29f53b25bf01709df457ece1496b Mon Sep 17 00:00:00 2001
From: wgrayson <wgrayson@vmware.com>
Date: Tue, 7 Dec 2021 14:29:21 -0800
Subject: [PATCH] Make Peer validation more comprehensive

Signed-off-by: wgrayson <wgrayson@vmware.com>
---
 pkg/controller/networkpolicy/validate.go | 23 ++++++++++++---
 test/e2e/antreapolicy_test.go            | 36 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go
index ec74a77d59d..2cd5e687c2f 100644
--- a/pkg/controller/networkpolicy/validate.go
+++ b/pkg/controller/networkpolicy/validate.go
@@ -17,6 +17,7 @@ package networkpolicy
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -461,6 +462,11 @@ func (v *antreaPolicyValidator) validateAppliedTo(ingress, egress []crdv1alpha1.
 	if numAppliedToInRules > 0 && (numAppliedToInRules != len(ingress)+len(egress)) {
 		return "appliedTo field should either be set in all rules or in none of them", false
 	}
+	for _, eachSpecAppliedTo := range specAppliedTo {
+		if eachSpecAppliedTo.Group != "" && numFieldsSetInPeer(eachSpecAppliedTo) > 1 {
+			return "group cannot be set with other peers in appliedTo", false
+		}
+	}
 	return "", true
 }
 
@@ -472,10 +478,7 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule
 			if peer.NamespaceSelector != nil && peer.Namespaces != nil {
 				return "namespaces and namespaceSelector cannot be set at the same time for a single NetworkPolicyPeer", false
 			}
-			if peer.Group == "" {
-				continue
-			}
-			if peer.PodSelector != nil || peer.IPBlock != nil || peer.NamespaceSelector != nil {
+			if peer.Group != "" && numFieldsSetInPeer(peer) > 1 {
 				return "group cannot be set with other peers in rules", false
 			}
 		}
@@ -504,6 +507,18 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule
 	return "", true
 }
 
+// numFieldsSetInPeer returns the number of fields in use of a peer.
+func numFieldsSetInPeer(peer crdv1alpha1.NetworkPolicyPeer) int {
+	num := 0
+	v := reflect.ValueOf(peer)
+	for i := 0; i < v.NumField(); i++ {
+		if !v.Field(i).IsZero() {
+			num++
+		}
+	}
+	return num
+}
+
 // validateTierForPolicy validates whether a referenced Tier exists.
 func (v *antreaPolicyValidator) validateTierForPolicy(tier string) (string, bool) {
 	// "tier" must exist before referencing
diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go
index 22e4ca4ac25..20d25681d4e 100644
--- a/test/e2e/antreapolicy_test.go
+++ b/test/e2e/antreapolicy_test.go
@@ -2720,6 +2720,41 @@ func testToServices(t *testing.T) {
 	}
 }
 
+func testGroupStandAlonePeer(t *testing.T) {
+	builder1 := &ClusterNetworkPolicySpecBuilder{}
+	builder1 = builder1.SetName("group-not-alone-in-applied-to").
+		SetTier("application").
+		SetPriority(1.0).
+		SetAppliedToGroup([]ACNPAppliedToSpec{{Group: "cg-1", NSSelector: map[string]string{"ns": "x"}}})
+	builder1.AddIngress(v1.ProtocolTCP, nil, nil, nil, nil, nil, nil, nil, nil,
+		true, nil, crdv1alpha1.RuleActionPass, "cg-2", "")
+	_, err := k8sUtils.CreateOrUpdateACNP(builder1.Get())
+	if err == nil {
+		t.Error("failure -- group shouldn't be able to used with other field in `AppliedTo`")
+		failOnError(k8sUtils.DeleteACNP(builder1.Name), t)
+		failOnError(waitForResourceDelete("", builder1.Name, resourceACNP, timeout), t)
+		time.Sleep(networkPolicyDelay)
+	}
+
+	builder2 := &ClusterNetworkPolicySpecBuilder{}
+	builder2 = builder2.SetName("group-not-alone-in-rule").
+		SetTier("application").
+		SetPriority(1.0).
+		SetAppliedToGroup([]ACNPAppliedToSpec{{Group: "cg-1"}})
+	builder2.AddIngress(v1.ProtocolTCP, nil, nil, nil, nil, map[string]string{"pod": "a"}, nil, nil, nil,
+		true, nil, crdv1alpha1.RuleActionPass, "cg-2", "")
+	builder2.AddEgress(v1.ProtocolTCP, nil, nil, nil, nil, nil, nil, nil, nil,
+		true, nil, crdv1alpha1.RuleActionPass, "cg-2", "")
+	builder2.Spec.Egress[0].To[0].FQDN = "google.com"
+	_, err = k8sUtils.CreateOrUpdateACNP(builder2.Get())
+	if err == nil {
+		t.Error("failure -- group shouldn't be able to used with other fields in ingress.from or egress.to")
+		failOnError(k8sUtils.DeleteACNP(builder2.Name), t)
+		failOnError(waitForResourceDelete("", builder2.Name, resourceACNP, timeout), t)
+		time.Sleep(networkPolicyDelay)
+	}
+}
+
 // executeTests runs all the tests in testList and prints results
 func executeTests(t *testing.T, testList []*TestCase) {
 	executeTestsWithData(t, testList, nil)
@@ -3067,6 +3102,7 @@ func TestAntreaPolicy(t *testing.T) {
 		t.Run("Case=ACNPFQDNPolicy", func(t *testing.T) { testFQDNPolicy(t) })
 		t.Run("Case=FQDNPolicyInCluster", func(t *testing.T) { testFQDNPolicyInClusterService(t) })
 		t.Run("Case=ACNPToServices", func(t *testing.T) { testToServices(t) })
+		t.Run("Case=ACNPGroupStandAlonePeer", func(t *testing.T) { testGroupStandAlonePeer(t) })
 	})
 	// print results for reachability tests
 	printResults()