Fix controller issue in handling empty pod labels for labelidentity (#…

…5404) When a Pod is created without any labels at all, its labelidentity will include "pod:<none>" as defined by FormatLabels in apimachinery. This case is not handled correctly by the label group index and can cause the controller to crash. Signed-off-by: Dyanngg <[email protected]>
antrea-io · Aug 22, 2023 · 3e2e829 · 3e2e829
1 parent bd3c008
commit 3e2e829
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 1 deletion.
diff --git a/multicluster/controllers/multicluster/member/labelidentity_controller.go b/multicluster/controllers/multicluster/member/labelidentity_controller.go
@@ -326,7 +326,7 @@ func GetNormalizedLabel(nsLabels, podLabels map[string]string, ns string) string
 		// label is guaranteed to have Namespace name information.
 		nsLabels[v1.LabelMetadataName] = ns
 	}
-	return "ns:" + labels.FormatLabels(nsLabels) + "&pod:" + labels.FormatLabels(podLabels)
+	return "ns:" + labels.Set(nsLabels).String() + "&pod:" + labels.Set(podLabels).String()
 }
 
 // getResourceExportNameForLabelIdentity retrieves the ResourceExport name for exporting

diff --git a/multicluster/controllers/multicluster/member/labelidentity_controller_test.go b/multicluster/controllers/multicluster/member/labelidentity_controller_test.go
@@ -271,6 +271,13 @@ func TestGetNormalizedLabel(t *testing.T) {
 			map[string]string{"region": "west"},
 			"ns:kubernetes.io/metadata.name=test-ns,region=west&pod:purpose=test",
 		},
+		{
+			"no Pod label",
+			"test-ns",
+			map[string]string{},
+			map[string]string{"region": "west"},
+			"ns:kubernetes.io/metadata.name=test-ns,region=west&pod:",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/pkg/controller/labelidentity/label_group_index.go b/pkg/controller/labelidentity/label_group_index.go
@@ -124,6 +124,9 @@ 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 == "" {
+		return m
+	}
 	kvs := strings.Split(s, ",")
 	for _, kv := range kvs {
 		kvpair := strings.Split(kv, "=")

diff --git a/pkg/controller/labelidentity/label_group_index_test.go b/pkg/controller/labelidentity/label_group_index_test.go
@@ -42,6 +42,8 @@ var (
 	selectorC     = types.NewGroupSelector("", pSelDB, nil, nil, nil)
 	selectorD     = types.NewGroupSelector("testing", pSelDB, nil, nil, nil)
 	selectorE     = types.NewGroupSelector("random", pSelDB, nil, nil, nil)
+	selectorF     = types.NewGroupSelector("", nil, nsSelTest, nil, nil)
+	selectorG     = types.NewGroupSelector("testing", nil, nil, nil, nil)
 	selectorItemA = &selectorItem{
 		selector: selectorA,
 	}
@@ -57,9 +59,16 @@ var (
 	selectorItemE = &selectorItem{
 		selector: selectorE,
 	}
+	selectorItemF = &selectorItem{
+		selector: selectorF,
+	}
+	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:"
 )
 
 func TestLabelIdentityMatch(t *testing.T) {
@@ -128,6 +137,41 @@ func TestLabelIdentityMatch(t *testing.T) {
 			selector:    selectorItemE,
 			expectMatch: false,
 		},
+		{
+			label:       labelD,
+			selector:    selectorItemA,
+			expectMatch: false,
+		},
+		{
+			label:       labelD,
+			selector:    selectorItemB,
+			expectMatch: false,
+		},
+		{
+			label:       labelD,
+			selector:    selectorItemD,
+			expectMatch: false,
+		},
+		{
+			label:       labelD,
+			selector:    selectorItemF,
+			expectMatch: true,
+		},
+		{
+			label:       labelA,
+			selector:    selectorItemG,
+			expectMatch: true,
+		},
+		{
+			label:       labelC,
+			selector:    selectorItemG,
+			expectMatch: false,
+		},
+		{
+			label:       labelD,
+			selector:    selectorItemG,
+			expectMatch: true,
+		},
 	}
 	for _, tt := range tests {
 		labelMatch := newLabelIdentityMatch(tt.label, 1)