Revert "Support configuring java runtime from configmap or secret (en…

…v.valueFrom)" (#3510) * Revert "Support configuring java runtime from configmap or secret (env.valueF…" This reverts commit 2b36f0d. * chlog (#3511)
open-telemetry · Dec 3, 2024 · 42a689e · 42a689e
1 parent bc34078
commit 42a689e
Show file tree

Hide file tree

Showing 12 changed files with 62 additions and 78 deletions.
diff --git a/.chloggen/revert-3379-otel-configmap.yaml b/.chloggen/revert-3379-otel-configmap.yaml
@@ -0,0 +1,19 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: auto-instrumentation
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Reverts PR 3379 which inadvertently broke users setting JAVA_TOOL_OPTIONS
+
+# One or more tracking issues related to the change
+issues: [3463]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  Reverts a previous PR which was causing JAVA_TOOL_OPTIONS to not be overriden when
+  set by users. This was resulting in application crashloopbackoffs for users relying
+  on java autoinstrumentation.
diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml
@@ -1,2 +1,3 @@
 resources:
 - manager.yaml
+
diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go
@@ -24,17 +24,23 @@ import (
 
 const (
 	envJavaToolsOptions   = "JAVA_TOOL_OPTIONS"
-	javaAgent             = "-javaagent:/otel-auto-instrumentation-java/javaagent.jar"
+	javaAgent             = " -javaagent:/otel-auto-instrumentation-java/javaagent.jar"
 	javaInitContainerName = initContainerName + "-java"
 	javaVolumeName        = volumeName + "-java"
 	javaInstrMountPath    = "/otel-auto-instrumentation-java"
 )
 
-func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.Pod {
+func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, error) {
 	volume := instrVolume(javaSpec.VolumeClaimTemplate, javaVolumeName, javaSpec.VolumeSizeLimit)
+
 	// caller checks if there is at least one container.
 	container := &pod.Spec.Containers[index]
 
+	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)
@@ -49,14 +55,14 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.P
 	}
 
 	idx := getIndexOfEnv(container.Env, envJavaToolsOptions)
-	if idx != -1 {
-		// https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/
-		javaJVMArgument = fmt.Sprintf("$(%s) %s", envJavaToolsOptions, javaJVMArgument)
+	if idx == -1 {
+		container.Env = append(container.Env, corev1.EnvVar{
+			Name:  envJavaToolsOptions,
+			Value: javaJVMArgument,
+		})
+	} else {
+		container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
 	}
-	container.Env = append(container.Env, corev1.EnvVar{
-		Name:  envJavaToolsOptions,
-		Value: javaJVMArgument,
-	})
 
 	container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
 		Name:      volume.Name,
@@ -91,5 +97,5 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) corev1.P
 		}
 
 	}
-	return pod
+	return pod, err
 }
diff --git a/pkg/instrumentation/javaagent_test.go b/pkg/instrumentation/javaagent_test.go
@@ -15,6 +15,7 @@
 package instrumentation
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -29,6 +30,7 @@ func TestInjectJavaagent(t *testing.T) {
 		v1alpha1.Java
 		pod      corev1.Pod
 		expected corev1.Pod
+		err      error
 	}{
 		{
 			name: "JAVA_TOOL_OPTIONS not defined",
@@ -81,6 +83,7 @@ func TestInjectJavaagent(t *testing.T) {
 					},
 				},
 			},
+			err: nil,
 		},
 		{
 			name: "add extensions to JAVA_TOOL_OPTIONS",
@@ -154,6 +157,7 @@ func TestInjectJavaagent(t *testing.T) {
 					},
 				},
 			},
+			err: nil,
 		},
 		{
 			name: "JAVA_TOOL_OPTIONS defined",
@@ -207,21 +211,18 @@ func TestInjectJavaagent(t *testing.T) {
 							Env: []corev1.EnvVar{
 								{
 									Name:  "JAVA_TOOL_OPTIONS",
-									Value: "-Dbaz=bar",
-								},
-								{
-									Name:  "JAVA_TOOL_OPTIONS",
-									Value: "$(JAVA_TOOL_OPTIONS) " + javaAgent,
+									Value: "-Dbaz=bar" + javaAgent,
 								},
 							},
 						},
 					},
 				},
 			},
+			err: nil,
 		},
 		{
 			name: "JAVA_TOOL_OPTIONS defined as ValueFrom",
-			Java: v1alpha1.Java{Image: "foo/bar:1", Resources: testResourceRequirements},
+			Java: v1alpha1.Java{Image: "foo/bar:1"},
 			pod: corev1.Pod{
 				Spec: corev1.PodSpec{
 					Containers: []corev1.Container{
@@ -238,57 +239,27 @@ func TestInjectJavaagent(t *testing.T) {
 			},
 			expected: corev1.Pod{
 				Spec: corev1.PodSpec{
-					Volumes: []corev1.Volume{
-						{
-							Name: "opentelemetry-auto-instrumentation-java",
-							VolumeSource: corev1.VolumeSource{
-								EmptyDir: &corev1.EmptyDirVolumeSource{
-									SizeLimit: &defaultVolumeLimitSize,
-								},
-							},
-						},
-					},
-					InitContainers: []corev1.Container{
-						{
-							Name:    "opentelemetry-auto-instrumentation-java",
-							Image:   "foo/bar:1",
-							Command: []string{"cp", "/javaagent.jar", "/otel-auto-instrumentation-java/javaagent.jar"},
-							VolumeMounts: []corev1.VolumeMount{{
-								Name:      "opentelemetry-auto-instrumentation-java",
-								MountPath: "/otel-auto-instrumentation-java",
-							}},
-							Resources: testResourceRequirements,
-						},
-					},
 					Containers: []corev1.Container{
 						{
-							VolumeMounts: []corev1.VolumeMount{
-								{
-									Name:      "opentelemetry-auto-instrumentation-java",
-									MountPath: "/otel-auto-instrumentation-java",
-								},
-							},
 							Env: []corev1.EnvVar{
 								{
 									Name:      "JAVA_TOOL_OPTIONS",
 									ValueFrom: &corev1.EnvVarSource{},
 								},
-								{
-									Name:  "JAVA_TOOL_OPTIONS",
-									Value: "$(JAVA_TOOL_OPTIONS) " + javaAgent,
-								},
 							},
 						},
 					},
 				},
 			},
+			err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envJavaToolsOptions),
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pod := injectJavaagent(test.Java, test.pod, 0)
+			pod, err := injectJavaagent(test.Java, test.pod, 0)
 			assert.Equal(t, test.expected, pod)
+			assert.Equal(t, test.err, err)
 		})
 	}
 }
diff --git a/pkg/instrumentation/sdk.go b/pkg/instrumentation/sdk.go
@@ -59,6 +59,7 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations
 	}
 	if insts.Java.Instrumentation != nil {
 		otelinst := *insts.Java.Instrumentation
+		var err error
 		i.logger.V(1).Info("injecting Java instrumentation into pod", "otelinst-namespace", otelinst.Namespace, "otelinst-name", otelinst.Name)
 
 		if len(insts.Java.Containers) == 0 {
@@ -67,10 +68,14 @@ func (i *sdkInjector) inject(ctx context.Context, insts languageInstrumentations
 
 		for _, container := range insts.Java.Containers {
 			index := getContainerIndex(container, pod)
-			pod = injectJavaagent(otelinst.Spec.Java, pod, index)
-			pod = i.injectCommonEnvVar(otelinst, pod, index)
-			pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index, index)
-			pod = i.setInitContainerSecurityContext(pod, pod.Spec.Containers[index].SecurityContext, javaInitContainerName)
+			pod, err = injectJavaagent(otelinst.Spec.Java, pod, index)
+			if err != nil {
+				i.logger.Info("Skipping javaagent injection", "reason", err.Error(), "container", pod.Spec.Containers[index].Name)
+			} else {
+				pod = i.injectCommonEnvVar(otelinst, pod, index)
+				pod = i.injectCommonSDKConfig(ctx, otelinst, ns, pod, index, index)
+				pod = i.setInitContainerSecurityContext(pod, pod.Spec.Containers[index].SecurityContext, javaInitContainerName)
+			}
 		}
 	}
 	if insts.NodeJS.Instrumentation != nil {

diff --git a/tests/e2e-instrumentation/instrumentation-java-multicontainer/01-assert.yaml b/tests/e2e-instrumentation/instrumentation-java-multicontainer/01-assert.yaml
@@ -25,7 +25,7 @@ spec:
     - name: SPLUNK_PROFILER_ENABLED
       value: "false"
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_EXPORTER
       value: otlp
     - name: OTEL_EXPORTER_OTLP_ENDPOINT
@@ -75,7 +75,7 @@ spec:
     - name: SPLUNK_PROFILER_ENABLED
       value: "false"
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_EXPORTER
       value: otlp
     - name: OTEL_EXPORTER_OTLP_ENDPOINT

diff --git a/tests/e2e-instrumentation/instrumentation-java-multicontainer/02-assert.yaml b/tests/e2e-instrumentation/instrumentation-java-multicontainer/02-assert.yaml
@@ -36,7 +36,7 @@ spec:
     - name: SPLUNK_PROFILER_ENABLED
       value: "false"
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_EXPORTER
       value: otlp
     - name: OTEL_EXPORTER_OTLP_ENDPOINT

diff --git a/tests/e2e-instrumentation/instrumentation-java-other-ns/03-assert.yaml b/tests/e2e-instrumentation/instrumentation-java-other-ns/03-assert.yaml
@@ -24,7 +24,7 @@ spec:
     - name: SPLUNK_PROFILER_ENABLED
       value: "false"
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_EXPORTER
       value: otlp
     - name: OTEL_EXPORTER_OTLP_ENDPOINT

diff --git a/tests/e2e-instrumentation/instrumentation-java-tls/01-assert.yaml b/tests/e2e-instrumentation/instrumentation-java-tls/01-assert.yaml
@@ -17,7 +17,7 @@ spec:
         fieldRef:
           fieldPath: status.podIP
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_SERVICE_NAME
       value: my-java
     - name: OTEL_EXPORTER_OTLP_ENDPOINT

diff --git a/tests/e2e-instrumentation/instrumentation-java/01-assert.yaml b/tests/e2e-instrumentation/instrumentation-java/01-assert.yaml
@@ -17,19 +17,14 @@ spec:
       valueFrom:
         fieldRef:
           fieldPath: status.podIP
-    - name: JAVA_TOOL_OPTIONS
-      valueFrom:
-        configMapKeyRef:
-          name: config-java
-          key: system-properties
     - name: OTEL_JAVAAGENT_DEBUG
       value: "true"
     - name: OTEL_INSTRUMENTATION_JDBC_ENABLED
       value: "false"
     - name: SPLUNK_PROFILER_ENABLED
       value: "false"
     - name: JAVA_TOOL_OPTIONS
-      value: '$(JAVA_TOOL_OPTIONS) -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_EXPORTER
       value: otlp
     - name: OTEL_EXPORTER_OTLP_ENDPOINT

diff --git a/tests/e2e-instrumentation/instrumentation-java/01-install-app.yaml b/tests/e2e-instrumentation/instrumentation-java/01-install-app.yaml
@@ -1,10 +1,3 @@
-apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: config-java
-data:
-  system-properties: "-Xmx256m -Xms64m"
----
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -29,12 +22,6 @@ spec:
       containers:
       - name: myapp
         image: ghcr.io/open-telemetry/opentelemetry-operator/e2e-test-app-java:main
-        env:
-        - name: JAVA_TOOL_OPTIONS
-          valueFrom:
-            configMapKeyRef:
-              name: config-java
-              key: system-properties
         securityContext:
           allowPrivilegeEscalation: false
           capabilities:

diff --git a/tests/e2e-multi-instrumentation/instrumentation-multi-multicontainer/01-assert.yaml b/tests/e2e-multi-instrumentation/instrumentation-multi-multicontainer/01-assert.yaml
@@ -89,7 +89,7 @@ spec:
     - name: OTEL_SERVICE_NAME
       value: javaapp
     - name: JAVA_TOOL_OPTIONS
-      value: '-javaagent:/otel-auto-instrumentation-java/javaagent.jar'
+      value: ' -javaagent:/otel-auto-instrumentation-java/javaagent.jar'
     - name: OTEL_TRACES_SAMPLER
       value: parentbased_traceidratio
     - name: OTEL_TRACES_SAMPLER_ARG