From 42a689e93bdeabded2ef4af08a9b5edc9b6e6486 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 3 Dec 2024 12:52:03 -0500 Subject: [PATCH] Revert "Support configuring java runtime from configmap or secret (env.valueFrom)" (#3510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "Support configuring java runtime from configmap or secret (env.valueF…" This reverts commit 2b36f0d6f9498e3c82185a4a18f0c855c4da4a57. * chlog (#3511) --- .chloggen/revert-3379-otel-configmap.yaml | 19 +++++++ config/manager/kustomization.yaml | 1 + pkg/instrumentation/javaagent.go | 26 ++++++---- pkg/instrumentation/javaagent_test.go | 49 ++++--------------- pkg/instrumentation/sdk.go | 13 +++-- .../01-assert.yaml | 4 +- .../02-assert.yaml | 2 +- .../03-assert.yaml | 2 +- .../instrumentation-java-tls/01-assert.yaml | 2 +- .../instrumentation-java/01-assert.yaml | 7 +-- .../instrumentation-java/01-install-app.yaml | 13 ----- .../01-assert.yaml | 2 +- 12 files changed, 62 insertions(+), 78 deletions(-) create mode 100755 .chloggen/revert-3379-otel-configmap.yaml diff --git a/.chloggen/revert-3379-otel-configmap.yaml b/.chloggen/revert-3379-otel-configmap.yaml new file mode 100755 index 0000000000..bd7b66223c --- /dev/null +++ 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 index 5c5f0b84cb..372a75ae43 100644 --- 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 index 1dafcd9cd7..ef91d296d8 100644 --- 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 index f52beb20a3..ea8d81305d 100644 --- 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 index 1ec6272836..87141a1cf8 100644 --- 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 index 09b2a5687b..a4dca94976 100644 --- 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 index 5bfa1ceff3..03c002d2d8 100644 --- 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 index ef36aa4c46..0b6ea1db84 100644 --- 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 index 6cb4d2d206..7ddecadb47 100644 --- 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 index f1af6b5218..cd8a8a37fe 100644 --- a/tests/e2e-instrumentation/instrumentation-java/01-assert.yaml +++ b/tests/e2e-instrumentation/instrumentation-java/01-assert.yaml @@ -17,11 +17,6 @@ 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 @@ -29,7 +24,7 @@ spec: - 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 index c3204ec290..4655644b5b 100644 --- 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 index 250223271b..3ba921ada1 100644 --- 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