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
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
60 changes: 52 additions & 8 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,29 +671,73 @@ 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
}

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.
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