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>
jaegertracing · Jul 14, 2021 · fab19c1 · fab19c1
1 parent eaa204d
commit fab19c1
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 24 deletions.
diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go
@@ -2,7 +2,6 @@ package deployment
 
 import (
 	"context"
-	"strings"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/viper"
@@ -118,6 +117,10 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
 
 	if !inject.Desired(dep, ns) {
 		// sidecar isn't desired for this deployment, skip remaining of the reconciliation
+		hasAgent, _ := inject.HasJaegerAgent(dep)
+		if hasAgent {
+			removeSideCar(r, dep, ctx)
+		}
 		return reconcile.Result{}, nil
 	}
 
@@ -166,32 +169,30 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
 		msg := "no suitable Jaeger instances found to inject a sidecar"
 		span.AddEvent(msg)
 		logger.Debug(msg)
-		hasAgent, _ := inject.HasJaegerAgent(dep)
-		annotationValue, hasDepAnnotation := dep.Annotations[inject.Annotation]
-		// If deployment has the annotation with false value and has an hasAgent, we need to clean it.
-		if hasAgent && hasDepAnnotation && strings.EqualFold(annotationValue, "false") {
-			jaegerInstance, hasLabel := dep.Labels[inject.Label]
-			if hasLabel {
-				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")
-				}
-			}
-		}
 	}
 
 	return reconcile.Result{}, nil
 }
 
+func removeSideCar(r *ReconcileDeployment, dep *appsv1.Deployment, ctx context.Context) {
+	jaegerInstance, hasLabel := dep.Labels[inject.Label]
+	if hasLabel {
+		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

diff --git a/pkg/inject/sidecar.go b/pkg/inject/sidecar.go
@@ -80,10 +80,10 @@ func Desired(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
 		"namespace":  dep.Namespace,
 		"deployment": dep.Name,
 	})
-	_, depExist := dep.Annotations[Annotation]
+	annotationValue, depExist := dep.Annotations[Annotation]
 	_, nsExist := ns.Annotations[Annotation]
 
-	if depExist {
+	if depExist && !strings.EqualFold(annotationValue, "false") {
 		logger.Debug("annotation present on deployment")
 		return true
 	}

diff --git a/pkg/inject/sidecar_test.go b/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) {