Skip to content

Commit 643592c

Browse files
HoriodinoHoriodino
authored
Fix: variable propagation priority in Instrumentation CR. (#4324)
* fix(auto-instrumentation): ensure global spec.env vars propagate correctly over defaults Signed-off-by: Praful Khanduri <[email protected]> * test(auto-instrumentation): add E2E and unit tests for spec.env propagation fix Signed-off-by: Praful Khanduri <[email protected]> * fix(auto-instrumentation): updated dotnet for propagate the default envs at last Signed-off-by: Praful Khanduri <[email protected]> fix(auto-instrumentation): updated nodejs & javaagent for propagate the default envs at last Signed-off-by: Praful Khanduri <[email protected]> test(auto-instrumentation): updated e2e & unit tests Signed-off-by: Praful Khanduri <[email protected] --------- Signed-off-by: Praful Khanduri <[email protected]> Signed-off-by: Praful Khanduri <[email protected]
1 parent aa012eb commit 643592c

File tree

35 files changed

+688
-345
lines changed

35 files changed

+688
-345
lines changed

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: bug_fix
3+
4+
# The name of the component, or a single word describing the area of concern
5+
component: auto-instrumentation
6+
7+
# A brief description of the change
8+
note: Fixes the precedence of `spec.env` in Instrumentation CR so global env vars correctly override defaults.
9+
10+
# One or more tracking issues related to the change
11+
issues: [4068]
12+
13+
# (Optional) One or more lines of additional information to render under the primary note
14+
subtext: |
15+
Previously, environment variables set under `spec.env` were ignored in favor of default instrumentation config,
16+
unless duplicated in each language block. This change ensures the correct order of precedence is applied:
17+
language-specific env vars > spec.env > defaults.

‎internal/instrumentation/dotnet.go‎

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -62,39 +62,13 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runt
6262
if getIndexOfEnv(dotNetSpec.Env, envDotNetOTelAutoHome) > -1 {
6363
return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec")
6464
}
65-
66-
coreClrProfilerPath := ""
67-
switch runtime {
68-
case "", dotNetRuntimeLinuxGlibc:
69-
coreClrProfilerPath = dotNetCoreClrProfilerGlibcPath
70-
case dotNetRuntimeLinuxMusl:
71-
coreClrProfilerPath = dotNetCoreClrProfilerMuslPath
72-
default:
65+
if runtime != "" && runtime != dotNetRuntimeLinuxGlibc && runtime != dotNetRuntimeLinuxMusl {
7366
return pod, fmt.Errorf("provided instrumentation.opentelemetry.io/dotnet-runtime annotation value '%s' is not supported", runtime)
7467
}
7568

7669
// inject .NET instrumentation spec env vars.
7770
container.Env = appendIfNotSet(container.Env, dotNetSpec.Env...)
7871

79-
const (
80-
doNotConcatEnvValues = false
81-
concatEnvValues = true
82-
)
83-
84-
setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues)
85-
86-
setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerID, doNotConcatEnvValues)
87-
88-
setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, coreClrProfilerPath, doNotConcatEnvValues)
89-
90-
setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues)
91-
92-
setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues)
93-
94-
setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues)
95-
96-
setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues)
97-
9872
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
9973
Name: volume.Name,
10074
MountPath: dotnetInstrMountPath,
@@ -118,6 +92,30 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runt
11892
return pod, nil
11993
}
12094

95+
func injectDefaultDotNetEnvVars(container *corev1.Container, runtime string) {
96+
coreClrProfilerPath := ""
97+
switch runtime {
98+
case "", dotNetRuntimeLinuxGlibc:
99+
coreClrProfilerPath = dotNetCoreClrProfilerGlibcPath
100+
case dotNetRuntimeLinuxMusl:
101+
coreClrProfilerPath = dotNetCoreClrProfilerMuslPath
102+
}
103+
104+
setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, false)
105+
106+
setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerID, false)
107+
108+
setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, coreClrProfilerPath, false)
109+
110+
setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, true)
111+
112+
setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, true)
113+
114+
setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, false)
115+
116+
setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, true)
117+
}
118+
121119
// setDotNetEnvVar function sets env var to the container if not exist already.
122120
// value of concatValues should be set to true if the env var supports multiple values separated by :.
123121
// 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
130128
})
131129
return
132130
}
131+
// Don't modify env var if it uses ValueFrom
132+
if container.Env[idx].ValueFrom != nil {
133+
return
134+
}
133135
if concatValues {
134136
container.Env[idx].Value = fmt.Sprintf("%s:%s", container.Env[idx].Value, envVarValue)
135137
}

‎internal/instrumentation/dotnet_test.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -194,10 +194,7 @@ func TestInjectDotNetSDK(t *testing.T) {
194194
Name: envDotNetSharedStore,
195195
Value: fmt.Sprintf("%s:%s", "/foo:/bar", dotNetSharedStorePath),
196196
},
197-
{
198-
Name: envDotNetOTelAutoHome,
199-
Value: dotNetOTelAutoHomePath,
200-
},
197+
{Name: envDotNetOTelAutoHome, Value: dotNetOTelAutoHomePath},
201198
},
202199
},
203200
},
@@ -537,11 +534,18 @@ func TestInjectDotNetSDK(t *testing.T) {
537534
},
538535
}
539536

537+
injector := sdkInjector{}
540538
for _, test := range tests {
541539
t.Run(test.name, func(t *testing.T) {
542540
pod, err := injectDotNetSDK(test.DotNet, test.pod, 0, test.runtime, v1alpha1.InstrumentationSpec{})
543-
assert.Equal(t, test.expected, pod)
544541
assert.Equal(t, test.err, err)
542+
if err == nil {
543+
pod = injector.injectDefaultDotNetEnvVarsWrapper(pod, 0, test.runtime)
544+
assert.Equal(t, test.expected, pod)
545+
assert.Equal(t, test.err, err)
546+
} else {
547+
assert.Equal(t, test.expected, pod)
548+
}
545549
})
546550
}
547551
}

‎internal/instrumentation/javaagent.go‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -36,21 +36,6 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int, instSpec
3636
// Create unique mount path for this container
3737
containerMountPath := fmt.Sprintf("%s-%s", javaInstrMountPath, container.Name)
3838

39-
javaJVMArgument := fmt.Sprintf(" -javaagent:%s/javaagent.jar", containerMountPath)
40-
if len(javaSpec.Extensions) > 0 {
41-
javaJVMArgument = javaJVMArgument + fmt.Sprintf(" -Dotel.javaagent.extensions=%s/extensions", containerMountPath)
42-
}
43-
44-
idx := getIndexOfEnv(container.Env, envJavaToolsOptions)
45-
if idx == -1 {
46-
container.Env = append(container.Env, corev1.EnvVar{
47-
Name: envJavaToolsOptions,
48-
Value: javaJVMArgument,
49-
})
50-
} else {
51-
container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
52-
}
53-
5439
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
5540
Name: volume.Name,
5641
MountPath: containerMountPath,
@@ -86,3 +71,35 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int, instSpec
8671
}
8772
return pod, err
8873
}
74+
75+
func getDefaultJavaEnvVars(pod corev1.Pod, index int, javaSpec v1alpha1.Java) []corev1.EnvVar {
76+
container := &pod.Spec.Containers[index]
77+
containerMountPath := fmt.Sprintf("%s-%s", javaInstrMountPath, container.Name)
78+
79+
javaJVMArgument := fmt.Sprintf(" -javaagent:%s/javaagent.jar", containerMountPath)
80+
if len(javaSpec.Extensions) > 0 {
81+
javaJVMArgument = javaJVMArgument + fmt.Sprintf(" -Dotel.javaagent.extensions=%s/extensions", containerMountPath)
82+
}
83+
84+
idx := getIndexOfEnv(container.Env, envJavaToolsOptions)
85+
if idx == -1 {
86+
return []corev1.EnvVar{
87+
{
88+
Name: envJavaToolsOptions,
89+
Value: javaJVMArgument,
90+
},
91+
}
92+
} else {
93+
// Don't modify JAVA_TOOL_OPTIONS if it uses ValueFrom
94+
if container.Env[idx].ValueFrom != nil {
95+
return []corev1.EnvVar{}
96+
}
97+
// JAVA_TOOL_OPTIONS present, append our argument to its value
98+
return []corev1.EnvVar{
99+
{
100+
Name: envJavaToolsOptions,
101+
Value: container.Env[idx].Value + javaJVMArgument,
102+
},
103+
}
104+
}
105+
}

‎internal/instrumentation/javaagent_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,15 +326,19 @@ func TestInjectJavaagent(t *testing.T) {
326326
},
327327
}
328328

329+
injector := sdkInjector{}
329330
for _, test := range tests {
330331
t.Run(test.name, func(t *testing.T) {
331332
pod := test.pod
332333
var err error
333334
for i := range pod.Spec.Containers {
334335
pod, err = injectJavaagent(test.Java, pod, i, v1alpha1.InstrumentationSpec{})
335336
}
336-
assert.Equal(t, test.expected, pod)
337337
assert.Equal(t, test.err, err)
338+
for i := range pod.Spec.Containers {
339+
injector.injectDefaultJavaEnvVars(pod, i, test.Java)
340+
}
341+
assert.Equal(t, test.expected, pod)
338342
})
339343
}
340344
}

‎internal/instrumentation/nodejs.go‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,6 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int, inst
3131
// inject NodeJS instrumentation spec env vars.
3232
container.Env = appendIfNotSet(container.Env, nodeJSSpec.Env...)
3333

34-
idx := getIndexOfEnv(container.Env, envNodeOptions)
35-
if idx == -1 {
36-
container.Env = append(container.Env, corev1.EnvVar{
37-
Name: envNodeOptions,
38-
Value: nodeRequireArgument,
39-
})
40-
} else if idx > -1 {
41-
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
42-
}
43-
4434
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
4535
Name: volume.Name,
4636
MountPath: nodejsInstrMountPath,
@@ -63,3 +53,29 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int, inst
6353
}
6454
return pod, nil
6555
}
56+
57+
func getDefaultNodeJSEnvVars(pod corev1.Pod, index int) []corev1.EnvVar {
58+
container := &pod.Spec.Containers[index]
59+
idx := getIndexOfEnv(container.Env, envNodeOptions)
60+
if idx == -1 {
61+
return []corev1.EnvVar{
62+
{
63+
Name: envNodeOptions,
64+
Value: nodeRequireArgument,
65+
},
66+
}
67+
} else if idx > -1 {
68+
// Don't modify NODE_OPTIONS if it uses ValueFrom
69+
if container.Env[idx].ValueFrom != nil {
70+
return []corev1.EnvVar{}
71+
}
72+
// NODE_OPTIONS is set, append the required argument
73+
return []corev1.EnvVar{
74+
{
75+
Name: envNodeOptions,
76+
Value: container.Env[idx].Value + nodeRequireArgument,
77+
},
78+
}
79+
}
80+
return []corev1.EnvVar{}
81+
}

‎internal/instrumentation/nodejs_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,11 +170,13 @@ func TestInjectNodeJSSDK(t *testing.T) {
170170
},
171171
}
172172

173+
injector := sdkInjector{}
173174
for _, test := range tests {
174175
t.Run(test.name, func(t *testing.T) {
175176
pod, err := injectNodeJSSDK(test.NodeJS, test.pod, 0, v1alpha1.InstrumentationSpec{})
176-
assert.Equal(t, test.expected, pod)
177177
assert.Equal(t, test.err, err)
178+
injector.injectDefaultNodeJSEnvVars(pod, 0)
179+
assert.Equal(t, test.expected, pod)
178180
})
179181
}
180182
}

0 commit comments

Comments
 (0)