CLOUDP-73217: fix volumes merging (#204)

Author: Nikolas De Giorgis

pkg/kube/podtemplatespec/podspec_template.go

@@ -265,19 +265,136 @@ 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 {
-	mergedVolumeMap := make(map[string]corev1.Volume)
+	defaultVolumesMap := createVolumesMap(defaultVolumes)
+	overrideVolumesMap := createVolumesMap(overrideVolumes)
 	mergedVolumes := []corev1.Volume{}
-	for _, v := range defaultVolumes {
-		mergedVolumeMap[v.Name] = v
-	}
 
-	for _, v := range overrideVolumes {
-		mergedVolumeMap[v.Name] = v
+	for _, defaultVolume := range defaultVolumes {
+		mergedVolume := defaultVolume
+		if overrideVolume, ok := overrideVolumesMap[defaultVolume.Name]; ok {
+			mergedVolume = mergeVolume(defaultVolume, overrideVolume)
+		}
+		mergedVolumes = append(mergedVolumes, mergedVolume)
 	}
 
-	for _, v := range mergedVolumeMap {
-		mergedVolumes = append(mergedVolumes, v)
+	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 {

pkg/kube/podtemplatespec/podspec_template_test.go

@@ -304,26 +304,26 @@ func TestMergeVolumes_DoesNotAddDuplicatesWithSameName(t *testing.T) {
 	})
 
 	overridePodSpec := getDefaultPodSpec()
-	defaultPodSpec.Spec.Volumes = append(defaultPodSpec.Spec.Volumes, corev1.Volume{
+	overridePodSpec.Spec.Volumes = append(overridePodSpec.Spec.Volumes, corev1.Volume{
 		Name: "new-volume",
 		VolumeSource: corev1.VolumeSource{
 			HostPath: &corev1.HostPathVolumeSource{
 				Path: "updated-host-path",
 			},
 		},
 	})
-	defaultPodSpec.Spec.Volumes = append(defaultPodSpec.Spec.Volumes, corev1.Volume{
+	overridePodSpec.Spec.Volumes = append(overridePodSpec.Spec.Volumes, corev1.Volume{
 		Name: "new-volume-3",
 	})
 
 	mergedPodSpecTemplate, err := MergePodTemplateSpecs(defaultPodSpec, overridePodSpec)
 	assert.NoError(t, err)
 
 	assert.Len(t, mergedPodSpecTemplate.Spec.Volumes, 3)
-	assert.Equal(t, mergedPodSpecTemplate.Spec.Volumes[0].Name, "new-volume")
-	assert.Equal(t, mergedPodSpecTemplate.Spec.Volumes[0].VolumeSource.HostPath.Path, "updated-host-path")
-	assert.Equal(t, mergedPodSpecTemplate.Spec.Volumes[1].Name, "new-volume-2")
-	assert.Equal(t, mergedPodSpecTemplate.Spec.Volumes[2].Name, "new-volume-3")
+	assert.Equal(t, "new-volume", mergedPodSpecTemplate.Spec.Volumes[0].Name)
+	assert.Equal(t, "updated-host-path", mergedPodSpecTemplate.Spec.Volumes[0].VolumeSource.HostPath.Path)
+	assert.Equal(t, "new-volume-2", mergedPodSpecTemplate.Spec.Volumes[1].Name)
+	assert.Equal(t, "new-volume-3", mergedPodSpecTemplate.Spec.Volumes[2].Name)
 }
 
 func TestMergeVolumeMounts(t *testing.T) {
@@ -342,6 +342,57 @@ func TestMergeVolumeMounts(t *testing.T) {
 	assert.Equal(t, []corev1.VolumeMount{vol2, vol0}, mergedVolumeMounts)
 }
 
+func TestMergeVolumesSecret(t *testing.T) {
+	permission := int32(416)
+	vol0 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "Secret-name"}}}}
+	vol1 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{DefaultMode: &permission}}}}
+	mergedVolumes := mergeVolumes(vol0, vol1)
+	assert.Len(t, mergedVolumes, 1)
+	volume := mergedVolumes[0]
+	assert.Equal(t, "volume", volume.Name)
+	assert.Equal(t, corev1.SecretVolumeSource{SecretName: "Secret-name", DefaultMode: &permission}, *volume.Secret)
+}
+
+func TestMergeNonNilValueNotFilledByOperator(t *testing.T) {
+	// Tests that providing a custom volume with a volume source
+	// That the operator does not manage overwrites the original
+	vol0 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "Secret-name"}}}}
+	vol1 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{GCEPersistentDisk: &corev1.GCEPersistentDiskVolumeSource{}}}}
+	mergedVolumes := mergeVolumes(vol0, vol1)
+	assert.Len(t, mergedVolumes, 1)
+	volume := mergedVolumes[0]
+	assert.Equal(t, "volume", volume.Name)
+	assert.Equal(t, corev1.GCEPersistentDiskVolumeSource{}, *volume.GCEPersistentDisk)
+	assert.Nil(t, volume.Secret)
+}
+
+func TestMergeNonNilValueFilledByOperatorButDifferent(t *testing.T) {
+	// Tests that providing a custom volume with a volume source
+	// That the operator does manage, but different from the one
+	// That already exists, overwrites the original
+	vol0 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "Secret-name"}}}}
+	vol1 := []corev1.Volume{{Name: "volume", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}}
+	mergedVolumes := mergeVolumes(vol0, vol1)
+	assert.Len(t, mergedVolumes, 1)
+	volume := mergedVolumes[0]
+	assert.Equal(t, "volume", volume.Name)
+	assert.Equal(t, corev1.EmptyDirVolumeSource{}, *volume.EmptyDir)
+	assert.Nil(t, volume.Secret)
+}
+
+func TestMergeVolumeAddVolume(t *testing.T) {
+	vol0 := []corev1.Volume{{Name: "volume0", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{}}}}
+	vol1 := []corev1.Volume{{Name: "volume1", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}}
+	mergedVolumes := mergeVolumes(vol0, vol1)
+	assert.Len(t, mergedVolumes, 2)
+	volume0 := mergedVolumes[0]
+	assert.Equal(t, "volume0", volume0.Name)
+	assert.Equal(t, corev1.SecretVolumeSource{}, *volume0.Secret)
+	volume1 := mergedVolumes[1]
+	assert.Equal(t, "volume1", volume1.Name)
+	assert.Equal(t, corev1.EmptyDirVolumeSource{}, *volume1.EmptyDir)
+}
+
 func int64Ref(i int64) *int64 {
 	return &i
 }