Avoid removing user labels

Reconcile managed labels on operands without
removing user added ones.
Add unit and func tests.

Signed-off-by: Simone Tiraboschi <[email protected]>

controllers/alerts/metrics_test.go

@@ -9,6 +9,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gstruct"
 
 	"github.com/machadovilaca/operator-observability/pkg/operatorrules"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
@@ -164,7 +165,7 @@ var _ = Describe("alert tests", func() {
 			pr := &monitoringv1.PrometheusRule{}
 			Expect(cl.Get(context.Background(), client.ObjectKey{Namespace: r.namespace, Name: ruleName}, pr)).Should(Succeed())
 
-			Expect(pr.Labels).Should(Equal(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring)))
+			Expect(pr.Labels).To(gstruct.MatchKeys(gstruct.IgnoreExtras, commontestutils.KeysFromSSMap(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring))))
 			Expect(ee.CheckEvents(expectedEvents)).To(BeTrue())
 			Expect(metrics.GetOverwrittenModificationsCount(monitoringv1.PrometheusRuleKind, ruleName)).Should(BeEquivalentTo(currentMetric))
 		})
@@ -433,7 +434,7 @@ var _ = Describe("alert tests", func() {
 			role := &rbacv1.Role{}
 			Expect(cl.Get(context.Background(), client.ObjectKey{Namespace: r.namespace, Name: roleName}, role)).Should(Succeed())
 
-			Expect(role.Labels).Should(Equal(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring)))
+			Expect(role.Labels).To(gstruct.MatchKeys(gstruct.IgnoreExtras, commontestutils.KeysFromSSMap(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring))))
 			Expect(ee.CheckEvents(expectedEvents)).To(BeTrue())
 			Expect(metrics.GetOverwrittenModificationsCount("Role", roleName)).Should(BeEquivalentTo(currentMetric))
 		})
@@ -628,7 +629,7 @@ var _ = Describe("alert tests", func() {
 			rb := &rbacv1.RoleBinding{}
 			Expect(cl.Get(context.Background(), client.ObjectKey{Namespace: r.namespace, Name: roleName}, rb)).Should(Succeed())
 
-			Expect(rb.Labels).Should(Equal(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring)))
+			Expect(rb.Labels).To(gstruct.MatchKeys(gstruct.IgnoreExtras, commontestutils.KeysFromSSMap(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring))))
 			Expect(ee.CheckEvents(expectedEvents)).To(BeTrue())
 			Expect(metrics.GetOverwrittenModificationsCount("RoleBinding", roleName)).Should(BeEquivalentTo(currentMetric))
 		})
@@ -869,7 +870,7 @@ var _ = Describe("alert tests", func() {
 			svc := &corev1.Service{}
 			Expect(cl.Get(context.Background(), client.ObjectKey{Namespace: r.namespace, Name: serviceName}, svc)).Should(Succeed())
 
-			Expect(svc.Labels).Should(Equal(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring)))
+			Expect(svc.Labels).To(gstruct.MatchKeys(gstruct.IgnoreExtras, commontestutils.KeysFromSSMap(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring))))
 			Expect(ee.CheckEvents(expectedEvents)).To(BeTrue())
 			Expect(metrics.GetOverwrittenModificationsCount("Service", serviceName)).Should(BeEquivalentTo(currentMetric))
 		})
@@ -1072,7 +1073,7 @@ var _ = Describe("alert tests", func() {
 			sm := &monitoringv1.ServiceMonitor{}
 			Expect(cl.Get(context.Background(), client.ObjectKey{Namespace: r.namespace, Name: serviceName}, sm)).Should(Succeed())
 
-			Expect(sm.Labels).Should(Equal(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring)))
+			Expect(sm.Labels).To(gstruct.MatchKeys(gstruct.IgnoreExtras, commontestutils.KeysFromSSMap(hcoutil.GetLabels(hcoutil.HyperConvergedName, hcoutil.AppComponentMonitoring))))
 			Expect(ee.CheckEvents(expectedEvents)).To(BeTrue())
 			Expect(metrics.GetOverwrittenModificationsCount("ServiceMonitor", serviceName)).Should(BeEquivalentTo(currentMetric))
 		})

controllers/alerts/reconciler.go

@@ -181,11 +181,11 @@ func getDeploymentReference(deployment *appsv1.Deployment) metav1.OwnerReference
 // return true if something was changed
 func updateCommonDetails(required, existing *metav1.ObjectMeta) bool {
 	if reflect.DeepEqual(required.OwnerReferences, existing.OwnerReferences) &&
-		reflect.DeepEqual(required.Labels, existing.Labels) {
+		hcoutil.CompareLabels(required, existing) {
 		return false
 	}
 
-	hcoutil.DeepCopyLabels(required, existing)
+	hcoutil.MergeLabels(required, existing)
 	if reqLen := len(required.OwnerReferences); reqLen > 0 {
 		refs := make([]metav1.OwnerReference, reqLen)
 		for i, ref := range required.OwnerReferences {

controllers/commontestutils/testUtils.go

@@ -7,6 +7,7 @@ import (
 	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo/v2" //nolint dot-imports
 	. "github.com/onsi/gomega"    //nolint dot-imports
+	"github.com/onsi/gomega/gstruct"
 	gomegatypes "github.com/onsi/gomega/types"
 	openshiftconfigv1 "github.com/openshift/api/config/v1"
 	consolev1 "github.com/openshift/api/console/v1"
@@ -445,3 +446,11 @@ func (ClusterInfoSRCPHAIMock) GetTLSSecurityProfile(_ *openshiftconfigv1.TLSSecu
 func (ClusterInfoSRCPHAIMock) RefreshAPIServerCR(_ context.Context, _ client.Client) error {
 	return nil
 }
+
+func KeysFromSSMap(ssmap map[string]string) gstruct.Keys {
+	keys := gstruct.Keys{}
+	for k, v := range ssmap {
+		keys[k] = Equal(v)
+	}
+	return keys
+}

controllers/operands/aaq.go

@@ -68,15 +68,15 @@ func (*aaqHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Spec, aaq.Spec) ||
-		!reflect.DeepEqual(found.Labels, aaq.Labels) {
+		!hcoutil.CompareLabels(found, aaq) {
 		overwritten := false
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing AAQ's Spec to new opinionated values")
 		} else {
 			req.Logger.Info("Reconciling an externally updated AAQ's Spec to its opinionated values")
 			overwritten = true
 		}
-		hcoutil.DeepCopyLabels(&aaq.ObjectMeta, &found.ObjectMeta)
+		hcoutil.MergeLabels(&aaq.ObjectMeta, &found.ObjectMeta)
 		aaq.Spec.DeepCopyInto(&found.Spec)
 		err := Client.Update(req.Ctx, found)
 		if err != nil {

controllers/operands/aaq_test.go

@@ -8,6 +8,7 @@ import (
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -278,6 +279,43 @@ var _ = Describe("AAQ tests", func() {
 			Expect(foundAAQ.Spec.CertConfig.Server.Duration.Duration.String()).Should(Equal("24h0m0s"))
 			Expect(foundAAQ.Spec.CertConfig.Server.RenewBefore.Duration.String()).Should(Equal("12h0m0s"))
 		})
+
+		It("should reconcile managed labels to default 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
+			}
+			for k, v := range expectedLabels {
+				outdatedResource.Labels[k] = "wrong_" + v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+
+			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() {

controllers/operands/cdi.go

@@ -67,15 +67,15 @@ func (*cdiHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Spec, cdi.Spec) ||
-		!reflect.DeepEqual(found.Labels, cdi.Labels) {
+		!util.CompareLabels(found, cdi) {
 		overwritten := false
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing CDI's Spec to new opinionated values")
 		} else {
 			req.Logger.Info("Reconciling an externally updated CDI's Spec to its opinionated values")
 			overwritten = true
 		}
-		util.DeepCopyLabels(&cdi.ObjectMeta, &found.ObjectMeta)
+		util.MergeLabels(&cdi.ObjectMeta, &found.ObjectMeta)
 		cdi.Spec.DeepCopyInto(&found.Spec)
 		err := Client.Update(req.Ctx, found)
 		if err != nil {

controllers/operands/cdi_test.go

@@ -96,6 +96,41 @@ var _ = Describe("CDI Operand", func() {
 			}))
 		})
 
+		It("should reconcile managed labels to default 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
+			}
+			for k, v := range expectedLabels {
+				outdatedResource.Labels[k] = "wrong_" + v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+
+			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())

controllers/operands/cliDownload.go

@@ -57,13 +57,13 @@ 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) ||
-		!reflect.DeepEqual(found.Labels, ccd.Labels) {
+		!util.CompareLabels(found, ccd) {
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing ConsoleCLIDownload's Spec to new opinionated values")
 		} else {
 			req.Logger.Info("Reconciling an externally updated ConsoleCLIDownload's Spec to its opinionated values")
 		}
-		util.DeepCopyLabels(&ccd.ObjectMeta, &found.ObjectMeta)
+		util.MergeLabels(&ccd.ObjectMeta, &found.ObjectMeta)
 		ccd.Spec.DeepCopyInto(&found.Spec)
 		err := Client.Update(req.Ctx, found)
 		if err != nil {
@@ -180,7 +180,7 @@ func (*cliDownloadsRouteHooks) updateCr(req *common.HcoRequest, Client client.Cl
 		} else {
 			req.Logger.Info("Reconciling an externally updated Route Spec to its opinionated values")
 		}
-		util.DeepCopyLabels(&route.ObjectMeta, &found.ObjectMeta)
+		util.MergeLabels(&route.ObjectMeta, &found.ObjectMeta)
 		route.Spec.DeepCopyInto(&found.Spec)
 		err := Client.Update(req.Ctx, found)
 		if err != nil {

controllers/operands/cliDownload_test.go

@@ -9,6 +9,7 @@ import (
 	consolev1 "github.com/openshift/api/console/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/tools/reference"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -100,6 +101,40 @@ var _ = Describe("CLI Download", func() {
 			}),
 		)
 
+		It("should reconcile managed labels to default 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
+			}
+			for k, v := range expectedLabels {
+				outdatedResource.Labels[k] = "wrong_" + v
+			}
+			outdatedResource.Labels[userLabelKey] = userLabelValue
+
+			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))
+		})
+
 	})
 })
 
@@ -251,8 +286,9 @@ var _ = Describe("Cli Downloads Route", func() {
 			Expect(hco.Status.RelatedObjects).To(ContainElement(*objectRefFound))
 		},
 			Entry("with modified labels", func(resource *routev1.Route) {
-				resource.Labels = map[string]string{"key": "value"}
+				resource.Labels = map[string]string{"app.kubernetes.io/managed-by": "value"}
 			}),
+			// TODO: add another test for additional labels
 			Entry("with modified port", func(resource *routev1.Route) {
 				resource.Spec.Port = &routev1.RoutePort{
 					TargetPort: intstr.IntOrString{IntVal: 1111},

controllers/operands/cmHandler.go

@@ -49,14 +49,17 @@ func (h cmHooks) updateCr(req *common.HcoRequest, Client client.Client, exists r
 	}
 
 	if !reflect.DeepEqual(found.Data, h.required.Data) ||
-		!reflect.DeepEqual(found.Labels, h.required.Labels) {
+		!util.CompareLabels(found, h.required) {
 		if req.HCOTriggered {
 			req.Logger.Info("Updating existing Configmap to new opinionated values", "name", h.required.Name)
 		} else {
 			req.Logger.Info("Reconciling an externally updated Configmap to its opinionated values", "name", h.required.Name)
 		}
-		util.DeepCopyLabels(&h.required.ObjectMeta, &found.ObjectMeta)
-		h.required.DeepCopyInto(found)
+		util.MergeLabels(&h.required.ObjectMeta, &found.ObjectMeta)
+		found.Data = make(map[string]string, len(h.required.Data))
+		for k, v := range h.required.Data {
+			found.Data[k] = v
+		}
 		err := Client.Update(req.Ctx, found)
 		if err != nil {
 			return false, false, err

controllers/operands/dashboard_test.go

@@ -165,5 +165,71 @@ var _ = Describe("Dashboard tests", func() {
 				Expect(ok).Should(BeTrue())
 			})
 		})
+
+		It("should reconcile managed labels to default 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 {
+					for k, v := range expectedLabels[foundResource.Name] {
+						foundResource.Labels[k] = "wrong_" + v
+					}
+					foundResource.Labels[userLabelKey] = userLabelValue
+					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))
+			}
+		})
 	})
 })