CLOUDP-80014: Move Volume/VolumeMount merging logic + tests into merge package (#289)

Author: chatton

pkg/controller/mongodb/build_statefulset_test.go

@@ -47,20 +47,20 @@ func assertStatefulSetIsBuiltCorrectly(t *testing.T, mdb mdbv1.MongoDB, sts *app
 	assert.Equal(t, mdb.Namespace, sts.Namespace)
 	assert.Equal(t, appsv1.RollingUpdateStatefulSetStrategyType, sts.Spec.UpdateStrategy.Type)
 	assert.Equal(t, operatorServiceAccountName, sts.Spec.Template.Spec.ServiceAccountName)
-	assert.Len(t, sts.Spec.Template.Spec.Containers[0].Env, 4)
-	assert.Len(t, sts.Spec.Template.Spec.Containers[1].Env, 1)
+	assert.Len(t, sts.Spec.Template.Spec.Containers[1].Env, 4)
+	assert.Len(t, sts.Spec.Template.Spec.Containers[0].Env, 1)
 
-	agentContainer := sts.Spec.Template.Spec.Containers[0]
+	agentContainer := sts.Spec.Template.Spec.Containers[1]
 	assert.Equal(t, "agent-image", agentContainer.Image)
 	probe := agentContainer.ReadinessProbe
 	assert.True(t, reflect.DeepEqual(probes.New(defaultReadiness()), *probe))
 	assert.Equal(t, probes.New(defaultReadiness()).FailureThreshold, probe.FailureThreshold)
 	assert.Equal(t, int32(5), probe.InitialDelaySeconds)
 	assert.Len(t, agentContainer.VolumeMounts, 4)
 
-	mongodContainer := sts.Spec.Template.Spec.Containers[1]
+	mongodContainer := sts.Spec.Template.Spec.Containers[0]
 	assert.Equal(t, "repo/mongo:4.2.2", mongodContainer.Image)
-	assert.NotNil(t, sts.Spec.Template.Spec.Containers[0].ReadinessProbe)
+	assert.NotNil(t, sts.Spec.Template.Spec.Containers[1].ReadinessProbe)
 	assert.Len(t, mongodContainer.VolumeMounts, 4)
 
 	initContainer := sts.Spec.Template.Spec.InitContainers[0]

pkg/controller/mongodb/replicaset_controller_test.go

@@ -157,13 +157,13 @@ func TestStatefulSet_IsCorrectlyConfigured(t *testing.T) {
 
 	assert.Len(t, sts.Spec.Template.Spec.Containers, 2)
 
-	agentContainer := sts.Spec.Template.Spec.Containers[0]
+	agentContainer := sts.Spec.Template.Spec.Containers[1]
 	assert.Equal(t, agentName, agentContainer.Name)
 	assert.Equal(t, os.Getenv(agentImageEnv), agentContainer.Image)
 	expectedProbe := probes.New(defaultReadiness())
 	assert.True(t, reflect.DeepEqual(&expectedProbe, agentContainer.ReadinessProbe))
 
-	mongodbContainer := sts.Spec.Template.Spec.Containers[1]
+	mongodbContainer := sts.Spec.Template.Spec.Containers[0]
 	assert.Equal(t, mongodbName, mongodbContainer.Name)
 	assert.Equal(t, "repo/mongo:4.2.2", mongodbContainer.Image)
 
@@ -529,8 +529,8 @@ func TestReplicaSet_IsScaledUpToDesiredMembers_WhenFirstCreated(t *testing.T) {
 
 func TestOpenshift_Configuration(t *testing.T) {
 	sts := performReconciliationAndGetStatefulSet(t, "openshift_mdb.yaml")
-	assert.Equal(t, "MANAGED_SECURITY_CONTEXT", sts.Spec.Template.Spec.Containers[0].Env[3].Name)
-	assert.Equal(t, "MANAGED_SECURITY_CONTEXT", sts.Spec.Template.Spec.Containers[1].Env[1].Name)
+	assert.Equal(t, "MANAGED_SECURITY_CONTEXT", sts.Spec.Template.Spec.Containers[1].Env[3].Name)
+	assert.Equal(t, "MANAGED_SECURITY_CONTEXT", sts.Spec.Template.Spec.Containers[0].Env[1].Name)
 }
 
 func TestVolumeClaimTemplates_Configuration(t *testing.T) {

pkg/kube/podtemplatespec/podspec_template.go

@@ -1,11 +1,10 @@
 package podtemplatespec
 
 import (
-	"sort"
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/merge"
 
 	"github.com/imdario/mergo"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/container"
-	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -229,27 +228,16 @@ func WithVolumeMounts(containerName string, volumeMounts ...corev1.VolumeMount)
 }
 
 func MergePodTemplateSpecs(defaultTemplate, overrideTemplate corev1.PodTemplateSpec) (corev1.PodTemplateSpec, error) {
-	// Containers need to be merged manually
-	mergedContainers, err := mergeContainers(defaultTemplate.Spec.Containers, overrideTemplate.Spec.Containers)
-	if err != nil {
-		return corev1.PodTemplateSpec{}, err
-	}
 
-	mergedTolerations := mergeTolerations(defaultTemplate.Spec.Tolerations, overrideTemplate.Spec.Tolerations)
-
-	// InitContainers need to be merged manually
-	mergedInitContainers, err := mergeContainers(defaultTemplate.Spec.InitContainers, overrideTemplate.Spec.InitContainers)
-	if err != nil {
-		return corev1.PodTemplateSpec{}, err
-	}
-
-	// Affinity needs to be merged manually
+	mergedContainers := merge.Containers(defaultTemplate.Spec.Containers, overrideTemplate.Spec.Containers)
+	mergedTolerations := merge.Tolerations(defaultTemplate.Spec.Tolerations, overrideTemplate.Spec.Tolerations)
+	mergedInitContainers := merge.Containers(defaultTemplate.Spec.InitContainers, overrideTemplate.Spec.InitContainers)
 	mergedAffinity, err := mergeAffinity(defaultTemplate.Spec.Affinity, overrideTemplate.Spec.Affinity)
 	if err != nil {
 		return corev1.PodTemplateSpec{}, err
 	}
 
-	mergedVolumes := mergeVolumes(defaultTemplate.Spec.Volumes, overrideTemplate.Spec.Volumes)
+	mergedVolumes := merge.Volumes(defaultTemplate.Spec.Volumes, overrideTemplate.Spec.Volumes)
 
 	// Everything else can be merged with mergo
 	mergedPodTemplateSpec := *defaultTemplate.DeepCopy()
@@ -265,254 +253,6 @@ func MergePodTemplateSpecs(defaultTemplate, overrideTemplate corev1.PodTemplateS
 	return mergedPodTemplateSpec, nil
 }
 
-func createKeyToPathMap(items []corev1.KeyToPath) map[string]corev1.KeyToPath {
-	itemsMap := make(map[string]corev1.KeyToPath)
-	for _, v := range items {
-		itemsMap[v.Key] = v
-	}
-	return itemsMap
-}
-
-func mergeKeyToPath(defaultKey corev1.KeyToPath, overrideKey corev1.KeyToPath) corev1.KeyToPath {
-	if defaultKey.Key != overrideKey.Key {
-		// This should not happen as we always merge by Key.
-		// If it does, we return the default as something's wrong
-		return defaultKey
-	}
-	if overrideKey.Path != "" {
-		defaultKey.Path = overrideKey.Path
-	}
-	if overrideKey.Mode != nil {
-		defaultKey.Mode = overrideKey.Mode
-	}
-	return defaultKey
-}
-
-func mergeKeyToPathItems(defaultItems []corev1.KeyToPath, overrideItems []corev1.KeyToPath) []corev1.KeyToPath {
-	// Merge Items array by KeyToPath.Key entry
-	defaultItemsMap := createKeyToPathMap(defaultItems)
-	overrideItemsMap := createKeyToPathMap(overrideItems)
-	var mergedItems []corev1.KeyToPath
-	for _, defaultItem := range defaultItemsMap {
-		mergedKey := defaultItem
-		if overrideItem, ok := overrideItemsMap[defaultItem.Key]; ok {
-			// Need to merge
-			mergedKey = mergeKeyToPath(defaultItem, overrideItem)
-		}
-		mergedItems = append(mergedItems, mergedKey)
-	}
-	for _, overrideItem := range overrideItemsMap {
-		if _, ok := defaultItemsMap[overrideItem.Key]; ok {
-			// Already merged
-			continue
-		}
-		mergedItems = append(mergedItems, overrideItem)
-
-	}
-	return mergedItems
-}
-
-func mergeVolume(defaultVolume corev1.Volume, overrideVolume corev1.Volume) corev1.Volume {
-	// Volume contains only Name and VolumeSource
-
-	// Merge VolumeSource
-	overrideSource := overrideVolume.VolumeSource
-	defaultSource := defaultVolume.VolumeSource
-	mergedVolume := defaultVolume
-
-	// Only one field must be non-nil.
-	// We merge if it is one of the ones we fill from the operator side:
-	// - EmptyDir
-	if overrideSource.EmptyDir != nil && defaultSource.EmptyDir != nil {
-		if overrideSource.EmptyDir.Medium != "" {
-			mergedVolume.EmptyDir.Medium = overrideSource.EmptyDir.Medium
-		}
-		if overrideSource.EmptyDir.SizeLimit != nil {
-			mergedVolume.EmptyDir.SizeLimit = overrideSource.EmptyDir.SizeLimit
-		}
-		return mergedVolume
-	}
-	// - Secret
-	if overrideSource.Secret != nil && defaultSource.Secret != nil {
-		if overrideSource.Secret.SecretName != "" {
-			mergedVolume.Secret.SecretName = overrideSource.Secret.SecretName
-		}
-		mergedVolume.Secret.Items = mergeKeyToPathItems(defaultSource.Secret.Items, overrideSource.Secret.Items)
-		if overrideSource.Secret.DefaultMode != nil {
-			mergedVolume.Secret.DefaultMode = overrideSource.Secret.DefaultMode
-		}
-		return mergedVolume
-	}
-	// - ConfigMap
-	if overrideSource.ConfigMap != nil && defaultSource.ConfigMap != nil {
-		if overrideSource.ConfigMap.LocalObjectReference.Name != "" {
-			mergedVolume.ConfigMap.LocalObjectReference.Name = overrideSource.ConfigMap.LocalObjectReference.Name
-		}
-		mergedVolume.ConfigMap.Items = mergeKeyToPathItems(defaultSource.ConfigMap.Items, overrideSource.ConfigMap.Items)
-		if overrideSource.ConfigMap.DefaultMode != nil {
-			mergedVolume.ConfigMap.DefaultMode = overrideSource.ConfigMap.DefaultMode
-		}
-		if overrideSource.ConfigMap.Optional != nil {
-			mergedVolume.ConfigMap.Optional = overrideSource.ConfigMap.Optional
-		}
-		return mergedVolume
-	}
-
-	// Otherwise we assume that the user provides every field
-	// and we just assign it and nil every other field
-	// We also do that in the case the user provides one of the three listed above
-	// but our volume has a different non-nil entry.
-
-	// this is effectively the same as just returning the overrideSource
-	mergedVolume.VolumeSource = overrideSource
-	return mergedVolume
-}
-
-func createVolumesMap(volumes []corev1.Volume) map[string]corev1.Volume {
-	volumesMap := make(map[string]corev1.Volume)
-	for _, v := range volumes {
-		volumesMap[v.Name] = v
-	}
-	return volumesMap
-}
-
-func mergeVolumes(defaultVolumes []corev1.Volume, overrideVolumes []corev1.Volume) []corev1.Volume {
-	defaultVolumesMap := createVolumesMap(defaultVolumes)
-	overrideVolumesMap := createVolumesMap(overrideVolumes)
-	mergedVolumes := []corev1.Volume{}
-
-	for _, defaultVolume := range defaultVolumes {
-		mergedVolume := defaultVolume
-		if overrideVolume, ok := overrideVolumesMap[defaultVolume.Name]; ok {
-			mergedVolume = mergeVolume(defaultVolume, overrideVolume)
-		}
-		mergedVolumes = append(mergedVolumes, mergedVolume)
-	}
-
-	for _, overrideVolume := range overrideVolumes {
-		if _, ok := defaultVolumesMap[overrideVolume.Name]; ok {
-			// Already Merged
-			continue
-		}
-		mergedVolumes = append(mergedVolumes, overrideVolume)
-	}
-
-	sort.SliceStable(mergedVolumes, func(i, j int) bool {
-		return mergedVolumes[i].Name < mergedVolumes[j].Name
-	})
-	return mergedVolumes
-}
-
-func mergeVolumeMounts(defaultMounts, overrideMounts []corev1.VolumeMount) ([]corev1.VolumeMount, error) {
-	defaultMountsMap := createMountsMap(defaultMounts)
-	overrideMountsMap := createMountsMap(overrideMounts)
-	mergedVolumeMounts := []corev1.VolumeMount{}
-	for _, defaultMount := range defaultMounts {
-		if overrideMount, ok := overrideMountsMap[defaultMount.Name]; ok {
-			// needs merge
-			if err := mergo.Merge(&defaultMount, overrideMount, mergo.WithAppendSlice); err != nil { //nolint
-				return nil, err
-			}
-		}
-		mergedVolumeMounts = append(mergedVolumeMounts, defaultMount)
-	}
-	for _, overrideMount := range overrideMounts {
-		if _, ok := defaultMountsMap[overrideMount.Name]; ok {
-			// already merged
-			continue
-		}
-		mergedVolumeMounts = append(mergedVolumeMounts, overrideMount)
-	}
-	return mergedVolumeMounts, nil
-}
-
-func createMountsMap(volumeMounts []corev1.VolumeMount) map[string]corev1.VolumeMount {
-	mountMap := make(map[string]corev1.VolumeMount)
-	for _, m := range volumeMounts {
-		mountMap[m.Name] = m
-	}
-	return mountMap
-}
-
-func createTolerationsMap(tolerations []corev1.Toleration) map[string]corev1.Toleration {
-	tolerationsMap := make(map[string]corev1.Toleration)
-	for _, t := range tolerations {
-		tolerationsMap[t.Key] = t
-	}
-	return tolerationsMap
-}
-
-func mergeTolerations(defaultTolerations, overrideTolerations []corev1.Toleration) []corev1.Toleration {
-	mergedTolerations := make([]corev1.Toleration, 0)
-	defaultMap := createTolerationsMap(defaultTolerations)
-	for _, v := range overrideTolerations {
-		defaultMap[v.Key] = v
-	}
-
-	for _, v := range defaultMap {
-		mergedTolerations = append(mergedTolerations, v)
-	}
-
-	if len(mergedTolerations) == 0 {
-		return nil
-	}
-
-	sort.SliceStable(mergedTolerations, func(i, j int) bool {
-		return mergedTolerations[i].Key < mergedTolerations[j].Key
-	})
-
-	return mergedTolerations
-}
-
-func mergeContainers(defaultContainers, customContainers []corev1.Container) ([]corev1.Container, error) {
-	defaultMap := createContainerMap(defaultContainers)
-	customMap := createContainerMap(customContainers)
-	mergedContainers := []corev1.Container{}
-	for _, defaultContainer := range defaultContainers {
-		if customContainer, ok := customMap[defaultContainer.Name]; ok {
-			// The container is present in both maps, so we need to merge
-			// MergeWithOverride mounts
-			mergedMounts, err := mergeVolumeMounts(defaultContainer.VolumeMounts, customContainer.VolumeMounts)
-			if err != nil {
-				return nil, err
-			}
-
-			mergedEnvs := envvar.MergeWithOverride(defaultContainer.Env, customContainer.Env)
-
-			if err := mergo.Merge(&defaultContainer, customContainer, mergo.WithOverride); err != nil { //nolint
-				return nil, err
-			}
-			// completely override any resources that were provided
-			// this prevents issues with custom requests giving errors due
-			// to the defaulted limits
-			defaultContainer.Resources = customContainer.Resources
-			defaultContainer.VolumeMounts = mergedMounts
-			defaultContainer.Env = mergedEnvs
-		}
-		// The default container was not modified by the override, so just add it
-		mergedContainers = append(mergedContainers, defaultContainer)
-	}
-
-	// Look for customContainers that were not merged into existing ones
-	for _, customContainer := range customContainers {
-		if _, ok := defaultMap[customContainer.Name]; ok {
-			continue
-		}
-		// Need to add it
-		mergedContainers = append(mergedContainers, customContainer)
-	}
-
-	return mergedContainers, nil
-}
-
-func createContainerMap(containers []corev1.Container) map[string]corev1.Container {
-	containerMap := make(map[string]corev1.Container)
-	for _, c := range containers {
-		containerMap[c.Name] = c
-	}
-	return containerMap
-}
-
 func mergeAffinity(defaultAffinity, overrideAffinity *corev1.Affinity) (*corev1.Affinity, error) {
 	if defaultAffinity == nil {
 		return overrideAffinity, nil