Address comments

go.mod

@@ -25,7 +25,6 @@ require (
 	github.com/gogo/protobuf v1.3.1
 	github.com/golang/mock v1.4.3
 	github.com/golang/protobuf v1.3.2
-	github.com/google/go-cmp v0.4.0
 	github.com/google/uuid v1.1.1
 	github.com/kevinburke/ssh_config v0.0.0-20190725054713-01f96b0aa0cd
 	github.com/pkg/errors v0.9.1

pkg/agent/controller/networkpolicy/cache.go

@@ -409,15 +409,11 @@ func (c *ruleCache) addAddressGroupLocked(group *v1beta1.AddressGroup) error {
 	for i := range group.Pods {
 		// Must not store address of loop iterator variable as it's the same
 		// address taking different values in each loop iteration, otherwise
-		// podSet would eventually contain only the last value.
+		// groupMemberSet would eventually contain only the last value.
 		// https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable
 		groupMemberSet.Insert(group.Pods[i].ToGroupMember())
 	}
 	for i := range group.GroupMembers {
-		// Must not store address of loop iterator variable as it's the same
-		// address taking different values in each loop iteration, otherwise
-		// podSet would eventually contain only the last value.
-		// https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable
 		groupMemberSet.Insert(&group.GroupMembers[i])
 	}
 

pkg/agent/controller/networkpolicy/reconciler.go

@@ -541,7 +541,7 @@ func (r *reconciler) update(lastRealized *lastRealized, newRule *CompletedRule,
 
 		memberByServicesMap, servicesMap := groupMembersByServices(newRule.Services, newRule.ToAddresses)
 		// Same as the process in `add`, we must ensure the group for the original services is present
-		// in podsByServicesMap, so that this group won't be removed and its "From" will be updated.
+		// in memberByServicesMap, so that this group won't be removed and its "From" will be updated.
 		svcKey := normalizeServices(newRule.Services)
 		if _, exists := memberByServicesMap[svcKey]; !exists {
 			memberByServicesMap[svcKey] = v1beta1.NewGroupMemberSet()

pkg/controller/networkpolicy/antreanetworkpolicy.go

@@ -127,8 +127,8 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *secv1alpha1.Net
 	// Create AppliedToGroup for each AppliedTo present in
 	// AntreaNetworkPolicy spec.
 	for _, at := range np.Spec.AppliedTo {
-		groupName := n.createAppliedToGroup(np.Namespace, at.PodSelector, at.NamespaceSelector, at.ExternalEntitySelector)
-		appliedToGroupNames = append(appliedToGroupNames, groupName)
+		appliedToGroupNames = append(appliedToGroupNames, n.createAppliedToGroup(
+			np.Namespace, at.PodSelector, at.NamespaceSelector, at.ExternalEntitySelector))
 	}
 	rules := make([]controlplane.NetworkPolicyRule, 0, len(np.Spec.Ingress)+len(np.Spec.Egress))
 	// Compute NetworkPolicyRule for Egress Rule.

pkg/controller/networkpolicy/antreanetworkpolicy_test.go

@@ -18,6 +18,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
@@ -325,3 +326,60 @@ func TestAddANP(t *testing.T) {
 		})
 	}
 }
+
+func TestDeleteANP(t *testing.T) {
+	selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}}
+	anpObj := getANP()
+	apgID := getNormalizedUID(toGroupSelector("test-ns", &selectorA, nil, nil).NormalizedName)
+	_, npc := newController()
+	npc.addANP(anpObj)
+	npc.deleteANP(anpObj)
+	_, found, _ := npc.appliedToGroupStore.Get(apgID)
+	assert.False(t, found, "expected AppliedToGroup to be deleted")
+	adgs := npc.addressGroupStore.List()
+	assert.Len(t, adgs, 0, "expected empty AddressGroup list")
+	key, _ := keyFunc(anpObj)
+	_, found, _ = npc.internalNetworkPolicyStore.Get(key)
+	assert.False(t, found, "expected internal NetworkPolicy to be deleted")
+}
+
+// util functions for testing.
+func getANP() *secv1alpha1.NetworkPolicy {
+	p10 := float64(10)
+	allowAction := secv1alpha1.RuleActionAllow
+	selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}}
+	selectorB := metav1.LabelSelector{MatchLabels: map[string]string{"foo2": "bar2"}}
+	selectorC := metav1.LabelSelector{MatchLabels: map[string]string{"foo3": "bar3"}}
+	ingressRules := []secv1alpha1.Rule{
+		{
+			From: []secv1alpha1.NetworkPolicyPeer{
+				{
+					NamespaceSelector: &selectorB,
+				},
+			},
+			Action: &allowAction,
+		},
+	}
+	egressRules := []secv1alpha1.Rule{
+		{
+			To: []secv1alpha1.NetworkPolicyPeer{
+				{
+					ExternalEntitySelector: &selectorC,
+				},
+			},
+			Action: &allowAction,
+		},
+	}
+	npObj := &secv1alpha1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "test-ns", Name: "test-anp"},
+		Spec: secv1alpha1.NetworkPolicySpec{
+			AppliedTo: []secv1alpha1.NetworkPolicyPeer{
+				{PodSelector: &selectorA},
+			},
+			Priority: p10,
+			Ingress:  ingressRules,
+			Egress:   egressRules,
+		},
+	}
+	return npObj
+}

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -21,13 +21,13 @@ package networkpolicy
 import (
 	"fmt"
 	"net"
+	"reflect"
 	"sort"
 	"strconv"
 	"strings"
 	"sync"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
 	uuid "github.com/satori/go.uuid"
 	v1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
@@ -124,7 +124,7 @@ type NetworkPolicyController struct {
 	namespaceListerSynced cache.InformerSynced
 
 	externalEntityInformer corev1a1informers.ExternalEntityInformer
-	// externalEntityLister is able to list/get ExternalEnitities and is populated by the shared informer passed to
+	// externalEntityLister is able to list/get ExternalEntities and is populated by the shared informer passed to
 	// NewNetworkPolicyController.
 	externalEntityLister corev1a1listers.ExternalEntityLister
 	// externalEntitySynced is a function which returns true if the ExternalEntity shared informer has been synced at least once.
@@ -235,14 +235,6 @@ func NewNetworkPolicyController(kubeClient clientset.Interface,
 		},
 		resyncPeriod,
 	)
-	externalEntityInformer.Informer().AddEventHandlerWithResyncPeriod(
-		cache.ResourceEventHandlerFuncs{
-			AddFunc:    n.addExternalEntity,
-			UpdateFunc: n.updateExternalEntity,
-			DeleteFunc: n.deleteExternalEntity,
-		},
-		resyncPeriod,
-	)
 	// Add handlers for NetworkPolicy events.
 	networkPolicyInformer.Informer().AddEventHandlerWithResyncPeriod(
 		cache.ResourceEventHandlerFuncs{
@@ -276,6 +268,14 @@ func NewNetworkPolicyController(kubeClient clientset.Interface,
 			},
 			resyncPeriod,
 		)
+		externalEntityInformer.Informer().AddEventHandlerWithResyncPeriod(
+			cache.ResourceEventHandlerFuncs{
+				AddFunc:    n.addExternalEntity,
+				UpdateFunc: n.updateExternalEntity,
+				DeleteFunc: n.deleteExternalEntity,
+			},
+			resyncPeriod,
+		)
 	}
 	return n
 }
@@ -385,7 +385,7 @@ func (n *NetworkPolicyController) createAppliedToGroup(npNsName string, pSel, nS
 	return appliedToGroupUID
 }
 
-// labelsMatchGroupSelector matches a ExternalEntity or Pod's labels to the
+// labelsMatchGroupSelector matches an ExternalEntity or Pod's labels to the
 // GroupSelector object and returns true, if and only if the labels
 // match any of the selector criteria present in the GroupSelector.
 func (n *NetworkPolicyController) labelsMatchGroupSelector(obj metav1.Object, ns *v1.Namespace, sel *antreatypes.GroupSelector) bool {
@@ -432,7 +432,7 @@ func (n *NetworkPolicyController) labelsMatchGroupSelector(obj metav1.Object, ns
 	} else if objSelector != nil {
 		// Selector only has a PodSelector/ExternalEntitySelector and no sel.Namespace.
 		// Pods/ExternalEntities must be matched from all Namespaces.
-		if !sel.PodSelector.Matches(labels.Set(obj.GetLabels())) {
+		if !objSelector.Matches(labels.Set(obj.GetLabels())) {
 			// pod/ee labels do not match PodSelector/ExternalEntitySelector.
 			return false
 		}
@@ -881,7 +881,7 @@ func (n *NetworkPolicyController) deletePod(old interface{}) {
 func (n *NetworkPolicyController) addExternalEntity(obj interface{}) {
 	defer n.heartbeat("addExternalEntity")
 	ee := obj.(*v1alpha1.ExternalEntity)
-	klog.V(2).Infof("Processing External Entity %s/%s ADD event, labels: %v", ee.Namespace, ee.Name, ee.Labels)
+	klog.V(2).Infof("Processing ExternalEntity %s/%s ADD event, labels: %v", ee.Namespace, ee.Name, ee.Labels)
 	// Find all AppliedToGroup keys which match the ExternalEntity's labels.
 	appliedToGroupKeySet := n.filterAppliedToGroupsForPodOrExternalEntity(ee)
 	// Find all AddressGroup keys which match the ExternalEntity's labels.
@@ -904,11 +904,13 @@ func (n *NetworkPolicyController) updateExternalEntity(oldObj, curObj interface{
 	curEE := curObj.(*v1alpha1.ExternalEntity)
 	klog.V(2).Infof("Processing ExternalEntity %s/%s UPDATE event, labels: %v", curEE.Namespace, curEE.Name, curEE.Labels)
 	// No need to trigger processing of groups if there is no change in the
-	// Pod labels or Pods Node or Pods IP.
+	// ExternalEntity labels or ExternalEntity's Endpoints.
 	labelsEqual := labels.Equals(labels.Set(oldEE.Labels), labels.Set(curEE.Labels))
+	specEqual := reflect.DeepEqual(oldEE.Spec, curEE.Spec)
 	// TODO: Right now two ExternalEntities are only considered equal if the list of Endpoints and
-	//  all NamedPorts in each Endpoint are of the exact order. This constraint might be too strict.
-	if labelsEqual && cmp.Equal(oldEE.Spec, curEE.Spec) {
+	//  all NamedPorts in each Endpoint are of the exact order. Considering implementing custom compare
+	//  method for the ExternalEntity spec to solve this and improve performance.
+	if labelsEqual && specEqual {
 		klog.V(4).Infof("No change in ExternalEntity %s/%s. Skipping NetworkPolicy evaluation.", curEE.Namespace, curEE.Name)
 		return
 	}
@@ -923,7 +925,7 @@ func (n *NetworkPolicyController) updateExternalEntity(oldObj, curObj interface{
 	var addressGroupKeys sets.String
 	// AppliedToGroup keys must be enqueued only if the ExternalEntity's spec has changed or
 	// if ExternalEntity's label change causes it to match new Groups.
-	if !cmp.Equal(oldEE.Spec, curEE.Spec) {
+	if !specEqual {
 		appliedToGroupKeys = oldAppliedToGroupKeySet.Union(curAppliedToGroupKeySet)
 	} else if !labelsEqual {
 		// No need to enqueue common AppliedToGroups as they already have latest Pod
@@ -932,7 +934,7 @@ func (n *NetworkPolicyController) updateExternalEntity(oldObj, curObj interface{
 	}
 	// AddressGroup keys must be enqueued only if the ExternalEntity's spec has changed or
 	// if ExternalEntity's label change causes it to match new Groups.
-	if !cmp.Equal(oldEE.Spec, curEE.Spec) {
+	if !specEqual {
 		addressGroupKeys = oldAddressGroupKeySet.Union(curAddressGroupKeySet)
 	} else if !labelsEqual {
 		// No need to enqueue common AddressGroups as they already have latest Pod
@@ -947,7 +949,7 @@ func (n *NetworkPolicyController) updateExternalEntity(oldObj, curObj interface{
 	}
 }
 
-// deletePod retrieves all AddressGroups and AppliedToGroups which match the Pod's
+// deleteExternalEntity retrieves all AddressGroups and AppliedToGroups which match the ExternalEntity's
 // labels and enqueues the groups key for further processing.
 func (n *NetworkPolicyController) deleteExternalEntity(old interface{}) {
 	ee, ok := old.(*v1alpha1.ExternalEntity)
@@ -1298,7 +1300,7 @@ func (n *NetworkPolicyController) syncAddressGroup(key string) error {
 		podSet.Insert(podToMemberPod(pod, true, false))
 	}
 	for _, entity := range externalEntities {
-		memberSet.Insert(externalEntityToGroupMember(entity, true))
+		memberSet.Insert(externalEntityToGroupMember(entity))
 	}
 	updatedAddressGroup := &antreatypes.AddressGroup{
 		Name:         addressGroup.Name,
@@ -1346,7 +1348,7 @@ func podToMemberPod(pod *v1.Pod, includeIP, includePodRef bool) *controlplane.Gr
 	return memberPod
 }
 
-func externalEntityToGroupMember(ee *v1alpha1.ExternalEntity, includeRef bool) *controlplane.GroupMember {
+func externalEntityToGroupMember(ee *v1alpha1.ExternalEntity) *controlplane.GroupMember {
 	memberEntity := &controlplane.GroupMember{}
 	for _, endpoint := range ee.Spec.Endpoints {
 		var networkingPorts []controlplane.NamedPort
@@ -1363,13 +1365,11 @@ func externalEntityToGroupMember(ee *v1alpha1.ExternalEntity, includeRef bool) *
 		}
 		memberEntity.Endpoints = append(memberEntity.Endpoints, ep)
 	}
-	if includeRef {
-		entityRef := controlplane.ExternalEntityReference{
-			Name:      ee.Name,
-			Namespace: ee.Namespace,
-		}
-		memberEntity.ExternalEntity = &entityRef
+	entityRef := controlplane.ExternalEntityReference{
+		Name:      ee.Name,
+		Namespace: ee.Namespace,
 	}
+	memberEntity.ExternalEntity = &entityRef
 	return memberEntity
 }
 
@@ -1380,8 +1380,7 @@ func (n *NetworkPolicyController) processSelector(groupSelector antreatypes.Grou
 		// Namespace presence indicates Pods and ExternalEnitities must be selected from the same Namespace.
 		if groupSelector.PodSelector != nil {
 			pods, _ = n.podLister.Pods(groupSelector.Namespace).List(groupSelector.PodSelector)
-		}
-		if groupSelector.ExternalEntitySelector != nil {
+		} else if groupSelector.ExternalEntitySelector != nil {
 			externalEntities, _ = n.externalEntityLister.ExternalEntities(groupSelector.Namespace).List(groupSelector.ExternalEntitySelector)
 		}
 	} else if groupSelector.NamespaceSelector != nil && (groupSelector.PodSelector != nil || groupSelector.ExternalEntitySelector != nil) {
@@ -1391,14 +1390,13 @@ func (n *NetworkPolicyController) processSelector(groupSelector antreatypes.Grou
 			if groupSelector.PodSelector != nil {
 				nsPods, _ := n.podLister.Pods(ns.Name).List(groupSelector.PodSelector)
 				pods = append(pods, nsPods...)
-			}
-			if groupSelector.ExternalEntitySelector != nil {
+			} else if groupSelector.ExternalEntitySelector != nil {
 				nsExtEntities, _ := n.externalEntityLister.ExternalEntities(ns.Name).List(groupSelector.ExternalEntitySelector)
 				externalEntities = append(externalEntities, nsExtEntities...)
 			}
 		}
 	} else if groupSelector.NamespaceSelector != nil {
-		// All the Pods from Namespaces matching the nsSelector must be selected.
+		// All the Pods and EEs from Namespaces matching the nsSelector must be selected.
 		namespaces, _ := n.namespaceLister.List(groupSelector.NamespaceSelector)
 		for _, ns := range namespaces {
 			nsPods, _ := n.podLister.Pods(ns.Name).List(labels.Everything())
@@ -1466,7 +1464,7 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 		if entitySet == nil {
 			entitySet = controlplane.GroupMemberSet{}
 		}
-		entitySet.Insert(externalEntityToGroupMember(extEntity, true))
+		entitySet.Insert(externalEntityToGroupMember(extEntity))
 		memberSetByNode[extEntity.Spec.ExternalNode] = entitySet
 		appGroupNodeNames.Insert(extEntity.Spec.ExternalNode)
 	}

pkg/controller/networkpolicy/networkpolicy_controller_test.go

@@ -1346,7 +1346,7 @@ func TestAddExternalEntity(t *testing.T) {
 			updatedInAddrGroup := updatedInAddrGroupObj.(*antreatypes.AddressGroup)
 			updatedOutAddrGroupObj, _, _ := npc.addressGroupStore.Get(outGroupID)
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
-			member := externalEntityToGroupMember(tt.addedExternalEntity, true)
+			member := externalEntityToGroupMember(tt.addedExternalEntity)
 			assert.Equal(t, tt.inAddressGroupMatch, updatedInAddrGroup.GroupMembers.Has(member))
 			assert.Equal(t, tt.outAddressGroupMatch, updatedOutAddrGroup.GroupMembers.Has(member))
 		})

pkg/controller/types/networkpolicy.go

@@ -54,12 +54,15 @@ type GroupSelector struct {
 	// If Namespace is set, NamespaceSelector can not be set. It means only Pods in this Namespace will be matched.
 	Namespace string
 	// This is a label selector which selects Pods. If Namespace is also set, it selects the Pods in the Namespace.
-	// If NamespaceSelector is also set, it selects the Pods in the Namespaces selected by NamespaceSelector.
+	// If NamespaceSelector is set instead, it selects the Pods in the Namespaces selected by NamespaceSelector.
 	// If Namespace and NamespaceSelector both are unset, it selects the Pods in all the Namespaces.
 	PodSelector labels.Selector
 	// This is a label selector which selects Namespaces. It this field is set, Namespace can not be set.
 	NamespaceSelector labels.Selector
-	// This is a label selector which selects external entities.
+	// This is a label selector which selects ExternalEntities. Within a group, ExternalEntitySelector cannot be
+	// set concurrently with PodSelector. If Namespace is also set, it selects the ExternalEntities in the Namespace.
+	// If NamespaceSelector is set instead, it selects ExternalEntities in the Namespaces selected by NamespaceSelector.
+	// If Namespace and NamespaceSelector both are unset, it selects the ExternalEntities in all the Namespaces.
 	ExternalEntitySelector labels.Selector
 }