fix: do not add barman-certificates projection if not needed (#354)

Closes #347 #364

Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
Co-authored-by: Armando Ruocco <[email protected]>
Co-authored-by: Niccolò Fei <[email protected]>

Author: 3 people

internal/cnpgi/operator/lifecycle.go

@@ -333,6 +333,7 @@ func reconcilePodSpec(
 			Drop: []corev1.Capability{"ALL"},
 		},
 	}
+	sidecarTemplate.RestartPolicy = ptr.To(corev1.ContainerRestartPolicyAlways)
 	sidecarTemplate.Resources = config.resources
 
 	// merge the main container envs if they aren't already set
@@ -368,20 +369,29 @@ func reconcilePodSpec(
 		}
 	}
 
-	if err := injectPluginSidecarPodSpec(spec, &sidecarTemplate, mainContainerName); err != nil {
-		return err
-	}
+	if len(config.certificates) > 0 {
+		sidecarTemplate.VolumeMounts = ensureVolumeMount(
+			sidecarTemplate.VolumeMounts,
+			corev1.VolumeMount{
+				Name:      barmanCertificatesVolumeName,
+				MountPath: metadata.BarmanCertificatesPath,
+			})
 
-	// inject the volume containing the certificates if needed
-	if !volumeListHasVolume(spec.Volumes, barmanCertificatesVolumeName) {
-		spec.Volumes = append(spec.Volumes, corev1.Volume{
+		spec.Volumes = ensureVolume(spec.Volumes, corev1.Volume{
 			Name: barmanCertificatesVolumeName,
 			VolumeSource: corev1.VolumeSource{
 				Projected: &corev1.ProjectedVolumeSource{
 					Sources: config.certificates,
 				},
 			},
 		})
+	} else {
+		sidecarTemplate.VolumeMounts = removeVolumeMount(sidecarTemplate.VolumeMounts, barmanCertificatesVolumeName)
+		spec.Volumes = removeVolume(spec.Volumes, barmanCertificatesVolumeName)
+	}
+
+	if err := injectPluginSidecarPodSpec(spec, &sidecarTemplate, mainContainerName); err != nil {
+		return err
 	}
 
 	return nil
@@ -407,7 +417,7 @@ func InjectPluginVolumePodSpec(spec *corev1.PodSpec, mainContainerName string) {
 		return
 	}
 
-	spec.Volumes = append(spec.Volumes, corev1.Volume{
+	spec.Volumes = ensureVolume(spec.Volumes, corev1.Volume{
 		Name: pluginVolumeName,
 		VolumeSource: corev1.VolumeSource{
 			EmptyDir: &corev1.EmptyDirVolumeSource{},
@@ -416,7 +426,7 @@ func InjectPluginVolumePodSpec(spec *corev1.PodSpec, mainContainerName string) {
 
 	for i := range spec.Containers {
 		if spec.Containers[i].Name == mainContainerName {
-			spec.Containers[i].VolumeMounts = append(
+			spec.Containers[i].VolumeMounts = ensureVolumeMount(
 				spec.Containers[i].VolumeMounts,
 				corev1.VolumeMount{
 					Name:      pluginVolumeName,
@@ -428,10 +438,6 @@ func InjectPluginVolumePodSpec(spec *corev1.PodSpec, mainContainerName string) {
 }
 
 // injectPluginSidecarPodSpec injects a plugin sidecar into a CNPG Pod spec.
-//
-// If the "injectMainContainerVolumes" flag is true, this will append all the volume
-// mounts that are used in the instance manager Pod to the passed sidecar
-// container, granting it superuser access to the PostgreSQL instance.
 func injectPluginSidecarPodSpec(
 	spec *corev1.PodSpec,
 	sidecar *corev1.Container,
@@ -440,12 +446,11 @@ func injectPluginSidecarPodSpec(
 	sidecar = sidecar.DeepCopy()
 	InjectPluginVolumePodSpec(spec, mainContainerName)
 
-	var volumeMounts []corev1.VolumeMount
 	sidecarContainerFound := false
 	mainContainerFound := false
 	for i := range spec.Containers {
 		if spec.Containers[i].Name == mainContainerName {
-			volumeMounts = spec.Containers[i].VolumeMounts
+			sidecar.VolumeMounts = ensureVolumeMount(sidecar.VolumeMounts, spec.Containers[i].VolumeMounts...)
 			mainContainerFound = true
 		}
 	}
@@ -457,38 +462,75 @@ func injectPluginSidecarPodSpec(
 	for i := range spec.InitContainers {
 		if spec.InitContainers[i].Name == sidecar.Name {
 			sidecarContainerFound = true
+			spec.InitContainers[i] = *sidecar
 		}
 	}
 
-	if sidecarContainerFound {
-		// The sidecar container was already added
-		return nil
+	if !sidecarContainerFound {
+		spec.InitContainers = append(spec.InitContainers, *sidecar)
 	}
 
-	// Do not modify the passed sidecar definition
-	sidecar.VolumeMounts = append(
-		sidecar.VolumeMounts,
-		corev1.VolumeMount{
-			Name:      barmanCertificatesVolumeName,
-			MountPath: metadata.BarmanCertificatesPath,
-		})
-	sidecar.VolumeMounts = append(sidecar.VolumeMounts, volumeMounts...)
-	sidecar.RestartPolicy = ptr.To(corev1.ContainerRestartPolicyAlways)
-	spec.InitContainers = append(spec.InitContainers, *sidecar)
-
 	return nil
 }
 
-// volumeListHasVolume check if a volume with a known name exists
-// in the volume list
-func volumeListHasVolume(volumes []corev1.Volume, name string) bool {
+// ensureVolume makes sure the passed volume is present in the list of volumes.
+// If the volume is already present, it is updated.
+func ensureVolume(volumes []corev1.Volume, volume corev1.Volume) []corev1.Volume {
+	volumeFound := false
 	for i := range volumes {
-		if volumes[i].Name == name {
-			return true
+		if volumes[i].Name == volume.Name {
+			volumeFound = true
+			volumes[i] = volume
 		}
 	}
 
-	return false
+	if !volumeFound {
+		volumes = append(volumes, volume)
+	}
+
+	return volumes
+}
+
+// ensureVolumeMount makes sure the passed volume mounts are present in the list of volume mounts.
+// If a volume mount is already present, it is updated.
+func ensureVolumeMount(mounts []corev1.VolumeMount, volumeMounts ...corev1.VolumeMount) []corev1.VolumeMount {
+	for _, mount := range volumeMounts {
+		mountFound := false
+		for i := range mounts {
+			if mounts[i].Name == mount.Name {
+				mountFound = true
+				mounts[i] = mount
+				break
+			}
+		}
+
+		if !mountFound {
+			mounts = append(mounts, mount)
+		}
+	}
+
+	return mounts
+}
+
+// removeVolume removes a volume with a known name from a list of volumes.
+func removeVolume(volumes []corev1.Volume, name string) []corev1.Volume {
+	var filteredVolumes []corev1.Volume
+	for _, volume := range volumes {
+		if volume.Name != name {
+			filteredVolumes = append(filteredVolumes, volume)
+		}
+	}
+	return filteredVolumes
+}
+
+func removeVolumeMount(mounts []corev1.VolumeMount, name string) []corev1.VolumeMount {
+	var filteredMounts []corev1.VolumeMount
+	for _, mount := range mounts {
+		if mount.Name != name {
+			filteredMounts = append(filteredMounts, mount)
+		}
+	}
+	return filteredMounts
 }
 
 // getCNPGJobRole gets the role associated to a CNPG job

internal/cnpgi/operator/lifecycle_test.go

@@ -209,3 +209,126 @@ var _ = Describe("LifecycleImplementation", func() {
 		})
 	})
 })
+
+var _ = Describe("Volume utilities", func() {
+	Describe("ensureVolume", func() {
+		It("adds a new volume if not present", func() {
+			volumes := []corev1.Volume{{Name: "vol1"}}
+			newVol := corev1.Volume{Name: "vol2"}
+			result := ensureVolume(volumes, newVol)
+			Expect(result).To(HaveLen(2))
+			Expect(result[1]).To(Equal(newVol))
+		})
+
+		It("updates an existing volume", func() {
+			volumes := []corev1.Volume{{Name: "vol1"}}
+			updatedVol := corev1.Volume{Name: "vol1", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}
+			result := ensureVolume(volumes, updatedVol)
+			Expect(result).To(HaveLen(1))
+			Expect(result[0]).To(Equal(updatedVol))
+		})
+	})
+
+	Describe("removeVolume", func() {
+		It("removes the specified volume", func() {
+			volumes := []corev1.Volume{
+				{Name: "vol1"},
+				{Name: "vol2"},
+				{Name: "vol3"},
+			}
+			result := removeVolume(volumes, "vol2")
+			Expect(result).To(HaveLen(2))
+			for _, v := range result {
+				Expect(v.Name).NotTo(Equal("vol2"))
+			}
+		})
+
+		It("returns the same list if the volume is not present", func() {
+			volumes := []corev1.Volume{{Name: "vol1"}, {Name: "vol2"}}
+			result := removeVolume(volumes, "vol3")
+			Expect(result).To(Equal(volumes))
+		})
+	})
+
+	Describe("removeVolumeMount", func() {
+		It("removes the specified volume mount", func() {
+			mounts := []corev1.VolumeMount{
+				{Name: "mount1"},
+				{Name: "mount2"},
+				{Name: "mount3"},
+			}
+			result := removeVolumeMount(mounts, "mount2")
+			Expect(result).To(HaveLen(2))
+			for _, m := range result {
+				Expect(m.Name).NotTo(Equal("mount2"))
+			}
+		})
+
+		It("returns the same list if the volume mount is not present", func() {
+			mounts := []corev1.VolumeMount{{Name: "mount1"}, {Name: "mount2"}}
+			result := removeVolumeMount(mounts, "mount3")
+			Expect(result).To(HaveLen(2))
+			Expect(result[0].Name).To(Equal("mount1"))
+			Expect(result[1].Name).To(Equal("mount2"))
+		})
+
+		It("handles empty input slice", func() {
+			var mounts []corev1.VolumeMount
+			result := removeVolumeMount(mounts, "mount1")
+			Expect(result).To(BeEmpty())
+		})
+	})
+
+	Describe("ensureVolumeMount", func() {
+		It("adds a new volume mount to an empty list", func() {
+			var mounts []corev1.VolumeMount
+			newMount := corev1.VolumeMount{Name: "mount1", MountPath: "/path1"}
+			result := ensureVolumeMount(mounts, newMount)
+			Expect(result).To(HaveLen(1))
+			Expect(result[0]).To(Equal(newMount))
+		})
+
+		It("adds a new volume mount to a non-empty list", func() {
+			mounts := []corev1.VolumeMount{{Name: "mount1", MountPath: "/path1"}}
+			newMount := corev1.VolumeMount{Name: "mount2", MountPath: "/path2"}
+			result := ensureVolumeMount(mounts, newMount)
+			Expect(result).To(HaveLen(2))
+			Expect(result[0].Name).To(Equal("mount1"))
+			Expect(result[1].Name).To(Equal("mount2"))
+		})
+
+		It("updates an existing volume mount", func() {
+			mounts := []corev1.VolumeMount{{Name: "mount1", MountPath: "/path1"}}
+			updatedMount := corev1.VolumeMount{Name: "mount1", MountPath: "/new-path"}
+			result := ensureVolumeMount(mounts, updatedMount)
+			Expect(result).To(HaveLen(1))
+			Expect(result[0].MountPath).To(Equal("/new-path"))
+		})
+
+		It("adds multiple new volume mounts", func() {
+			mounts := []corev1.VolumeMount{{Name: "mount1", MountPath: "/path1"}}
+			newMount1 := corev1.VolumeMount{Name: "mount2", MountPath: "/path2"}
+			newMount2 := corev1.VolumeMount{Name: "mount3", MountPath: "/path3"}
+			result := ensureVolumeMount(mounts, newMount1, newMount2)
+			Expect(result).To(HaveLen(3))
+			Expect(result[0].Name).To(Equal("mount1"))
+			Expect(result[1].Name).To(Equal("mount2"))
+			Expect(result[2].Name).To(Equal("mount3"))
+		})
+
+		It("handles a mix of new and existing volume mounts", func() {
+			mounts := []corev1.VolumeMount{
+				{Name: "mount1", MountPath: "/path1"},
+				{Name: "mount2", MountPath: "/path2"},
+			}
+			updatedMount := corev1.VolumeMount{Name: "mount1", MountPath: "/new-path"}
+			newMount := corev1.VolumeMount{Name: "mount3", MountPath: "/path3"}
+			result := ensureVolumeMount(mounts, updatedMount, newMount)
+			Expect(result).To(HaveLen(3))
+			Expect(result[0].Name).To(Equal("mount1"))
+			Expect(result[0].MountPath).To(Equal("/new-path"))
+			Expect(result[1].Name).To(Equal("mount2"))
+			Expect(result[2].Name).To(Equal("mount3"))
+		})
+	})
+})