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.

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

65 changes: 24 additions & 41 deletions pkg/instrumentation/dotnet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -40,11 +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 {
// caller checks if there is at least one container
container := pod.Spec.Containers[index]
func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, error) {

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

err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore)
if err != nil {
return pod, err
}

// inject .NET instrumentation spec env vars.
for _, env := range dotNetSpec.Env {
idx := getIndexOfEnv(container.Env, env.Name)
if idx == -1 {
Expand All @@ -57,40 +62,26 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
concatEnvValues = true
)

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

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

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

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

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

if !trySetEnvVar(logger, &container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues) {
return pod
}
setDotNetEnvVar(container, envDotNetOTelAutoHome, dotNetOTelAutoHomePath, doNotConcatEnvValues)
Copy link
Member

@pellared pellared Oct 10, 2022

Choose a reason for hiding this comment

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

I think that we should additionally validate that the dotNetOTelAutoHomePath env var was not set in the original container. Otherwise, we cannot auto-instrument the .NET app. If someone set it then it would mean that somebody has already set the .NET AutoInstrumentation in the container.

@Kielek do you agree? I think it would be better addressed in a separate issue/PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. We should address this one in separate PR specific to .Net.

Copy link
Member

Choose a reason for hiding this comment

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

I created #1156. Free free to resolve this comment.


if !trySetEnvVar(logger, &container, envDotNetSharedStore, dotNetSharedStorePath, concatEnvValues) {
return pod
}
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,
Expand All @@ -108,30 +99,22 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
}},
})
}

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

func trySetEnvVar(logger logr.Logger, container *corev1.Container, envVarName string, envVarValue string, concatValues bool) bool {
// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest describing what concatValues is supposed to offer.
AFAIK it should be set to true if the env var supports multiple values supported by :. If it is set to false, the original container's env var value has priority.

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

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
}
138 changes: 8 additions & 130 deletions pkg/instrumentation/dotnet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"fmt"
"testing"

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

Expand All @@ -31,6 +30,7 @@ func TestInjectDotNetSDK(t *testing.T) {
v1alpha1.DotNet
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",
Expand Down Expand Up @@ -105,6 +105,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",
Expand Down Expand Up @@ -210,102 +211,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
},
{
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{},
},
},
},
},
},
},
},
{
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{},
},
},
},
},
},
},
},
{
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{},
},
},
},
},
},
},
err: nil,
},
{
name: "DOTNET_STARTUP_HOOKS defined as ValueFrom",
Expand Down Expand Up @@ -338,6 +244,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetStartupHook),
},
{
name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom",
Expand Down Expand Up @@ -370,6 +277,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envDotNetAdditionalDeps),
},
{
name: "DOTNET_SHARED_STORE defined as ValueFrom",
Expand Down Expand Up @@ -402,45 +310,15 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
},
{
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{},
},
},
},
},
},
},
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 := 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.err, err)
})
}
}
24 changes: 10 additions & 14 deletions pkg/instrumentation/javaagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -26,11 +25,16 @@ const (
javaJVMArgument = " -javaagent:/otel-auto-instrumentation/javaagent.jar"
)

func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.Pod {
// caller checks if there is at least one container
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]

// inject env vars
err := validateContainerEnv(container.Env, envJavaToolsOptions)
if err != nil {
return pod, err
}

// inject Java instrumentation spec env vars.
for _, env := range javaSpec.Env {
idx := getIndexOfEnv(container.Env, env.Name)
if idx == -1 {
Expand All @@ -45,22 +49,15 @@ 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
}

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

}

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,
Expand All @@ -78,6 +75,5 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
}},
})
}

return pod
return pod, err
}
Loading