Fix: variable propagation priority in Instrumentation CR.

.chloggen/fix-cr spec.env-defaults.yaml

@@ -0,0 +1,17 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern
+component: auto-instrumentation
+
+# A brief description of the change
+note: Fixes the precedence of `spec.env` in Instrumentation CR so global env vars correctly override defaults.
+
+# One or more tracking issues related to the change
+issues: [4068]
+
+# (Optional) One or more lines of additional information to render under the primary note
+subtext: |
+  Previously, environment variables set under `spec.env` were ignored in favor of default instrumentation config, 
+  unless duplicated in each language block. This change ensures the correct order of precedence is applied:
+  language-specific env vars > spec.env > defaults.

internal/instrumentation/dotnet.go

@@ -62,39 +62,13 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runt
 	if getIndexOfEnv(dotNetSpec.Env, envDotNetOTelAutoHome) > -1 {
 		return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec")
 	}
-
-	coreClrProfilerPath := ""
-	switch runtime {
-	case "", dotNetRuntimeLinuxGlibc:
-		coreClrProfilerPath = dotNetCoreClrProfilerGlibcPath
-	case dotNetRuntimeLinuxMusl:
-		coreClrProfilerPath = dotNetCoreClrProfilerMuslPath
-	default:
+	if runtime != "" && runtime != dotNetRuntimeLinuxGlibc && runtime != dotNetRuntimeLinuxMusl {
 		return pod, fmt.Errorf("provided instrumentation.opentelemetry.io/dotnet-runtime annotation value '%s' is not supported", runtime)
 	}
 
 	// inject .NET instrumentation spec env vars.
 	container.Env = appendIfNotSet(container.Env, dotNetSpec.Env...)
 
-	const (
-		doNotConcatEnvValues = false
-		concatEnvValues      = true
-	)
-
-	setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerID, doNotConcatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, coreClrProfilerPath, doNotConcatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues)
-
-	setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues)
-
 	container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
 		Name:      volume.Name,
 		MountPath: dotnetInstrMountPath,
@@ -118,6 +92,30 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runt
 	return pod, nil
 }
 
+func injectDefaultDotNetEnvVars(container *corev1.Container, runtime string) {
+	coreClrProfilerPath := ""
+	switch runtime {
+	case "", dotNetRuntimeLinuxGlibc:
+		coreClrProfilerPath = dotNetCoreClrProfilerGlibcPath
+	case dotNetRuntimeLinuxMusl:
+		coreClrProfilerPath = dotNetCoreClrProfilerMuslPath
+	}
+
+	setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, false)
+
+	setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerID, false)
+
+	setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, coreClrProfilerPath, false)
+
+	setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, true)
+
+	setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, true)
+
+	setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, false)
+
+	setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, true)
+}
+
 // setDotNetEnvVar function sets env var to the container if not exist already.
 // value of concatValues should be set to true if the env var supports multiple values separated by :.
 // If it is set to false, the original container's env var value has priority.
@@ -130,6 +128,10 @@ func setDotNetEnvVar(container *corev1.Container, envVarName string, envVarValue
 		})
 		return
 	}
+	// Don't modify env var if it uses ValueFrom
+	if container.Env[idx].ValueFrom != nil {
+		return
+	}
 	if concatValues {
 		container.Env[idx].Value = fmt.Sprintf("%s:%s", container.Env[idx].Value, envVarValue)
 	}

internal/instrumentation/dotnet_test.go

@@ -194,10 +194,7 @@ func TestInjectDotNetSDK(t *testing.T) {
 									Name:  envDotNetSharedStore,
 									Value: fmt.Sprintf("%s:%s", "/foo:/bar", dotNetSharedStorePath),
 								},
-								{
-									Name:  envDotNetOTelAutoHome,
-									Value: dotNetOTelAutoHomePath,
-								},
+								{Name: envDotNetOTelAutoHome, Value: dotNetOTelAutoHomePath},
 							},
 						},
 					},
@@ -537,11 +534,18 @@ func TestInjectDotNetSDK(t *testing.T) {
 		},
 	}
 
+	injector := sdkInjector{}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			pod, err := injectDotNetSDK(test.DotNet, test.pod, 0, test.runtime, v1alpha1.InstrumentationSpec{})
-			assert.Equal(t, test.expected, pod)
 			assert.Equal(t, test.err, err)
+			if err == nil {
+				pod = injector.injectDefaultDotNetEnvVarsWrapper(pod, 0, test.runtime)
+				assert.Equal(t, test.expected, pod)
+				assert.Equal(t, test.err, err)
+			} else {
+				assert.Equal(t, test.expected, pod)
+			}
 		})
 	}
 }

internal/instrumentation/javaagent.go

@@ -36,21 +36,6 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int, instSpec
 	// Create unique mount path for this container
 	containerMountPath := fmt.Sprintf("%s-%s", javaInstrMountPath, container.Name)
 
-	javaJVMArgument := fmt.Sprintf(" -javaagent:%s/javaagent.jar", containerMountPath)
-	if len(javaSpec.Extensions) > 0 {
-		javaJVMArgument = javaJVMArgument + fmt.Sprintf(" -Dotel.javaagent.extensions=%s/extensions", containerMountPath)
-	}
-
-	idx := getIndexOfEnv(container.Env, envJavaToolsOptions)
-	if idx == -1 {
-		container.Env = append(container.Env, corev1.EnvVar{
-			Name:  envJavaToolsOptions,
-			Value: javaJVMArgument,
-		})
-	} else {
-		container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
-	}
-
 	container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
 		Name:      volume.Name,
 		MountPath: containerMountPath,
@@ -86,3 +71,35 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int, instSpec
 	}
 	return pod, err
 }
+
+func getDefaultJavaEnvVars(pod corev1.Pod, index int, javaSpec v1alpha1.Java) []corev1.EnvVar {
+	container := &pod.Spec.Containers[index]
+	containerMountPath := fmt.Sprintf("%s-%s", javaInstrMountPath, container.Name)
+
+	javaJVMArgument := fmt.Sprintf(" -javaagent:%s/javaagent.jar", containerMountPath)
+	if len(javaSpec.Extensions) > 0 {
+		javaJVMArgument = javaJVMArgument + fmt.Sprintf(" -Dotel.javaagent.extensions=%s/extensions", containerMountPath)
+	}
+
+	idx := getIndexOfEnv(container.Env, envJavaToolsOptions)
+	if idx == -1 {
+		return []corev1.EnvVar{
+			{
+				Name:  envJavaToolsOptions,
+				Value: javaJVMArgument,
+			},
+		}
+	} else {
+		// Don't modify JAVA_TOOL_OPTIONS if it uses ValueFrom
+		if container.Env[idx].ValueFrom != nil {
+			return []corev1.EnvVar{}
+		}
+		// JAVA_TOOL_OPTIONS present, append our argument to its value
+		return []corev1.EnvVar{
+			{
+				Name:  envJavaToolsOptions,
+				Value: container.Env[idx].Value + javaJVMArgument,
+			},
+		}
+	}
+}

internal/instrumentation/javaagent_test.go

@@ -326,15 +326,19 @@ func TestInjectJavaagent(t *testing.T) {
 		},
 	}
 
+	injector := sdkInjector{}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			pod := test.pod
 			var err error
 			for i := range pod.Spec.Containers {
 				pod, err = injectJavaagent(test.Java, pod, i, v1alpha1.InstrumentationSpec{})
 			}
-			assert.Equal(t, test.expected, pod)
 			assert.Equal(t, test.err, err)
+			for i := range pod.Spec.Containers {
+				injector.injectDefaultJavaEnvVars(pod, i, test.Java)
+			}
+			assert.Equal(t, test.expected, pod)
 		})
 	}
 }

internal/instrumentation/nodejs.go

@@ -31,16 +31,6 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int, inst
 	// inject NodeJS instrumentation spec env vars.
 	container.Env = appendIfNotSet(container.Env, nodeJSSpec.Env...)
 
-	idx := getIndexOfEnv(container.Env, envNodeOptions)
-	if idx == -1 {
-		container.Env = append(container.Env, corev1.EnvVar{
-			Name:  envNodeOptions,
-			Value: nodeRequireArgument,
-		})
-	} else if idx > -1 {
-		container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
-	}
-
 	container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
 		Name:      volume.Name,
 		MountPath: nodejsInstrMountPath,
@@ -63,3 +53,29 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int, inst
 	}
 	return pod, nil
 }
+
+func getDefaultNodeJSEnvVars(pod corev1.Pod, index int) []corev1.EnvVar {
+	container := &pod.Spec.Containers[index]
+	idx := getIndexOfEnv(container.Env, envNodeOptions)
+	if idx == -1 {
+		return []corev1.EnvVar{
+			{
+				Name:  envNodeOptions,
+				Value: nodeRequireArgument,
+			},
+		}
+	} else if idx > -1 {
+		// Don't modify NODE_OPTIONS if it uses ValueFrom
+		if container.Env[idx].ValueFrom != nil {
+			return []corev1.EnvVar{}
+		}
+		// NODE_OPTIONS is set, append the required argument
+		return []corev1.EnvVar{
+			{
+				Name:  envNodeOptions,
+				Value: container.Env[idx].Value + nodeRequireArgument,
+			},
+		}
+	}
+	return []corev1.EnvVar{}
+}

internal/instrumentation/nodejs_test.go

@@ -170,11 +170,13 @@ func TestInjectNodeJSSDK(t *testing.T) {
 		},
 	}
 
+	injector := sdkInjector{}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			pod, err := injectNodeJSSDK(test.NodeJS, test.pod, 0, v1alpha1.InstrumentationSpec{})
-			assert.Equal(t, test.expected, pod)
 			assert.Equal(t, test.err, err)
+			injector.injectDefaultNodeJSEnvVars(pod, 0)
+			assert.Equal(t, test.expected, pod)
 		})
 	}
 }