Fix additional cases in labels handling (kubevirt#2820)

Fix additional cases in labels handling on operands, add more unit tests. Signed-off-by: Simone Tiraboschi <[email protected]>
enp0s3 · Feb 22, 2024 · 448793d · 448793d
1 parent 66ead5e
commit 448793d
Show file tree

Hide file tree

Showing 22 changed files with 576 additions and 10 deletions.
diff --git a/controllers/operands/aaq.go b/controllers/operands/aaq.go
@@ -68,7 +68,7 @@ func (*aaqHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Spec, aaq.Spec) ||
-		!hcoutil.CompareLabels(found, aaq) {
+		!hcoutil.CompareLabels(aaq, found) {
 		overwritten := false
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing AAQ's Spec to new opinionated values")

diff --git a/controllers/operands/aaq_test.go b/controllers/operands/aaq_test.go
@@ -316,6 +316,40 @@ var _ = Describe("AAQ tests", func() {
 			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 		})
 
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			hco.Spec.ApplicationAwareConfig = &v1beta1.ApplicationAwareConfigurations{}
+			hco.Spec.FeatureGates.EnableApplicationAwareQuota = ptr.To(true)
+			outdatedResource := NewAAQWithNameOnly(hco)
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+			delete(outdatedResource.Labels, hcoutil.AppLabelVersion)
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := newAAQHandler(cl, commontestutils.GetScheme())
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &aaqv1alpha1.AAQ{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
+
 	})
 
 	Context("check cache", func() {

diff --git a/controllers/operands/cdi.go b/controllers/operands/cdi.go
@@ -67,7 +67,7 @@ func (*cdiHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Spec, cdi.Spec) ||
-		!util.CompareLabels(found, cdi) {
+		!util.CompareLabels(cdi, found) {
 		overwritten := false
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing CDI's Spec to new opinionated values")

diff --git a/controllers/operands/cdi_test.go b/controllers/operands/cdi_test.go
@@ -131,6 +131,39 @@ var _ = Describe("CDI Operand", func() {
 			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 		})
 
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			outdatedResource, err := NewCDI(hco)
+			Expect(err).ToNot(HaveOccurred())
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+			delete(outdatedResource.Labels, hcoutil.AppLabelVersion)
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := (*genericOperand)(newCdiHandler(cl, commontestutils.GetScheme()))
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &cdiv1beta1.CDI{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
+
 		It("should set default UninstallStrategy if missing", func() {
 			expectedResource, err := NewCDI(hco)
 			Expect(err).ToNot(HaveOccurred())

diff --git a/controllers/operands/cliDownload.go b/controllers/operands/cliDownload.go
@@ -57,7 +57,7 @@ func (*cliDownloadHooks) updateCr(req *common.HcoRequest, Client client.Client,
 		return false, false, errors.New("can't convert to ConsoleCLIDownload")
 	}
 	if !reflect.DeepEqual(found.Spec, ccd.Spec) ||
-		!util.CompareLabels(found, ccd) {
+		!util.CompareLabels(ccd, found) {
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing ConsoleCLIDownload's Spec to new opinionated values")
 		} else {

diff --git a/controllers/operands/cliDownload_test.go b/controllers/operands/cliDownload_test.go
@@ -135,6 +135,38 @@ var _ = Describe("CLI Download", func() {
 			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 		})
 
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			outdatedResource := NewConsoleCLIDownload(hco)
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+			delete(outdatedResource.Labels, hcoutil.AppLabelVersion)
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := (*genericOperand)(newCliDownloadHandler(cl, commontestutils.GetScheme()))
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &consolev1.ConsoleCLIDownload{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
+
 	})
 })
 

diff --git a/controllers/operands/cmHandler.go b/controllers/operands/cmHandler.go
@@ -49,7 +49,7 @@ func (h cmHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Data, h.required.Data) ||
-		!util.CompareLabels(found, h.required) {
+		!util.CompareLabels(h.required, found) {
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing Configmap to new opinionated values", "name", h.required.Name)
 		} else {

diff --git a/controllers/operands/dashboard_test.go b/controllers/operands/dashboard_test.go
@@ -16,6 +16,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	"github.com/kubevirt/hyperconverged-cluster-operator/controllers/commontestutils"
+	hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
 )
 
 var _ = Describe("Dashboard tests", func() {
@@ -231,5 +232,69 @@ var _ = Describe("Dashboard tests", func() {
 				Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 			}
 		})
+
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+
+			_ = os.Setenv(dashboardManifestLocationVarName, testFilesLocation)
+			cli := commontestutils.InitClient([]client.Object{})
+			handlers, err := getDashboardHandlers(logger, cli, schemeForTest, hco)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(handlers).To(HaveLen(1))
+
+			cmList := &corev1.ConfigMapList{}
+
+			req := commontestutils.NewReq(hco)
+
+			By("apply the CMs", func() {
+				res := handlers[0].ensure(req)
+				Expect(res.Err).ToNot(HaveOccurred())
+				Expect(res.Created).To(BeTrue())
+
+				Expect(cli.List(context.TODO(), cmList)).To(Succeed())
+				Expect(cmList.Items).To(HaveLen(1))
+				Expect(cmList.Items[0].Name).Should(Equal("grafana-dashboard-kubevirt-top-consumers"))
+			})
+
+			expectedLabels := make(map[string]map[string]string)
+
+			By("getting opinionated labels", func() {
+				for _, cm := range cmList.Items {
+					expectedLabels[cm.Name] = make(map[string]string)
+					for k, v := range cm.Labels {
+						expectedLabels[cm.Name][k] = v
+					}
+				}
+			})
+
+			By("altering the cm objects", func() {
+				for _, foundResource := range cmList.Items {
+					foundResource.Labels[userLabelKey] = userLabelValue
+					delete(foundResource.Labels, hcoutil.AppLabelVersion)
+					err = cli.Update(context.TODO(), &foundResource)
+					Expect(err).ToNot(HaveOccurred())
+				}
+			})
+
+			By("reconciling cm objects", func() {
+				for _, handler := range handlers {
+					res := handler.ensure(req)
+					Expect(res.UpgradeDone).To(BeFalse())
+					Expect(res.Updated).To(BeTrue())
+					Expect(res.Err).ToNot(HaveOccurred())
+				}
+			})
+
+			foundResourcesList := &corev1.ConfigMapList{}
+			Expect(cli.List(context.TODO(), foundResourcesList)).To(Succeed())
+
+			for _, foundResource := range foundResourcesList.Items {
+				for k, v := range expectedLabels[foundResource.Name] {
+					Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+				}
+				Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+			}
+		})
 	})
 })
diff --git a/controllers/operands/deploymentHandler.go b/controllers/operands/deploymentHandler.go
@@ -99,7 +99,7 @@ func (h *deploymentHooks) updateCr(req *common.HcoRequest, Client client.Client,
 // We need to check only certain fields in the deployment resource, since some of the fields
 // are being set by k8s.
 func hasCorrectDeploymentFields(found *appsv1.Deployment, required *appsv1.Deployment) bool {
-	return util.CompareLabels(found, required) &&
+	return util.CompareLabels(required, found) &&
 		reflect.DeepEqual(found.Spec.Selector, required.Spec.Selector) &&
 		reflect.DeepEqual(found.Spec.Replicas, required.Spec.Replicas) &&
 		reflect.DeepEqual(found.Spec.Template.Spec.Containers, required.Spec.Template.Spec.Containers) &&

diff --git a/controllers/operands/deploymentHandler_test.go b/controllers/operands/deploymentHandler_test.go
@@ -71,7 +71,8 @@ var _ = Describe("Deployment Handler", func() {
 			expectedDeployment.DeepCopyInto(modifiedDeployment)
 			// modify only the labels
 			gotLabels := modifiedDeployment.GetLabels()
-			gotLabels["key2"] = "value2"
+			gotLabels["key1"] = "wrongValue1"
+			gotLabels["key2"] = "newValue2"
 			modifiedDeployment.SetLabels(gotLabels)
 			modifiedDeployment.ObjectMeta.UID = "oldObjectUID"
 
@@ -138,6 +139,45 @@ var _ = Describe("Deployment Handler", func() {
 			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 		})
 
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			outdatedResource := NewExpectedDeployment(hco)
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+
+			removed := false
+			for k := range outdatedResource.Labels {
+				if !removed {
+					delete(outdatedResource.Labels, k)
+					removed = true
+				}
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := newDeploymentHandler(cl, commontestutils.GetScheme(), NewExpectedDeployment, hco)
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &appsv1.Deployment{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
+
 	})
 
 })

diff --git a/controllers/operands/kubevirt.go b/controllers/operands/kubevirt.go
@@ -249,7 +249,7 @@ func (*kubevirtHooks) updateCr(req *common.HcoRequest, Client client.Client, exi
 	}
 
 	if !reflect.DeepEqual(found.Spec, virt.Spec) ||
-		!hcoutil.CompareLabels(found, virt) ||
+		!hcoutil.CompareLabels(virt, found) ||
 		!isAnnotationStateMeetingRequirements(virt.Annotations, found.Annotations) {
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing KubeVirt's Spec to new opinionated values")

diff --git a/controllers/operands/kubevirtConsolePlugin_test.go b/controllers/operands/kubevirtConsolePlugin_test.go
@@ -222,6 +222,44 @@ var _ = Describe("Kubevirt Console Plugin", func() {
 			}
 			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
 		})
+
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			outdatedResource := NewExpectedDeployment(hco)
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+			removed := false
+			for k := range outdatedResource.Labels {
+				if !removed {
+					delete(outdatedResource.Labels, k)
+					removed = true
+				}
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := newDeploymentHandler(cl, commontestutils.GetScheme(), NewExpectedDeployment, hco)
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &appsv1.Deployment{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
 	})
 
 	Context("Kubevirt Console Plugin and UI Proxy Deployments", func() {

diff --git a/controllers/operands/kubevirt_test.go b/controllers/operands/kubevirt_test.go
@@ -306,6 +306,40 @@ Version: 1.2.3`)
 				outdatedResource.Labels[k] = "wrong_" + v
 			}
 			outdatedResource.Labels[userLabelKey] = userLabelValue
+			delete(outdatedResource.Labels, "app.kubernetes.io/version")
+
+			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
+			handler := (*genericOperand)(newKubevirtHandler(cl, commontestutils.GetScheme()))
+
+			res := handler.ensure(req)
+			Expect(res.UpgradeDone).To(BeFalse())
+			Expect(res.Updated).To(BeTrue())
+			Expect(res.Err).ToNot(HaveOccurred())
+
+			foundResource := &kubevirtcorev1.KubeVirt{}
+			Expect(
+				cl.Get(context.TODO(),
+					types.NamespacedName{Name: outdatedResource.Name, Namespace: outdatedResource.Namespace},
+					foundResource),
+			).ToNot(HaveOccurred())
+
+			for k, v := range expectedLabels {
+				Expect(foundResource.Labels).To(HaveKeyWithValue(k, v))
+			}
+			Expect(foundResource.Labels).To(HaveKeyWithValue(userLabelKey, userLabelValue))
+		})
+
+		It("should reconcile managed labels to default on label deletion without touching user added ones", func() {
+			const userLabelKey = "userLabelKey"
+			const userLabelValue = "userLabelValue"
+			outdatedResource, err := NewKubeVirt(hco)
+			Expect(err).ToNot(HaveOccurred())
+			expectedLabels := make(map[string]string, len(outdatedResource.Labels))
+			for k, v := range outdatedResource.Labels {
+				expectedLabels[k] = v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+			delete(outdatedResource.Labels, hcoutil.AppLabelVersion)
 
 			cl := commontestutils.InitClient([]client.Object{hco, outdatedResource})
 			handler := (*genericOperand)(newKubevirtHandler(cl, commontestutils.GetScheme()))