Skip to content

Commit

Permalink
[release-1.11] Avoid removing user labels (#2743) (#2812)
Browse files Browse the repository at this point in the history 
* [release-1.11] Avoid removing user labels (#2743)

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

This is a manual cherry-pick of #2743

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

* Fix additional cases in labels handling (#2820)

Fix additional cases in labels handling on operands,
add more unit tests.

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

---------

Signed-off-by: Simone Tiraboschi <[email protected]>
  • Loading branch information
@tiraboschi
tiraboschi authored Mar 4, 2024
1 parent f848f67 commit 4db31f9
Show file tree
Hide file tree
Showing 44 changed files with 2,095 additions and 68 deletions.
12 changes: 7 additions & 5 deletions controllers/alerts/metrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gstruct"

monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -155,7 +157,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))
})
Expand Down Expand Up @@ -406,7 +408,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))
})
Expand Down Expand Up @@ -601,7 +603,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))
})
Expand Down Expand Up @@ -842,7 +844,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))
})
Expand Down Expand Up @@ -1045,7 +1047,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))
})
Expand Down
4 changes: 2 additions & 2 deletions controllers/alerts/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,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 {
Expand Down
9 changes: 9 additions & 0 deletions controllers/commontestutils/testUtils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
4 changes: 2 additions & 2 deletions controllers/operands/aaq.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(aaq, found) {
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 {
Expand Down
72 changes: 72 additions & 0 deletions controllers/operands/aaq_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -278,6 +279,77 @@ 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))
})

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() {
Expand Down
4 changes: 2 additions & 2 deletions controllers/operands/cdi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(cdi, found) {
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 {
Expand Down
68 changes: 68 additions & 0 deletions controllers/operands/cdi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,74 @@ 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 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())
Expand Down
6 changes: 3 additions & 3 deletions controllers/operands/cliDownload.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(ccd, found) {
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 {
Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 4db31f9

Please sign in to comment.