CLOUDP-73217: fix volumes merging #204
Conversation
| 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) | ||
| } |
There was a problem hiding this comment.
Probably unnecessary, but I wanted to also have a "simple" tests that just performs the very basic
| 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) | ||
| } |
There was a problem hiding this comment.
This one tests the case in which the user provides a custom config with one of the values that the operator manages. We expect the original one to be set to nil and to just see the new one
| 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) |
There was a problem hiding this comment.
This one tests the case in which the user provides a custom config with one of the values that the operator do not manage . We expect the original one to be set to nil and to just see the new one
| 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) |
There was a problem hiding this comment.
We are merging the secretSource so we expect it to contain both values
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
Nice job LGTM, everything looks great, just one tiny comment but feel free to ignore! Will leave it up to you 👍
| if len(overrideSource.ConfigMap.Items) > 0 { | ||
| mergedVolume.ConfigMap.Items = mergeKeyToPathItems(defaultSource.ConfigMap.Items, overrideSource.ConfigMap.Items) | ||
| } |
There was a problem hiding this comment.
[nit] can we just always merge without needing to do this check? We can just merged with an empty list
There was a problem hiding this comment.
I actually tried that before I added the if, maybe you can help me out on this. If I remove it the test fails like this:
expected: v1.SecretVolumeSource{SecretName:"Secret-name", Items:[]v1.KeyToPath(nil), DefaultMode:(*int32)(0xc000029430), Optional:(*bool)(nil)} actual : v1.SecretVolumeSource{SecretName:"Secret-name", Items:[]v1.KeyToPath{}, DefaultMode:(*int32)(0xc000029430), Optional:(*bool)(nil)}
There was a problem hiding this comment.
the only difference is
Items:[]v1.KeyToPath(nil)
vs
Items:[]v1.KeyToPath{}
There was a problem hiding this comment.
NVM figured it out :)
This PR fixes the volume merging by manually merging by name volumes and volumesources
As discussed with @chatton, for now we focus on merging the values of VolumeSource for which the operator fills default values (Secret, ConfigMap, EmptyDir) and we overwrite the volume if a different one is provided.
A few new unit tests are introduced as well.
More comments to come inline
All Submissions: