Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate all env. vars. before starting injecting env. vars #1141

Merged
merged 10 commits into from
Oct 12, 2022
Merged

Validate all env. vars. before starting injecting env. vars #1141

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
34127db
skips env var injection and sdk configurations if agent injection is …
avadhut123pisal Oct 5, 2022
134c44e
mutate container at the last of SDK injection step
avadhut123pisal Oct 5, 2022
928b2e0
Merge branch 'main' of github.com:avadhut123pisal/opentelemetry-opera…
avadhut123pisal Oct 8, 2022
ff8d0e3
validate first and then mutate the container with env variables
avadhut123pisal Oct 8, 2022
2a6569c
fixes go lint issues
avadhut123pisal Oct 8, 2022
0b2fce7
incorporates review comments
avadhut123pisal Oct 10, 2022
d703287
fixes go lint issue
avadhut123pisal Oct 10, 2022
cea2e0d
Merge branch 'main' into prevent-incomplete-auto-instrumentation
avadhut123pisal Oct 10, 2022
2710133
removes return statement in case of failed instrumentation
avadhut123pisal Oct 12, 2022
400fc2a
Merge branch 'prevent-incomplete-auto-instrumentation' of github.com:…
avadhut123pisal Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions pkg/instrumentation/dotnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const (
dotNetStartupHookPath = "/otel-auto-instrumentation/netcoreapp3.1/OpenTelemetry.AutoInstrumentation.StartupHook.dll"
)

func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) corev1.Pod {
func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, bool) {

// caller checks if there is at least one container
container := pod.Spec.Containers[index]

Expand All @@ -58,31 +59,31 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
)

if !trySetEnvVar(logger, &container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) {
return pod
return pod, true
}

if !trySetEnvVar(logger, &container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) {
return pod
return pod, true
}

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Expand Down Expand Up @@ -110,7 +111,7 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
}

pod.Spec.Containers[index] = container
return pod
return pod, false
}

func trySetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) bool {
Expand Down
17 changes: 14 additions & 3 deletions pkg/instrumentation/dotnet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ func TestInjectDotNetSDK(t *testing.T) {
tests := []struct {
name string
v1alpha1.DotNet
pod corev1.Pod
expected corev1.Pod
pod corev1.Pod
expected corev1.Pod
sdkInjectionSkipped bool
}{
{
name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_SHARED_STORE, DOTNET_ADDITIONAL_DEPS, OTEL_DOTNET_AUTO_HOME not defined",
Expand Down Expand Up @@ -105,6 +106,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE, OTEL_DOTNET_AUTO_HOME defined",
Expand Down Expand Up @@ -210,6 +212,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "CORECLR_ENABLE_PROFILING defined as ValueFrom",
Expand Down Expand Up @@ -242,6 +245,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "CORECLR_PROFILER defined as ValueFrom",
Expand Down Expand Up @@ -274,6 +278,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "CORECLR_PROFILER_PATH defined as ValueFrom",
Expand Down Expand Up @@ -306,6 +311,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "DOTNET_STARTUP_HOOKS defined as ValueFrom",
Expand Down Expand Up @@ -338,6 +344,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom",
Expand Down Expand Up @@ -370,6 +377,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "DOTNET_SHARED_STORE defined as ValueFrom",
Expand Down Expand Up @@ -402,6 +410,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
{
name: "OTEL_DOTNET_AUTO_HOME defined as ValueFrom",
Expand Down Expand Up @@ -434,13 +443,15 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0)
pod, sdkInjectionSkipped := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the contract to the positive value? sdkInjectionSkipped -> sdkInjected. It will requires changes in all places.

BTW current scenario looks like isDisabled=true, which is usually hard to understand and maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to resolve.

assert.Equal(t, test.expected, pod)
assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped)
})
}
}
11 changes: 5 additions & 6 deletions pkg/instrumentation/javaagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ const (
javaJVMArgument = " -javaagent:/otel-auto-instrumentation/javaagent.jar"
)

func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.Pod {
func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, bool) {
// caller checks if there is at least one container
container := &pod.Spec.Containers[index]
container := pod.Spec.Containers[index]

// inject env vars
for _, env := range javaSpec.Env {
Expand All @@ -48,11 +48,10 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
if container.Env[idx].ValueFrom != nil {
// TODO add to status object or submit it as an event
logger.Info("Skipping javaagent injection, the container defines JAVA_TOOL_OPTIONS env var value via ValueFrom", "container", container.Name)
return pod
return pod, true
}

container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument

}

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Expand All @@ -78,6 +77,6 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
}},
})
}

return pod
pod.Spec.Containers[index] = container
return pod, false
}
11 changes: 8 additions & 3 deletions pkg/instrumentation/javaagent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ func TestInjectJavaagent(t *testing.T) {
tests := []struct {
name string
v1alpha1.Java
pod corev1.Pod
expected corev1.Pod
pod corev1.Pod
expected corev1.Pod
sdkInjectionSkipped bool
}{
{
name: "JAVA_TOOL_OPTIONS not defined",
Expand Down Expand Up @@ -80,6 +81,7 @@ func TestInjectJavaagent(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "JAVA_TOOL_OPTIONS defined",
Expand Down Expand Up @@ -137,6 +139,7 @@ func TestInjectJavaagent(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "JAVA_TOOL_OPTIONS defined as ValueFrom",
Expand Down Expand Up @@ -169,13 +172,15 @@ func TestInjectJavaagent(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := injectJavaagent(logr.Discard(), test.Java, test.pod, 0)
pod, sdkInjectionSkipped := injectJavaagent(logr.Discard(), test.Java, test.pod, 0)
assert.Equal(t, test.expected, pod)
assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped)
})
}
}
10 changes: 5 additions & 5 deletions pkg/instrumentation/nodejs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ const (
nodeRequireArgument = " --require /otel-auto-instrumentation/autoinstrumentation.js"
)

func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) corev1.Pod {
func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (corev1.Pod, bool) {
// caller checks if there is at least one container
container := &pod.Spec.Containers[index]
container := pod.Spec.Containers[index]

// inject env vars
for _, env := range nodeJSSpec.Env {
Expand All @@ -48,7 +48,7 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.
if container.Env[idx].ValueFrom != nil {
// TODO add to status object or submit it as an event
logger.Info("Skipping NodeJS SDK injection, the container defines NODE_OPTIONS env var value via ValueFrom", "container", container.Name)
return pod
return pod, true
}

container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
Expand Down Expand Up @@ -78,6 +78,6 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.
}},
})
}

return pod
pod.Spec.Containers[index] = container
return pod, false
}
11 changes: 8 additions & 3 deletions pkg/instrumentation/nodejs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ func TestInjectNodeJSSDK(t *testing.T) {
tests := []struct {
name string
v1alpha1.NodeJS
pod corev1.Pod
expected corev1.Pod
pod corev1.Pod
expected corev1.Pod
sdkInjectionSkipped bool
}{
{
name: "NODE_OPTIONS not defined",
Expand Down Expand Up @@ -80,6 +81,7 @@ func TestInjectNodeJSSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "NODE_OPTIONS defined",
Expand Down Expand Up @@ -137,6 +139,7 @@ func TestInjectNodeJSSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "NODE_OPTIONS defined as ValueFrom",
Expand Down Expand Up @@ -169,13 +172,15 @@ func TestInjectNodeJSSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := injectNodeJSSDK(logr.Discard(), test.NodeJS, test.pod, 0)
pod, sdkInjectionSkipped := injectNodeJSSDK(logr.Discard(), test.NodeJS, test.pod, 0)
assert.Equal(t, test.expected, pod)
assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped)
})
}
}
10 changes: 5 additions & 5 deletions pkg/instrumentation/python.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ const (
pythonPathSuffix = "/otel-auto-instrumentation"
)

func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.Pod, index int) corev1.Pod {
func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.Pod, index int) (corev1.Pod, bool) {
// caller checks if there is at least one container
container := &pod.Spec.Containers[index]
container := pod.Spec.Containers[index]

// inject env vars
for _, env := range pythonSpec.Env {
Expand All @@ -52,7 +52,7 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
if container.Env[idx].ValueFrom != nil {
// TODO add to status object or submit it as an event
logger.Info("Skipping Python SDK injection, the container defines PYTHONPATH env var value via ValueFrom", "container", container.Name)
return pod
return pod, true
}

container.Env[idx].Value = fmt.Sprintf("%s:%s:%s", pythonPathPrefix, container.Env[idx].Value, pythonPathSuffix)
Expand Down Expand Up @@ -91,6 +91,6 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
}},
})
}

return pod
pod.Spec.Containers[index] = container
return pod, false
}
12 changes: 9 additions & 3 deletions pkg/instrumentation/python_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ func TestInjectPythonSDK(t *testing.T) {
tests := []struct {
name string
v1alpha1.Python
pod corev1.Pod
expected corev1.Pod
pod corev1.Pod
expected corev1.Pod
sdkInjectionSkipped bool
}{
{
name: "PYTHONPATH not defined",
Expand Down Expand Up @@ -85,6 +86,7 @@ func TestInjectPythonSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "PYTHONPATH defined",
Expand Down Expand Up @@ -146,6 +148,7 @@ func TestInjectPythonSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "OTEL_TRACES_EXPORTER defined",
Expand Down Expand Up @@ -207,6 +210,7 @@ func TestInjectPythonSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: false,
},
{
name: "PYTHONPATH defined as ValueFrom",
Expand Down Expand Up @@ -239,13 +243,15 @@ func TestInjectPythonSDK(t *testing.T) {
},
},
},
sdkInjectionSkipped: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := injectPythonSDK(logr.Discard(), test.Python, test.pod, 0)
pod, sdkInjectionSkipped := injectPythonSDK(logr.Discard(), test.Python, test.pod, 0)
assert.Equal(t, test.expected, pod)
assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped)
})
}
}
Loading