From 6718bdc7468cf571bdb6809eb3e5b5ed18653a99 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Thu, 21 Oct 2021 11:09:39 +0200 Subject: [PATCH 1/3] Avoid updating the association status when no association --- pkg/controller/association/reconciler.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller/association/reconciler.go b/pkg/controller/association/reconciler.go index 3b40daad62..447154465e 100644 --- a/pkg/controller/association/reconciler.go +++ b/pkg/controller/association/reconciler.go @@ -409,6 +409,10 @@ func (r *Reconciler) updateStatus(ctx context.Context, associated commonv1.Assoc } newStatus = associated.AssociationStatusMap(r.AssociationType) + // shortcut if the two maps are nil or empty + if len(oldStatus) == 0 && len(newStatus) == 0 { + return nil + } if !reflect.DeepEqual(oldStatus, newStatus) { if err := r.Status().Update(ctx, associated); err != nil { return err From fbb7cafd746833942dffbde69dd3dc976f7baa9e Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Thu, 21 Oct 2021 14:32:15 +0200 Subject: [PATCH 2/3] Unit test --- .../association/controller/es_monitoring.go | 8 +++- .../controller/es_monitoring_test.go | 48 +++++++++++++++++++ pkg/controller/association/reconciler.go | 24 +++++++++- 3 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 pkg/controller/association/controller/es_monitoring_test.go diff --git a/pkg/controller/association/controller/es_monitoring.go b/pkg/controller/association/controller/es_monitoring.go index 232dbf2f79..b05fb331d6 100644 --- a/pkg/controller/association/controller/es_monitoring.go +++ b/pkg/controller/association/controller/es_monitoring.go @@ -35,7 +35,11 @@ const ( // Beats are configured to collect monitoring metrics and logs data of the associated Elasticsearch and send // them to the Elasticsearch referenced in the association. func AddEsMonitoring(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params operator.Parameters) error { - return association.AddAssociationController(mgr, accessReviewer, params, association.AssociationInfo{ + return association.AddAssociationController(mgr, accessReviewer, params, esMonitoringAssociationInfo()) +} + +func esMonitoringAssociationInfo() association.AssociationInfo { + return association.AssociationInfo{ AssociatedObjTemplate: func() commonv1.Associated { return &esv1.Elasticsearch{} }, ReferencedObjTemplate: func() client.Object { return &esv1.Elasticsearch{} }, ReferencedResourceVersion: referencedElasticsearchStatusVersion, @@ -64,5 +68,5 @@ func AddEsMonitoring(mgr manager.Manager, accessReviewer rbac.AccessReviewer, pa return user.StackMonitoringUserRole, nil }, }, - }) + } } diff --git a/pkg/controller/association/controller/es_monitoring_test.go b/pkg/controller/association/controller/es_monitoring_test.go new file mode 100644 index 0000000000..2e54ff4d96 --- /dev/null +++ b/pkg/controller/association/controller/es_monitoring_test.go @@ -0,0 +1,48 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" + "github.com/elastic/cloud-on-k8s/pkg/controller/association" + "github.com/elastic/cloud-on-k8s/pkg/controller/common/annotation" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" +) + +var ( + sampleES = esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "esns", + Name: "esname", + Annotations: map[string]string{ + annotation.ControllerVersionAnnotation: "1.5.0", + }, + ResourceVersion: "42"}, + Status: esv1.ElasticsearchStatus{Version: "7.15.0"}, + } +) + +// Test_EsMonitoringReconciler_NoAssociation tests that an Elasticsearch resource is not updated by the EsMonitoring +// reconciler when there is no EsMonitoring association. Covers the bug https://github.com/elastic/cloud-on-k8s/issues/4985. +func Test_EsMonitoringReconciler_NoAssociation(t *testing.T) { + es := sampleES + resourceVersion := es.ResourceVersion + r := association.NewTestAssociationReconciler(esMonitoringAssociationInfo(), &es) + _, err := r.Reconcile(context.Background(), reconcile.Request{NamespacedName: k8s.ExtractNamespacedName(&es)}) + require.NoError(t, err) + // should not update the Elasticsearch resource + var updatedEs esv1.Elasticsearch + err = r.Get(context.Background(), k8s.ExtractNamespacedName(&es), &updatedEs) + require.NoError(t, err) + // resource version should not changed + require.Equal(t, resourceVersion, updatedEs.ResourceVersion) +} diff --git a/pkg/controller/association/reconciler.go b/pkg/controller/association/reconciler.go index 447154465e..255daecd94 100644 --- a/pkg/controller/association/reconciler.go +++ b/pkg/controller/association/reconciler.go @@ -10,16 +10,17 @@ import ( "reflect" "time" + "github.com/go-logr/logr" "go.elastic.co/apm" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/go-logr/logr" - + "github.com/elastic/cloud-on-k8s/pkg/about" commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common" @@ -450,3 +451,22 @@ func (r *Reconciler) onDelete(ctx context.Context, associated types.NamespacedNa r.log(associated).Error(err, "Error while trying to delete orphaned resources. Continuing.") } } + +// NewTestAssociationReconciler creates a new AssociationReconciler given an AssociationInfo for testing. +func NewTestAssociationReconciler(assocInfo AssociationInfo, runtimeObjs ...runtime.Object) Reconciler { + return Reconciler{ + AssociationInfo: assocInfo, + Client: k8s.NewFakeClient(runtimeObjs...), + accessReviewer: rbac.NewPermissiveAccessReviewer(), + watches: watches.NewDynamicWatches(), + recorder: record.NewFakeRecorder(10), + Parameters: operator.Parameters{ + OperatorInfo: about.OperatorInfo{ + BuildInfo: about.BuildInfo{ + Version: "1.5.0", + }, + }, + }, + logger: log.WithName("test"), + } +} From 0dac34a9812556413f0d372d2de34b398a0d37f0 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Thu, 21 Oct 2021 15:01:08 +0200 Subject: [PATCH 3/3] Update pkg/controller/association/controller/es_monitoring_test.go Co-authored-by: Peter Brachwitz --- pkg/controller/association/controller/es_monitoring_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/association/controller/es_monitoring_test.go b/pkg/controller/association/controller/es_monitoring_test.go index 2e54ff4d96..5d5c156684 100644 --- a/pkg/controller/association/controller/es_monitoring_test.go +++ b/pkg/controller/association/controller/es_monitoring_test.go @@ -43,6 +43,6 @@ func Test_EsMonitoringReconciler_NoAssociation(t *testing.T) { var updatedEs esv1.Elasticsearch err = r.Get(context.Background(), k8s.ExtractNamespacedName(&es), &updatedEs) require.NoError(t, err) - // resource version should not changed + // resource version should not have changed require.Equal(t, resourceVersion, updatedEs.ResourceVersion) }