Skip to content

Commit b593fa7

Browse files
avadhut123pisalavadhut123pisal
authored
Validate all env. vars. before starting injecting env. vars (open-telemetry#1141)
* skips env var injection and sdk configurations if agent injection is skipped * mutate container at the last of SDK injection step * validate first and then mutate the container with env variables * fixes go lint issues * incorporates review comments * fixes go lint issue * removes return statement in case of failed instrumentation
1 parent 5d0f221 commit b593fa7

File tree

9 files changed

+132
-237
lines changed

9 files changed

+132
-237
lines changed

‎pkg/instrumentation/dotnet.go

+24-41
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ package instrumentation
1717
import (
1818
"fmt"
1919

20-
"github.com/go-logr/logr"
2120
corev1 "k8s.io/api/core/v1"
2221

2322
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
@@ -40,11 +39,17 @@ const (
4039
dotNetStartupHookPath = "/otel-auto-instrumentation/netcoreapp3.1/OpenTelemetry.AutoInstrumentation.StartupHook.dll"
4140
)
4241

43-
func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) corev1.Pod {
44-
// caller checks if there is at least one container
45-
container := pod.Spec.Containers[index]
42+
func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, error) {
4643

47-
// inject env vars
44+
// caller checks if there is at least one container.
45+
container := &pod.Spec.Containers[index]
46+
47+
err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore)
48+
if err != nil {
49+
return pod, err
50+
}
51+
52+
// inject .NET instrumentation spec env vars.
4853
for _, env := range dotNetSpec.Env {
4954
idx := getIndexOfEnv(container.Env, env.Name)
5055
if idx == -1 {
@@ -57,40 +62,26 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
5762
concatEnvValues = true
5863
)
5964

60-
if !trySetEnvVar(logger, &container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) {
61-
return pod
62-
}
65+
setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues)
6366

64-
if !trySetEnvVar(logger, &container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) {
65-
return pod
66-
}
67+
setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues)
6768

68-
if !trySetEnvVar(logger, &container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) {
69-
return pod
70-
}
69+
setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues)
7170

72-
if !trySetEnvVar(logger, &container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) {
73-
return pod
74-
}
71+
setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues)
7572

76-
if !trySetEnvVar(logger, &container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) {
77-
return pod
78-
}
73+
setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues)
7974

80-
if !trySetEnvVar(logger, &container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) {
81-
return pod
82-
}
75+
setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues)
8376

84-
if !trySetEnvVar(logger, &container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) {
85-
return pod
86-
}
77+
setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues)
8778

8879
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
8980
Name: volumeName,
9081
MountPath: "/otel-auto-instrumentation",
9182
})
9283

93-
// We just inject Volumes and init containers for the first processed container
84+
// We just inject Volumes and init containers for the first processed container.
9485
if isInitContainerMissing(pod) {
9586
pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
9687
Name: volumeName,
@@ -108,30 +99,22 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
10899
}},
109100
})
110101
}
111-
112-
pod.Spec.Containers[index] = container
113-
return pod
102+
return pod, nil
114103
}
115104

116-
func trySetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) bool {
105+
// setDotNetEnvVar function sets env var to the container if not exist already.
106+
// value of concatValues should be set to true if the env var supports multiple values separated by :.
107+
// If it is set to false, the original container's env var value has priority.
108+
func setDotNetEnvVar(container *corev1.Container, envVarName string, envVarValue string, concatValues bool) {
117109
idx := getIndexOfEnv(container.Env, envVarName)
118110
if idx < 0 {
119111
container.Env = append(container.Env, corev1.EnvVar{
120112
Name: envVarName,
121113
Value: envVarValue,
122114
})
123-
return true
115+
return
124116
}
125-
126-
if container.Env[idx].ValueFrom != nil {
127-
// TODO add to status object or submit it as an event
128-
logger.Info("Skipping DotNet SDK injection, the container defines env var value via ValueFrom", "envVar", envVarName, "container", container.Name)
129-
return false
130-
}
131-
132117
if concatValues {
133118
container.Env[idx].Value = fmt.Sprintf("%s:%s", container.Env[idx].Value, envVarValue)
134119
}
135-
136-
return true
137120
}

‎pkg/instrumentation/dotnet_test.go

+8-130
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"fmt"
1919
"testing"
2020

21-
"github.com/go-logr/logr"
2221
"github.com/stretchr/testify/assert"
2322
corev1 "k8s.io/api/core/v1"
2423

@@ -31,6 +30,7 @@ func TestInjectDotNetSDK(t *testing.T) {
3130
v1alpha1.DotNet
3231
pod corev1.Pod
3332
expected corev1.Pod
33+
err error
3434
}{
3535
{
3636
name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_SHARED_STORE, DOTNET_ADDITIONAL_DEPS, OTEL_DOTNET_AUTO_HOME not defined",
@@ -105,6 +105,7 @@ func TestInjectDotNetSDK(t *testing.T) {
105105
},
106106
},
107107
},
108+
err: nil,
108109
},
109110
{
110111
name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE, OTEL_DOTNET_AUTO_HOME defined",
@@ -210,102 +211,7 @@ func TestInjectDotNetSDK(t *testing.T) {
210211
},
211212
},
212213
},
213-
},
214-
{
215-
name: "CORECLR_ENABLE_PROFILING defined as ValueFrom",
216-
DotNet: v1alpha1.DotNet{Image: "foo/bar:1"},
217-
pod: corev1.Pod{
218-
Spec: corev1.PodSpec{
219-
Containers: []corev1.Container{
220-
{
221-
Env: []corev1.EnvVar{
222-
{
223-
Name: envDotNetCoreClrEnableProfiling,
224-
ValueFrom: &corev1.EnvVarSource{},
225-
},
226-
},
227-
},
228-
},
229-
},
230-
},
231-
expected: corev1.Pod{
232-
Spec: corev1.PodSpec{
233-
Containers: []corev1.Container{
234-
{
235-
Env: []corev1.EnvVar{
236-
{
237-
Name: envDotNetCoreClrEnableProfiling,
238-
ValueFrom: &corev1.EnvVarSource{},
239-
},
240-
},
241-
},
242-
},
243-
},
244-
},
245-
},
246-
{
247-
name: "CORECLR_PROFILER defined as ValueFrom",
248-
DotNet: v1alpha1.DotNet{Image: "foo/bar:1"},
249-
pod: corev1.Pod{
250-
Spec: corev1.PodSpec{
251-
Containers: []corev1.Container{
252-
{
253-
Env: []corev1.EnvVar{
254-
{
255-
Name: envDotNetCoreClrProfiler,
256-
ValueFrom: &corev1.EnvVarSource{},
257-
},
258-
},
259-
},
260-
},
261-
},
262-
},
263-
expected: corev1.Pod{
264-
Spec: corev1.PodSpec{
265-
Containers: []corev1.Container{
266-
{
267-
Env: []corev1.EnvVar{
268-
{
269-
Name: envDotNetCoreClrProfiler,
270-
ValueFrom: &corev1.EnvVarSource{},
271-
},
272-
},
273-
},
274-
},
275-
},
276-
},
277-
},
278-
{
279-
name: "CORECLR_PROFILER_PATH defined as ValueFrom",
280-
DotNet: v1alpha1.DotNet{Image: "foo/bar:1"},
281-
pod: corev1.Pod{
282-
Spec: corev1.PodSpec{
283-
Containers: []corev1.Container{
284-
{
285-
Env: []corev1.EnvVar{
286-
{
287-
Name: envDotNetCoreClrProfilerPath,
288-
ValueFrom: &corev1.EnvVarSource{},
289-
},
290-
},
291-
},
292-
},
293-
},
294-
},
295-
expected: corev1.Pod{
296-
Spec: corev1.PodSpec{
297-
Containers: []corev1.Container{
298-
{
299-
Env: []corev1.EnvVar{
300-
{
301-
Name: envDotNetCoreClrProfilerPath,
302-
ValueFrom: &corev1.EnvVarSource{},
303-
},
304-
},
305-
},
306-
},
307-
},
308-
},
214+
err: nil,
309215
},
310216
{
311217
name: "DOTNET_STARTUP_HOOKS defined as ValueFrom",
@@ -338,6 +244,7 @@ func TestInjectDotNetSDK(t *testing.T) {
338244
},
339245
},
340246
},
247+
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetStartupHook),
341248
},
342249
{
343250
name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom",
@@ -370,6 +277,7 @@ func TestInjectDotNetSDK(t *testing.T) {
370277
},
371278
},
372279
},
280+
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetAdditionalDeps),
373281
},
374282
{
375283
name: "DOTNET_SHARED_STORE defined as ValueFrom",
@@ -402,45 +310,15 @@ func TestInjectDotNetSDK(t *testing.T) {
402310
},
403311
},
404312
},
405-
},
406-
{
407-
name: "OTEL_DOTNET_AUTO_HOME defined as ValueFrom",
408-
DotNet: v1alpha1.DotNet{Image: "foo/bar:1"},
409-
pod: corev1.Pod{
410-
Spec: corev1.PodSpec{
411-
Containers: []corev1.Container{
412-
{
413-
Env: []corev1.EnvVar{
414-
{
415-
Name: envDotNetOTelAutoHome,
416-
ValueFrom: &corev1.EnvVarSource{},
417-
},
418-
},
419-
},
420-
},
421-
},
422-
},
423-
expected: corev1.Pod{
424-
Spec: corev1.PodSpec{
425-
Containers: []corev1.Container{
426-
{
427-
Env: []corev1.EnvVar{
428-
{
429-
Name: envDotNetOTelAutoHome,
430-
ValueFrom: &corev1.EnvVarSource{},
431-
},
432-
},
433-
},
434-
},
435-
},
436-
},
313+
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetSharedStore),
437314
},
438315
}
439316

440317
for _, test := range tests {
441318
t.Run(test.name, func(t *testing.T) {
442-
pod := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0)
319+
pod, err := injectDotNetSDK(test.DotNet, test.pod, 0)
443320
assert.Equal(t, test.expected, pod)
321+
assert.Equal(t, test.err, err)
444322
})
445323
}
446324
}

‎pkg/instrumentation/javaagent.go

+10-14
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
package instrumentation
1616

1717
import (
18-
"github.com/go-logr/logr"
1918
corev1 "k8s.io/api/core/v1"
2019

2120
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
@@ -26,11 +25,16 @@ const (
2625
javaJVMArgument = " -javaagent:/otel-auto-instrumentation/javaagent.jar"
2726
)
2827

29-
func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.Pod {
30-
// caller checks if there is at least one container
28+
func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, error) {
29+
// caller checks if there is at least one container.
3130
container := &pod.Spec.Containers[index]
3231

33-
// inject env vars
32+
err := validateContainerEnv(container.Env, envJavaToolsOptions)
33+
if err != nil {
34+
return pod, err
35+
}
36+
37+
// inject Java instrumentation spec env vars.
3438
for _, env := range javaSpec.Env {
3539
idx := getIndexOfEnv(container.Env, env.Name)
3640
if idx == -1 {
@@ -45,22 +49,15 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
4549
Value: javaJVMArgument,
4650
})
4751
} else {
48-
if container.Env[idx].ValueFrom != nil {
49-
// TODO add to status object or submit it as an event
50-
logger.Info("Skipping javaagent injection, the container defines JAVA_TOOL_OPTIONS env var value via ValueFrom", "container", container.Name)
51-
return pod
52-
}
53-
5452
container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
55-
5653
}
5754

5855
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
5956
Name: volumeName,
6057
MountPath: "/otel-auto-instrumentation",
6158
})
6259

63-
// We just inject Volumes and init containers for the first processed container
60+
// We just inject Volumes and init containers for the first processed container.
6461
if isInitContainerMissing(pod) {
6562
pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
6663
Name: volumeName,
@@ -78,6 +75,5 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
7875
}},
7976
})
8077
}
81-
82-
return pod
78+
return pod, err
8379
}

0 commit comments

Comments
 (0)