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

Fix Pod Mutation loop #953

Merged
merged 10 commits into from
Jul 4, 2022
+81 −5
Merged

Fix Pod Mutation loop #953

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c7974da
chore(webhookhandler): check if pod already instrumented
mat-rumian Jun 27, 2022
cfb8b81
chore(instrumentations): add check for volume and instrumentation env…
mat-rumian Jun 27, 2022
5acbe82
Merge branch 'open-telemetry:main' into fix-pod-mutation-loop
mat-rumian Jun 27, 2022
7f3989c
chore(webhookhandler): add helper tests
mat-rumian Jun 28, 2022
5dbafa7
chore(instrumentation): add helper tests
mat-rumian Jun 28, 2022
b906f7d
chore(instrumentation): update volume checking method
mat-rumian Jun 28, 2022
b5f1cc1
chore(lint): fix
mat-rumian Jun 28, 2022
99346db
chore(webhookhandler): move out pod instrumentation check
mat-rumian Jun 30, 2022
1fdb631
chore(instrumentation): add pod instrumentation inject check
mat-rumian Jun 30, 2022
08e0c1d
chore(instrumentation): clean up
mat-rumian Jul 4, 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
34 changes: 33 additions & 1 deletion pkg/instrumentation/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@

package instrumentation

import corev1 "k8s.io/api/core/v1"
import (
"strings"

corev1 "k8s.io/api/core/v1"
)

// Calculate if we already inject InitContainers.
func IsInitContainerMissing(pod corev1.Pod) bool {
Expand All @@ -25,3 +29,31 @@ func IsInitContainerMissing(pod corev1.Pod) bool {
}
return true
}

// Check if opentelemetry-auto-instrumentation volume is on the list.
func IsOtAIVolumeMissing(volumeMounts []corev1.VolumeMount) bool {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
for _, volumeMount := range volumeMounts {
if volumeMount.Name == volumeName {
return false
}
}
return true
}

// Check if EnvVar value contains instrumentation string.
func IsEnvVarValueInstrumentationMissing(envVar corev1.EnvVar, instrumentation string) bool {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
if strings.Contains(envVar.Value, instrumentation) {
return false
}
return true
}

// Checks if Pod is already instrumented by checking Instrumentation InitContainer presence.
func IsAutoInstrumentationInjected(pod corev1.Pod) bool {
for _, cont := range pod.Spec.InitContainers {
if cont.Name == initContainerName {
return true
}
}
return false
}
131 changes: 131 additions & 0 deletions pkg/instrumentation/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,134 @@ func TestInitContainerMissing(t *testing.T) {
})
}
}

func TestOtAiVolumeMissing(t *testing.T) {
tests := []struct {
name string
volume []corev1.VolumeMount
expected bool
}{
{
name: "Volume_Already_Inject",
volume: []corev1.VolumeMount{
{
Name: volumeName,
},
},
expected: false,
},
{
name: "Volume_Absent_1",
volume: []corev1.VolumeMount{
{
Name: "magic-volume",
},
},
expected: true,
},
{
name: "Volume_Absent_2",
volume: []corev1.VolumeMount{},
expected: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsOtAIVolumeMissing(test.volume)
assert.Equal(t, test.expected, result)
})
}
}

func TestEnvVarInstrumentationValueMissing(t *testing.T) {
tests := []struct {
name string
envVar corev1.EnvVar
instrumentationStr string
expected bool
}{
{
name: "EnvVar_Instrumentation_Value_Already_Inject",
envVar: corev1.EnvVar{
Name: envJavaToolsOptions,
Value: javaJVMArgument,
},
instrumentationStr: javaJVMArgument,
expected: false,
},
{
name: "EnvVar_Instrumentation_Value_Absent_1",
envVar: corev1.EnvVar{

Name: envNodeOptions,
Value: "some-magic-node-options",
},
instrumentationStr: envNodeOptions,
expected: true,
}, {
name: "EnvVar_Instrumentation_Value_Absent_2",
envVar: corev1.EnvVar{},
instrumentationStr: envNodeOptions,
expected: true,
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsEnvVarValueInstrumentationMissing(test.envVar, test.instrumentationStr)
assert.Equal(t, test.expected, result)
})
}
}

func TestAutoInstrumentationInjected(t *testing.T) {
tests := []struct {
name string
pod corev1.Pod
expected bool
}{
{
name: "AutoInstrumentation_Already_Inject",
pod: corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: "magic-init",
},
{
Name: initContainerName,
},
},
},
},
expected: true,
},
{
name: "AutoInstrumentation_Absent_1",
pod: corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: "magic-init",
},
},
},
},
expected: false,
},
{
name: "AutoInstrumentation_Absent_2",
pod: corev1.Pod{
Spec: corev1.PodSpec{},
},
expected: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsAutoInstrumentationInjected(test.pod)
assert.Equal(t, test.expected, result)
})
}
}
16 changes: 11 additions & 5 deletions pkg/instrumentation/javaagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,18 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
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

if IsEnvVarValueInstrumentationMissing(container.Env[idx], javaJVMArgument) {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
}
}

if IsOtAIVolumeMissing(container.VolumeMounts) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check needed?

The IsAutoInstrumentationInjected is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So IsOtAIVolumeMissing and IsEnvVarValueInstrumentationMissing were applied before I've added check for pod mutation. When Pod was mutated few times k8s was throwing errors like duplicated volume name. In case of env var it happened that e.g. javaJVMArgument was added few times to JAVA_TOOL_OPTIONS. So I left this checks for safety for other unexpected mutations.

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 to remove this and IsEnvVarValueInstrumentationMissing as well and rely only on the init container for simplicity.

We should keep it here only if there is a known scenario which the init container does not cover

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
}
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
if IsInitContainerMissing(pod) {
Expand Down
16 changes: 11 additions & 5 deletions pkg/instrumentation/nodejs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,18 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.
logger.Info("Skipping NodeJS SDK injection, the container defines NODE_OPTIONS env var value via ValueFrom", "container", container.Name)
return pod
}
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument

if IsEnvVarValueInstrumentationMissing(container.Env[idx], nodeRequireArgument) {
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
}
}

if IsOtAIVolumeMissing(container.VolumeMounts) {
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
}
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
if IsInitContainerMissing(pod) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/instrumentation/podmutator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ func NewMutator(logger logr.Logger, client client.Client) *instPodMutator {
func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod corev1.Pod) (corev1.Pod, error) {
logger := pm.Logger.WithValues("namespace", pod.Namespace, "name", pod.Name)

// We check if Pod is already instrumented.
if IsAutoInstrumentationInjected(pod) {
logger.Info("Skipping pod instrumentation - already instrumented")
return pod, nil
}

var inst *v1alpha1.Instrumentation
var err error

Expand Down
15 changes: 10 additions & 5 deletions pkg/instrumentation/python.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
logger.Info("Skipping Python SDK injection, the container defines PYTHONPATH env var value via ValueFrom", "container", container.Name)
return pod
}
container.Env[idx].Value = fmt.Sprintf("%s:%s:%s", pythonPathPrefix, container.Env[idx].Value, pythonPathSuffix)

if IsEnvVarValueInstrumentationMissing(container.Env[idx], pythonPathPrefix) {
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.
Expand All @@ -66,10 +69,12 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
})
}

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
if IsOtAIVolumeMissing(container.VolumeMounts) {
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
if IsInitContainerMissing(pod) {
Expand Down