diff --git a/.chloggen/fix-volumemount-aliasing.yaml b/.chloggen/fix-volumemount-aliasing.yaml new file mode 100644 index 0000000000..bb2e3ac5d2 --- /dev/null +++ b/.chloggen/fix-volumemount-aliasing.yaml @@ -0,0 +1,16 @@ +# 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, opamp, target allocator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix Env slice aliasing in Apache HTTPD, Nginx, OpAMP Bridge, and Target Allocator container builders when the spec slice has spare backing-array capacity + +# One or more tracking issues related to the change +issues: [4954] + +# (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: diff --git a/internal/instrumentation/apachehttpd.go b/internal/instrumentation/apachehttpd.go index 89f9e4af91..53c7d145a8 100644 --- a/internal/instrumentation/apachehttpd.go +++ b/internal/instrumentation/apachehttpd.go @@ -76,10 +76,10 @@ func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod Args: []string{"cp -r " + apacheConfDir + "/* " + apacheAgentConfDirFull}, Env: container.Env, EnvFrom: container.EnvFrom, - VolumeMounts: append(container.VolumeMounts, corev1.VolumeMount{ + VolumeMounts: slices.Concat(container.VolumeMounts, []corev1.VolumeMount{{ Name: apacheAgentConfigVolume, MountPath: apacheAgentConfDirFull, - }), + }}), Resources: apacheSpec.Resources, SecurityContext: container.SecurityContext, ImagePullPolicy: container.ImagePullPolicy, diff --git a/internal/instrumentation/apachehttpd_test.go b/internal/instrumentation/apachehttpd_test.go index 06ee1b0df1..764b2b9243 100644 --- a/internal/instrumentation/apachehttpd_test.go +++ b/internal/instrumentation/apachehttpd_test.go @@ -542,6 +542,29 @@ func TestInjectApacheHttpdagentUnknownNamespace(t *testing.T) { } } +// Regression test: when the container's VolumeMounts slice has spare capacity +// (e.g. after Istio injection), append aliasing corrupts the clone init +// container's mounts. +func TestInjectApacheHttpdagentVolumemountAliasing(t *testing.T) { + // Spare capacity is the trigger — simulates a prior webhook leaving room. + mounts := make([]corev1.VolumeMount, 0, 4) + mounts = append(mounts, + corev1.VolumeMount{Name: "a", MountPath: "/a"}, + corev1.VolumeMount{Name: "b", MountPath: "/b"}, + ) + + pod := corev1.Pod{Spec: corev1.PodSpec{Containers: []corev1.Container{{VolumeMounts: mounts}}}} + result := injectApacheHttpdagent(logr.Discard(), v1alpha1.ApacheHttpd{Image: "foo/bar:1"}, + pod, false, &pod.Spec.Containers[0], "http://otlp:4317", + map[string]string{string(semconv.K8SDeploymentNameKey): "svc"}, v1alpha1.InstrumentationSpec{}) + + clone := result.Spec.InitContainers[0] + lastMount := clone.VolumeMounts[len(clone.VolumeMounts)-1] + assert.Equal(t, apacheAgentConfigVolume, lastMount.Name, + "clone's config mount was corrupted by slice aliasing: %v", clone.VolumeMounts) + assert.Equal(t, apacheAgentConfDirFull, lastMount.MountPath) +} + func TestApacheInitContainerMissing(t *testing.T) { tests := []struct { name string diff --git a/internal/instrumentation/nginx.go b/internal/instrumentation/nginx.go index 0b658181a3..c97c0e174b 100644 --- a/internal/instrumentation/nginx.go +++ b/internal/instrumentation/nginx.go @@ -92,10 +92,10 @@ export %[4]s="$( { nginx -v ; } 2>&1 )" && echo ${%[4]s##*/} > %[3]s/version.txt Args: []string{nginxAgentCommands}, Env: container.Env, EnvFrom: container.EnvFrom, - VolumeMounts: append(container.VolumeMounts, corev1.VolumeMount{ + VolumeMounts: slices.Concat(container.VolumeMounts, []corev1.VolumeMount{{ Name: nginxAgentConfigVolume, MountPath: nginxAgentConfDirFull, - }), + }}), Resources: nginxSpec.Resources, SecurityContext: container.SecurityContext, ImagePullPolicy: container.ImagePullPolicy, diff --git a/internal/instrumentation/nginx_test.go b/internal/instrumentation/nginx_test.go index fb9fc14df7..1ef8e45293 100644 --- a/internal/instrumentation/nginx_test.go +++ b/internal/instrumentation/nginx_test.go @@ -604,6 +604,27 @@ func TestInjectNginxUnknownNamespace(t *testing.T) { } } +// Regression test: append aliasing corrupts clone init container mounts when +// the VolumeMounts slice has spare capacity. +func TestInjectNginxSDKVolumemountAliasing(t *testing.T) { + mounts := make([]corev1.VolumeMount, 0, 4) + mounts = append(mounts, + corev1.VolumeMount{Name: "a", MountPath: "/a"}, + corev1.VolumeMount{Name: "b", MountPath: "/b"}, + ) + + pod := corev1.Pod{Spec: corev1.PodSpec{Containers: []corev1.Container{{VolumeMounts: mounts}}}} + result := injectNginxSDK(logr.Discard(), v1alpha1.Nginx{Image: "foo/bar:1"}, + pod, false, &pod.Spec.Containers[0], "http://otlp:4317", + map[string]string{string(semconv.K8SDeploymentNameKey): "svc"}, v1alpha1.InstrumentationSpec{}) + + clone := result.Spec.InitContainers[0] + lastMount := clone.VolumeMounts[len(clone.VolumeMounts)-1] + assert.Equal(t, nginxAgentConfigVolume, lastMount.Name, + "clone's config mount was corrupted by slice aliasing: %v", clone.VolumeMounts) + assert.Equal(t, nginxAgentConfDirFull, lastMount.MountPath) +} + func TestNginxInitContainerMissing(t *testing.T) { tests := []struct { name string diff --git a/internal/manifests/opampbridge/container.go b/internal/manifests/opampbridge/container.go index 5cb0e6422a..d089b9aee9 100644 --- a/internal/manifests/opampbridge/container.go +++ b/internal/manifests/opampbridge/container.go @@ -4,6 +4,8 @@ package opampbridge import ( + "slices" + "github.com/go-logr/logr" "github.com/operator-framework/operator-lib/proxy" corev1 "k8s.io/api/core/v1" @@ -30,7 +32,7 @@ func Container(cfg config.Config, _ logr.Logger, opampBridge v1alpha1.OpAMPBridg volumeMounts = append(volumeMounts, opampBridge.Spec.VolumeMounts...) } - envVars := opampBridge.Spec.Env + envVars := slices.Clone(opampBridge.Spec.Env) if opampBridge.Spec.Env == nil { envVars = []corev1.EnvVar{} } diff --git a/internal/manifests/opampbridge/container_test.go b/internal/manifests/opampbridge/container_test.go index 02a3800f35..f327023497 100644 --- a/internal/manifests/opampbridge/container_test.go +++ b/internal/manifests/opampbridge/container_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" logf "sigs.k8s.io/controller-runtime/pkg/log" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -65,3 +66,28 @@ func TestContainerVolumes(t *testing.T) { assert.Len(t, c.VolumeMounts, 1) assert.Equal(t, naming.OpAMPBridgeConfigMapVolume(), c.VolumeMounts[0].Name) } + +// Regression test: when Spec.Env has spare backing-array capacity, +// the container's Env must not share the underlying array with the spec. +func TestContainerEnvAliasing(t *testing.T) { + env := make([]corev1.EnvVar, 0, 10) + env = append(env, corev1.EnvVar{Name: "USER_VAR", Value: "val"}) + + opampBridge := v1alpha1.OpAMPBridge{ + Spec: v1alpha1.OpAMPBridgeSpec{ + Env: env, + }, + } + cfg := config.New() + + c := Container(cfg, logger, opampBridge) + + // Mutate the original spec — container must not be affected. + opampBridge.Spec.Env = append(opampBridge.Spec.Env, + corev1.EnvVar{Name: "intruder", Value: "bad"}) + + for _, e := range c.Env { + assert.NotEqual(t, "intruder", e.Name, + "container Env shares backing array with spec") + } +} diff --git a/internal/manifests/targetallocator/container.go b/internal/manifests/targetallocator/container.go index fb9d4922cd..dffe86faf0 100644 --- a/internal/manifests/targetallocator/container.go +++ b/internal/manifests/targetallocator/container.go @@ -48,7 +48,7 @@ func Container(cfg config.Config, _ logr.Logger, instance v1alpha1.TargetAllocat }} volumeMounts = append(volumeMounts, instance.Spec.VolumeMounts...) - envVars := instance.Spec.Env + envVars := slices.Clone(instance.Spec.Env) if envVars == nil { envVars = []corev1.EnvVar{} } diff --git a/internal/manifests/targetallocator/container_test.go b/internal/manifests/targetallocator/container_test.go index e307fced31..8d2b45a715 100644 --- a/internal/manifests/targetallocator/container_test.go +++ b/internal/manifests/targetallocator/container_test.go @@ -644,6 +644,33 @@ func TestContainerEnvFrom(t *testing.T) { assert.Contains(t, c.EnvFrom, envFrom2) } +// Regression test: when Spec.Env has spare backing-array capacity, +// the container's Env must not share the underlying array with the spec. +func TestContainerEnvAliasing(t *testing.T) { + env := make([]corev1.EnvVar, 0, 10) + env = append(env, corev1.EnvVar{Name: "USER_VAR", Value: "val"}) + + targetAllocator := v1alpha1.TargetAllocator{ + Spec: v1alpha1.TargetAllocatorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Env: env, + }, + }, + } + cfg := config.New() + + c := Container(cfg, logger, targetAllocator) + + // Mutate the original spec — container must not be affected. + targetAllocator.Spec.Env = append(targetAllocator.Spec.Env, + corev1.EnvVar{Name: "intruder", Value: "bad"}) + + for _, e := range c.Env { + assert.NotEqual(t, "intruder", e.Name, + "container Env shares backing array with spec") + } +} + func TestContainerImagePullPolicy(t *testing.T) { // prepare targetAllocator := v1alpha1.TargetAllocator{