fix: ignore next webhook admission request after reverting

internal/controller/dash0_controller.go

@@ -28,6 +28,8 @@ import (
 )
 
 const (
+	FinalizerId = "operator.dash0.com/finalizer"
+
 	workkloadTypeLabel     = "workload type"
 	workloadNamespaceLabel = "workload namespace"
 	workloadNameLabel      = "workload name"
@@ -570,7 +572,7 @@ func (r *Dash0Reconciler) checkImminentDeletionAndHandleFinalizers(
 			return isMarkedForDeletion, err
 		}
 	} else {
-		if controllerutil.ContainsFinalizer(dash0CustomResource, util.FinalizerId) {
+		if controllerutil.ContainsFinalizer(dash0CustomResource, FinalizerId) {
 			err := r.runCleanupActions(ctx, dash0CustomResource, logger)
 			if err != nil {
 				// error has already been logged in runCleanupActions
@@ -601,7 +603,7 @@ func (r *Dash0Reconciler) runCleanupActions(
 		return err
 	}
 
-	controllerutil.RemoveFinalizer(dash0CustomResource, util.FinalizerId)
+	controllerutil.RemoveFinalizer(dash0CustomResource, FinalizerId)
 	if err = r.Update(ctx, dash0CustomResource); err != nil {
 		logger.Error(err, "Failed to remove the finalizer from the Dash0 custom resource, requeuing reconcile request.")
 		return err
@@ -613,7 +615,7 @@ func (r *Dash0Reconciler) addFinalizerIfNecessary(
 	ctx context.Context,
 	dash0CustomResource *operatorv1alpha1.Dash0,
 ) error {
-	finalizerHasBeenAdded := controllerutil.AddFinalizer(dash0CustomResource, util.FinalizerId)
+	finalizerHasBeenAdded := controllerutil.AddFinalizer(dash0CustomResource, FinalizerId)
 	if finalizerHasBeenAdded {
 		return r.Update(ctx, dash0CustomResource)
 	}
@@ -794,6 +796,9 @@ func (r *Dash0Reconciler) handleJobOnUninstrumentation(ctx context.Context, job
 			// There was an attempt to instrument this job (probably by the controller), which has not been successful.
 			// We only need remove the labels from that instrumentation attempt to clean up.
 			newWorkloadModifier(r.Versions, &logger).RemoveLabelsFromImmutableJob(&job)
+
+			// Apparently for jobs we do not need to set the "dash0.webhook.ignore.once" label, since changing there
+			// labels does not trigger a new admission request.
 			return r.Client.Update(ctx, &job)
 		}
 	}, &logger)
@@ -905,6 +910,10 @@ func (r *Dash0Reconciler) revertWorkloadInstrumentation(
 		}
 		hasBeenModified = workload.revert(&logger)
 		if hasBeenModified {
+			// Changing the workload spec sometimes triggers a new admission request, which would re-instrument the
+			// workload via the webhook immediately. To prevent this, we add a label that the webhook can check to
+			// prevent instrumentation.
+			util.AddWebhookIgnoreOnceLabel(objectMeta)
 			return r.Client.Update(ctx, workload.asClientObject())
 		} else {
 			return nil

internal/controller/dash0_controller_test.go

@@ -233,7 +233,9 @@ var _ = Describe("Dash0 Controller", func() {
 			triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload")
 
 			VerifySuccessfulUninstrumentationEvent(ctx, clientset, namespace, name, "controller")
-			VerifyUnmodifiedCronJob(GetCronJob(ctx, k8sClient, namespace, name))
+			cronJob = GetCronJob(ctx, k8sClient, namespace, name)
+			VerifyUnmodifiedCronJob(cronJob)
+			VerifyWebhookIgnoreOnceLabelIsPresent(&cronJob.ObjectMeta)
 		})
 
 		It("should revert an instrumented daemon set", func() {
@@ -255,7 +257,9 @@ var _ = Describe("Dash0 Controller", func() {
 			triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload")
 
 			VerifySuccessfulUninstrumentationEvent(ctx, clientset, namespace, name, "controller")
-			VerifyUnmodifiedDaemonSet(GetDaemonSet(ctx, k8sClient, namespace, name))
+			daemonSet = GetDaemonSet(ctx, k8sClient, namespace, name)
+			VerifyUnmodifiedDaemonSet(daemonSet)
+			VerifyWebhookIgnoreOnceLabelIsPresent(&daemonSet.ObjectMeta)
 		})
 
 		It("should revert an instrumented deployment", func() {
@@ -277,7 +281,9 @@ var _ = Describe("Dash0 Controller", func() {
 			triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload")
 
 			VerifySuccessfulUninstrumentationEvent(ctx, clientset, namespace, name, "controller")
-			VerifyUnmodifiedDeployment(GetDeployment(ctx, k8sClient, namespace, name))
+			deployment = GetDeployment(ctx, k8sClient, namespace, name)
+			VerifyUnmodifiedDeployment(deployment)
+			VerifyWebhookIgnoreOnceLabelIsPresent(&deployment.ObjectMeta)
 		})
 
 		It("should record a failure event when attempting to revert an existing instrumenting job (which has been instrumented by the webhook)", func() {
@@ -349,10 +355,12 @@ var _ = Describe("Dash0 Controller", func() {
 			triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload")
 
 			VerifySuccessfulUninstrumentationEvent(ctx, clientset, namespace, name, "controller")
-			VerifyUnmodifiedReplicaSet(GetReplicaSet(ctx, k8sClient, namespace, name))
+			replicaSet = GetReplicaSet(ctx, k8sClient, namespace, name)
+			VerifyUnmodifiedReplicaSet(replicaSet)
+			VerifyWebhookIgnoreOnceLabelIsPresent(&replicaSet.ObjectMeta)
 		})
 
-		It("should not leave existing uninstrumented replica sets owned by deployments alone", func() {
+		It("should leave existing uninstrumented replica sets owned by deployments alone", func() {
 			// We trigger one reconcile request before creating any workload and before deleting the Dash0 custom
 			// resource, just to get the `isFirstReconcile` logic out of the way and to add the finalizer.
 			// Alternatively, we could just add the finalizer here directly, but this approach is closer to what usually
@@ -393,7 +401,9 @@ var _ = Describe("Dash0 Controller", func() {
 			triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload")
 
 			VerifySuccessfulUninstrumentationEvent(ctx, clientset, namespace, name, "controller")
-			VerifyUnmodifiedStatefulSet(GetStatefulSet(ctx, k8sClient, namespace, name))
+			statefulSet = GetStatefulSet(ctx, k8sClient, namespace, name)
+			VerifyUnmodifiedStatefulSet(statefulSet)
+			VerifyWebhookIgnoreOnceLabelIsPresent(&statefulSet.ObjectMeta)
 		})
 	})
 })
@@ -460,7 +470,7 @@ func verifyDash0ResourceStatus(ctx context.Context, expectedStatus metav1.Condit
 }
 
 func removeFinalizer(ctx context.Context, dash0CustomResource *operatorv1alpha1.Dash0) {
-	finalizerHasBeenRemoved := controllerutil.RemoveFinalizer(dash0CustomResource, util.FinalizerId)
+	finalizerHasBeenRemoved := controllerutil.RemoveFinalizer(dash0CustomResource, FinalizerId)
 	if finalizerHasBeenRemoved {
 		Expect(k8sClient.Update(ctx, dash0CustomResource)).To(Succeed())
 	}

internal/util/labels.go

@@ -0,0 +1,27 @@
+// SPDX-FileCopyrightText: Copyright 2024 Dash0 Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package util
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const (
+	InstrumentedLabelKey              = "dash0.instrumented"
+	OperatorVersionLabelKey           = "dash0.operator.version"
+	InitContainerImageVersionLabelKey = "dash0.initcontainer.image.version"
+	InstrumentedByLabelKey            = "dash0.instrumented.by"
+	WebhookIgnoreOnceLabelKey         = "dash0.webhook.ignore.once"
+)
+
+func AddWebhookIgnoreOnceLabel(meta *metav1.ObjectMeta) {
+	AddLabel(meta, WebhookIgnoreOnceLabelKey, "true")
+}
+
+func AddLabel(meta *metav1.ObjectMeta, key string, value string) {
+	if meta.Labels == nil {
+		meta.Labels = make(map[string]string, 1)
+	}
+	meta.Labels[key] = value
+}

internal/webhook/dash0_webhook.go

@@ -12,6 +12,7 @@ import (
 	"github.com/go-logr/logr"
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
@@ -109,8 +110,11 @@ func (h *Handler) handleCronJob(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&cronJob.ObjectMeta) {
+		return h.postProcess(request, cronJob, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyCronJob(cronJob)
-	return h.postProcess(request, cronJob, hasBeenModified, logger)
+	return h.postProcess(request, cronJob, hasBeenModified, false, logger)
 }
 
 func (h *Handler) handleDaemonSet(
@@ -123,8 +127,11 @@ func (h *Handler) handleDaemonSet(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&daemonSet.ObjectMeta) {
+		return h.postProcess(request, daemonSet, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyDaemonSet(daemonSet)
-	return h.postProcess(request, daemonSet, hasBeenModified, logger)
+	return h.postProcess(request, daemonSet, hasBeenModified, false, logger)
 }
 
 func (h *Handler) handleDeployment(
@@ -137,8 +144,11 @@ func (h *Handler) handleDeployment(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&deployment.ObjectMeta) {
+		return h.postProcess(request, deployment, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyDeployment(deployment)
-	return h.postProcess(request, deployment, hasBeenModified, logger)
+	return h.postProcess(request, deployment, hasBeenModified, false, logger)
 }
 
 func (h *Handler) handleJob(
@@ -151,8 +161,11 @@ func (h *Handler) handleJob(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&job.ObjectMeta) {
+		return h.postProcess(request, job, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyJob(job)
-	return h.postProcess(request, job, hasBeenModified, logger)
+	return h.postProcess(request, job, hasBeenModified, false, logger)
 }
 
 func (h *Handler) handleReplicaSet(
@@ -165,8 +178,11 @@ func (h *Handler) handleReplicaSet(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&replicaSet.ObjectMeta) {
+		return h.postProcess(request, replicaSet, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyReplicaSet(replicaSet)
-	return h.postProcess(request, replicaSet, hasBeenModified, logger)
+	return h.postProcess(request, replicaSet, hasBeenModified, false, logger)
 }
 
 func (h *Handler) handleStatefulSet(
@@ -179,8 +195,11 @@ func (h *Handler) handleStatefulSet(
 	if failed {
 		return responseIfFailed
 	}
+	if isIgnored(&statefulSet.ObjectMeta) {
+		return h.postProcess(request, statefulSet, false, true, logger)
+	}
 	hasBeenModified := h.newWorkloadModifier(logger).ModifyStatefulSet(statefulSet)
-	return h.postProcess(request, statefulSet, hasBeenModified, logger)
+	return h.postProcess(request, statefulSet, hasBeenModified, false, logger)
 }
 
 func (h *Handler) preProcess(
@@ -196,13 +215,25 @@ func (h *Handler) preProcess(
 	return admission.Response{}, false
 }
 
+func isIgnored(meta *metav1.ObjectMeta) bool {
+	if meta.Labels == nil {
+		return false
+	}
+	if value, ok := meta.Labels[util.WebhookIgnoreOnceLabelKey]; ok && value == "true" {
+		delete(meta.Labels, util.WebhookIgnoreOnceLabelKey)
+		return true
+	}
+	return false
+}
+
 func (h *Handler) postProcess(
 	request admission.Request,
 	resource runtime.Object,
 	hasBeenModified bool,
+	ignored bool,
 	logger *logr.Logger,
 ) admission.Response {
-	if !hasBeenModified {
+	if !ignored && !hasBeenModified {
 		logger.Info("Dash0 instrumentation already present, no modification by webhook is necessary.")
 		util.QueueNoInstrumentationNecessaryEvent(h.Recorder, resource, "webhook")
 		return admission.Allowed("no changes")
@@ -214,6 +245,12 @@ func (h *Handler) postProcess(
 		return admission.Allowed(fmt.Errorf("error when marshalling modfied resource to JSON: %w", err).Error())
 	}
 
+	if ignored {
+		logger.Info(fmt.Sprintf("Ignoring this admission request due to the presence of %s.", util.WebhookIgnoreOnceLabelKey))
+		// deliberately not queueing an event for this case
+		return admission.PatchResponseFromRaw(request.Object.Raw, marshalled)
+	}
+
 	logger.Info("The webhook has added Dash0 instrumentation to the workload.")
 	util.QueueSuccessfulInstrumentationEvent(h.Recorder, resource, "webhook")
 	return admission.PatchResponseFromRaw(request.Object.Raw, marshalled)

internal/webhook/dash0_webhook_test.go

@@ -6,6 +6,8 @@ package webhook
 import (
 	"fmt"
 
+	"github.com/dash0hq/dash0-operator/internal/util"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -139,4 +141,61 @@ var _ = Describe("Dash0 Webhook", func() {
 			VerifyModifiedStatefulSet(statefulSet, BasicInstrumentedPodSpecExpectations)
 		})
 	})
+
+	Context("when seeing the ignore once label", func() {
+		It("should not instrument a cron job that has the label, but remove the label", func() {
+			workload := BasicCronJob(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetCronJob(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedCronJob(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+
+		It("should not instrument a daemonset that has the label, but remove the label", func() {
+			workload := BasicDaemonSet(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetDaemonSet(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedDaemonSet(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+
+		It("should not instrument a deployment that has the label, but remove the label", func() {
+			workload := BasicDeployment(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetDeployment(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedDeployment(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+
+		It("should not instrument a job that has the label, but remove the label", func() {
+			workload := BasicJob(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetJob(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedJob(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+
+		It("should not instrument an orphan replica set that has the label, but remove the label", func() {
+			workload := BasicReplicaSet(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetReplicaSet(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedReplicaSet(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+
+		It("should not instrument a stateful set that has the label, but remove the label", func() {
+			workload := BasicStatefulSet(TestNamespaceName, DeploymentName)
+			AddLabel(&workload.ObjectMeta, util.WebhookIgnoreOnceLabelKey, "true")
+			CreateWorkload(ctx, k8sClient, workload)
+			workload = GetStatefulSet(ctx, k8sClient, TestNamespaceName, DeploymentName)
+			VerifyUnmodifiedStatefulSet(workload)
+			VerifyWebhookIgnoreOnceLabelIsAbesent(&workload.ObjectMeta)
+		})
+	})
+
 })

internal/workloads/workload_modifier.go

@@ -288,17 +288,10 @@ func (m *ResourceModifier) addInstrumentationLabels(
 	meta *metav1.ObjectMeta,
 	hasBeenInstrumented bool,
 ) {
-	m.addLabel(meta, util.InstrumentedLabelKey, strconv.FormatBool(hasBeenInstrumented))
-	m.addLabel(meta, util.OperatorVersionLabelKey, m.instrumentationMetadata.OperatorVersion)
-	m.addLabel(meta, util.InitContainerImageVersionLabelKey, m.instrumentationMetadata.InitContainerImageVersion)
-	m.addLabel(meta, util.InstrumentedByLabelKey, m.instrumentationMetadata.InstrumentedBy)
-}
-
-func (m *ResourceModifier) addLabel(meta *metav1.ObjectMeta, key string, value string) {
-	if meta.Labels == nil {
-		meta.Labels = make(map[string]string, 1)
-	}
-	meta.Labels[key] = value
+	util.AddLabel(meta, util.InstrumentedLabelKey, strconv.FormatBool(hasBeenInstrumented))
+	util.AddLabel(meta, util.OperatorVersionLabelKey, m.instrumentationMetadata.OperatorVersion)
+	util.AddLabel(meta, util.InitContainerImageVersionLabelKey, m.instrumentationMetadata.InitContainerImageVersion)
+	util.AddLabel(meta, util.InstrumentedByLabelKey, m.instrumentationMetadata.InstrumentedBy)
 }
 
 func (m *ResourceModifier) RevertCronJob(cronJob *batchv1.CronJob) bool {