Skip to content

🐛 OCPBUGS-76380: fix(deploymentConfig): merge volumes/volumeMounts by name instead of appending #2582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
camilamacedo86 wants to merge 1 commit into
base: main
Choose a base branch
from
+230 −6
Open

🐛 OCPBUGS-76380: fix(deploymentConfig): merge volumes/volumeMounts by name instead of appending #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions internal/operator-controller/rukpak/render/registryv1/generators/generators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,19 +671,32 @@ func applyEnvironmentFromConfig(deployment *appsv1.Deployment, config *config.De
}
}

// applyVolumeConfig appends volumes to the deployment's pod spec.
// This follows OLMv0 behavior:
// applyVolumeConfig merges volumes into the deployment's pod spec.
// Volumes from config override existing volumes with the same name.
// This differs from OLMv0, which appends volumes without checking for duplicates:
// https://github.com/operator-framework/operator-lifecycle-manager/blob/v0.39.0/pkg/controller/operators/olm/overrides/inject/inject.go#L104-L117
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...)
existingVolMap := make(map[string]int, len(deployment.Spec.Template.Spec.Volumes))
for i, vol := range deployment.Spec.Template.Spec.Volumes {
existingVolMap[vol.Name] = i
}

for _, configVol := range config.Volumes {
if idx, exists := existingVolMap[configVol.Name]; exists {
deployment.Spec.Template.Spec.Volumes[idx] = configVol
} else {
deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, configVol)
}
}
}

// applyVolumeMountConfig appends volume mounts to all containers in the deployment.
// This follows OLMv0 behavior:
// applyVolumeMountConfig merges volume mounts into all containers in the deployment.
// Volume mounts from config override existing volume mounts with the same name.
// This differs from OLMv0, which appends volume mounts without checking for duplicates:
// https://github.com/operator-framework/operator-lifecycle-manager/blob/v0.39.0/pkg/controller/operators/olm/overrides/inject/inject.go#L149-L165
func applyVolumeMountConfig(deployment *appsv1.Deployment, config *config.DeploymentConfig) {
if len(config.VolumeMounts) == 0 {
Expand All @@ -692,7 +705,19 @@ 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...)

existingMountMap := make(map[string]int, len(container.VolumeMounts))
for idx, mount := range container.VolumeMounts {
existingMountMap[mount.Name] = idx
}

for _, configMount := range config.VolumeMounts {
if idx, exists := existingMountMap[configMount.Name]; exists {
container.VolumeMounts[idx] = configMount
} else {
container.VolumeMounts = append(container.VolumeMounts, configMount)
}
}
}
}

Expand Down
199 changes: 199 additions & 0 deletions internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading