Skip to content

Commit

Permalink
Validate all env. vars. before starting injecting env. vars (#1141)
Browse files Browse the repository at this point in the history 
* skips env var injection and sdk configurations if agent injection is skipped

* mutate container at the last of SDK injection step

* validate first and then mutate the container with env variables

* fixes go lint issues

* incorporates review comments

* fixes go lint issue

* removes return statement in case of failed instrumentation
  • Loading branch information
@avadhut123pisal
avadhut123pisal authored Oct 12, 2022
1 parent 6d4a9a1 commit 992b681
Show file tree
Hide file tree
Showing 9 changed files with 132 additions and 237 deletions.
65 changes: 24 additions & 41 deletions pkg/instrumentation/dotnet.go
View file
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)

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) {
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
View file
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
View file
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

0 comments on commit 992b681

Please sign in to comment.