Fix GroupMember misuse in AppliedTo and AddressGroups

pkg/apis/controlplane/sets.go

@@ -25,7 +25,9 @@ type groupMemberKey string
 type GroupMemberSet map[groupMemberKey]*GroupMember
 
 // normalizeGroupMember calculates the groupMemberKey of the provided
-// GroupMember based on the Pod's namespaced name or IP.
+// GroupMember based on the Pod/ExternalEntity's namespaced name or IP.
+// For GroupMembers used in appliedToGroups, key is namespaced name.
+// For GroupMembers used in addressGroups, key is IPs of the entity.
 func normalizeGroupMember(member *GroupMember) groupMemberKey {
 	// "/" is illegal in Namespace and name so is safe as the delimiter.
 	const delimiter = "/"

pkg/apis/controlplane/v1beta1/conversion.go

@@ -134,10 +134,12 @@ func Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(in *GroupMemberP
 // This function doesn't match the pattern of conversion function which requires the last parameter to be
 // conversion.Scope so won't be registered to schema.
 func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, includePodRef bool) error {
-	if in.Pod == nil || len(in.IPs) > 1 {
-		return fmt.Errorf("cannot convert ExternalEntity or dual stack Pod into GroupMemberPod")
-	}
+	// PodRef should only be included for GroupMembers in AppliedToGroups. For GroupMembers
+	// in AddressGroups, in.Pod is expected to be nil.
 	if includePodRef {
+		if in.Pod == nil || len(in.IPs) > 1 {
+			return fmt.Errorf("cannot convert ExternalEntity or dual stack Pod into GroupMemberPod")
+		}
 		out.Pod = &PodReference{
 			Name:      in.Pod.Name,
 			Namespace: in.Pod.Namespace,

pkg/apis/controlplane/v1beta2/sets.go

@@ -27,7 +27,9 @@ type groupMemberKey string
 type GroupMemberSet map[groupMemberKey]*GroupMember
 
 // normalizeGroupMember calculates the groupMemberKey of the provided
-// GroupMember based on the Pod's namespaced name or IP.
+// GroupMember based on the Pod/ExternalEntity's namespaced name or IP.
+// For GroupMembers used in appliedToGroups, key is namespaced name.
+// For GroupMembers used in addressGroups, key is IPs of the entity.
 func normalizeGroupMember(member *GroupMember) groupMemberKey {
 	// "/" is illegal in Namespace and name so is safe as the delimiter.
 	const delimiter = "/"

pkg/controller/networkpolicy/clustergroup.go

@@ -187,11 +187,14 @@ func (n *NetworkPolicyController) syncInternalGroup(key string) error {
 		pods, _ := n.processSelector(groupSelector)
 		memberSet := controlplane.GroupMemberSet{}
 		for _, pod := range pods {
-			if pod.Status.PodIP == "" {
+			if len(pod.Status.PodIPs) == 0 {
 				// No need to insert Pod IPAddress when it is unset.
 				continue
 			}
-			memberSet.Insert(podToGroupMember(pod, true))
+			// When the internalGroup is referenced in Antrea policies, the groupMembers
+			// will only include reference in case of appliedToGroup, and only include
+			// IPs in case of addressGroup.
+			memberSet.Insert(podToGroupMember(pod, true, true))
 		}
 		// Update the internal Group object in the store with the Pods as GroupMembers.
 		updatedGrp := &antreatypes.Group{

pkg/controller/networkpolicy/external_entity_test.go

@@ -19,6 +19,7 @@
 package networkpolicy
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -30,6 +31,98 @@ import (
 	antreatypes "github.com/vmware-tanzu/antrea/pkg/controller/types"
 )
 
+func TestExternalEntityToGroupMember(t *testing.T) {
+	testEntity := &v1alpha2.ExternalEntity{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "eeA",
+			Namespace: "nsA",
+			Labels: map[string]string{
+				"app": "test",
+			},
+		},
+		Spec: v1alpha2.ExternalEntitySpec{
+			Endpoints: []v1alpha2.Endpoint{
+				{
+					IP:   "22.33.44.55",
+					Name: "vm1",
+				},
+			},
+			Ports: []v1alpha2.NamedPort{
+				{
+					Port:     80,
+					Name:     "http",
+					Protocol: "tcp",
+				},
+			},
+			ExternalNode: "cloud-node-1",
+		},
+	}
+	tests := []struct {
+		name        string
+		inputEE     *v1alpha2.ExternalEntity
+		expMemberEE controlplane.GroupMember
+		includeIP   bool
+		includeRef  bool
+	}{
+		{
+			name:    "ee-with-ip",
+			inputEE: testEntity,
+			expMemberEE: controlplane.GroupMember{
+				IPs: []controlplane.IPAddress{ipStrToIPAddress(testEntity.Spec.Endpoints[0].IP)},
+				Ports: []controlplane.NamedPort{
+					{
+						Port:     80,
+						Name:     "http",
+						Protocol: "tcp",
+					},
+				},
+			},
+			includeIP:  true,
+			includeRef: false,
+		},
+		{
+			name:    "ee-with-ref",
+			inputEE: testEntity,
+			expMemberEE: controlplane.GroupMember{
+				ExternalEntity: &controlplane.ExternalEntityReference{
+					Name:      testEntity.Name,
+					Namespace: testEntity.Namespace,
+				},
+				Ports: []controlplane.NamedPort{
+					{
+						Port:     80,
+						Name:     "http",
+						Protocol: "tcp",
+					},
+				},
+			},
+			includeIP:  false,
+			includeRef: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			actualMemberEE := externalEntityToGroupMember(tt.inputEE, tt.includeIP, tt.includeRef)
+			// Case where the ExternalEntity reference must not be populated.
+			if !tt.includeRef {
+				if actualMemberEE.ExternalEntity != nil {
+					t.Errorf("externalEntityToGroupMember() got unexpected ExternalEntity reference %v, want nil", actualMemberEE.ExternalEntity)
+				}
+			} else if !reflect.DeepEqual(*(*actualMemberEE).ExternalEntity, *(tt.expMemberEE).ExternalEntity) {
+				t.Errorf("externalEntityToGroupMember() got unexpected EEReference %v, want %v", *(*actualMemberEE).ExternalEntity, *(tt.expMemberEE).ExternalEntity)
+			}
+			// Case where the IPAddress must not be populated.
+			if !tt.includeIP {
+				if len(actualMemberEE.IPs) > 0 {
+					t.Errorf("externalEntityToGroupMember() got unexpected IP %v, want nil", actualMemberEE.IPs)
+				}
+			} else if !comparePodIPs(actualMemberEE.IPs, tt.expMemberEE.IPs) {
+				t.Errorf("externalEntityToGroupMember() got unexpected IP %v, want %v", actualMemberEE.IPs, tt.expMemberEE.IPs)
+			}
+		})
+	}
+}
+
 func TestAddExternalEntity(t *testing.T) {
 	selectorSpec := metav1.LabelSelector{
 		MatchLabels: map[string]string{"group": "appliedTo"},
@@ -139,7 +232,7 @@ func TestAddExternalEntity(t *testing.T) {
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
 			var member *controlplane.GroupMember
 			if ee, ok := tt.addedExternalEntity.(*v1alpha2.ExternalEntity); ok {
-				member = externalEntityToGroupMember(ee)
+				member = externalEntityToGroupMember(ee, true, false)
 			}
 			assert.Equal(t, tt.inAddressGroupMatch, updatedInAddrGroup.GroupMembers.Has(member))
 			assert.Equal(t, tt.outAddressGroupMatch, updatedOutAddrGroup.GroupMembers.Has(member))
@@ -300,7 +393,7 @@ func TestDeleteExternalEntity(t *testing.T) {
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
 			var member *controlplane.GroupMember
 			if ee, ok := tt.addedExternalEntity.(*v1alpha2.ExternalEntity); ok {
-				member = externalEntityToGroupMember(ee)
+				member = externalEntityToGroupMember(ee, true, true)
 			}
 
 			if tt.inAddressGroupMatch {

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -551,6 +551,7 @@ func (n *NetworkPolicyController) filterAddressGroupsForPodOrExternalEntity(obj
 		addrGroup := group.(*antreatypes.AddressGroup)
 		if n.labelsMatchGroupSelector(obj, ns, &addrGroup.Selector) {
 			matchingKeySet.Insert(addrGroup.Name)
+			fmt.Println("a match!")
 			klog.V(2).Infof("%s/%s matched AddressGroup %s", obj.GetNamespace(), obj.GetName(), addrGroup.Name)
 		}
 	}
@@ -1301,14 +1302,14 @@ func (n *NetworkPolicyController) syncAddressGroup(key string) error {
 	pods, externalEntities := n.processSelector(groupSelector)
 	memberSet := controlplane.GroupMemberSet{}
 	for _, pod := range pods {
-		if pod.Status.PodIP == "" {
+		if len(pod.Status.PodIPs) == 0 {
 			// No need to insert Pod IPAddress when it is unset.
 			continue
 		}
-		memberSet.Insert(podToGroupMember(pod, true))
+		memberSet.Insert(podToGroupMember(pod, true, false))
 	}
 	for _, entity := range externalEntities {
-		memberSet.Insert(externalEntityToGroupMember(entity))
+		memberSet.Insert(externalEntityToGroupMember(entity, true, false))
 	}
 	updatedAddressGroup := &antreatypes.AddressGroup{
 		Name:         addressGroup.Name,
@@ -1322,12 +1323,12 @@ func (n *NetworkPolicyController) syncAddressGroup(key string) error {
 	return nil
 }
 
-// podToGroupMember is util function to convert a Pod to a GroupMember type.
+// podToGroupMember is a util function to convert a Pod to GroupMember type.
 // A controlplane.NamedPort item will be set in the GroupMember, only if the
-// Pod contains a Port with the name field set. PodReference will also be set
-// for converting GroupMember to GroupMemberPod for clients using older version
-// of the controlplane API.
-func podToGroupMember(pod *v1.Pod, includeIP bool) *controlplane.GroupMember {
+// Pod contains a Port with the name field set. PodReference should only be set
+// if the GroupMember is used in an appliedToGroup. For GroupMembers in
+// addressGroups, podRef will be nil.
+func podToGroupMember(pod *v1.Pod, includeIP, includeRef bool) *controlplane.GroupMember {
 	memberPod := &controlplane.GroupMember{}
 	for _, container := range pod.Spec.Containers {
 		for _, port := range container.Ports {
@@ -1346,35 +1347,42 @@ func podToGroupMember(pod *v1.Pod, includeIP bool) *controlplane.GroupMember {
 			memberPod.IPs = append(memberPod.IPs, ipStrToIPAddress(podIP.IP))
 		}
 	}
-	podRef := controlplane.PodReference{
-		Name:      pod.Name,
-		Namespace: pod.Namespace,
+	if includeRef {
+		podRef := controlplane.PodReference{
+			Name:      pod.Name,
+			Namespace: pod.Namespace,
+		}
+		memberPod.Pod = &podRef
 	}
-	memberPod.Pod = &podRef
 	return memberPod
 }
 
-func externalEntityToGroupMember(ee *v1alpha2.ExternalEntity) *controlplane.GroupMember {
+// externalEntityToGroupMember is a util function to convert an ExternalEntity to
+// GroupMember type. ExternalEntityReference should only be set if the GroupMember
+// is used in an appliedToGroup. For GroupMembers in addressGroups, eeRef will be nil.
+func externalEntityToGroupMember(ee *v1alpha2.ExternalEntity, includeIP, includeRef bool) *controlplane.GroupMember {
 	memberEntity := &controlplane.GroupMember{}
 	namedPorts := make([]controlplane.NamedPort, len(ee.Spec.Ports))
-	var ips []controlplane.IPAddress
 	for i, port := range ee.Spec.Ports {
 		namedPorts[i] = controlplane.NamedPort{
 			Port:     port.Port,
 			Name:     port.Name,
 			Protocol: controlplane.Protocol(port.Protocol),
 		}
 	}
-	for _, ep := range ee.Spec.Endpoints {
-		ips = append(ips, ipStrToIPAddress(ep.IP))
+	if includeIP {
+		for _, ep := range ee.Spec.Endpoints {
+			memberEntity.IPs = append(memberEntity.IPs, ipStrToIPAddress(ep.IP))
+		}
 	}
-	eeRef := controlplane.ExternalEntityReference{
-		Name:      ee.Name,
-		Namespace: ee.Namespace,
+	if includeRef {
+		eeRef := controlplane.ExternalEntityReference{
+			Name:      ee.Name,
+			Namespace: ee.Namespace,
+		}
+		memberEntity.ExternalEntity = &eeRef
 	}
-	memberEntity.ExternalEntity = &eeRef
 	memberEntity.Ports = namedPorts
-	memberEntity.IPs = ips
 	return memberEntity
 }
 
@@ -1451,7 +1459,7 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 		if podSet == nil {
 			podSet = controlplane.GroupMemberSet{}
 		}
-		podSet.Insert(podToGroupMember(pod, false))
+		podSet.Insert(podToGroupMember(pod, false, true))
 		// Update the Pod references by Node.
 		memberSetByNode[pod.Spec.NodeName] = podSet
 		// Update the NodeNames in order to set the SpanMeta for AppliedToGroup.
@@ -1466,7 +1474,7 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 		if entitySet == nil {
 			entitySet = controlplane.GroupMemberSet{}
 		}
-		entitySet.Insert(externalEntityToGroupMember(extEntity))
+		entitySet.Insert(externalEntityToGroupMember(extEntity, true, true))
 		memberSetByNode[extEntity.Spec.ExternalNode] = entitySet
 		appGroupNodeNames.Insert(extEntity.Spec.ExternalNode)
 	}