Remove sidecar when annotation is removed or set to false (#1508)

* first approach for removing sidecar when deploy annotation is set to false

Signed-off-by: Marco Freyre <marco.fz85@gmail.com>

* fixed missing condition test on sidecar/needed when deploy annotation is set to false, added method for remove sidecar when is not needed in deploy controller

Signed-off-by: Marco Freyre <marco.fz85@gmail.com>

* removeSidecar refactored as instance method

Signed-off-by: Marco Freyre <marco.fz85@gmail.com>

* inject.desired refactor

Signed-off-by: Marco Freyre <marco.fz85@gmail.com>

Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pkg/controller/deployment/deployment_controller.go

@@ -115,11 +115,6 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
 		span.AddEvent(msg, trace.WithAttributes(attribute.String("error", err.Error())))
 	}
 
-	if !inject.Desired(dep, ns) {
-		// sidecar isn't desired for this deployment, skip remaining of the reconciliation
-		return reconcile.Result{}, nil
-	}
-
 	jaegers := &v1.JaegerList{}
 	opts := []client.ListOption{}
 
@@ -132,23 +127,23 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
 		return reconcile.Result{}, tracing.HandleError(err, span)
 	}
 
-	jaeger := inject.Select(dep, ns, jaegers)
-	if jaeger != nil && jaeger.GetDeletionTimestamp() == nil {
-		logger := logger.WithFields(log.Fields{
-			"jaeger":           jaeger.Name,
-			"jaeger-namespace": jaeger.Namespace,
-		})
-		if jaeger.Namespace != dep.Namespace {
-			if err := r.reconcileConfigMaps(ctx, jaeger, dep); err != nil {
-				msg := "failed to reconcile config maps for the namespace"
-				logger.WithError(err).Error(msg)
-				span.AddEvent(msg)
+	if inject.Needed(dep, ns) {
+		jaeger := inject.Select(dep, ns, jaegers)
+		if jaeger != nil && jaeger.GetDeletionTimestamp() == nil {
+			logger := logger.WithFields(log.Fields{
+				"jaeger":           jaeger.Name,
+				"jaeger-namespace": jaeger.Namespace,
+			})
+			if jaeger.Namespace != dep.Namespace {
+				if err := r.reconcileConfigMaps(ctx, jaeger, dep); err != nil {
+					msg := "failed to reconcile config maps for the namespace"
+					logger.WithError(err).Error(msg)
+					span.AddEvent(msg)
+				}
 			}
-		}
 
-		// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
-		// Verified that jaeger instance was found and is not marked for deletion.
-		if inject.Needed(dep, ns) {
+			// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
+			// Verified that jaeger instance was found and is not marked for deletion.
 			{
 				msg := "injecting Jaeger Agent sidecar"
 				logger.Info(msg)
@@ -160,16 +155,45 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
 				logger.WithError(err).Error("failed to update deployment with sidecar")
 				return reconcile.Result{}, tracing.HandleError(err, span)
 			}
+
+		} else {
+			msg := "no suitable Jaeger instances found to inject a sidecar"
+			span.AddEvent(msg)
+			logger.Debug(msg)
 		}
+
 	} else {
-		msg := "no suitable Jaeger instances found to inject a sidecar"
-		span.AddEvent(msg)
-		logger.Debug(msg)
+		hasAgent, _ := inject.HasJaegerAgent(dep)
+		if hasAgent {
+			_, hasLabel := dep.Labels[inject.Label]
+			if hasLabel {
+				r.removeSidecar(ctx, dep)
+			}
+
+		}
+		return reconcile.Result{}, nil
 	}
 
 	return reconcile.Result{}, nil
 }
 
+func (r *ReconcileDeployment) removeSidecar(ctx context.Context, dep *appsv1.Deployment) {
+	jaegerInstance := dep.Labels[inject.Label]
+	log.WithFields(log.Fields{
+		"deployment": dep.Name,
+		"namespace":  dep.Namespace,
+		"jaeger":     jaegerInstance,
+	}).Info("Removing Jaeger Agent sidecar")
+	patch := client.MergeFrom(dep.DeepCopy())
+	inject.CleanSidecar(jaegerInstance, dep)
+	if err := r.client.Patch(ctx, dep, patch); err != nil {
+		log.WithFields(log.Fields{
+			"deploymentName":      dep.Name,
+			"deploymentNamespace": dep.Namespace,
+		}).WithError(err).Error("error cleaning orphaned deployment")
+	}
+}
+
 func (r *ReconcileDeployment) syncOnJaegerChanges(event handler.MapObject) []reconcile.Request {
 	reconciliations := []reconcile.Request{}
 	nss := map[string]corev1.Namespace{} // namespace cache

pkg/inject/sidecar.go

@@ -75,20 +75,20 @@ func Sidecar(jaeger *v1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment {
 }
 
 // Desired determines whether a sidecar is desired, based on the annotation from both the deployment and the namespace
-func Desired(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
+func desired(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
 	logger := log.WithFields(log.Fields{
 		"namespace":  dep.Namespace,
 		"deployment": dep.Name,
 	})
-	_, depExist := dep.Annotations[Annotation]
-	_, nsExist := ns.Annotations[Annotation]
+	depAnnotationValue, depExist := dep.Annotations[Annotation]
+	nsAnnotationValue, nsExist := ns.Annotations[Annotation]
 
-	if depExist {
+	if depExist && !strings.EqualFold(depAnnotationValue, "false") {
 		logger.Debug("annotation present on deployment")
 		return true
 	}
 
-	if nsExist {
+	if nsExist && !strings.EqualFold(nsAnnotationValue, "false") {
 		logger.Debug("annotation present on namespace")
 		return true
 	}
@@ -98,7 +98,7 @@ func Desired(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
 
 // Needed determines whether a pod needs to get a sidecar injected or not
 func Needed(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
-	if !Desired(dep, ns) {
+	if !desired(dep, ns) {
 		return false
 	}
 

pkg/inject/sidecar_test.go

@@ -346,6 +346,11 @@ func TestSidecarNeeded(t *testing.T) {
 			ns:     ns(map[string]string{Annotation: "true"}),
 			needed: false,
 		},
+		{
+			dep:    dep(map[string]string{Annotation: "false"}, map[string]string{}),
+			ns:     ns(map[string]string{}),
+			needed: false,
+		},
 	}
 	for _, test := range tests {
 		t.Run(fmt.Sprintf("dep:%s, ns: %s", test.dep.Annotations, test.ns.Annotations), func(t *testing.T) {