Fix Pod Mutation loop (open-telemetry#953)

* chore(webhookhandler): check if pod already instrumented * chore(instrumentations): add check for volume and instrumentation env var value * chore(webhookhandler): add helper tests * chore(instrumentation): add helper tests * chore(instrumentation): update volume checking method * chore(lint): fix * chore(webhookhandler): move out pod instrumentation check * chore(instrumentation): add pod instrumentation inject check * chore(instrumentation): clean up
ItielOlenick · Jul 4, 2022 · 31b8e80 · 31b8e80
1 parent 20a5536
commit 31b8e80
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 5 deletions.
diff --git a/pkg/instrumentation/helper.go b/pkg/instrumentation/helper.go
@@ -17,11 +17,21 @@ package instrumentation
 import corev1 "k8s.io/api/core/v1"
 
 // Calculate if we already inject InitContainers.
-func IsInitContainerMissing(pod corev1.Pod) bool {
+func isInitContainerMissing(pod corev1.Pod) bool {
 	for _, initContainer := range pod.Spec.InitContainers {
 		if initContainer.Name == initContainerName {
 			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
+}
diff --git a/pkg/instrumentation/helper_test.go b/pkg/instrumentation/helper_test.go
@@ -67,7 +67,59 @@ func TestInitContainerMissing(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			result := IsInitContainerMissing(test.pod)
+			result := isInitContainerMissing(test.pod)
+			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)
 		})
 	}

diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go
@@ -50,15 +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
+
 	}
+
 	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) {
+	if isInitContainerMissing(pod) {
 		pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
 			Name: volumeName,
 			VolumeSource: corev1.VolumeSource{

diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go
@@ -50,15 +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
+
 	}
+
 	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) {
+	if isInitContainerMissing(pod) {
 		pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
 			Name: volumeName,
 			VolumeSource: corev1.VolumeSource{

diff --git a/pkg/instrumentation/podmutator.go b/pkg/instrumentation/podmutator.go
@@ -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
 

diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go
@@ -54,7 +54,9 @@ 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)
+
 	}
 
 	// Set OTEL_TRACES_EXPORTER to HTTP exporter if not set by user because it is what our autoinstrumentation supports.
@@ -72,7 +74,7 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
 	})
 
 	// We just inject Volumes and init containers for the first processed container
-	if IsInitContainerMissing(pod) {
+	if isInitContainerMissing(pod) {
 		pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
 			Name: volumeName,
 			VolumeSource: corev1.VolumeSource{