Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Oct 12, 2022
Merged

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

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 24 additions & 41 deletions pkg/instrumentation/dotnet.go
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
Open in desktop
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