From 918823dbf1c78e5460f83af50bf85be6c1aefafe Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Wed, 28 May 2025 18:12:53 +0200 Subject: [PATCH] fix: do not add barman-certificates projection if not needed (#354) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #347 #364 Signed-off-by: Marco Nenciarini Signed-off-by: Armando Ruocco Signed-off-by: Niccolò Fei Co-authored-by: Armando Ruocco Co-authored-by: Niccolò Fei --- internal/cnpgi/operator/lifecycle.go | 110 +++++++++++++------ internal/cnpgi/operator/lifecycle_test.go | 123 ++++++++++++++++++++++ 2 files changed, 199 insertions(+), 34 deletions(-) diff --git a/internal/cnpgi/operator/lifecycle.go b/internal/cnpgi/operator/lifecycle.go index 308c9e7..f2a7307 100644 --- a/internal/cnpgi/operator/lifecycle.go +++ b/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,13 +369,15 @@ 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{ @@ -382,6 +385,13 @@ func reconcilePodSpec( }, }, }) + } 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 diff --git a/internal/cnpgi/operator/lifecycle_test.go b/internal/cnpgi/operator/lifecycle_test.go index 3d94a3b..c3235de 100644 --- a/internal/cnpgi/operator/lifecycle_test.go +++ b/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")) + }) + }) +})