From 3dfb4837e11e0f9b4ea46fad5239738567e8ce5a Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 20 Mar 2026 15:29:29 +0100 Subject: [PATCH] fix(deploymentConfig): merge volumes/volumeMounts by name instead of appending Generated-by: Cursour/Claude --- .../registryv1/generators/generators.go | 60 +++++- .../registryv1/generators/generators_test.go | 199 ++++++++++++++++++ 2 files changed, 251 insertions(+), 8 deletions(-) diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators.go index 9815c4dba..b3b64b1e8 100644 --- a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go +++ b/internal/operator-controller/rukpak/render/registryv1/generators/generators.go @@ -671,20 +671,18 @@ func applyEnvironmentFromConfig(deployment *appsv1.Deployment, config *config.De } } -// applyVolumeConfig appends volumes to the deployment's pod spec. -// This follows OLMv0 behavior: -// https://github.com/operator-framework/operator-lifecycle-manager/blob/v0.39.0/pkg/controller/operators/olm/overrides/inject/inject.go#L104-L117 +// applyVolumeConfig merges volumes into the deployment's pod spec. +// Volumes from config override existing volumes with the same name. func applyVolumeConfig(deployment *appsv1.Deployment, config *config.DeploymentConfig) { if len(config.Volumes) == 0 { return } - deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, config.Volumes...) + deployment.Spec.Template.Spec.Volumes = mergeVolumes(deployment.Spec.Template.Spec.Volumes, config.Volumes) } -// applyVolumeMountConfig appends volume mounts to all containers in the deployment. -// This follows OLMv0 behavior: -// https://github.com/operator-framework/operator-lifecycle-manager/blob/v0.39.0/pkg/controller/operators/olm/overrides/inject/inject.go#L149-L165 +// applyVolumeMountConfig merges volume mounts into all containers in the deployment. +// Volume mounts from config override existing volume mounts with the same name. func applyVolumeMountConfig(deployment *appsv1.Deployment, config *config.DeploymentConfig) { if len(config.VolumeMounts) == 0 { return @@ -692,8 +690,54 @@ func applyVolumeMountConfig(deployment *appsv1.Deployment, config *config.Deploy for i := range deployment.Spec.Template.Spec.Containers { container := &deployment.Spec.Template.Spec.Containers[i] - container.VolumeMounts = append(container.VolumeMounts, config.VolumeMounts...) + container.VolumeMounts = mergeVolumeMounts(container.VolumeMounts, config.VolumeMounts) + } +} + +// mergeVolumes merges newVolumes into existing volumes, overriding volumes with matching names. +func mergeVolumes(existing []corev1.Volume, newVolumes []corev1.Volume) []corev1.Volume { + merged := make([]corev1.Volume, len(existing)) + copy(merged, existing) + + for _, newVolume := range newVolumes { + found := false + for i := range merged { + if merged[i].Name == newVolume.Name { + // Override the existing volume with the new one + merged[i] = newVolume + found = true + break + } + } + if !found { + // Append if this is a new volume + merged = append(merged, newVolume) + } + } + return merged +} + +// mergeVolumeMounts merges newVolumeMounts into existing volume mounts, overriding mounts with matching names. +func mergeVolumeMounts(existing []corev1.VolumeMount, newVolumeMounts []corev1.VolumeMount) []corev1.VolumeMount { + merged := make([]corev1.VolumeMount, len(existing)) + copy(merged, existing) + + for _, newVolumeMount := range newVolumeMounts { + found := false + for i := range merged { + if merged[i].Name == newVolumeMount.Name { + // Override the existing volume mount with the new one + merged[i] = newVolumeMount + found = true + break + } + } + if !found { + // Append if this is a new volume mount + merged = append(merged, newVolumeMount) + } } + return merged } // applyTolerationsConfig appends tolerations to the deployment's pod spec. diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go index 22ce6d28b..61432cb1c 100644 --- a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go @@ -3085,6 +3085,205 @@ func Test_BundleCSVDeploymentGenerator_WithDeploymentConfig(t *testing.T) { require.Equal(t, "existing_value", dep.Spec.Template.Spec.Containers[0].Env[0].Value) }, }, + { + name: "merges volumes from deployment config - overrides matching names", + bundle: &bundle.RegistryV1{ + CSV: clusterserviceversion.Builder(). + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{ + Name: "test-deployment", + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "manager"}, + }, + Volumes: []corev1.Volume{ + { + Name: "bundle-emptydir-vol", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: "existing-vol", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + }, + }, + }, + }, + }, + ).Build(), + }, + opts: render.Options{ + InstallNamespace: "test-ns", + TargetNamespaces: []string{"test-ns"}, + DeploymentConfig: &config.DeploymentConfig{ + Volumes: []corev1.Volume{ + { + Name: "bundle-emptydir-vol", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "test-cm-vol", + }, + }, + }, + }, + { + Name: "config-secret-vol", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: "test-secret-vol", + }, + }, + }, + }, + }, + }, + verify: func(t *testing.T, objs []client.Object) { + require.Len(t, objs, 1) + dep := objs[0].(*appsv1.Deployment) + volumes := dep.Spec.Template.Spec.Volumes + + // Should have 3 volumes total: + // - bundle-emptydir-vol (overridden to ConfigMap) + // - existing-vol (unchanged) + // - config-secret-vol (new) + require.Len(t, volumes, 3) + + // Verify bundle-emptydir-vol was overridden (now ConfigMap, not EmptyDir) + var bundleVol *corev1.Volume + for i := range volumes { + if volumes[i].Name == "bundle-emptydir-vol" { + bundleVol = &volumes[i] + break + } + } + require.NotNil(t, bundleVol, "bundle-emptydir-vol should exist") + require.NotNil(t, bundleVol.ConfigMap, "bundle-emptydir-vol should be ConfigMap") + require.Equal(t, "test-cm-vol", bundleVol.ConfigMap.Name) + require.Nil(t, bundleVol.EmptyDir, "bundle-emptydir-vol should not be EmptyDir") + + // Verify existing-vol remains unchanged + var existingVol *corev1.Volume + for i := range volumes { + if volumes[i].Name == "existing-vol" { + existingVol = &volumes[i] + break + } + } + require.NotNil(t, existingVol, "existing-vol should exist") + require.NotNil(t, existingVol.EmptyDir, "existing-vol should still be EmptyDir") + + // Verify config-secret-vol was added + var secretVol *corev1.Volume + for i := range volumes { + if volumes[i].Name == "config-secret-vol" { + secretVol = &volumes[i] + break + } + } + require.NotNil(t, secretVol, "config-secret-vol should exist") + require.NotNil(t, secretVol.Secret, "config-secret-vol should be Secret") + require.Equal(t, "test-secret-vol", secretVol.Secret.SecretName) + }, + }, + { + name: "merges volumeMounts from deployment config - overrides matching names", + bundle: &bundle.RegistryV1{ + CSV: clusterserviceversion.Builder(). + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{ + Name: "test-deployment", + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "manager", + VolumeMounts: []corev1.VolumeMount{ + { + Name: "bundle-vol", + MountPath: "/old/path", + }, + { + Name: "existing-vol", + MountPath: "/existing/path", + }, + }, + }, + }, + }, + }, + }, + }, + ).Build(), + }, + opts: render.Options{ + InstallNamespace: "test-ns", + TargetNamespaces: []string{"test-ns"}, + DeploymentConfig: &config.DeploymentConfig{ + VolumeMounts: []corev1.VolumeMount{ + { + Name: "bundle-vol", + MountPath: "/new/path", + }, + { + Name: "config-vol", + MountPath: "/config/path", + }, + }, + }, + }, + verify: func(t *testing.T, objs []client.Object) { + require.Len(t, objs, 1) + dep := objs[0].(*appsv1.Deployment) + volumeMounts := dep.Spec.Template.Spec.Containers[0].VolumeMounts + + // Should have 3 volume mounts total: + // - bundle-vol (overridden to /new/path) + // - existing-vol (unchanged) + // - config-vol (new) + require.Len(t, volumeMounts, 3) + + // Verify bundle-vol was overridden + var bundleMount *corev1.VolumeMount + for i := range volumeMounts { + if volumeMounts[i].Name == "bundle-vol" { + bundleMount = &volumeMounts[i] + break + } + } + require.NotNil(t, bundleMount, "bundle-vol should exist") + require.Equal(t, "/new/path", bundleMount.MountPath, "bundle-vol mount path should be overridden") + + // Verify existing-vol remains unchanged + var existingMount *corev1.VolumeMount + for i := range volumeMounts { + if volumeMounts[i].Name == "existing-vol" { + existingMount = &volumeMounts[i] + break + } + } + require.NotNil(t, existingMount, "existing-vol should exist") + require.Equal(t, "/existing/path", existingMount.MountPath) + + // Verify config-vol was added + var configMount *corev1.VolumeMount + for i := range volumeMounts { + if volumeMounts[i].Name == "config-vol" { + configMount = &volumeMounts[i] + break + } + } + require.NotNil(t, configMount, "config-vol should exist") + require.Equal(t, "/config/path", configMount.MountPath) + }, + }, } { t.Run(tc.name, func(t *testing.T) { objs, err := generators.BundleCSVDeploymentGenerator(tc.bundle, tc.opts)