Refactor additional volume helpers

internal/controller/postgrescluster/instance.go

@@ -36,6 +36,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
 	"github.com/crunchydata/postgres-operator/internal/tracing"
+	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -1213,7 +1214,7 @@ func (r *Reconciler) reconcileInstance(
 
 	// mount additional volumes to the Postgres instance containers
 	if err == nil && spec.Volumes != nil && len(spec.Volumes.Additional) > 0 {
-		missingContainers := AddAdditionalVolumesToSpecifiedContainers(&instance.Spec.Template, spec.Volumes.Additional)
+		missingContainers := util.AddAdditionalVolumesAndMounts(&instance.Spec.Template.Spec, spec.Volumes.Additional)
 
 		if len(missingContainers) > 0 {
 			r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",

internal/controller/postgrescluster/pgbackrest.go

@@ -721,7 +721,7 @@ func (r *Reconciler) generateRepoHostIntent(ctx context.Context, postgresCluster
 
 	// mount additional volumes to the repo host containers
 	if repoHost != nil && repoHost.Volumes != nil && len(repoHost.Volumes.Additional) > 0 {
-		missingContainers := AddAdditionalVolumesToSpecifiedContainers(&repo.Spec.Template, repoHost.Volumes.Additional)
+		missingContainers := util.AddAdditionalVolumesAndMounts(&repo.Spec.Template.Spec, repoHost.Volumes.Additional)
 
 		if len(missingContainers) > 0 {
 			r.Recorder.Eventf(postgresCluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",
@@ -884,31 +884,15 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
 		jobSpec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster)
 		pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)
 
-		// If the user has specified a PVC to use as a log volume via the PGBackRestCloudLogVolume
-		// annotation, check for the PVC. If we find it, mount it to the backup job.
-		// Otherwise, create a warning event.
+		// Mount the PVC named in the "pgbackrest-cloud-log-volume" annotation, if any.
 		if logVolumeName := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolumeName != "" {
-			logVolume := &corev1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      logVolumeName,
-					Namespace: postgresCluster.GetNamespace(),
-				},
-			}
-			err := errors.WithStack(r.Client.Get(ctx,
-				client.ObjectKeyFromObject(logVolume), logVolume))
-			if err != nil {
-				// PVC not retrieved, create warning event
-				r.Recorder.Event(postgresCluster, corev1.EventTypeWarning, "PGBackRestCloudLogVolumeNotFound", err.Error())
-			} else {
-				// We successfully found the specified PVC, so we will add it to the backup job
-				util.AddVolumeAndMountsToPod(&jobSpec.Template.Spec, logVolume)
-			}
+			util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolumeName)
 		}
 	}
 
 	// mount additional volumes to the job containers
 	if jobs != nil && jobs.Volumes != nil && len(jobs.Volumes.Additional) > 0 {
-		missingContainers := AddAdditionalVolumesToSpecifiedContainers(&jobSpec.Template, jobs.Volumes.Additional)
+		missingContainers := util.AddAdditionalVolumesAndMounts(&jobSpec.Template.Spec, jobs.Volumes.Additional)
 
 		if len(missingContainers) > 0 {
 			r.Recorder.Eventf(postgresCluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",
@@ -1408,7 +1392,7 @@ func (r *Reconciler) generateRestoreJobIntent(cluster *v1beta1.PostgresCluster,
 	job.Spec.Template.Spec.PriorityClassName = initialize.FromPointer(dataSource.PriorityClassName)
 
 	if dataSource.Volumes != nil && len(dataSource.Volumes.Additional) > 0 {
-		missingContainers := AddAdditionalVolumesToSpecifiedContainers(&job.Spec.Template, dataSource.Volumes.Additional)
+		missingContainers := util.AddAdditionalVolumesAndMounts(&job.Spec.Template.Spec, dataSource.Volumes.Additional)
 
 		if len(missingContainers) > 0 {
 			r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",

internal/controller/postgrescluster/pgbackrest_test.go

@@ -2947,96 +2947,14 @@ volumes:
 		})
 	})
 
-	t.Run("CloudLogVolumeAnnotationNoPvc", func(t *testing.T) {
-		recorder := events.NewRecorder(t, runtime.Scheme)
-		r.Recorder = recorder
-
-		cluster.Namespace = ns.Name
-		cluster.Annotations = map[string]string{}
-		cluster.Annotations[naming.PGBackRestCloudLogVolume] = "some-pvc"
-		spec := r.generateBackupJobSpecIntent(ctx,
-			&cluster, v1beta1.PGBackRestRepo{},
-			"",
-			nil, nil,
-		)
-		assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
-containers:
-- command:
-  - /bin/pgbackrest
-  - backup
-  - --stanza=db
-  - --repo=
-  name: pgbackrest
-  resources: {}
-  securityContext:
-    allowPrivilegeEscalation: false
-    capabilities:
-      drop:
-      - ALL
-    privileged: false
-    readOnlyRootFilesystem: true
-    runAsNonRoot: true
-    seccompProfile:
-      type: RuntimeDefault
-  volumeMounts:
-  - mountPath: /etc/pgbackrest/conf.d
-    name: pgbackrest-config
-    readOnly: true
-  - mountPath: /tmp
-    name: tmp
-enableServiceLinks: false
-restartPolicy: Never
-securityContext:
-  fsGroup: 26
-  fsGroupChangePolicy: OnRootMismatch
-volumes:
-- name: pgbackrest-config
-  projected:
-    sources:
-    - configMap:
-        items:
-        - key: pgbackrest_cloud.conf
-          path: pgbackrest_cloud.conf
-        name: hippo-test-pgbackrest-config
-    - secret:
-        items:
-        - key: pgbackrest.ca-roots
-          path: ~postgres-operator/tls-ca.crt
-        - key: pgbackrest-client.crt
-          path: ~postgres-operator/client-tls.crt
-        - key: pgbackrest-client.key
-          mode: 384
-          path: ~postgres-operator/client-tls.key
-        name: hippo-test-pgbackrest
-- emptyDir:
-    sizeLimit: 16Mi
-  name: tmp
-		`))
-
-		assert.Equal(t, len(recorder.Events), 1)
-		assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
-		assert.Equal(t, recorder.Events[0].Reason, "PGBackRestCloudLogVolumeNotFound")
-		assert.Equal(t, recorder.Events[0].Note, "persistentvolumeclaims \"some-pvc\" not found")
-	})
-
-	t.Run("CloudLogVolumeAnnotationPvcInPlace", func(t *testing.T) {
+	t.Run("CloudLogVolumeAnnotation", func(t *testing.T) {
 		recorder := events.NewRecorder(t, runtime.Scheme)
 		r.Recorder = recorder
 
 		cluster.Namespace = ns.Name
 		cluster.Annotations = map[string]string{}
 		cluster.Annotations[naming.PGBackRestCloudLogVolume] = "another-pvc"
 
-		pvc := &corev1.PersistentVolumeClaim{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "another-pvc",
-				Namespace: ns.Name,
-			},
-			Spec: corev1.PersistentVolumeClaimSpec(testVolumeClaimSpec()),
-		}
-		err := r.Client.Create(ctx, pvc)
-		assert.NilError(t, err)
-
 		spec := r.generateBackupJobSpecIntent(ctx,
 			&cluster, v1beta1.PGBackRestRepo{},
 			"",

internal/controller/postgrescluster/pgbouncer.go

@@ -25,6 +25,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/pgbouncer"
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
+	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -476,8 +477,8 @@ func (r *Reconciler) generatePGBouncerDeployment(
 	AddTMPEmptyDir(&deploy.Spec.Template)
 
 	// mount additional volumes to the pgbouncer containers
-	if err == nil && cluster.Spec.Proxy.PGBouncer.Volumes != nil && len(cluster.Spec.Proxy.PGBouncer.Volumes.Additional) > 0 {
-		missingContainers := AddAdditionalVolumesToSpecifiedContainers(&deploy.Spec.Template, cluster.Spec.Proxy.PGBouncer.Volumes.Additional)
+	if volumes := cluster.Spec.Proxy.PGBouncer.Volumes; err == nil && volumes != nil && len(volumes.Additional) > 0 {
+		missingContainers := util.AddAdditionalVolumesAndMounts(&deploy.Spec.Template.Spec, volumes.Additional)
 
 		if len(missingContainers) > 0 {
 			r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",

internal/controller/postgrescluster/util.go

@@ -13,11 +13,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/util/rand"
-	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
-	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
 var tmpDirSizeLimit = resource.MustParse("16Mi")
@@ -287,81 +285,3 @@ func safeHash32(content func(w io.Writer) error) (string, error) {
 	}
 	return rand.SafeEncodeString(fmt.Sprint(hash.Sum32())), nil
 }
-
-// AdditionalVolumeMount returns the name and mount path of the additional volume.
-func AdditionalVolumeMount(name string, readOnly bool) corev1.VolumeMount {
-	return corev1.VolumeMount{
-		Name:      fmt.Sprintf("volumes-%s", name),
-		MountPath: "/volumes/" + name,
-		ReadOnly:  readOnly,
-	}
-}
-
-// AddAdditionalVolumesToSpecifiedContainers adds additional volumes to the specified
-// containers in the specified pod
-// AddAdditionalVolumesToSpecifiedContainers adds the volumes to the pod
-// as `volumes-<additionalVolumeRequest.Name>`
-// and adds the directory to the path `/volumes/<additionalVolumeRequest.Name>`
-func AddAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec,
-	additionalVolumes []v1beta1.AdditionalVolume) []string {
-
-	missingContainers := []string{}
-	for _, additionalVolumeRequest := range additionalVolumes {
-
-		additionalVolumeMount := AdditionalVolumeMount(
-			additionalVolumeRequest.Name,
-			additionalVolumeRequest.ReadOnly,
-		)
-
-		additionalVolume := corev1.Volume{
-			Name: additionalVolumeMount.Name,
-			VolumeSource: corev1.VolumeSource{
-				PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
-					ClaimName: additionalVolumeRequest.ClaimName,
-					ReadOnly:  additionalVolumeMount.ReadOnly,
-				},
-			},
-		}
-
-		// Create a set of all the requested containers,
-		// then in the loops below when we attach the volume to a container,
-		// we can safely remove that container name from the set.
-		// This gives us a way to track the containers that are requested but not found.
-		// This relies on `containers` and `initContainers` together being unique.
-		// - https://github.com/kubernetes/api/blob/b40c1cacbb902b21a7e0c7bf0967321860c1a632/core/v1/types.go#L3895C27-L3896C33
-		names := sets.New(additionalVolumeRequest.Containers...)
-		allContainers := false
-		// If the containers list is omitted, we add the volume to all containers
-		if additionalVolumeRequest.Containers == nil {
-			allContainers = true
-		}
-
-		for i := range template.Spec.Containers {
-			if allContainers || names.Has(template.Spec.Containers[i].Name) {
-				template.Spec.Containers[i].VolumeMounts = append(
-					template.Spec.Containers[i].VolumeMounts,
-					additionalVolumeMount)
-
-				names.Delete(template.Spec.Containers[i].Name)
-			}
-		}
-
-		for i := range template.Spec.InitContainers {
-			if allContainers || names.Has(template.Spec.InitContainers[i].Name) {
-				template.Spec.InitContainers[i].VolumeMounts = append(
-					template.Spec.InitContainers[i].VolumeMounts,
-					additionalVolumeMount)
-
-				names.Delete(template.Spec.InitContainers[i].Name)
-
-			}
-		}
-
-		missingContainers = append(missingContainers, names.UnsortedList()...)
-
-		template.Spec.Volumes = append(
-			template.Spec.Volumes,
-			additionalVolume)
-	}
-	return missingContainers
-}