Handle stale LabelIdentities created for empty Pod labels correctly a…

…fter upgrade

This is a follow-up on PR antrea-io#5404 which intends to address issue antrea-io#5403.
In upgrade cases, Antrea controller will still receive LabelIdentity add events
for LabelIdentities created by previous controller, which it needs to handle
correctly. Also, the stale controller logic needs to be updated to ensure that
LabelIdentities created with old normalization method are cleaned up after upgrade.

Signed-off-by: Dyanngg <dingyang@vmware.com>

multicluster/controllers/multicluster/stale_controller.go

@@ -339,7 +339,7 @@ func (c *StaleResCleanupController) cleanupLabelIdentityResourceExport(ctx conte
 		if !ok {
 			continue
 		}
-		normalizedLabel := podNSlabel + "&pod:" + labels.FormatLabels(p.Labels)
+		normalizedLabel := podNSlabel + "&pod:" + labels.Set(p.Labels).String()
 		delete(staleResExpItems, normalizedLabel)
 	}
 	for _, r := range staleResExpItems {

multicluster/controllers/multicluster/stale_controller_test.go

@@ -315,10 +315,18 @@ func TestStaleController_CleanupResourceExport(t *testing.T) {
 					},
 				},
 			},
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "test-ns",
+					Name:      "test-pod-empty-labels",
+				},
+			},
 		},
 	}
 	labelNormalizedExist := "ns:kubernetes.io/metadata.name=test-ns,purpose=test&pod:app=web"
 	labelNormalizedNonExist := "ns:kubernetes.io/metadata.name=test-ns,purpose=test&pod:app=db"
+	labelNormalizedEmptyLabels := "ns:kubernetes.io/metadata.name=test-ns,purpose=test&pod:"
+	labelNormalizedEmptyLabelsStale := "ns:kubernetes.io/metadata.name=test-ns,purpose=test&pod:<none>"
 	toKeepLabelResExport := mcv1alpha1.ResourceExport{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "default",
@@ -335,6 +343,22 @@ func TestStaleController_CleanupResourceExport(t *testing.T) {
 			},
 		},
 	}
+	toKeepLabelResExportEmptyLabels := mcv1alpha1.ResourceExport{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+			Name:      "cluster-a-" + common.HashLabelIdentity(labelNormalizedEmptyLabels),
+			Labels: map[string]string{
+				constants.SourceKind:      constants.LabelIdentityKind,
+				constants.SourceClusterID: "cluster-a",
+			},
+		},
+		Spec: mcv1alpha1.ResourceExportSpec{
+			Kind: constants.LabelIdentityKind,
+			LabelIdentity: &mcv1alpha1.LabelIdentityExport{
+				NormalizedLabel: labelNormalizedEmptyLabels,
+			},
+		},
+	}
 	toDeleteLabelResExport := mcv1alpha1.ResourceExport{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "default",
@@ -351,6 +375,22 @@ func TestStaleController_CleanupResourceExport(t *testing.T) {
 			},
 		},
 	}
+	toDeleteLabelResExportEmptyLabels := mcv1alpha1.ResourceExport{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+			Name:      "cluster-a-" + common.HashLabelIdentity(labelNormalizedEmptyLabelsStale),
+			Labels: map[string]string{
+				constants.SourceKind:      constants.LabelIdentityKind,
+				constants.SourceClusterID: "cluster-a",
+			},
+		},
+		Spec: mcv1alpha1.ResourceExportSpec{
+			Kind: constants.LabelIdentityKind,
+			LabelIdentity: &mcv1alpha1.LabelIdentityExport{
+				NormalizedLabel: labelNormalizedEmptyLabelsStale,
+			},
+		},
+	}
 	tests := []struct {
 		name                   string
 		existSvcList           *corev1.ServiceList
@@ -378,6 +418,8 @@ func TestStaleController_CleanupResourceExport(t *testing.T) {
 					toKeepSvcResExport,
 					toKeepLabelResExport,
 					toDeleteLabelResExport,
+					toKeepLabelResExportEmptyLabels,
+					toDeleteLabelResExportEmptyLabels,
 					svcResExportFromOtherCluster,
 				},
 			},
@@ -399,8 +441,8 @@ func TestStaleController_CleanupResourceExport(t *testing.T) {
 			err := fakeRemoteClient.List(context.TODO(), resExpList, &client.ListOptions{})
 			resExpLen := len(resExpList.Items)
 			if err == nil {
-				if resExpLen != 3 {
-					t.Errorf("Should only THREE valid ResourceExports left but got %v", resExpLen)
+				if resExpLen != 4 {
+					t.Errorf("Should only FOUR valid ResourceExports left but got %v", resExpLen)
 				}
 			} else {
 				t.Errorf("Should list ResourceExport successfully but got err = %v", err)

pkg/controller/labelidentity/label_group_index.go

@@ -124,7 +124,12 @@ func (l *labelIdentityMatch) matches(s *selectorItem) bool {
 // constructMapFromLabelString parses label string of format "app=client,env=dev" into a map.
 func constructMapFromLabelString(s string) map[string]string {
 	m := map[string]string{}
-	if s == "" {
+	// Before https://github.com/antrea-io/antrea/issues/5403 is fixed, LabelIdentities created
+	// for Pods with an empty label set will include a <none> string. Handling for such LabelIdentities
+	// are needed, as in multi-cluster controller upgrade cases, these LabelIdentities created by
+	// previous controller still need to be processed, but will be cleaned up by the stale controller
+	// eventually.
+	if s == "" || s == "<none>" {
 		return m
 	}
 	kvs := strings.Split(s, ",")

pkg/controller/labelidentity/label_group_index_test.go

@@ -65,10 +65,11 @@ var (
 	selectorItemG = &selectorItem{
 		selector: selectorG,
 	}
-	labelA = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:app=web"
-	labelB = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:app=db"
-	labelC = "ns:kubernetes.io/metadata.name=nomatch,purpose=nomatch&pod:app=db"
-	labelD = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:"
+	labelA     = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:app=web"
+	labelB     = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:app=db"
+	labelC     = "ns:kubernetes.io/metadata.name=nomatch,purpose=nomatch&pod:app=db"
+	labelD     = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:"
+	labelStale = "ns:kubernetes.io/metadata.name=testing,purpose=test&pod:<none>"
 )
 
 func TestLabelIdentityMatch(t *testing.T) {
@@ -172,6 +173,11 @@ func TestLabelIdentityMatch(t *testing.T) {
 			selector:    selectorItemG,
 			expectMatch: true,
 		},
+		{
+			label:       labelStale,
+			selector:    selectorItemA,
+			expectMatch: false,
+		},
 	}
 	for _, tt := range tests {
 		labelMatch := newLabelIdentityMatch(tt.label, 1)