From 17bbea6fd63a29e7ba9b0f59bc2ca9a482ebc780 Mon Sep 17 00:00:00 2001 From: Bastian Krol Date: Mon, 17 Jun 2024 11:45:47 +0200 Subject: [PATCH] chore: write log when ignoring workloads with dash0.com/enable=false ENG-1841 --- internal/controller/dash0_controller.go | 55 +++++++---- internal/util/labels.go | 4 +- internal/webhook/dash0_webhook.go | 10 +- internal/workloads/workload_modifier.go | 6 -- internal/workloads/workload_modifier_test.go | 96 -------------------- internal/workloads/workloads_suite_test.go | 2 +- 6 files changed, 50 insertions(+), 123 deletions(-) diff --git a/internal/controller/dash0_controller.go b/internal/controller/dash0_controller.go index 41586a63..0ea12f13 100644 --- a/internal/controller/dash0_controller.go +++ b/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 { diff --git a/internal/util/labels.go b/internal/util/labels.go index b1954d97..f82ad76f 100644 --- a/internal/util/labels.go +++ b/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, } ) diff --git a/internal/webhook/dash0_webhook.go b/internal/webhook/dash0_webhook.go index 34be8ac3..42dff63b 100644 --- a/internal/webhook/dash0_webhook.go +++ b/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) diff --git a/internal/workloads/workload_modifier.go b/internal/workloads/workload_modifier.go index a555f993..bb6ad31f 100644 --- a/internal/workloads/workload_modifier.go +++ b/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) diff --git a/internal/workloads/workload_modifier_test.go b/internal/workloads/workload_modifier_test.go index afe0bbe5..a49044dc 100644 --- a/internal/workloads/workload_modifier_test.go +++ b/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,14 +152,6 @@ 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) @@ -199,14 +159,6 @@ var _ = Describe("Dash0 Workload Modification", func() { 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) - }) }) }) diff --git a/internal/workloads/workloads_suite_test.go b/internal/workloads/workloads_suite_test.go index f3023e34..2a36c850 100644 --- a/internal/workloads/workloads_suite_test.go +++ b/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") }