From 34127db5ed2cb14a38fe589b4154388aba5c3ca4 Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Wed, 5 Oct 2022 15:29:47 +0530 Subject: [PATCH 01/11] skips env var injection and sdk configurations if agent injection is skipped --- pkg/instrumentation/dotnet.go | 19 +++++++------- pkg/instrumentation/dotnet_test.go | 17 ++++++++++--- pkg/instrumentation/javaagent.go | 6 ++--- pkg/instrumentation/javaagent_test.go | 11 +++++--- pkg/instrumentation/nodejs.go | 6 ++--- pkg/instrumentation/nodejs_test.go | 11 +++++--- pkg/instrumentation/python.go | 6 ++--- pkg/instrumentation/python_test.go | 12 ++++++--- pkg/instrumentation/sdk.go | 36 ++++++++++++++++++--------- 9 files changed, 82 insertions(+), 42 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index 210db334f0..e10d1238fb 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -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] @@ -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{ @@ -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 { diff --git a/pkg/instrumentation/dotnet_test.go b/pkg/instrumentation/dotnet_test.go index 00177e15f0..c5ecd881f0 100644 --- a/pkg/instrumentation/dotnet_test.go +++ b/pkg/instrumentation/dotnet_test.go @@ -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", @@ -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", @@ -210,6 +212,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "CORECLR_ENABLE_PROFILING defined as ValueFrom", @@ -242,6 +245,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "CORECLR_PROFILER defined as ValueFrom", @@ -274,6 +278,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "CORECLR_PROFILER_PATH defined as ValueFrom", @@ -306,6 +311,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "DOTNET_STARTUP_HOOKS defined as ValueFrom", @@ -338,6 +344,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom", @@ -370,6 +377,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "DOTNET_SHARED_STORE defined as ValueFrom", @@ -402,6 +410,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: true, }, { name: "OTEL_DOTNET_AUTO_HOME defined as ValueFrom", @@ -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) assert.Equal(t, test.expected, pod) + assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped) }) } } diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index 1d534a4b1d..d233105919 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -26,7 +26,7 @@ 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] @@ -48,7 +48,7 @@ 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 @@ -79,5 +79,5 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, }) } - return pod + return pod, false } diff --git a/pkg/instrumentation/javaagent_test.go b/pkg/instrumentation/javaagent_test.go index d97853bbfb..ff2c3ed439 100644 --- a/pkg/instrumentation/javaagent_test.go +++ b/pkg/instrumentation/javaagent_test.go @@ -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", @@ -80,6 +81,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "JAVA_TOOL_OPTIONS defined", @@ -137,6 +139,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "JAVA_TOOL_OPTIONS defined as ValueFrom", @@ -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) }) } } diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index 5a26fd6adf..a01478db6f 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -26,7 +26,7 @@ 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] @@ -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 @@ -79,5 +79,5 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. }) } - return pod + return pod, false } diff --git a/pkg/instrumentation/nodejs_test.go b/pkg/instrumentation/nodejs_test.go index e0a187f0ab..34c74fffe9 100644 --- a/pkg/instrumentation/nodejs_test.go +++ b/pkg/instrumentation/nodejs_test.go @@ -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", @@ -80,6 +81,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "NODE_OPTIONS defined", @@ -137,6 +139,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "NODE_OPTIONS defined as ValueFrom", @@ -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) }) } } diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index e2c7495b0a..920d234c35 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -30,7 +30,7 @@ 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] @@ -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) @@ -92,5 +92,5 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. }) } - return pod + return pod, false } diff --git a/pkg/instrumentation/python_test.go b/pkg/instrumentation/python_test.go index 2d510fca38..fb7a88caf4 100644 --- a/pkg/instrumentation/python_test.go +++ b/pkg/instrumentation/python_test.go @@ -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", @@ -85,6 +86,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "PYTHONPATH defined", @@ -146,6 +148,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "OTEL_TRACES_EXPORTER defined", @@ -207,6 +210,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, + sdkInjectionSkipped: false, }, { name: "PYTHONPATH defined as ValueFrom", @@ -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) }) } } diff --git a/pkg/instrumentation/sdk.go b/pkg/instrumentation/sdk.go index 346fc77681..522959982d 100644 --- a/pkg/instrumentation/sdk.go +++ b/pkg/instrumentation/sdk.go @@ -68,31 +68,43 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations // in the future we can define an annotation to configure this if insts.Java != nil { otelinst := *insts.Java + sdkInjectionSkipped := false i.logger.V(1).Info("injecting java instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod = injectJavaagent(i.logger, otelinst.Spec.Java, pod, index) - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + pod, sdkInjectionSkipped = injectJavaagent(i.logger, otelinst.Spec.Java, pod, index) + if !sdkInjectionSkipped { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + } } if insts.NodeJS != nil { otelinst := *insts.NodeJS + sdkInjectionSkipped := false i.logger.V(1).Info("injecting nodejs instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod = injectNodeJSSDK(i.logger, otelinst.Spec.NodeJS, pod, index) - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + pod, sdkInjectionSkipped = injectNodeJSSDK(i.logger, otelinst.Spec.NodeJS, pod, index) + if !sdkInjectionSkipped { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + } } if insts.Python != nil { otelinst := *insts.Python + sdkInjectionSkipped := false i.logger.V(1).Info("injecting python instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod = injectPythonSDK(i.logger, otelinst.Spec.Python, pod, index) - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + pod, sdkInjectionSkipped = injectPythonSDK(i.logger, otelinst.Spec.Python, pod, index) + if !sdkInjectionSkipped { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + } } if insts.DotNet != nil { otelinst := *insts.DotNet + sdkInjectionSkipped := false i.logger.V(1).Info("injecting dotnet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod = injectDotNetSDK(i.logger, otelinst.Spec.DotNet, pod, index) - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + pod, sdkInjectionSkipped = injectDotNetSDK(i.logger, otelinst.Spec.DotNet, pod, index) + if !sdkInjectionSkipped { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + } } if insts.Sdk != nil { otelinst := *insts.Sdk From 134c44ea5deb2ac5d0e6f67442d0be8515501dcc Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Wed, 5 Oct 2022 16:03:05 +0530 Subject: [PATCH 02/11] mutate container at the last of SDK injection step --- pkg/instrumentation/javaagent.go | 5 ++--- pkg/instrumentation/nodejs.go | 4 ++-- pkg/instrumentation/python.go | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index d233105919..06e85cd0ea 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -28,7 +28,7 @@ const ( 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 { @@ -52,7 +52,6 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, } container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument - } container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ @@ -78,6 +77,6 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, }}, }) } - + pod.Spec.Containers[index] = container return pod, false } diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index a01478db6f..7d6c717323 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -28,7 +28,7 @@ const ( 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 { @@ -78,6 +78,6 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. }}, }) } - + pod.Spec.Containers[index] = container return pod, false } diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index 920d234c35..5f415a7f13 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -32,7 +32,7 @@ const ( 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 { @@ -91,6 +91,6 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. }}, }) } - + pod.Spec.Containers[index] = container return pod, false } From ff8d0e360a25350bf0ae42eb53505e91be50349f Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Sat, 8 Oct 2022 12:27:54 +0530 Subject: [PATCH 03/11] validate first and then mutate the container with env variables --- pkg/instrumentation/dotnet.go | 57 ++++------ pkg/instrumentation/dotnet_test.go | 152 ++------------------------ pkg/instrumentation/javaagent.go | 20 ++-- pkg/instrumentation/javaagent_test.go | 16 +-- pkg/instrumentation/nodejs.go | 21 ++-- pkg/instrumentation/nodejs_test.go | 16 +-- pkg/instrumentation/python.go | 21 ++-- pkg/instrumentation/python_test.go | 18 +-- pkg/instrumentation/sdk.go | 38 +++++-- 9 files changed, 111 insertions(+), 248 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index e10d1238fb..8c684b7381 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -43,9 +43,16 @@ const ( 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] + container := &pod.Spec.Containers[index] - // inject env vars + // validate container environment variables + err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore) + if err != nil { + logger.Info("Skipping DotNet SDK injection", "reason:", err.Error(), "container Name", container.Name) + return pod, false + } + + // inject .Net instrumentation spec env vars for _, env := range dotNetSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -58,33 +65,19 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1. concatEnvValues = true ) - if !trySetEnvVar(logger, &container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) - if !trySetEnvVar(logger, &container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) { - return pod, true - } + setDotNetEnvVar(logger, container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ Name: volumeName, @@ -109,30 +102,20 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1. }}, }) } - - pod.Spec.Containers[index] = container - return pod, false + return pod, true } -func trySetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) bool { +// set env var to the container +func setDotNetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) { idx := getIndexOfEnv(container.Env, envVarName) if idx < 0 { container.Env = append(container.Env, corev1.EnvVar{ Name: envVarName, Value: envVarValue, }) - return true + return } - - if container.Env[idx].ValueFrom != nil { - // TODO add to status object or submit it as an event - logger.Info("Skipping DotNet SDK injection, the container defines env var value via ValueFrom", "envVar", envVarName, "container", container.Name) - return false - } - if concatValues { container.Env[idx].Value = fmt.Sprintf("%s:%s", container.Env[idx].Value, envVarValue) } - - return true } diff --git a/pkg/instrumentation/dotnet_test.go b/pkg/instrumentation/dotnet_test.go index c5ecd881f0..6ea91dc0c6 100644 --- a/pkg/instrumentation/dotnet_test.go +++ b/pkg/instrumentation/dotnet_test.go @@ -29,9 +29,9 @@ func TestInjectDotNetSDK(t *testing.T) { tests := []struct { name string v1alpha1.DotNet - pod corev1.Pod - expected corev1.Pod - sdkInjectionSkipped bool + pod corev1.Pod + expected corev1.Pod + sdkInjected 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", @@ -106,7 +106,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE, OTEL_DOTNET_AUTO_HOME defined", @@ -212,106 +212,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, - }, - { - name: "CORECLR_ENABLE_PROFILING defined as ValueFrom", - DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, - pod: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrEnableProfiling, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - expected: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrEnableProfiling, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - sdkInjectionSkipped: true, - }, - { - name: "CORECLR_PROFILER defined as ValueFrom", - DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, - pod: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrProfiler, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - expected: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrProfiler, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - sdkInjectionSkipped: true, - }, - { - name: "CORECLR_PROFILER_PATH defined as ValueFrom", - DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, - pod: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrProfilerPath, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - expected: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetCoreClrProfilerPath, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - sdkInjectionSkipped: true, + sdkInjected: true, }, { name: "DOTNET_STARTUP_HOOKS defined as ValueFrom", @@ -344,7 +245,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, + sdkInjected: false, }, { name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom", @@ -377,7 +278,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, + sdkInjected: false, }, { name: "DOTNET_SHARED_STORE defined as ValueFrom", @@ -410,48 +311,15 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, - }, - { - name: "OTEL_DOTNET_AUTO_HOME defined as ValueFrom", - DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, - pod: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetOTelAutoHome, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - expected: corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Env: []corev1.EnvVar{ - { - Name: envDotNetOTelAutoHome, - ValueFrom: &corev1.EnvVarSource{}, - }, - }, - }, - }, - }, - }, - sdkInjectionSkipped: true, + sdkInjected: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjectionSkipped := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0) + pod, sdkInjected := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped) + assert.Equal(t, test.sdkInjected, sdkInjected) }) } } diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index 06e85cd0ea..03e092d9b8 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -28,9 +28,16 @@ const ( 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 + // validate container environment variables + err := validateContainerEnv(container.Env, envJavaToolsOptions) + if err != nil { + logger.Info("Skipping javaagent injection", "reason:", err.Error(), "container Name", container.Name) + return pod, false + } + + // inject Java instrumentation spec env vars for _, env := range javaSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -45,12 +52,6 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, Value: javaJVMArgument, }) } else { - 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, true - } - container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument } @@ -77,6 +78,5 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, }}, }) } - pod.Spec.Containers[index] = container - return pod, false + return pod, true } diff --git a/pkg/instrumentation/javaagent_test.go b/pkg/instrumentation/javaagent_test.go index ff2c3ed439..df9ca5069f 100644 --- a/pkg/instrumentation/javaagent_test.go +++ b/pkg/instrumentation/javaagent_test.go @@ -28,9 +28,9 @@ func TestInjectJavaagent(t *testing.T) { tests := []struct { name string v1alpha1.Java - pod corev1.Pod - expected corev1.Pod - sdkInjectionSkipped bool + pod corev1.Pod + expected corev1.Pod + sdkInjected bool }{ { name: "JAVA_TOOL_OPTIONS not defined", @@ -81,7 +81,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "JAVA_TOOL_OPTIONS defined", @@ -139,7 +139,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "JAVA_TOOL_OPTIONS defined as ValueFrom", @@ -172,15 +172,15 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, + sdkInjected: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjectionSkipped := injectJavaagent(logr.Discard(), test.Java, test.pod, 0) + pod, sdkInjected := injectJavaagent(logr.Discard(), test.Java, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped) + assert.Equal(t, test.sdkInjected, sdkInjected) }) } } diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index 7d6c717323..d413d2a62b 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -28,9 +28,16 @@ const ( 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 + // validate container environment variables + err := validateContainerEnv(container.Env, envNodeOptions) + if err != nil { + logger.Info("Skipping NodeJS SDK injection", "reason:", err.Error(), "container Name", container.Name) + return pod, false + } + + // inject NodeJS instrumentation spec env vars for _, env := range nodeJSSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -45,14 +52,7 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. Value: nodeRequireArgument, }) } else if idx > -1 { - 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, true - } - container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument - } container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ @@ -78,6 +78,5 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. }}, }) } - pod.Spec.Containers[index] = container - return pod, false + return pod, true } diff --git a/pkg/instrumentation/nodejs_test.go b/pkg/instrumentation/nodejs_test.go index 34c74fffe9..2f9c0e1c6b 100644 --- a/pkg/instrumentation/nodejs_test.go +++ b/pkg/instrumentation/nodejs_test.go @@ -28,9 +28,9 @@ func TestInjectNodeJSSDK(t *testing.T) { tests := []struct { name string v1alpha1.NodeJS - pod corev1.Pod - expected corev1.Pod - sdkInjectionSkipped bool + pod corev1.Pod + expected corev1.Pod + sdkInjected bool }{ { name: "NODE_OPTIONS not defined", @@ -81,7 +81,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "NODE_OPTIONS defined", @@ -139,7 +139,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "NODE_OPTIONS defined as ValueFrom", @@ -172,15 +172,15 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, + sdkInjected: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjectionSkipped := injectNodeJSSDK(logr.Discard(), test.NodeJS, test.pod, 0) + pod, sdkInjected := injectNodeJSSDK(logr.Discard(), test.NodeJS, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped) + assert.Equal(t, test.sdkInjected, sdkInjected) }) } } diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index 5f415a7f13..c045240773 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -32,9 +32,16 @@ const ( 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 + // validate container environment variables + err := validateContainerEnv(container.Env, envPythonPath) + if err != nil { + logger.Info("Skipping Python SDK injection", "reason:", err.Error(), "container Name", container.Name) + return pod, false + } + + // inject Python instrumentation spec env vars for _, env := range pythonSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -49,14 +56,7 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. Value: fmt.Sprintf("%s:%s", pythonPathPrefix, pythonPathSuffix), }) } else if idx > -1 { - 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, true - } - container.Env[idx].Value = fmt.Sprintf("%s:%s:%s", pythonPathPrefix, container.Env[idx].Value, pythonPathSuffix) - } // Set OTEL_TRACES_EXPORTER to HTTP exporter if not set by user because it is what our autoinstrumentation supports. @@ -91,6 +91,5 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. }}, }) } - pod.Spec.Containers[index] = container - return pod, false + return pod, true } diff --git a/pkg/instrumentation/python_test.go b/pkg/instrumentation/python_test.go index fb7a88caf4..8a0b41519d 100644 --- a/pkg/instrumentation/python_test.go +++ b/pkg/instrumentation/python_test.go @@ -29,9 +29,9 @@ func TestInjectPythonSDK(t *testing.T) { tests := []struct { name string v1alpha1.Python - pod corev1.Pod - expected corev1.Pod - sdkInjectionSkipped bool + pod corev1.Pod + expected corev1.Pod + sdkInjected bool }{ { name: "PYTHONPATH not defined", @@ -86,7 +86,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "PYTHONPATH defined", @@ -148,7 +148,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "OTEL_TRACES_EXPORTER defined", @@ -210,7 +210,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: false, + sdkInjected: true, }, { name: "PYTHONPATH defined as ValueFrom", @@ -243,15 +243,15 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjectionSkipped: true, + sdkInjected: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjectionSkipped := injectPythonSDK(logr.Discard(), test.Python, test.pod, 0) + pod, sdkInjected := injectPythonSDK(logr.Discard(), test.Python, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjectionSkipped, sdkInjectionSkipped) + assert.Equal(t, test.sdkInjected, sdkInjected) }) } } diff --git a/pkg/instrumentation/sdk.go b/pkg/instrumentation/sdk.go index 522959982d..1403b67081 100644 --- a/pkg/instrumentation/sdk.go +++ b/pkg/instrumentation/sdk.go @@ -68,40 +68,40 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations // in the future we can define an annotation to configure this if insts.Java != nil { otelinst := *insts.Java - sdkInjectionSkipped := false + sdkInjected := false i.logger.V(1).Info("injecting java instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjectionSkipped = injectJavaagent(i.logger, otelinst.Spec.Java, pod, index) - if !sdkInjectionSkipped { + pod, sdkInjected = injectJavaagent(i.logger, otelinst.Spec.Java, pod, index) + if sdkInjected { pod = i.injectCommonEnvVar(otelinst, pod, index) pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } } if insts.NodeJS != nil { otelinst := *insts.NodeJS - sdkInjectionSkipped := false + sdkInjected := false i.logger.V(1).Info("injecting nodejs instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjectionSkipped = injectNodeJSSDK(i.logger, otelinst.Spec.NodeJS, pod, index) - if !sdkInjectionSkipped { + pod, sdkInjected = injectNodeJSSDK(i.logger, otelinst.Spec.NodeJS, pod, index) + if sdkInjected { pod = i.injectCommonEnvVar(otelinst, pod, index) pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } } if insts.Python != nil { otelinst := *insts.Python - sdkInjectionSkipped := false + sdkInjected := false i.logger.V(1).Info("injecting python instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjectionSkipped = injectPythonSDK(i.logger, otelinst.Spec.Python, pod, index) - if !sdkInjectionSkipped { + pod, sdkInjected = injectPythonSDK(i.logger, otelinst.Spec.Python, pod, index) + if sdkInjected { pod = i.injectCommonEnvVar(otelinst, pod, index) pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } } if insts.DotNet != nil { otelinst := *insts.DotNet - sdkInjectionSkipped := false + sdkInjected := false i.logger.V(1).Info("injecting dotnet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjectionSkipped = injectDotNetSDK(i.logger, otelinst.Spec.DotNet, pod, index) - if !sdkInjectionSkipped { + pod, sdkInjected = injectDotNetSDK(i.logger, otelinst.Spec.DotNet, pod, index) + if sdkInjected { pod = i.injectCommonEnvVar(otelinst, pod, index) pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } @@ -389,3 +389,17 @@ func moveEnvToListEnd(envs []corev1.EnvVar, idx int) []corev1.EnvVar { return envs } + +func validateContainerEnv(envs []corev1.EnvVar, envsToBeValidated ...string) error { + for _, envToBeValidated := range envsToBeValidated { + for _, containerEnv := range envs { + if containerEnv.Name == envToBeValidated { + if containerEnv.ValueFrom != nil { + return fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", containerEnv.Name) + } + break + } + } + } + return nil +} From 2a6569c3b168e9f80dda10916d96579bcf4993d5 Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Sat, 8 Oct 2022 12:57:17 +0530 Subject: [PATCH 04/11] fixes go lint issues --- pkg/instrumentation/dotnet.go | 26 +++++++++++++------------- pkg/instrumentation/javaagent.go | 8 ++++---- pkg/instrumentation/nodejs.go | 4 ++-- pkg/instrumentation/python.go | 6 +++--- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index 8c684b7381..0323831036 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -42,17 +42,17 @@ const ( 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 + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables + // validate container environment variables. err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore) if err != nil { logger.Info("Skipping DotNet SDK injection", "reason:", err.Error(), "container Name", container.Name) return pod, false } - // inject .Net instrumentation spec env vars + // inject .Net instrumentation spec env vars. for _, env := range dotNetSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -65,26 +65,26 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1. concatEnvValues = true ) - setDotNetEnvVar(logger, container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) + setDotNetEnvVar(container, envDotNetCoreClrEnableProfiling, dotNetCoreClrEnableProfilingEnabled, doNotConcatEnvValues) - setDotNetEnvVar(logger, container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) + setDotNetEnvVar(container, envDotNetCoreClrProfiler, dotNetCoreClrProfilerId, doNotConcatEnvValues) - setDotNetEnvVar(logger, container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) + setDotNetEnvVar(container, envDotNetCoreClrProfilerPath, dotNetCoreClrProfilerPath, doNotConcatEnvValues) - setDotNetEnvVar(logger, container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) + setDotNetEnvVar(container, envDotNetStartupHook, dotNetStartupHookPath, concatEnvValues) - setDotNetEnvVar(logger, container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) + setDotNetEnvVar(container, envDotNetAdditionalDeps, dotNetAdditionalDepsPath, concatEnvValues) - setDotNetEnvVar(logger, container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) + setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) - setDotNetEnvVar(logger, container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) + setDotNetEnvVar(container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ Name: volumeName, MountPath: "/otel-auto-instrumentation", }) - // We just inject Volumes and init containers for the first processed container + // We just inject Volumes and init containers for the first processed container. if isInitContainerMissing(pod) { pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ Name: volumeName, @@ -105,8 +105,8 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1. return pod, true } -// set env var to the container -func setDotNetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) { +// set env var to the container. +func setDotNetEnvVar(container *corev1.Container, envVarName string, envVarValue string, concatValues bool) { idx := getIndexOfEnv(container.Env, envVarName) if idx < 0 { container.Env = append(container.Env, corev1.EnvVar{ diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index 03e092d9b8..987fb8cdcd 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -27,17 +27,17 @@ const ( ) 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 + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables + // validate container environment variables. err := validateContainerEnv(container.Env, envJavaToolsOptions) if err != nil { logger.Info("Skipping javaagent injection", "reason:", err.Error(), "container Name", container.Name) return pod, false } - // inject Java instrumentation spec env vars + // inject Java instrumentation spec env vars. for _, env := range javaSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -60,7 +60,7 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, MountPath: "/otel-auto-instrumentation", }) - // We just inject Volumes and init containers for the first processed container + // We just inject Volumes and init containers for the first processed container. if isInitContainerMissing(pod) { pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ Name: volumeName, diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index d413d2a62b..5a8596641a 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -27,7 +27,7 @@ const ( ) 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 + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] // validate container environment variables @@ -37,7 +37,7 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. return pod, false } - // inject NodeJS instrumentation spec env vars + // inject NodeJS instrumentation spec env vars. for _, env := range nodeJSSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index c045240773..ed6dc3a1ad 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -31,7 +31,7 @@ const ( ) 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 + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] // validate container environment variables @@ -41,7 +41,7 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. return pod, false } - // inject Python instrumentation spec env vars + // inject Python instrumentation spec env vars. for _, env := range pythonSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -73,7 +73,7 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. MountPath: "/otel-auto-instrumentation", }) - // We just inject Volumes and init containers for the first processed container + // We just inject Volumes and init containers for the first processed container. if isInitContainerMissing(pod) { pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ Name: volumeName, From 0b2fce741281c94fd071da7ccee2b76adf895a0f Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Mon, 10 Oct 2022 19:55:58 +0530 Subject: [PATCH 05/11] incorporates review comments --- pkg/instrumentation/dotnet.go | 15 ++++--- pkg/instrumentation/dotnet_test.go | 21 +++++----- pkg/instrumentation/javaagent.go | 9 ++-- pkg/instrumentation/javaagent_test.go | 18 ++++---- pkg/instrumentation/nodejs.go | 9 ++-- pkg/instrumentation/nodejs_test.go | 18 ++++---- pkg/instrumentation/python.go | 12 ++---- pkg/instrumentation/python_test.go | 19 ++++----- pkg/instrumentation/sdk.go | 59 +++++++++++++++------------ 9 files changed, 87 insertions(+), 93 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index 0323831036..47263bb91a 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -17,7 +17,6 @@ package instrumentation import ( "fmt" - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -40,19 +39,17 @@ 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, bool) { +func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, error) { // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables. err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore) if err != nil { - logger.Info("Skipping DotNet SDK injection", "reason:", err.Error(), "container Name", container.Name) - return pod, false + return pod, err } - // inject .Net instrumentation spec env vars. + // inject .NET instrumentation spec env vars. for _, env := range dotNetSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) if idx == -1 { @@ -102,10 +99,12 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1. }}, }) } - return pod, true + return pod, nil } -// set env var to the container. +// 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. func setDotNetEnvVar(container *corev1.Container, envVarName string, envVarValue string, concatValues bool) { idx := getIndexOfEnv(container.Env, envVarName) if idx < 0 { diff --git a/pkg/instrumentation/dotnet_test.go b/pkg/instrumentation/dotnet_test.go index 6ea91dc0c6..aa0978c985 100644 --- a/pkg/instrumentation/dotnet_test.go +++ b/pkg/instrumentation/dotnet_test.go @@ -18,7 +18,6 @@ import ( "fmt" "testing" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -29,9 +28,9 @@ func TestInjectDotNetSDK(t *testing.T) { tests := []struct { name string v1alpha1.DotNet - pod corev1.Pod - expected corev1.Pod - sdkInjected bool + pod corev1.Pod + expected corev1.Pod + err error }{ { name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_SHARED_STORE, DOTNET_ADDITIONAL_DEPS, OTEL_DOTNET_AUTO_HOME not defined", @@ -106,7 +105,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE, OTEL_DOTNET_AUTO_HOME defined", @@ -212,7 +211,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "DOTNET_STARTUP_HOOKS defined as ValueFrom", @@ -245,7 +244,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetStartupHook), }, { name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom", @@ -278,7 +277,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetAdditionalDeps), }, { name: "DOTNET_SHARED_STORE defined as ValueFrom", @@ -311,15 +310,15 @@ func TestInjectDotNetSDK(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetSharedStore), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjected := injectDotNetSDK(logr.Discard(), test.DotNet, test.pod, 0) + pod, err := injectDotNetSDK(test.DotNet, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjected, sdkInjected) + assert.Equal(t, test.err, err) }) } } diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index 987fb8cdcd..02688ac0d9 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -15,7 +15,6 @@ package instrumentation import ( - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -26,15 +25,13 @@ const ( javaJVMArgument = " -javaagent:/otel-auto-instrumentation/javaagent.jar" ) -func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, bool) { +func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, error) { // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables. err := validateContainerEnv(container.Env, envJavaToolsOptions) if err != nil { - logger.Info("Skipping javaagent injection", "reason:", err.Error(), "container Name", container.Name) - return pod, false + return pod, err } // inject Java instrumentation spec env vars. @@ -78,5 +75,5 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, }}, }) } - return pod, true + return pod, err } diff --git a/pkg/instrumentation/javaagent_test.go b/pkg/instrumentation/javaagent_test.go index df9ca5069f..ab3a6f885d 100644 --- a/pkg/instrumentation/javaagent_test.go +++ b/pkg/instrumentation/javaagent_test.go @@ -15,9 +15,9 @@ package instrumentation import ( + "fmt" "testing" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -28,9 +28,9 @@ func TestInjectJavaagent(t *testing.T) { tests := []struct { name string v1alpha1.Java - pod corev1.Pod - expected corev1.Pod - sdkInjected bool + pod corev1.Pod + expected corev1.Pod + err error }{ { name: "JAVA_TOOL_OPTIONS not defined", @@ -81,7 +81,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "JAVA_TOOL_OPTIONS defined", @@ -139,7 +139,7 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "JAVA_TOOL_OPTIONS defined as ValueFrom", @@ -172,15 +172,15 @@ func TestInjectJavaagent(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envJavaToolsOptions), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjected := injectJavaagent(logr.Discard(), test.Java, test.pod, 0) + pod, err := injectJavaagent(test.Java, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjected, sdkInjected) + assert.Equal(t, test.err, err) }) } } diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index 5a8596641a..d1a4e180e0 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -15,7 +15,6 @@ package instrumentation import ( - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -26,15 +25,13 @@ const ( nodeRequireArgument = " --require /otel-auto-instrumentation/autoinstrumentation.js" ) -func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (corev1.Pod, bool) { +func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (corev1.Pod, error) { // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables err := validateContainerEnv(container.Env, envNodeOptions) if err != nil { - logger.Info("Skipping NodeJS SDK injection", "reason:", err.Error(), "container Name", container.Name) - return pod, false + return pod, err } // inject NodeJS instrumentation spec env vars. @@ -78,5 +75,5 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1. }}, }) } - return pod, true + return pod, nil } diff --git a/pkg/instrumentation/nodejs_test.go b/pkg/instrumentation/nodejs_test.go index 2f9c0e1c6b..3054f029a9 100644 --- a/pkg/instrumentation/nodejs_test.go +++ b/pkg/instrumentation/nodejs_test.go @@ -15,9 +15,9 @@ package instrumentation import ( + "fmt" "testing" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -28,9 +28,9 @@ func TestInjectNodeJSSDK(t *testing.T) { tests := []struct { name string v1alpha1.NodeJS - pod corev1.Pod - expected corev1.Pod - sdkInjected bool + pod corev1.Pod + expected corev1.Pod + err error }{ { name: "NODE_OPTIONS not defined", @@ -81,7 +81,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "NODE_OPTIONS defined", @@ -139,7 +139,7 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "NODE_OPTIONS defined as ValueFrom", @@ -172,15 +172,15 @@ func TestInjectNodeJSSDK(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envNodeOptions), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjected := injectNodeJSSDK(logr.Discard(), test.NodeJS, test.pod, 0) + pod, err := injectNodeJSSDK(test.NodeJS, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjected, sdkInjected) + assert.Equal(t, test.err, err) }) } } diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index ed6dc3a1ad..e142a05b35 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -17,10 +17,8 @@ package instrumentation import ( "fmt" - "github.com/go-logr/logr" - corev1 "k8s.io/api/core/v1" - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + corev1 "k8s.io/api/core/v1" ) const ( @@ -30,15 +28,13 @@ const ( pythonPathSuffix = "/otel-auto-instrumentation" ) -func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.Pod, index int) (corev1.Pod, bool) { +func injectPythonSDK(pythonSpec v1alpha1.Python, pod corev1.Pod, index int) (corev1.Pod, error) { // caller checks if there is at least one container. container := &pod.Spec.Containers[index] - // validate container environment variables err := validateContainerEnv(container.Env, envPythonPath) if err != nil { - logger.Info("Skipping Python SDK injection", "reason:", err.Error(), "container Name", container.Name) - return pod, false + return pod, err } // inject Python instrumentation spec env vars. @@ -91,5 +87,5 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1. }}, }) } - return pod, true + return pod, nil } diff --git a/pkg/instrumentation/python_test.go b/pkg/instrumentation/python_test.go index 8a0b41519d..836a912f11 100644 --- a/pkg/instrumentation/python_test.go +++ b/pkg/instrumentation/python_test.go @@ -18,7 +18,6 @@ import ( "fmt" "testing" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -29,9 +28,9 @@ func TestInjectPythonSDK(t *testing.T) { tests := []struct { name string v1alpha1.Python - pod corev1.Pod - expected corev1.Pod - sdkInjected bool + pod corev1.Pod + expected corev1.Pod + err error }{ { name: "PYTHONPATH not defined", @@ -86,7 +85,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "PYTHONPATH defined", @@ -148,7 +147,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "OTEL_TRACES_EXPORTER defined", @@ -210,7 +209,7 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjected: true, + err: nil, }, { name: "PYTHONPATH defined as ValueFrom", @@ -243,15 +242,15 @@ func TestInjectPythonSDK(t *testing.T) { }, }, }, - sdkInjected: false, + err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envPythonPath), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - pod, sdkInjected := injectPythonSDK(logr.Discard(), test.Python, test.pod, 0) + pod, err := injectPythonSDK(test.Python, test.pod, 0) assert.Equal(t, test.expected, pod) - assert.Equal(t, test.sdkInjected, sdkInjected) + assert.Equal(t, test.err, err) }) } } diff --git a/pkg/instrumentation/sdk.go b/pkg/instrumentation/sdk.go index 1403b67081..e6493fc2b8 100644 --- a/pkg/instrumentation/sdk.go +++ b/pkg/instrumentation/sdk.go @@ -64,47 +64,54 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations } } - // inject only to the first container for now - // in the future we can define an annotation to configure this if insts.Java != nil { otelinst := *insts.Java - sdkInjected := false - i.logger.V(1).Info("injecting java instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjected = injectJavaagent(i.logger, otelinst.Spec.Java, pod, index) - if sdkInjected { - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + var err error + i.logger.V(1).Info("injecting Java instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) + pod, err = injectJavaagent(otelinst.Spec.Java, pod, index) + if err != nil { + i.logger.Info("Skipping javaagent injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) + return pod } + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.NodeJS != nil { otelinst := *insts.NodeJS - sdkInjected := false - i.logger.V(1).Info("injecting nodejs instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjected = injectNodeJSSDK(i.logger, otelinst.Spec.NodeJS, pod, index) - if sdkInjected { - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + var err error + i.logger.V(1).Info("injecting NodeJS instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) + pod, err = injectNodeJSSDK(otelinst.Spec.NodeJS, pod, index) + if err != nil { + i.logger.Info("Skipping NodeJS SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) + return pod } + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.Python != nil { otelinst := *insts.Python - sdkInjected := false - i.logger.V(1).Info("injecting python instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjected = injectPythonSDK(i.logger, otelinst.Spec.Python, pod, index) - if sdkInjected { - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + var err error + i.logger.V(1).Info("injecting Python instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) + pod, err = injectPythonSDK(otelinst.Spec.Python, pod, index) + if err != nil { + i.logger.Info("Skipping Python SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) + return pod } + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.DotNet != nil { otelinst := *insts.DotNet - sdkInjected := false - i.logger.V(1).Info("injecting dotnet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) - pod, sdkInjected = injectDotNetSDK(i.logger, otelinst.Spec.DotNet, pod, index) - if sdkInjected { - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + var err error + i.logger.V(1).Info("injecting Dotnet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) + pod, err = injectDotNetSDK(otelinst.Spec.DotNet, pod, index) + if err != nil { + i.logger.Info("Skipping DotNet SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) + return pod } + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) + } if insts.Sdk != nil { otelinst := *insts.Sdk From d703287d5c9b82d294bab7efd08ea3703438f06a Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Mon, 10 Oct 2022 20:34:31 +0530 Subject: [PATCH 06/11] fixes go lint issue --- pkg/instrumentation/python.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index e142a05b35..4cf07f5230 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -17,8 +17,9 @@ package instrumentation import ( "fmt" - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" corev1 "k8s.io/api/core/v1" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" ) const ( From 2710133930cc441ef154bfec9c36ca94dc38ecfd Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Wed, 12 Oct 2022 15:02:13 +0530 Subject: [PATCH 07/11] removes return statement in case of failed instrumentation --- pkg/instrumentation/sdk.go | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/pkg/instrumentation/sdk.go b/pkg/instrumentation/sdk.go index e6493fc2b8..b126427d75 100644 --- a/pkg/instrumentation/sdk.go +++ b/pkg/instrumentation/sdk.go @@ -71,10 +71,10 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations pod, err = injectJavaagent(otelinst.Spec.Java, pod, index) if err != nil { i.logger.Info("Skipping javaagent injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) - return pod + } else { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.NodeJS != nil { otelinst := *insts.NodeJS @@ -83,10 +83,10 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations pod, err = injectNodeJSSDK(otelinst.Spec.NodeJS, pod, index) if err != nil { i.logger.Info("Skipping NodeJS SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) - return pod + } else { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.Python != nil { otelinst := *insts.Python @@ -95,23 +95,22 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations pod, err = injectPythonSDK(otelinst.Spec.Python, pod, index) if err != nil { i.logger.Info("Skipping Python SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) - return pod + } else { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } if insts.DotNet != nil { otelinst := *insts.DotNet var err error - i.logger.V(1).Info("injecting Dotnet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) + i.logger.V(1).Info("injecting DotNet instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name) pod, err = injectDotNetSDK(otelinst.Spec.DotNet, pod, index) if err != nil { i.logger.Info("Skipping DotNet SDK injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name) - return pod + } else { + pod = i.injectCommonEnvVar(otelinst, pod, index) + pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } - pod = i.injectCommonEnvVar(otelinst, pod, index) - pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) - } if insts.Sdk != nil { otelinst := *insts.Sdk @@ -119,7 +118,6 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations pod = i.injectCommonEnvVar(otelinst, pod, index) pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index) } - return pod } From 3faad7c01d277879a53a5018aa324e8d0d5dfb59 Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Sat, 15 Oct 2022 11:00:22 +0530 Subject: [PATCH 08/11] skips auto-instrumentation if OTEL_DOTNET_AUTO_HOME is already set --- pkg/instrumentation/dotnet.go | 12 ++++++ pkg/instrumentation/dotnet_test.go | 60 +++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index 47263bb91a..f28ac56369 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -49,6 +49,18 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (cor return pod, err } + // check if OTEL_DOTNET_AUTO_HOME env var is already set in the container + // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container + if getIndexOfEnv(container.Env, envDotNetOTelAutoHome) > -1 { + return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container") + } + + // check if OTEL_DOTNET_AUTO_HOME env var is already set in the .NET instrumentatiom spec + // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container + if getIndexOfEnv(dotNetSpec.Env, envDotNetOTelAutoHome) > -1 { + return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec") + } + // inject .NET instrumentation spec env vars. for _, env := range dotNetSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) diff --git a/pkg/instrumentation/dotnet_test.go b/pkg/instrumentation/dotnet_test.go index aa0978c985..eab7fb85c2 100644 --- a/pkg/instrumentation/dotnet_test.go +++ b/pkg/instrumentation/dotnet_test.go @@ -108,7 +108,7 @@ func TestInjectDotNetSDK(t *testing.T) { err: nil, }, { - name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE, OTEL_DOTNET_AUTO_HOME defined", + name: "CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH, DOTNET_STARTUP_HOOKS, DOTNET_ADDITIONAL_DEPS, DOTNET_SHARED_STORE defined", DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, pod: corev1.Pod{ Spec: corev1.PodSpec{ @@ -139,10 +139,6 @@ func TestInjectDotNetSDK(t *testing.T) { Name: envDotNetSharedStore, Value: "/foo:/bar", }, - { - Name: envDotNetOTelAutoHome, - Value: "/foo:/bar", - }, }, }, }, @@ -204,7 +200,7 @@ func TestInjectDotNetSDK(t *testing.T) { }, { Name: envDotNetOTelAutoHome, - Value: "/foo:/bar", + Value: dotNetOTelAutoHomePath, }, }, }, @@ -312,6 +308,58 @@ func TestInjectDotNetSDK(t *testing.T) { }, err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetSharedStore), }, + { + name: "OTEL_DOTNET_AUTO_HOME already set in the container", + DotNet: v1alpha1.DotNet{Image: "foo/bar:1"}, + pod: corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Env: []corev1.EnvVar{ + { + Name: envDotNetOTelAutoHome, + Value: "/otel-dotnet-auto", + }, + }, + }, + }, + }, + }, + expected: corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Env: []corev1.EnvVar{ + { + Name: envDotNetOTelAutoHome, + Value: "/otel-dotnet-auto", + }, + }, + }, + }, + }, + }, + err: fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container"), + }, + { + name: "OTEL_DOTNET_AUTO_HOME already set in the .NET instrumentation spec", + DotNet: v1alpha1.DotNet{Image: "foo/bar:1", Env: []corev1.EnvVar{{Name: envDotNetOTelAutoHome, Value: dotNetOTelAutoHomePath}}}, + pod: corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {}, + }, + }, + }, + expected: corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {}, + }, + }, + }, + err: fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec"), + }, } for _, test := range tests { From 306e1be1989f1cc4f0ef89a06c4c130242fa35f6 Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Mon, 17 Oct 2022 13:49:08 +0530 Subject: [PATCH 09/11] use errors.New() to get the error object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit use errors.New() to get the error object instead of fmt.Errorf() Co-authored-by: Robert Pająk --- pkg/instrumentation/dotnet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index f28ac56369..9d1c47b3e1 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -52,7 +52,7 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (cor // check if OTEL_DOTNET_AUTO_HOME env var is already set in the container // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container if getIndexOfEnv(container.Env, envDotNetOTelAutoHome) > -1 { - return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container") + return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container") } // check if OTEL_DOTNET_AUTO_HOME env var is already set in the .NET instrumentatiom spec From e405425f55b70e1480a3b831a96aa1d8ecb3ef6e Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Mon, 17 Oct 2022 13:49:59 +0530 Subject: [PATCH 10/11] use errors.New() to get the error object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit use errors.New() to get the error object instead of fmt.Errorf() Co-authored-by: Robert Pająk --- pkg/instrumentation/dotnet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index 9d1c47b3e1..6c3c41ee18 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -58,7 +58,7 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (cor // check if OTEL_DOTNET_AUTO_HOME env var is already set in the .NET instrumentatiom spec // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container if getIndexOfEnv(dotNetSpec.Env, envDotNetOTelAutoHome) > -1 { - return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec") + return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec") } // inject .NET instrumentation spec env vars. From ee33528659021c18d59725924826480c3535e264 Mon Sep 17 00:00:00 2001 From: Avadhut Pisal Date: Mon, 17 Oct 2022 13:55:19 +0530 Subject: [PATCH 11/11] replaces fmt.Errorf with error.New --- pkg/instrumentation/dotnet.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index f28ac56369..c86614a47a 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -15,6 +15,7 @@ package instrumentation import ( + "errors" "fmt" corev1 "k8s.io/api/core/v1" @@ -52,13 +53,13 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (cor // check if OTEL_DOTNET_AUTO_HOME env var is already set in the container // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container if getIndexOfEnv(container.Env, envDotNetOTelAutoHome) > -1 { - return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container") + return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the container") } // check if OTEL_DOTNET_AUTO_HOME env var is already set in the .NET instrumentatiom spec // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container if getIndexOfEnv(dotNetSpec.Env, envDotNetOTelAutoHome) > -1 { - return pod, fmt.Errorf("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec") + return pod, errors.New("OTEL_DOTNET_AUTO_HOME environment variable is already set in the .NET instrumentation spec") } // inject .NET instrumentation spec env vars.