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 5 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
64 changes: 24 additions & 40 deletions pkg/instrumentation/dotnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,19 @@ 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(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, bool) {

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

// validate container environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that most of the comments are redundant.

IMO // caller checks if there is at least one container. it is valid comment, but putting information // validate container environment variables. in front of validateContainerEnv does not make sense.

Please check whole PR in this context.

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

err := validateContainerEnv(container.Env, envDotNetStartupHook, envDotNetAdditionalDeps, envDotNetSharedStore)
if err != nil {
logger.Info("Skipping DotNet SDK injection", "reason:", err.Error(), "container Name", container.Name)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is how logr should be used

Suggested change
logger.Info("Skipping DotNet SDK injection", "reason:", err.Error(), "container Name", container.Name)
logger.Info("Skipping DotNet SDK injection", "reason", err.Error(), "container", container.Name)

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

return pod, false
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we return error instead of bool. The caller is already doing some loggging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pellared I didn't get your point (The caller is already doing some logging).

Copy link
Member

Choose a reason for hiding this comment

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

See: https://github.com/avadhut123pisal/opentelemetry-operator/blob/2a6569c3b168e9f80dda10916d96579bcf4993d5/pkg/instrumentation/sdk.go#L102-L103

I also think that as a rule of thumb the function should either log or return an error. Returning false looks like returning an error without a description.

Copy link
Member

Choose a reason for hiding this comment

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

What is more, if injectDotNetSDK would not log, then the logger would not be need as an argument and the signature would become:

func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int) (corev1.Pod, error)

Copy link
Contributor Author

@avadhut123pisal avadhut123pisal 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.

If we return the error from injectDotNetSDK and not log in injectDotNetSDK, then on the caller side, https://github.com/avadhut123pisal/opentelemetry-operator/blob/2a6569c3b168e9f80dda10916d96579bcf4993d5/pkg/instrumentation/sdk.go#L103
should we use that err value to just handle the condition and to log the message like this ?

		pod, err = injectJavaagent(i.logger, 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
		}

Because if want to propagate the error further in the call stack then we need to modify the signature of inject function too.

Copy link
Member

Choose a reason for hiding this comment

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

That is correct

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

}

// inject .Net instrumentation spec env vars.
Copy link
Member

Choose a reason for hiding this comment

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

typo:

Suggested change
// inject .Net instrumentation spec env vars.
// inject .NET instrumentation spec env vars.

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

for _, env := range dotNetSpec.Env {
idx := getIndexOfEnv(container.Env, env.Name)
if idx == -1 {
Expand All @@ -57,40 +65,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 +102,20 @@ func injectDotNetSDK(logger logr.Logger, dotNetSpec v1alpha1.DotNet, pod corev1.
}},
})
}

pod.Spec.Containers[index] = container
return pod
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(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
}
141 changes: 10 additions & 131 deletions pkg/instrumentation/dotnet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ func TestInjectDotNetSDK(t *testing.T) {
tests := []struct {
name string
v1alpha1.DotNet
pod corev1.Pod
expected corev1.Pod
pod corev1.Pod
expected corev1.Pod
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",
Expand Down Expand Up @@ -105,6 +106,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
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",
Expand Down Expand Up @@ -210,102 +212,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{},
},
},
},
},
},
},
sdkInjected: true,
},
{
name: "DOTNET_STARTUP_HOOKS defined as ValueFrom",
Expand Down Expand Up @@ -338,6 +245,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjected: false,
},
{
name: "DOTNET_ADDITIONAL_DEPS defined as ValueFrom",
Expand Down Expand Up @@ -370,6 +278,7 @@ func TestInjectDotNetSDK(t *testing.T) {
},
},
},
sdkInjected: false,
},
{
name: "DOTNET_SHARED_STORE defined as ValueFrom",
Expand Down Expand Up @@ -402,45 +311,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{},
},
},
},
},
},
},
sdkInjected: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := 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.sdkInjected, sdkInjected)
})
}
}
25 changes: 12 additions & 13 deletions pkg/instrumentation/javaagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,18 @@ 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(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]

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

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

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pod := 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.sdkInjected, sdkInjected)
})
}
}
Loading