chore: write log when ignoring workloads with dash0.com/enable=false

ENG-1841

internal/controller/dash0_controller.go

@@ -383,10 +383,6 @@ func (r *Dash0Reconciler) addLabelsToImmutableJobsOnInstrumentation(
 	job batchv1.Job,
 	reconcileLogger *logr.Logger,
 ) {
-	if job.DeletionTimestamp != nil {
-		// do not modify resources that are being deleted
-		return
-	}
 	logger := reconcileLogger.WithValues(
 		workkloadTypeLabel,
 		"Job",
@@ -395,6 +391,16 @@ func (r *Dash0Reconciler) addLabelsToImmutableJobsOnInstrumentation(
 		workloadNameLabel,
 		job.GetName(),
 	)
+	if job.DeletionTimestamp != nil {
+		// do not modify resources that are being deleted
+		logger.Info("not instrumenting this workload since it is about to be deleted (a deletion timestamp is set)")
+		return
+	}
+	if util.HasOptedOutOfInstrumenationForWorkload(&job.ObjectMeta) {
+		logger.Info("not instrumenting this workload due to dash0.com/enable=false")
+		return
+	}
+
 	retryErr := util.Retry("labelling immutable job", func() error {
 		if err := r.Client.Get(ctx, client.ObjectKey{
 			Namespace: job.GetNamespace(),
@@ -480,10 +486,6 @@ func (r *Dash0Reconciler) instrumentWorkload(
 ) {
 	objectMeta := workload.getObjectMeta()
 	kind := workload.getKind()
-	if objectMeta.DeletionTimestamp != nil {
-		// do not modify resources that are being deleted
-		return
-	}
 	logger := reconcileLogger.WithValues(
 		workkloadTypeLabel,
 		kind,
@@ -492,6 +494,15 @@ func (r *Dash0Reconciler) instrumentWorkload(
 		workloadNameLabel,
 		objectMeta.GetName(),
 	)
+	if objectMeta.DeletionTimestamp != nil {
+		// do not modify resources that are being deleted
+		logger.Info("not instrumenting this workload since it is about to be deleted (a deletion timestamp is set)")
+		return
+	}
+	if util.HasOptedOutOfInstrumenationForWorkload(workload.getObjectMeta()) {
+		logger.Info("not instrumenting this workload due to dash0.com/enable=false")
+		return
+	}
 
 	hasBeenModified := false
 	retryErr := util.Retry(fmt.Sprintf("instrumenting %s", kind), func() error {
@@ -754,10 +765,6 @@ func (r *Dash0Reconciler) findAndHandleJobOnUninstrumentation(
 }
 
 func (r *Dash0Reconciler) handleJobOnUninstrumentation(ctx context.Context, job batchv1.Job, reconcileLogger *logr.Logger) {
-	if job.DeletionTimestamp != nil {
-		// do not modify resources that are being deleted
-		return
-	}
 	logger := reconcileLogger.WithValues(
 		workkloadTypeLabel,
 		"Job",
@@ -766,6 +773,16 @@ func (r *Dash0Reconciler) handleJobOnUninstrumentation(ctx context.Context, job
 		workloadNameLabel,
 		job.GetName(),
 	)
+	if job.DeletionTimestamp != nil {
+		// do not modify resources that are being deleted
+		logger.Info("not uninstrumenting this workload since it is about to be deleted (a deletion timestamp is set)")
+		return
+	}
+	// Note: In contrast to the instrumentation logic, there is no need to check for dash.com/enable=false here:
+	// If it is set, the workload would not have been instrumented in the first place, hence the label selector filter
+	// looking for dash0.com/instrumented=true would not have matched. Or if the workload is actually instrumented,
+	// although it has dash0.com/enabled=false it must have been set after the instrumentation, in which case
+	// uninstrumenting it is the correct thing to do.
 
 	createImmutableWorkloadsError := false
 	retryErr := util.Retry("removing labels from immutable job", func() error {
@@ -872,10 +889,6 @@ func (r *Dash0Reconciler) revertWorkloadInstrumentation(
 ) {
 	objectMeta := workload.getObjectMeta()
 	kind := workload.getKind()
-	if objectMeta.DeletionTimestamp != nil {
-		// do not modify resources that are being deleted
-		return
-	}
 	logger := reconcileLogger.WithValues(
 		workkloadTypeLabel,
 		kind,
@@ -884,6 +897,16 @@ func (r *Dash0Reconciler) revertWorkloadInstrumentation(
 		workloadNameLabel,
 		objectMeta.GetName(),
 	)
+	if objectMeta.DeletionTimestamp != nil {
+		// do not modify resources that are being deleted
+		logger.Info("not uninstrumenting this workload since it is about to be deleted (a deletion timestamp is set)")
+		return
+	}
+	// Note: In contrast to the instrumentation logic, there is no need to check for dash.com/enable=false here:
+	// If it is set, the workload would not have been instrumented in the first place, hence the label selector filter
+	// looking for dash0.com/instrumented=true would not have matched. Or if the workload is actually instrumented,
+	// although it has dash0.com/enabled=false it must have been set after the instrumentation, in which case
+	// uninstrumenting it is the correct thing to do.
 
 	hasBeenModified := false
 	retryErr := util.Retry(fmt.Sprintf("uninstrumenting %s", kind), func() error {

internal/util/labels.go

@@ -33,10 +33,10 @@ const (
 
 var (
 	WorkloadsWithoutDash0InstrumentedLabelFilter = metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("!%s,%s != false", instrumentedLabelKey, dash0EnableLabelKey),
+		LabelSelector: fmt.Sprintf("!%s", instrumentedLabelKey),
 	}
 	WorkloadsWithDash0InstrumentedLabelFilter = metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("%s,%s != false", instrumentedLabelKey, dash0EnableLabelKey),
+		LabelSelector: instrumentedLabelKey,
 	}
 )
 

internal/webhook/dash0_webhook.go

@@ -37,7 +37,7 @@ type resourceHandler func(h *Handler, request admission.Request, gvkLabel string
 type routing map[string]map[string]map[string]resourceHandler
 
 const (
-	optOutAdmissionAllowedMessage = "not instrumenting this resource due to dash0.com/enable=false"
+	optOutAdmissionAllowedMessage = "not instrumenting this workload due to dash0.com/enable=false"
 )
 
 var (
@@ -178,6 +178,7 @@ func (h *Handler) handleCronJob(
 		return h.postProcess(request, cronJob, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&cronJob.ObjectMeta) {
+		logger.Info(optOutAdmissionAllowedMessage)
 		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyCronJob(cronJob)
@@ -198,6 +199,7 @@ func (h *Handler) handleDaemonSet(
 		return h.postProcess(request, daemonSet, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&daemonSet.ObjectMeta) {
+		logger.Info(optOutAdmissionAllowedMessage)
 		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyDaemonSet(daemonSet)
@@ -218,6 +220,7 @@ func (h *Handler) handleDeployment(
 		return h.postProcess(request, deployment, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&deployment.ObjectMeta) {
+		logger.Info(optOutAdmissionAllowedMessage)
 		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyDeployment(deployment)
@@ -238,6 +241,7 @@ func (h *Handler) handleJob(
 		return h.postProcess(request, job, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&job.ObjectMeta) {
+		logger.Info(optOutAdmissionAllowedMessage)
 		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyJob(job)
@@ -258,6 +262,7 @@ func (h *Handler) handleReplicaSet(
 		return h.postProcess(request, replicaSet, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&replicaSet.ObjectMeta) {
+		logger.Info(optOutAdmissionAllowedMessage)
 		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyReplicaSet(replicaSet)
@@ -278,7 +283,8 @@ func (h *Handler) handleStatefulSet(
 		return h.postProcess(request, statefulSet, false, true, logger)
 	}
 	if util.HasOptedOutOfInstrumenationForWorkload(&statefulSet.ObjectMeta) {
-		return admission.Allowed("not instrumenting this resource due to dash0.com/out-out=true")
+		logger.Info(optOutAdmissionAllowedMessage)
+		return admission.Allowed(optOutAdmissionAllowedMessage)
 	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyStatefulSet(statefulSet)
 	return h.postProcess(request, statefulSet, hasBeenModified, false, logger)

internal/workloads/workload_modifier.go

@@ -88,9 +88,6 @@ func (m *ResourceModifier) ModifyStatefulSet(statefulSet *appsv1.StatefulSet) bo
 }
 
 func (m *ResourceModifier) modifyResource(podTemplateSpec *corev1.PodTemplateSpec, meta *metav1.ObjectMeta) bool {
-	if util.HasOptedOutOfInstrumenationForWorkload(meta) {
-		return false
-	}
 	hasBeenModified := m.modifyPodSpec(&podTemplateSpec.Spec)
 	if hasBeenModified {
 		util.AddInstrumentationLabels(meta, true, m.instrumentationMetadata)
@@ -318,9 +315,6 @@ func (m *ResourceModifier) RevertStatefulSet(statefulSet *appsv1.StatefulSet) bo
 }
 
 func (m *ResourceModifier) revertResource(podTemplateSpec *corev1.PodTemplateSpec, meta *metav1.ObjectMeta) bool {
-	if util.HasOptedOutOfInstrumenationForWorkload(meta) {
-		return false
-	}
 	if util.InstrumenationAttemptHasFailed(meta) {
 		// resource has never been instrumented successfully, only remove labels
 		util.RemoveInstrumentationLabels(meta)

internal/workloads/workload_modifier_test.go

@@ -112,14 +112,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			})
 		})
 
-		It("should not touch a deployment that has opted out of instrumentation", func() {
-			workload := DeploymentWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyDeployment(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyDeploymentWithOptOutLabel(workload)
-		})
-
 		It("should instrument a basic cron job", func() {
 			workload := BasicCronJob(TestNamespaceName, CronJobNamePrefix)
 			result := workloadModifier.ModifyCronJob(workload)
@@ -128,14 +120,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations)
 		})
 
-		It("should not touch a cron job that has opted out of instrumentation", func() {
-			workload := CronJobWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyCronJob(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyCronJobWithOptOutLabel(workload)
-		})
-
 		It("should instrument a basic daemon set", func() {
 			workload := BasicDaemonSet(TestNamespaceName, DaemonSetNamePrefix)
 			result := workloadModifier.ModifyDaemonSet(workload)
@@ -144,14 +128,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations)
 		})
 
-		It("should not touch a daemon set that has opted out of instrumentation", func() {
-			workload := DaemonSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyDaemonSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyDaemonSetWithOptOutLabel(workload)
-		})
-
 		It("should instrument a basic job", func() {
 			workload := BasicJob(TestNamespaceName, JobNamePrefix)
 			result := workloadModifier.ModifyJob(workload)
@@ -160,14 +136,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations)
 		})
 
-		It("should not touch a job that has opted out of instrumentation", func() {
-			workload := JobWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyJob(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyJobWithOptOutLabel(workload)
-		})
-
 		It("should instrument a basic replica set", func() {
 			workload := BasicReplicaSet(TestNamespaceName, ReplicaSetNamePrefix)
 			result := workloadModifier.ModifyReplicaSet(workload)
@@ -184,29 +152,13 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedReplicaSet(workload)
 		})
 
-		It("should not touch a replica set that has opted out of instrumentation", func() {
-			workload := ReplicaSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyReplicaSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyReplicaSetWithOptOutLabel(workload)
-		})
-
 		It("should instrument a basic stateful set", func() {
 			workload := BasicStatefulSet(TestNamespaceName, StatefulSetNamePrefix)
 			result := workloadModifier.ModifyStatefulSet(workload)
 
 			Expect(result).To(BeTrue())
 			VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations)
 		})
-
-		It("should not touch a stateful set that has opted out of instrumentation", func() {
-			workload := StatefulSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.ModifyStatefulSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyStatefulSetWithOptOutLabel(workload)
-		})
 	})
 
 	Context("when reverting workloads", func() {
@@ -218,14 +170,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedDeployment(workload)
 		})
 
-		It("should not touch deployments that have opted out of instrumentation", func() {
-			workload := DeploymentWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertDeployment(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyDeploymentWithOptOutLabel(workload)
-		})
-
 		It("should remove Dash0 from a instrumented deployment that has multiple containers, and already has volumes and init containers previous to being instrumented", func() {
 			workload := InstrumentedDeploymentWithMoreBellsAndWhistles(TestNamespaceName, DeploymentNamePrefix)
 			result := workloadModifier.RevertDeployment(workload)
@@ -263,14 +207,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedCronJob(workload)
 		})
 
-		It("should not touch cron jobs that have opted out of instrumentation", func() {
-			workload := CronJobWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertCronJob(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyCronJobWithOptOutLabel(workload)
-		})
-
 		It("should remove Dash0 from an instrumented daemon set", func() {
 			workload := InstrumentedDaemonSet(TestNamespaceName, DaemonSetNamePrefix)
 			result := workloadModifier.RevertDaemonSet(workload)
@@ -279,14 +215,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedDaemonSet(workload)
 		})
 
-		It("should not touch daemon sets that have opted out of instrumentation", func() {
-			workload := DaemonSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertDaemonSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyDaemonSetWithOptOutLabel(workload)
-		})
-
 		It("should remove Dash0 from an instrumented job", func() {
 			workload := InstrumentedJob(TestNamespaceName, JobNamePrefix)
 			result := workloadModifier.RevertJob(workload)
@@ -295,14 +223,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedJob(workload)
 		})
 
-		It("should not touch jobs that have opted out of instrumentation", func() {
-			workload := JobWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertJob(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyJobWithOptOutLabel(workload)
-		})
-
 		It("should remove Dash0 from an instrumented replica set", func() {
 			workload := InstrumentedReplicaSet(TestNamespaceName, ReplicaSetNamePrefix)
 			result := workloadModifier.RevertReplicaSet(workload)
@@ -311,14 +231,6 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			VerifyUnmodifiedReplicaSet(workload)
 		})
 
-		It("should not touch replica sets that have opted out of instrumentation", func() {
-			workload := ReplicaSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertReplicaSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyReplicaSetWithOptOutLabel(workload)
-		})
-
 		It("should not remove Dash0 from a replica set that is owned by a deployment", func() {
 			workload := InstrumentedReplicaSetOwnedByDeployment(TestNamespaceName, ReplicaSetNamePrefix)
 			result := workloadModifier.RevertReplicaSet(workload)
@@ -334,13 +246,5 @@ var _ = Describe("Dash0 Workload Modification", func() {
 			Expect(result).To(BeTrue())
 			VerifyUnmodifiedStatefulSet(workload)
 		})
-
-		It("should not touch a stateful sets that have opted out of instrumentation", func() {
-			workload := StatefulSetWithOptOutLabel(TestNamespaceName, DeploymentNamePrefix)
-			result := workloadModifier.RevertStatefulSet(workload)
-
-			Expect(result).To(BeFalse())
-			VerifyStatefulSetWithOptOutLabel(workload)
-		})
 	})
 })

internal/workloads/workloads_suite_test.go

@@ -7,7 +7,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-func TestK8sresources(t *testing.T) {
+func TestWorkloadModifications(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Workload Modifications Suite")
 }