From 92ec0e729077293a133eab724b7e650fe5dd9c68 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Mon, 21 Sep 2020 17:20:39 +0100 Subject: [PATCH 1/2] CLOUDP-73217: fix volumes merging --- pkg/kube/podtemplatespec/podspec_template.go | 137 +++++++++++++++++- .../podtemplatespec/podspec_template_test.go | 63 +++++++- 2 files changed, 186 insertions(+), 14 deletions(-) diff --git a/pkg/kube/podtemplatespec/podspec_template.go b/pkg/kube/podtemplatespec/podspec_template.go index b8a11aee1..b44630600 100644 --- a/pkg/kube/podtemplatespec/podspec_template.go +++ b/pkg/kube/podtemplatespec/podspec_template.go @@ -265,19 +265,140 @@ 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) + 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 + } + if len(overrideSource.Secret.Items) > 0 { + 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 + } + if len(overrideSource.ConfigMap.Items) > 0 { + 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 { diff --git a/pkg/kube/podtemplatespec/podspec_template_test.go b/pkg/kube/podtemplatespec/podspec_template_test.go index 2fbfb615f..bfcce3840 100644 --- a/pkg/kube/podtemplatespec/podspec_template_test.go +++ b/pkg/kube/podtemplatespec/podspec_template_test.go @@ -304,7 +304,7 @@ 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{ @@ -312,7 +312,7 @@ func TestMergeVolumes_DoesNotAddDuplicatesWithSameName(t *testing.T) { }, }, }) - defaultPodSpec.Spec.Volumes = append(defaultPodSpec.Spec.Volumes, corev1.Volume{ + overridePodSpec.Spec.Volumes = append(overridePodSpec.Spec.Volumes, corev1.Volume{ Name: "new-volume-3", }) @@ -320,10 +320,10 @@ func TestMergeVolumes_DoesNotAddDuplicatesWithSameName(t *testing.T) { 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 } From c315822f0ee8325f96aef1dd3e4056b517724438 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 22 Sep 2020 09:37:54 +0100 Subject: [PATCH 2/2] removed unneeded check --- pkg/kube/podtemplatespec/podspec_template.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/pkg/kube/podtemplatespec/podspec_template.go b/pkg/kube/podtemplatespec/podspec_template.go index b44630600..245ff558c 100644 --- a/pkg/kube/podtemplatespec/podspec_template.go +++ b/pkg/kube/podtemplatespec/podspec_template.go @@ -292,7 +292,7 @@ func mergeKeyToPathItems(defaultItems []corev1.KeyToPath, overrideItems []corev1 // Merge Items array by KeyToPath.Key entry defaultItemsMap := createKeyToPathMap(defaultItems) overrideItemsMap := createKeyToPathMap(overrideItems) - mergedItems := []corev1.KeyToPath{} + var mergedItems []corev1.KeyToPath for _, defaultItem := range defaultItemsMap { mergedKey := defaultItem if overrideItem, ok := overrideItemsMap[defaultItem.Key]; ok { @@ -337,9 +337,7 @@ func mergeVolume(defaultVolume corev1.Volume, overrideVolume corev1.Volume) core if overrideSource.Secret.SecretName != "" { mergedVolume.Secret.SecretName = overrideSource.Secret.SecretName } - if len(overrideSource.Secret.Items) > 0 { - mergedVolume.Secret.Items = mergeKeyToPathItems(defaultSource.Secret.Items, overrideSource.Secret.Items) - } + mergedVolume.Secret.Items = mergeKeyToPathItems(defaultSource.Secret.Items, overrideSource.Secret.Items) if overrideSource.Secret.DefaultMode != nil { mergedVolume.Secret.DefaultMode = overrideSource.Secret.DefaultMode } @@ -350,9 +348,7 @@ func mergeVolume(defaultVolume corev1.Volume, overrideVolume corev1.Volume) core if overrideSource.ConfigMap.LocalObjectReference.Name != "" { mergedVolume.ConfigMap.LocalObjectReference.Name = overrideSource.ConfigMap.LocalObjectReference.Name } - if len(overrideSource.ConfigMap.Items) > 0 { - mergedVolume.ConfigMap.Items = mergeKeyToPathItems(defaultSource.ConfigMap.Items, overrideSource.ConfigMap.Items) - } + mergedVolume.ConfigMap.Items = mergeKeyToPathItems(defaultSource.ConfigMap.Items, overrideSource.ConfigMap.Items) if overrideSource.ConfigMap.DefaultMode != nil { mergedVolume.ConfigMap.DefaultMode = overrideSource.ConfigMap.DefaultMode }