Fix AddressGroup memberKey not updated on pod IP update (antrea-io#1808)

* Fix identical GroupMember key across pod IP update

* Add UT and improve agent AddressGroup update logic

pkg/agent/controller/networkpolicy/reconciler.go

@@ -542,8 +542,8 @@ func (r *reconciler) update(lastRealized *lastRealized, newRule *CompletedRule,
 	if newRule.Direction == v1beta2.DirectionIn {
 		from1 := groupMembersToOFAddresses(newRule.FromAddresses)
 		from2 := ipBlocksToOFAddresses(newRule.From.IPBlocks, r.ipv4Enabled, r.ipv6Enabled)
-		addedFrom := groupMembersToOFAddresses(newRule.FromAddresses.Difference(lastRealized.FromAddresses))
-		deletedFrom := groupMembersToOFAddresses(lastRealized.FromAddresses.Difference(newRule.FromAddresses))
+		addedFrom := ipsToOFAddresses(newRule.FromAddresses.IPDifference(lastRealized.FromAddresses))
+		deletedFrom := ipsToOFAddresses(lastRealized.FromAddresses.IPDifference(newRule.FromAddresses))
 
 		podsByServicesMap, servicesMap := groupMembersByServices(newRule.Services, newRule.TargetMembers)
 		for svcKey, pods := range podsByServicesMap {

pkg/apis/controlplane/sets.go

@@ -14,7 +14,12 @@
 
 package controlplane
 
-import "strings"
+import (
+	"net"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+)
 
 // groupMemberKey is used to uniquely identify GroupMember.
 type groupMemberKey string
@@ -25,7 +30,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 and IPs.
+// For GroupMembers in appliedToGroups, the IPs are not set, so the
+// generated key does not contain IP information.
 func normalizeGroupMember(member *GroupMember) groupMemberKey {
 	// "/" is illegal in Namespace and name so is safe as the delimiter.
 	const delimiter = "/"
@@ -38,10 +45,9 @@ func normalizeGroupMember(member *GroupMember) groupMemberKey {
 		b.WriteString(member.ExternalEntity.Namespace)
 		b.WriteString(delimiter)
 		b.WriteString(member.ExternalEntity.Name)
-	} else if len(member.IPs) != 0 {
-		for _, ip := range member.IPs {
-			b.Write(ip)
-		}
+	}
+	for _, ip := range member.IPs {
+		b.Write(ip)
 	}
 	return groupMemberKey(b.String())
 }
@@ -84,6 +90,22 @@ func (s GroupMemberSet) Difference(o GroupMemberSet) GroupMemberSet {
 	return result
 }
 
+// IPDifference returns a String set of GroupMember IPs that are not in o.
+func (s GroupMemberSet) IPDifference(o GroupMemberSet) sets.String {
+	sIPs, oIPs := sets.NewString(), sets.NewString()
+	for _, m := range s {
+		for _, ip := range m.IPs {
+			sIPs.Insert(net.IP(ip).String())
+		}
+	}
+	for _, m := range o {
+		for _, ip := range m.IPs {
+			oIPs.Insert(net.IP(ip).String())
+		}
+	}
+	return sIPs.Difference(oIPs)
+}
+
 // Union returns a new set which includes items in either m or o.
 func (s GroupMemberSet) Union(o GroupMemberSet) GroupMemberSet {
 	result := GroupMemberSet{}

pkg/apis/controlplane/v1beta2/sets.go

@@ -15,7 +15,10 @@
 package v1beta2
 
 import (
+	"net"
 	"strings"
+
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // groupMemberKey is used to uniquely identify GroupMember.
@@ -27,7 +30,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 and IPs.
+// For GroupMembers in appliedToGroups, the IPs are not set, so the
+// generated key does not contain IP information.
 func normalizeGroupMember(member *GroupMember) groupMemberKey {
 	// "/" is illegal in Namespace and name so is safe as the delimiter.
 	const delimiter = "/"
@@ -40,10 +45,9 @@ func normalizeGroupMember(member *GroupMember) groupMemberKey {
 		b.WriteString(member.ExternalEntity.Namespace)
 		b.WriteString(delimiter)
 		b.WriteString(member.ExternalEntity.Name)
-	} else if len(member.IPs) != 0 {
-		for _, ip := range member.IPs {
-			b.Write(ip)
-		}
+	}
+	for _, ip := range member.IPs {
+		b.Write(ip)
 	}
 	return groupMemberKey(b.String())
 }
@@ -86,6 +90,22 @@ func (s GroupMemberSet) Difference(o GroupMemberSet) GroupMemberSet {
 	return result
 }
 
+// IPDifference returns a String set of GroupMember IPs that are not in o.
+func (s GroupMemberSet) IPDifference(o GroupMemberSet) sets.String {
+	sIPs, oIPs := sets.NewString(), sets.NewString()
+	for _, m := range s {
+		for _, ip := range m.IPs {
+			sIPs.Insert(net.IP(ip).String())
+		}
+	}
+	for _, m := range o {
+		for _, ip := range m.IPs {
+			oIPs.Insert(net.IP(ip).String())
+		}
+	}
+	return sIPs.Difference(oIPs)
+}
+
 // Union returns a new set which includes items in either m or o.
 func (s GroupMemberSet) Union(o GroupMemberSet) GroupMemberSet {
 	result := GroupMemberSet{}

pkg/controller/networkpolicy/antreanetworkpolicy_test.go

@@ -309,8 +309,8 @@ func TestProcessAntreaNetworkPolicy(t *testing.T) {
 					},
 				},
 				AppliedToGroups: []string{
-					getNormalizedUID(toGroupSelector("ns3", &selectorA, nil, nil).NormalizedName),
 					getNormalizedUID(toGroupSelector("ns3", &selectorB, nil, nil).NormalizedName),
+					getNormalizedUID(toGroupSelector("ns3", &selectorA, nil, nil).NormalizedName),
 				},
 				AppliedToPerRule: true,
 			},

pkg/controller/networkpolicy/external_entity_test.go

@@ -19,6 +19,7 @@
 package networkpolicy
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -30,6 +31,69 @@ 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
+	}{
+		{
+			name:    "ee-with-ip",
+			inputEE: testEntity,
+			expMemberEE: controlplane.GroupMember{
+				ExternalEntity: &controlplane.ExternalEntityReference{
+					Name:      testEntity.Name,
+					Namespace: testEntity.Namespace,
+				},
+				IPs: []controlplane.IPAddress{ipStrToIPAddress(testEntity.Spec.Endpoints[0].IP)},
+				Ports: []controlplane.NamedPort{
+					{
+						Port:     80,
+						Name:     "http",
+						Protocol: "tcp",
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			actualMemberEE := externalEntityToGroupMember(tt.inputEE)
+			if !reflect.DeepEqual(*(*actualMemberEE).ExternalEntity, *(tt.expMemberEE).ExternalEntity) {
+				t.Errorf("externalEntityToGroupMember() got unexpected EEReference %v, want %v", *(*actualMemberEE).ExternalEntity, *(tt.expMemberEE).ExternalEntity)
+			}
+			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"},

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -83,9 +83,9 @@ var (
 	// uuidNamespace is a uuid.UUID type generated from a string to be
 	// used to generate uuid.UUID for internal Antrea objects like
 	// AppliedToGroup, AddressGroup etc.
-	// 5a5e7dd9-e3fb-49bb-b263-9bab25c95841 was generated using
+	// e4f24a48-ca1f-4d5b-819c-ea7632b22115 was generated using
 	// uuid.NewV4() function.
-	uuidNamespace = uuid.FromStringOrNil("5a5e7dd9-e3fb-49bb-b263-9bab25c95841")
+	uuidNamespace = uuid.FromStringOrNil("e4f24a48-ca1f-4d5b-819c-ea7632b22115")
 
 	// matchAllPeer is a NetworkPolicyPeer matching all source/destination IP addresses. Both IPv4 Any (0.0.0.0/0) and
 	// IPv6 Any (::/0) are added into the IPBlocks, and Antrea Agent should decide if both two are used according the
@@ -1237,7 +1237,7 @@ 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
 		}

pkg/controller/networkpolicy/networkpolicy_controller_test.go

@@ -943,6 +943,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        false,
@@ -971,6 +974,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        false,
@@ -1002,6 +1008,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        true,
@@ -1030,6 +1039,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        false,
@@ -1058,6 +1070,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        false,
@@ -1091,6 +1106,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        true,
@@ -1173,6 +1191,9 @@ func TestAddPod(t *testing.T) {
 						},
 					},
 					PodIP: "1.2.3.4",
+					PodIPs: []corev1.PodIP{
+						{IP: "1.2.3.4"},
+					},
 				},
 			},
 			appGroupMatch:        false,
@@ -2536,22 +2557,22 @@ func TestPodToGroupMember(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			actualMemberPod := podToGroupMember(tt.inputPod, tt.includeIP)
 			if !reflect.DeepEqual(*(*actualMemberPod).Pod, *(tt.expMemberPod).Pod) {
-				t.Errorf("podToMemberPod() got unexpected PodReference %v, want %v", *(*actualMemberPod).Pod, *(tt.expMemberPod).Pod)
+				t.Errorf("podToGroupMember() got unexpected PodReference %v, want %v", *(*actualMemberPod).Pod, *(tt.expMemberPod).Pod)
 			}
 			// Case where the IPAddress must not be populated.
 			if !tt.includeIP {
 				if len(actualMemberPod.IPs) > 0 {
-					t.Errorf("podToMemberPod() got unexpected IP %v, want nil", actualMemberPod.IPs)
+					t.Errorf("podToGroupMember() got unexpected IP %v, want nil", actualMemberPod.IPs)
 				}
 			} else if !comparePodIPs(actualMemberPod.IPs, tt.expMemberPod.IPs) {
-				t.Errorf("podToMemberPod() got unexpected IP %v, want %v", actualMemberPod.IPs, tt.expMemberPod.IPs)
+				t.Errorf("podToGroupMember() got unexpected IP %v, want %v", actualMemberPod.IPs, tt.expMemberPod.IPs)
 			}
 			if !tt.namedPort {
 				if len(actualMemberPod.Ports) > 0 {
-					t.Errorf("podToMemberPod() got unexpected Ports %v, want []", actualMemberPod.Ports)
+					t.Errorf("podToGroupMember() got unexpected Ports %v, want []", actualMemberPod.Ports)
 				}
 			} else if !reflect.DeepEqual(actualMemberPod.Ports, tt.expMemberPod.Ports) {
-				t.Errorf("podToMemberPod() got unexpected Ports %v, want %v", actualMemberPod.Ports, tt.expMemberPod.Ports)
+				t.Errorf("podToGroupMember() got unexpected Ports %v, want %v", actualMemberPod.Ports, tt.expMemberPod.Ports)
 			}
 		})
 	}