From 318203facbcc1e9f9317c9a7a94b5c76c47d57ba Mon Sep 17 00:00:00 2001
From: Michael Morello <michael.morello@elastic.co>
Date: Tue, 15 Oct 2019 16:01:39 +0200
Subject: [PATCH] Fix result of the APM Server controller  (#1991)

* Fix result of the APM server
---
 .../apmserver/apmserver_controller.go         | 22 +++---
 .../apmserver/apmserver_controller_test.go    | 68 +++++++++++++++++++
 2 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/pkg/controller/apmserver/apmserver_controller.go b/pkg/controller/apmserver/apmserver_controller.go
index a6c3ee1958..d4b9e76e2a 100644
--- a/pkg/controller/apmserver/apmserver_controller.go
+++ b/pkg/controller/apmserver/apmserver_controller.go
@@ -246,7 +246,15 @@ func (r *ReconcileApmServer) doReconcile(request reconcile.Request, as *apmv1bet
 
 	state.UpdateApmServerExternalService(*svc)
 
-	return r.updateStatus(state)
+	// update status
+	err = r.updateStatus(state)
+	if err != nil && errors.IsConflict(err) {
+		log.V(1).Info("Conflict while updating status", "namespace", as.Namespace, "as", as.Name)
+		return reconcile.Result{Requeue: true}, nil
+	}
+	res, err := results.WithError(err).Aggregate()
+	k8s.EmitErrorEvent(r.recorder, err, as, events.EventReconciliationError, "Reconciliation error: %v", err)
+	return res, err
 }
 
 func (r *ReconcileApmServer) reconcileApmServerSecret(as *apmv1beta1.ApmServer) (*corev1.Secret, error) {
@@ -441,22 +449,16 @@ func (r *ReconcileApmServer) reconcileApmServerDeployment(
 	return state, nil
 }
 
-func (r *ReconcileApmServer) updateStatus(state State) (reconcile.Result, error) {
+func (r *ReconcileApmServer) updateStatus(state State) error {
 	current := state.originalApmServer
 	if reflect.DeepEqual(current.Status, state.ApmServer.Status) {
-		return state.Result, nil
+		return nil
 	}
 	if state.ApmServer.Status.IsDegraded(current.Status) {
 		r.recorder.Event(current, corev1.EventTypeWarning, events.EventReasonUnhealthy, "Apm Server health degraded")
 	}
 	log.Info("Updating status", "namespace", state.ApmServer.Namespace, "as_name", state.ApmServer.Name, "iteration", atomic.LoadUint64(&r.iteration))
-	err := r.Status().Update(state.ApmServer)
-	if err != nil && errors.IsConflict(err) {
-		log.V(1).Info("Conflict while updating status")
-		return reconcile.Result{Requeue: true}, nil
-	}
-
-	return state.Result, err
+	return r.Status().Update(state.ApmServer)
 }
 
 // finalizersFor returns the list of finalizers applying to a given APM deployment
diff --git a/pkg/controller/apmserver/apmserver_controller_test.go b/pkg/controller/apmserver/apmserver_controller_test.go
index 84d8036cc1..5feb37ac33 100644
--- a/pkg/controller/apmserver/apmserver_controller_test.go
+++ b/pkg/controller/apmserver/apmserver_controller_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/defaults"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/deployment"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/keystore"
+	"github.com/elastic/cloud-on-k8s/pkg/controller/common/operator"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/watches"
 	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
 	"github.com/go-test/deep"
@@ -25,6 +26,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 var certSecretName = "test-apm-server-apm-http-certs-internal" // nolint
@@ -354,3 +356,69 @@ func TestReconcileApmServer_deploymentParams(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcileApmServer_doReconcile(t *testing.T) {
+	require.NoError(t, apmv1beta1.AddToScheme(scheme.Scheme))
+	type fields struct {
+		resources      []runtime.Object
+		recorder       record.EventRecorder
+		dynamicWatches watches.DynamicWatches
+		Parameters     operator.Parameters
+	}
+	type args struct {
+		request reconcile.Request
+	}
+	tests := []struct {
+		name        string
+		as          apmv1beta1.ApmServer
+		fields      fields
+		args        args
+		wantRequeue bool
+		wantErr     bool
+	}{
+		{
+			name: "If no error ensure a requeue is scheduled for CA",
+			as: apmv1beta1.ApmServer{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "apmserver",
+					Namespace: "default",
+				},
+			},
+			fields: fields{
+				resources:      []runtime.Object{},
+				recorder:       record.NewFakeRecorder(100),
+				dynamicWatches: watches.NewDynamicWatches(),
+				Parameters: operator.Parameters{
+					CACertRotation: certificates.RotationParams{
+						Validity:     certificates.DefaultCertValidity,
+						RotateBefore: certificates.DefaultRotateBefore,
+					},
+				},
+			},
+			args: args{
+				request: reconcile.Request{},
+			},
+			wantRequeue: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r := &ReconcileApmServer{
+				Client:         k8s.WrapClient(fake.NewFakeClientWithScheme(scheme.Scheme, &tt.as)),
+				scheme:         scheme.Scheme,
+				recorder:       tt.fields.recorder,
+				dynamicWatches: tt.fields.dynamicWatches,
+				Parameters:     tt.fields.Parameters,
+			}
+			got, err := r.doReconcile(tt.args.request, tt.as.DeepCopy())
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ReconcileApmServer.doReconcile() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			require.NotNil(t, got)
+			require.Equal(t, got.Requeue, tt.wantRequeue)
+			// We just check that the requeue is not zero
+			require.True(t, got.RequeueAfter > 0)
+		})
+	}
+}