From 300a765a64a42d98dcc6d9a66dccc534b610ab65 Mon Sep 17 00:00:00 2001 From: Bastian Krol Date: Thu, 4 Jul 2024 08:35:07 +0200 Subject: [PATCH] fix(workloads): remove pre-0.6.0 --require from NODE_OPTIONS --- CONTRIBUTING.md | 7 ++ internal/controller/dash0_controller.go | 2 + internal/controller/dash0_controller_test.go | 26 +++---- internal/webhook/dash0_webhook_test.go | 34 ++++----- internal/workloads/workload_modifier.go | 41 +++++++---- internal/workloads/workload_modifier_test.go | 74 ++++++++++++++++---- test/util/verification.go | 8 +-- 7 files changed, 129 insertions(+), 63 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 357f4810..92b40676 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -167,6 +167,13 @@ they deploy in their `AfterAll`/`AfterEach` hooks. The scripts in `test-resource Run `make help` for more information on all potential `make` targets. More information can be found via the [Kubebuilder Documentation](https://book.kubebuilder.io/introduction.html) +## Migration Strategy When Updating Instrumentation Values + +When a new release of the operator changes the instrumentation values (new or changed environment variables, new labels, +new volumes etc.), we need to make sure that previously instrumented workloads are updated correctly. This should always +be accompanied by corresponding tests (for example new test cases in `workload_modifier_test.go`, see the test suite +`"when updating instrumentation from 0.5.1 to 0.6.0"` for an example). + ## Contributing No contribution guidelines are available at this point. diff --git a/internal/controller/dash0_controller.go b/internal/controller/dash0_controller.go index ecf941e1..c8024f01 100644 --- a/internal/controller/dash0_controller.go +++ b/internal/controller/dash0_controller.go @@ -661,6 +661,8 @@ func (r *Dash0Reconciler) instrumentWorkload( } else if util.HasBeenInstrumentedSuccessfully(objectMeta) { // No change necessary, this workload has already been instrumented and an opt-out label (which would need to // trigger uninstrumentation) has not been added since it has been instrumented. + // This check likely needs to change, see + // https://linear.app/dash0/issue/ENG-1955/updating-the-operator-should-run-a-first-reconcile-on-all-workloads-to return false } else if util.HasOptedOutOfInstrumenationAndIsUninstrumented(workload.getObjectMeta()) { logger.Info("not instrumenting this workload due to dash0.com/enable=false") diff --git a/internal/controller/dash0_controller_test.go b/internal/controller/dash0_controller_test.go index 6a482c67..abe44886 100644 --- a/internal/controller/dash0_controller_test.go +++ b/internal/controller/dash0_controller_test.go @@ -174,7 +174,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { triggerReconcileRequest(ctx, reconciler, "") verifyStatusConditionAndSuccessfulInstrumentationEvent(ctx, namespace, name) - VerifyModifiedCronJob(GetCronJob(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(GetCronJob(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) It("should instrument an existing daemon set", func() { @@ -186,7 +186,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { triggerReconcileRequest(ctx, reconciler, "") verifyStatusConditionAndSuccessfulInstrumentationEvent(ctx, namespace, name) - VerifyModifiedDaemonSet(GetDaemonSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(GetDaemonSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) It("should instrument an existing deployment", func() { @@ -251,7 +251,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { triggerReconcileRequest(ctx, reconciler, "") verifyStatusConditionAndSuccessfulInstrumentationEvent(ctx, namespace, name) - VerifyModifiedReplicaSet(GetReplicaSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(GetReplicaSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) It("should not instrument an existing replicaset owned by a deployment", func() { @@ -275,7 +275,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { triggerReconcileRequest(ctx, reconciler, "") verifyStatusConditionAndSuccessfulInstrumentationEvent(ctx, namespace, name) - VerifyModifiedStatefulSet(GetStatefulSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(GetStatefulSet(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) }) @@ -445,7 +445,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedCronJob(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedCronJob(GetCronJob(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(GetCronJob(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) @@ -454,7 +454,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedDaemonSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedDaemonSet(GetDaemonSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(GetDaemonSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) @@ -463,7 +463,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedDeployment(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedDeployment(GetDeployment(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(GetDeployment(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) @@ -472,7 +472,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedJob(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedJob(GetJob(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedJob(GetJob(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) @@ -481,7 +481,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedReplicaSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedReplicaSet(GetReplicaSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(GetReplicaSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) @@ -490,7 +490,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { workload := CreateInstrumentedStatefulSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) triggerReconcileRequest(ctx, reconciler, "") - VerifyModifiedStatefulSet(GetStatefulSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(GetStatefulSet(ctx, k8sClient, TestNamespaceName, name), BasicInstrumentedPodSpecExpectations()) VerifyNoEvents(ctx, clientset, TestNamespaceName) }) }) @@ -595,7 +595,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { "been successful. Error message: Dash0 cannot remove the instrumentation from the existing job "+ "test-namespace/%s, since this type of workload is immutable.", name), ) - VerifyModifiedJob(GetJob(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedJob(GetJob(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) It("should remove instrumentation labels from an existing job for which an instrumentation attempt has failed", func() { @@ -639,7 +639,7 @@ var _ = Describe("The Dash0 controller", Ordered, func() { triggerReconcileRequest(ctx, reconciler, "Trigger a reconcile request to revert the instrumented workload") VerifyNoEvents(ctx, clientset, namespace) - VerifyModifiedPod(GetPod(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedPod(GetPod(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) }) It("should leave existing uninstrumented pod owned by a replica set alone", func() { @@ -946,7 +946,7 @@ func verifyDeploymentIsBeingInstrumented(ctx context.Context, reconciler *Dash0R triggerReconcileRequest(ctx, reconciler, "") verifyStatusConditionAndSuccessfulInstrumentationEvent(ctx, namespace, name) - VerifyModifiedDeployment(GetDeployment(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(GetDeployment(ctx, k8sClient, namespace, name), BasicInstrumentedPodSpecExpectations()) return createdObjects } diff --git a/internal/webhook/dash0_webhook_test.go b/internal/webhook/dash0_webhook_test.go index 584953ee..0d8cf0db 100644 --- a/internal/webhook/dash0_webhook_test.go +++ b/internal/webhook/dash0_webhook_test.go @@ -118,7 +118,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicCronJob(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetCronJob(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -127,7 +127,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicDaemonSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetDaemonSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -136,7 +136,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicJob(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetJob(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -145,7 +145,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicPod(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetPod(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -163,7 +163,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicReplicaSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetReplicaSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -181,7 +181,7 @@ var _ = Describe("The Dash0 webhook", func() { workload := CreateBasicStatefulSet(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetStatefulSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) }) @@ -332,7 +332,7 @@ var _ = Describe("The Dash0 webhook", func() { RemoveOptOutLabel(&workload.ObjectMeta) UpdateWorkload(ctx, k8sClient, workload) workload = GetCronJob(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -344,7 +344,7 @@ var _ = Describe("The Dash0 webhook", func() { RemoveOptOutLabel(&workload.ObjectMeta) UpdateWorkload(ctx, k8sClient, workload) workload = GetDaemonSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -356,7 +356,7 @@ var _ = Describe("The Dash0 webhook", func() { RemoveOptOutLabel(&workload.ObjectMeta) UpdateWorkload(ctx, k8sClient, workload) workload = GetDeployment(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -371,7 +371,7 @@ var _ = Describe("The Dash0 webhook", func() { RemoveOptOutLabel(&workload.ObjectMeta) UpdateWorkload(ctx, k8sClient, workload) workload = GetReplicaSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -383,7 +383,7 @@ var _ = Describe("The Dash0 webhook", func() { RemoveOptOutLabel(&workload.ObjectMeta) UpdateWorkload(ctx, k8sClient, workload) workload = GetStatefulSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) }) @@ -397,7 +397,7 @@ var _ = Describe("The Dash0 webhook", func() { UpdateLabel(&workload.ObjectMeta, "dash0.com/enable", "true") UpdateWorkload(ctx, k8sClient, workload) workload = GetCronJob(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -409,7 +409,7 @@ var _ = Describe("The Dash0 webhook", func() { UpdateLabel(&workload.ObjectMeta, "dash0.com/enable", "true") UpdateWorkload(ctx, k8sClient, workload) workload = GetDaemonSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -421,7 +421,7 @@ var _ = Describe("The Dash0 webhook", func() { UpdateLabel(&workload.ObjectMeta, "dash0.com/enable", "true") UpdateWorkload(ctx, k8sClient, workload) workload = GetDeployment(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -436,7 +436,7 @@ var _ = Describe("The Dash0 webhook", func() { UpdateLabel(&workload.ObjectMeta, "dash0.com/enable", "true") UpdateWorkload(ctx, k8sClient, workload) workload = GetReplicaSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) @@ -448,7 +448,7 @@ var _ = Describe("The Dash0 webhook", func() { UpdateLabel(&workload.ObjectMeta, "dash0.com/enable", "true") UpdateWorkload(ctx, k8sClient, workload) workload = GetStatefulSet(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") }) }) @@ -614,7 +614,7 @@ func verifyDeploymentIsBeingInstrumented(createdObjects []client.Object) []clien workload := CreateBasicDeployment(ctx, k8sClient, TestNamespaceName, name) createdObjects = append(createdObjects, workload) workload = GetDeployment(ctx, k8sClient, TestNamespaceName, name) - VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) VerifySuccessfulInstrumentationEvent(ctx, clientset, TestNamespaceName, name, "webhook") return createdObjects } diff --git a/internal/workloads/workload_modifier.go b/internal/workloads/workload_modifier.go index 948c0181..10037077 100644 --- a/internal/workloads/workload_modifier.go +++ b/internal/workloads/workload_modifier.go @@ -31,6 +31,7 @@ const ( // envVarLdPreloadValue = "/__dash0__/preload/inject.so" envVarNodeOptionsName = "NODE_OPTIONS" envVarNodeOptionsValue = "--require /__dash0__/instrumentation/node.js/node_modules/@dash0hq/opentelemetry" + envVarNodeOptionsValue_0_5_1 = "--require /opt/dash0/instrumentation/node.js/node_modules/@dash0hq/opentelemetry" envVarDash0CollectorBaseUrlName = "DASH0_OTEL_COLLECTOR_BASE_URL" ) @@ -237,7 +238,7 @@ func (m *ResourceModifier) addMount(container *corev1.Container) { func (m *ResourceModifier) addEnvironmentVariables(container *corev1.Container, perContainerLogger logr.Logger) { // For now, we directly modify NODE_OPTIONS. Consider migrating to an LD_PRELOAD hook at some point. - m.addOrPrependToEnvironmentVariable(container, envVarNodeOptionsName, envVarNodeOptionsValue, perContainerLogger) + m.handleNodeOptionsEnvVar(container, perContainerLogger) m.addOrReplaceEnvironmentVariable( container, @@ -246,39 +247,51 @@ func (m *ResourceModifier) addEnvironmentVariables(container *corev1.Container, ) } -func (m *ResourceModifier) addOrPrependToEnvironmentVariable( +func (m *ResourceModifier) handleNodeOptionsEnvVar( container *corev1.Container, - name string, - value string, perContainerLogger logr.Logger, ) { if container.Env == nil { container.Env = make([]corev1.EnvVar, 0) } idx := slices.IndexFunc(container.Env, func(c corev1.EnvVar) bool { - return c.Name == name + return c.Name == envVarNodeOptionsName }) if idx < 0 { container.Env = append(container.Env, corev1.EnvVar{ - Name: name, - Value: value, + Name: envVarNodeOptionsName, + Value: envVarNodeOptionsValue, }) } else { - envVar := container.Env[idx] - previousValue := envVar.Value - if previousValue == "" && envVar.ValueFrom != nil { + // Note: This needs to be a point to the env var, otherwise updates would only be local to this function. + envVar := &container.Env[idx] + if envVar.Value == "" && envVar.ValueFrom != nil { perContainerLogger.Info( fmt.Sprintf( "Dash0 cannot prepend anything to the environment variable %s as it is specified via "+ "ValueFrom. This container will not be instrumented.", - name)) + envVarNodeOptionsName)) return } - if strings.Contains(previousValue, envVarNodeOptionsValue) { - return + + // update from 0.5.1 or earlier to 0.6.0: remove old --require instruction + if strings.Contains(envVar.Value, envVarNodeOptionsValue_0_5_1) { + // the three slightly different ReplaceAll calls handle all cases: the old require at the beginning of the + // string, in the middle, at the end, or the only content of NODE_OPTIONS. The point is that if NODE_OPTIONS + // has other content as well, we need to remove an extra space either before or after the old --require. + envVar.Value = strings.ReplaceAll(envVar.Value, envVarNodeOptionsValue_0_5_1+" ", "") + envVar.Value = strings.ReplaceAll(envVar.Value, " "+envVarNodeOptionsValue_0_5_1, "") + envVar.Value = strings.ReplaceAll(envVar.Value, envVarNodeOptionsValue_0_5_1, "") + } + + if !strings.Contains(envVar.Value, envVarNodeOptionsValue) { + if envVar.Value == "" { + envVar.Value = envVarNodeOptionsValue + } else { + envVar.Value = fmt.Sprintf("%s %s", envVarNodeOptionsValue, envVar.Value) + } } - container.Env[idx].Value = fmt.Sprintf("%s %s", value, previousValue) } } diff --git a/internal/workloads/workload_modifier_test.go b/internal/workloads/workload_modifier_test.go index 6e454b6a..62e947ed 100644 --- a/internal/workloads/workload_modifier_test.go +++ b/internal/workloads/workload_modifier_test.go @@ -46,7 +46,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyCronJob(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations()) }) It("should instrument a basic daemon set", func() { @@ -54,7 +54,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyDaemonSet(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations()) }) It("should add Dash0 to a basic deployment", func() { @@ -62,7 +62,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyDeployment(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) }) It("should instrument a deployment that has multiple containers, and already has volumes and init containers", func() { @@ -134,7 +134,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyJob(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations()) }) It("should instrument a basic ownerless pod", func() { @@ -142,7 +142,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyPod(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations()) }) It("should not instrument a basic pod owned by another higher level workload", func() { @@ -158,7 +158,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyReplicaSet(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) }) It("should not instrument a basic replica set that is owned by a deployment", func() { @@ -174,7 +174,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.ModifyStatefulSet(workload) Expect(hasBeenModified).To(BeTrue()) - VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations()) }) }) @@ -186,7 +186,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyCronJob(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedCronJob(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -197,7 +197,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyDaemonSet(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDaemonSet(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -208,7 +208,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyDeployment(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -219,7 +219,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyJob(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedJob(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -230,7 +230,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyPod(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedPod(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -241,7 +241,7 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyReplicaSet(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) @@ -252,11 +252,55 @@ var _ = Describe("Dash0 Workload Modification", func() { instrumentedOnce := workload.DeepCopy() hasBeenModified = workloadModifier.ModifyStatefulSet(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedStatefulSet(workload, BasicInstrumentedPodSpecExpectations()) Expect(reflect.DeepEqual(instrumentedOnce, workload)).To(BeTrue()) }) }) + Describe("when updating instrumentation from 0.5.1 to 0.6.0", func() { + It("should remove the old --require from NODE_OPTIONS (when it is the only content of NODE_OPTIONS)", func() { + workload := InstrumentedDeployment(TestNamespaceName, DeploymentNamePrefix) + Expect(workload.Spec.Template.Spec.Containers[0].Env[0].Name).To(Equal("NODE_OPTIONS")) + workload.Spec.Template.Spec.Containers[0].Env[0].Value = "--require /opt/dash0/instrumentation/node.js/node_modules/@dash0hq/opentelemetry" + hasBeenModified := workloadModifier.ModifyDeployment(workload) + Expect(hasBeenModified).To(BeTrue()) + VerifyModifiedDeployment(workload, BasicInstrumentedPodSpecExpectations()) + }) + + It("should remove the old --require from NODE_OPTIONS (when it is at the start of NODE_OPTIONS)", func() { + workload := InstrumentedDeployment(TestNamespaceName, DeploymentNamePrefix) + Expect(workload.Spec.Template.Spec.Containers[0].Env[0].Name).To(Equal("NODE_OPTIONS")) + workload.Spec.Template.Spec.Containers[0].Env[0].Value = "--require /opt/dash0/instrumentation/node.js/node_modules/@dash0hq/opentelemetry --require /something/else" + hasBeenModified := workloadModifier.ModifyDeployment(workload) + Expect(hasBeenModified).To(BeTrue()) + expectations := BasicInstrumentedPodSpecExpectations() + expectations.Containers[0].NodeOptionsValue = "--require /__dash0__/instrumentation/node.js/node_modules/@dash0hq/opentelemetry --require /something/else" + VerifyModifiedDeployment(workload, expectations) + }) + + It("should remove the old --require from NODE_OPTIONS (when it is at the end of NODE_OPTIONS)", func() { + workload := InstrumentedDeployment(TestNamespaceName, DeploymentNamePrefix) + Expect(workload.Spec.Template.Spec.Containers[0].Env[0].Name).To(Equal("NODE_OPTIONS")) + workload.Spec.Template.Spec.Containers[0].Env[0].Value = "--require /something/else --require /opt/dash0/instrumentation/node.js/node_modules/@dash0hq/opentelemetry" + hasBeenModified := workloadModifier.ModifyDeployment(workload) + Expect(hasBeenModified).To(BeTrue()) + expectations := BasicInstrumentedPodSpecExpectations() + expectations.Containers[0].NodeOptionsValue = "--require /__dash0__/instrumentation/node.js/node_modules/@dash0hq/opentelemetry --require /something/else" + VerifyModifiedDeployment(workload, expectations) + }) + + It("should remove the old --require from NODE_OPTIONS (when it is in the middle of NODE_OPTIONS)", func() { + workload := InstrumentedDeployment(TestNamespaceName, DeploymentNamePrefix) + Expect(workload.Spec.Template.Spec.Containers[0].Env[0].Name).To(Equal("NODE_OPTIONS")) + workload.Spec.Template.Spec.Containers[0].Env[0].Value = "--require /something/else --require /opt/dash0/instrumentation/node.js/node_modules/@dash0hq/opentelemetry --require /another/thing" + hasBeenModified := workloadModifier.ModifyDeployment(workload) + Expect(hasBeenModified).To(BeTrue()) + expectations := BasicInstrumentedPodSpecExpectations() + expectations.Containers[0].NodeOptionsValue = "--require /__dash0__/instrumentation/node.js/node_modules/@dash0hq/opentelemetry --require /something/else --require /another/thing" + VerifyModifiedDeployment(workload, expectations) + }) + }) + Describe("when reverting workloads", func() { It("should remove Dash0 from an instrumented cron job", func() { workload := InstrumentedCronJob(TestNamespaceName, CronJobNamePrefix) @@ -324,7 +368,7 @@ var _ = Describe("Dash0 Workload Modification", func() { hasBeenModified := workloadModifier.RevertReplicaSet(workload) Expect(hasBeenModified).To(BeFalse()) - VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations) + VerifyModifiedReplicaSet(workload, BasicInstrumentedPodSpecExpectations()) }) It("should remove Dash0 from an instrumented stateful set", func() { diff --git a/test/util/verification.go b/test/util/verification.go index 0e7ccae1..cdc93216 100644 --- a/test/util/verification.go +++ b/test/util/verification.go @@ -37,8 +37,8 @@ type PodSpecExpectations struct { Containers []ContainerExpectations } -var ( - BasicInstrumentedPodSpecExpectations = PodSpecExpectations{ +func BasicInstrumentedPodSpecExpectations() PodSpecExpectations { + return PodSpecExpectations{ Volumes: 1, Dash0VolumeIdx: 0, InitContainers: 1, @@ -53,7 +53,7 @@ var ( "http://dash0-operator-opentelemetry-collector.dash0-system.svc.cluster.local:4318", }}, } -) +} func VerifyModifiedCronJob(resource *batchv1.CronJob, expectations PodSpecExpectations) { verifyPodSpec(resource.Spec.JobTemplate.Spec.Template.Spec, expectations) @@ -126,7 +126,7 @@ func VerifyModifiedJob(resource *batchv1.Job, expectations PodSpecExpectations) } func VerifyModifiedJobAfterUnsuccessfulOptOut(resource *batchv1.Job) { - verifyPodSpec(resource.Spec.Template.Spec, BasicInstrumentedPodSpecExpectations) + verifyPodSpec(resource.Spec.Template.Spec, BasicInstrumentedPodSpecExpectations()) jobLabels := resource.ObjectMeta.Labels Expect(jobLabels["dash0.com/instrumented"]).To(Equal("true")) Expect(jobLabels["dash0.com/operator-image"]).To(Equal("some-registry.com_1234_dash0hq_operator-controller_1.2.3"))