From f1c922cdf171e8e1854959a5d926a44208ce85d8 Mon Sep 17 00:00:00 2001 From: Dyanngg Date: Mon, 28 Aug 2023 13:10:32 -0700 Subject: [PATCH] Handle stale LabelIdentities created for empty Pod labels correctly after upgrade This is a follow-up on PR #5404 which intends to address issue #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 --- .../multicluster/stale_controller.go | 2 +- .../multicluster/stale_controller_test.go | 46 ++++++++++++++++++- .../labelidentity/label_group_index.go | 7 ++- .../labelidentity/label_group_index_test.go | 14 ++++-- 4 files changed, 61 insertions(+), 8 deletions(-) diff --git a/multicluster/controllers/multicluster/stale_controller.go b/multicluster/controllers/multicluster/stale_controller.go index 5c1b031d48a..a84bfbc2abf 100644 --- a/multicluster/controllers/multicluster/stale_controller.go +++ b/multicluster/controllers/multicluster/stale_controller.go @@ -339,7 +339,7 @@ func (c *StaleResCleanupController) cleanupLabelIdentityResourceExport(commonAre 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 { diff --git a/multicluster/controllers/multicluster/stale_controller_test.go b/multicluster/controllers/multicluster/stale_controller_test.go index 97949ebd5ca..a3f7a47738a 100644 --- a/multicluster/controllers/multicluster/stale_controller_test.go +++ b/multicluster/controllers/multicluster/stale_controller_test.go @@ -312,10 +312,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:" toKeepLabelResExport := mcsv1alpha1.ResourceExport{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -332,6 +340,22 @@ func TestStaleController_CleanupResourceExport(t *testing.T) { }, }, } + toKeepLabelResExportEmptyLabels := mcsv1alpha1.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: mcsv1alpha1.ResourceExportSpec{ + Kind: constants.LabelIdentityKind, + LabelIdentity: &mcsv1alpha1.LabelIdentityExport{ + NormalizedLabel: labelNormalizedEmptyLabels, + }, + }, + } toDeleteLabelResExport := mcsv1alpha1.ResourceExport{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -348,6 +372,22 @@ func TestStaleController_CleanupResourceExport(t *testing.T) { }, }, } + toDeleteLabelResExportEmptyLabels := mcsv1alpha1.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: mcsv1alpha1.ResourceExportSpec{ + Kind: constants.LabelIdentityKind, + LabelIdentity: &mcsv1alpha1.LabelIdentityExport{ + NormalizedLabel: labelNormalizedEmptyLabelsStale, + }, + }, + } tests := []struct { name string existSvcList *corev1.ServiceList @@ -375,6 +415,8 @@ func TestStaleController_CleanupResourceExport(t *testing.T) { toKeepSvcResExport, toKeepLabelResExport, toDeleteLabelResExport, + toKeepLabelResExportEmptyLabels, + toDeleteLabelResExportEmptyLabels, svcResExportFromOtherCluster, }, }, @@ -396,8 +438,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) diff --git a/pkg/controller/labelidentity/label_group_index.go b/pkg/controller/labelidentity/label_group_index.go index 79d3d4d633a..7de65a845ea 100644 --- a/pkg/controller/labelidentity/label_group_index.go +++ b/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 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 == "" { return m } kvs := strings.Split(s, ",") diff --git a/pkg/controller/labelidentity/label_group_index_test.go b/pkg/controller/labelidentity/label_group_index_test.go index 651118a2fef..b710d95c8aa 100644 --- a/pkg/controller/labelidentity/label_group_index_test.go +++ b/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:" ) 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)