Run backup command for cloud-based repos in the backup job.

Add/adjust tests for cloud repo backup job changes.

Author: dsessler7

internal/controller/postgrescluster/instance.go

@@ -1176,7 +1176,7 @@ func (r *Reconciler) reconcileInstance(
 	}
 	if err == nil {
 		instanceCertificates, err = r.reconcileInstanceCertificates(
-			ctx, cluster, spec, instance, rootCA)
+			ctx, cluster, spec, instance, rootCA, backupsSpecFound)
 	}
 	if err == nil {
 		postgresDataVolume, err = r.reconcilePostgresDataVolume(ctx, cluster, spec, instance, clusterVolumes, nil)
@@ -1370,10 +1370,8 @@ func addPGBackRestToInstancePodSpec(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
 	instanceCertificates *corev1.Secret, instancePod *corev1.PodSpec,
 ) {
-	if pgbackrest.RepoHostVolumeDefined(cluster) {
-		pgbackrest.AddServerToInstancePod(ctx, cluster, instancePod,
-			instanceCertificates.Name)
-	}
+	pgbackrest.AddServerToInstancePod(ctx, cluster, instancePod,
+		instanceCertificates.Name)
 
 	pgbackrest.AddConfigToInstancePod(cluster, instancePod)
 }
@@ -1422,7 +1420,7 @@ func (r *Reconciler) reconcileInstanceConfigMap(
 func (r *Reconciler) reconcileInstanceCertificates(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
 	spec *v1beta1.PostgresInstanceSetSpec, instance *appsv1.StatefulSet,
-	root *pki.RootCertificateAuthority,
+	root *pki.RootCertificateAuthority, backupsSpecFound bool,
 ) (*corev1.Secret, error) {
 	existing := &corev1.Secret{ObjectMeta: naming.InstanceCertificates(instance)}
 	err := errors.WithStack(client.IgnoreNotFound(
@@ -1465,7 +1463,7 @@ func (r *Reconciler) reconcileInstanceCertificates(
 			root.Certificate, leafCert.Certificate,
 			leafCert.PrivateKey, instanceCerts)
 	}
-	if err == nil {
+	if err == nil && backupsSpecFound {
 		err = pgbackrest.InstanceCertificates(ctx, cluster,
 			root.Certificate, leafCert.Certificate, leafCert.PrivateKey,
 			instanceCerts)

internal/controller/postgrescluster/instance_test.go

@@ -546,49 +546,7 @@ func TestAddPGBackRestToInstancePodSpec(t *testing.T) {
 		},
 	}
 
-	t.Run("NoVolumeRepo", func(t *testing.T) {
-		cluster := cluster.DeepCopy()
-		cluster.Spec.Backups.PGBackRest.Repos = nil
-
-		out := pod.DeepCopy()
-		addPGBackRestToInstancePodSpec(ctx, cluster, &certificates, out)
-
-		// Only Containers and Volumes fields have changed.
-		assert.DeepEqual(t, pod, *out, cmpopts.IgnoreFields(pod, "Containers", "Volumes"))
-
-		// Only database container has mounts.
-		// Other containers are ignored.
-		assert.Assert(t, cmp.MarshalMatches(out.Containers, `
-- name: database
-  resources: {}
-  volumeMounts:
-  - mountPath: /etc/pgbackrest/conf.d
-    name: pgbackrest-config
-    readOnly: true
-- name: other
-  resources: {}
-		`))
-
-		// Instance configuration files but no certificates.
-		// Other volumes are ignored.
-		assert.Assert(t, cmp.MarshalMatches(out.Volumes, `
-- name: other
-- name: postgres-data
-- name: postgres-wal
-- name: pgbackrest-config
-  projected:
-    sources:
-    - configMap:
-        items:
-        - key: pgbackrest_instance.conf
-          path: pgbackrest_instance.conf
-        - key: config-hash
-          path: config-hash
-        name: hippo-pgbackrest-config
-		`))
-	})
-
-	t.Run("OneVolumeRepo", func(t *testing.T) {
+	t.Run("CloudOrVolumeSameBehavior", func(t *testing.T) {
 		alwaysExpect := func(t testing.TB, result *corev1.PodSpec) {
 			// Only Containers and Volumes fields have changed.
 			assert.DeepEqual(t, pod, *result, cmpopts.IgnoreFields(pod, "Containers", "Volumes"))
@@ -637,21 +595,31 @@ func TestAddPGBackRestToInstancePodSpec(t *testing.T) {
 			`))
 		}
 
-		cluster := cluster.DeepCopy()
-		cluster.Spec.Backups.PGBackRest.Repos = []v1beta1.PGBackRestRepo{
+		clusterWithVolume := cluster.DeepCopy()
+		clusterWithVolume.Spec.Backups.PGBackRest.Repos = []v1beta1.PGBackRestRepo{
 			{
 				Name:   "repo1",
 				Volume: new(v1beta1.RepoPVC),
 			},
 		}
 
-		out := pod.DeepCopy()
-		addPGBackRestToInstancePodSpec(ctx, cluster, &certificates, out)
-		alwaysExpect(t, out)
+		clusterWithCloudRepo := cluster.DeepCopy()
+		clusterWithCloudRepo.Spec.Backups.PGBackRest.Repos = []v1beta1.PGBackRestRepo{
+			{
+				Name: "repo1",
+				GCS:  new(v1beta1.RepoGCS),
+			},
+		}
+
+		outWithVolume := pod.DeepCopy()
+		addPGBackRestToInstancePodSpec(ctx, clusterWithVolume, &certificates, outWithVolume)
+		alwaysExpect(t, outWithVolume)
 
-		// The TLS server is added and configuration mounted.
-		// It has PostgreSQL volumes mounted while other volumes are ignored.
-		assert.Assert(t, cmp.MarshalMatches(out.Containers, `
+		outWithCloudRepo := pod.DeepCopy()
+		addPGBackRestToInstancePodSpec(ctx, clusterWithCloudRepo, &certificates, outWithCloudRepo)
+		alwaysExpect(t, outWithCloudRepo)
+
+		outContainers := `
 - name: database
   resources: {}
   volumeMounts:
@@ -739,7 +707,12 @@ func TestAddPGBackRestToInstancePodSpec(t *testing.T) {
   - mountPath: /etc/pgbackrest/conf.d
     name: pgbackrest-config
     readOnly: true
-		`))
+		`
+
+		// The TLS server is added and configuration mounted.
+		// It has PostgreSQL volumes mounted while other volumes are ignored.
+		assert.Assert(t, cmp.MarshalMatches(outWithVolume.Containers, outContainers))
+		assert.Assert(t, cmp.MarshalMatches(outWithCloudRepo.Containers, outContainers))
 
 		t.Run("CustomResources", func(t *testing.T) {
 			cluster := cluster.DeepCopy()
@@ -756,7 +729,7 @@ func TestAddPGBackRestToInstancePodSpec(t *testing.T) {
 				},
 			}
 
-			before := out.DeepCopy()
+			before := outWithVolume.DeepCopy()
 			out := pod.DeepCopy()
 			addPGBackRestToInstancePodSpec(ctx, cluster, &certificates, out)
 			alwaysExpect(t, out)

internal/controller/postgrescluster/pgbackrest.go

@@ -23,7 +23,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
@@ -775,12 +774,7 @@ func (r *Reconciler) generateRepoVolumeIntent(postgresCluster *v1beta1.PostgresC
 // generateBackupJobSpecIntent generates a JobSpec for a pgBackRest backup job
 func generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.PostgresCluster,
 	repo v1beta1.PGBackRestRepo, serviceAccountName string,
-	labels, annotations map[string]string, opts ...string) (*batchv1.JobSpec, error) {
-
-	selector, containerName, err := getPGBackRestExecSelector(postgresCluster, repo)
-	if err != nil {
-		return nil, errors.WithStack(err)
-	}
+	labels, annotations map[string]string, opts ...string) *batchv1.JobSpec {
 
 	repoIndex := regexRepoIndex.FindString(repo.Name)
 	cmdOpts := []string{
@@ -795,21 +789,31 @@ func generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.P
 	cmdOpts = append(cmdOpts, opts...)
 
 	container := corev1.Container{
-		Command: []string{"/opt/crunchy/bin/pgbackrest"},
-		Env: []corev1.EnvVar{
-			{Name: "COMMAND", Value: "backup"},
-			{Name: "COMMAND_OPTS", Value: strings.Join(cmdOpts, " ")},
-			{Name: "COMPARE_HASH", Value: "true"},
-			{Name: "CONTAINER", Value: containerName},
-			{Name: "NAMESPACE", Value: postgresCluster.GetNamespace()},
-			{Name: "SELECTOR", Value: selector.String()},
-		},
 		Image:           config.PGBackRestContainerImage(postgresCluster),
 		ImagePullPolicy: postgresCluster.Spec.ImagePullPolicy,
 		Name:            naming.PGBackRestRepoContainerName,
 		SecurityContext: initialize.RestrictedSecurityContext(),
 	}
 
+	// If the repo that we are backing up to is a local volume, we will configure
+	// the job to use the pgbackrest go binary to exec into the repo host and run
+	// the backup. If the repo is a cloud-based repo, we will run the pgbackrest
+	// backup command directly in the job pod.
+	if repo.Volume != nil {
+		container.Command = []string{"/opt/crunchy/bin/pgbackrest"}
+		container.Env = []corev1.EnvVar{
+			{Name: "COMMAND", Value: "backup"},
+			{Name: "COMMAND_OPTS", Value: strings.Join(cmdOpts, " ")},
+			{Name: "COMPARE_HASH", Value: "true"},
+			{Name: "CONTAINER", Value: naming.PGBackRestRepoContainerName},
+			{Name: "NAMESPACE", Value: postgresCluster.GetNamespace()},
+			{Name: "SELECTOR", Value: naming.PGBackRestDedicatedSelector(postgresCluster.GetName()).String()},
+		}
+	} else {
+		container.Command = []string{"/bin/pgbackrest", "backup"}
+		container.Command = append(container.Command, cmdOpts...)
+	}
+
 	if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil {
 		container.Resources = postgresCluster.Spec.Backups.PGBackRest.Jobs.Resources
 	}
@@ -855,13 +859,16 @@ func generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.P
 	jobSpec.Template.Spec.ImagePullSecrets = postgresCluster.Spec.ImagePullSecrets
 
 	// add pgBackRest configs to template
-	if containerName == naming.PGBackRestRepoContainerName {
+	if repo.Volume != nil {
 		pgbackrest.AddConfigToRepoPod(postgresCluster, &jobSpec.Template.Spec)
 	} else {
-		pgbackrest.AddConfigToInstancePod(postgresCluster, &jobSpec.Template.Spec)
+		// If we are doing a cloud repo backup, we need to give pgbackrest proper permissions
+		// to read certificate files
+		jobSpec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster)
+		pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)
 	}
 
-	return jobSpec, nil
+	return jobSpec
 }
 
 // +kubebuilder:rbac:groups="",resources="configmaps",verbs={delete,list}
@@ -2017,14 +2024,12 @@ func (r *Reconciler) copyConfigurationResources(ctx context.Context, cluster,
 	return nil
 }
 
-// reconcilePGBackRestConfig is responsible for reconciling the pgBackRest ConfigMaps and Secrets.
+// reconcilePGBackRestConfig is responsible for reconciling the pgBackRest ConfigMaps.
 func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context,
 	postgresCluster *v1beta1.PostgresCluster,
 	repoHostName, configHash, serviceName, serviceNamespace string,
 	instanceNames []string) error {
 
-	log := logging.FromContext(ctx).WithValues("reconcileResource", "repoConfig")
-
 	backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName,
 		configHash, serviceName, serviceNamespace, instanceNames)
 	if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig,
@@ -2035,12 +2040,6 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context,
 		return errors.WithStack(err)
 	}
 
-	repoHostConfigured := pgbackrest.RepoHostVolumeDefined(postgresCluster)
-	if !repoHostConfigured {
-		log.V(1).Info("skipping SSH reconciliation, no repo hosts configured")
-		return nil
-	}
-
 	return nil
 }
 
@@ -2442,11 +2441,8 @@ func (r *Reconciler) reconcileManualBackup(ctx context.Context,
 	backupJob.ObjectMeta.Labels = labels
 	backupJob.ObjectMeta.Annotations = annotations
 
-	spec, err := generateBackupJobSpecIntent(ctx, postgresCluster, repo,
+	spec := generateBackupJobSpecIntent(ctx, postgresCluster, repo,
 		serviceAccount.GetName(), labels, annotations, backupOpts...)
-	if err != nil {
-		return errors.WithStack(err)
-	}
 
 	backupJob.Spec = *spec
 
@@ -2535,11 +2531,15 @@ func (r *Reconciler) reconcileReplicaCreateBackup(ctx context.Context,
 		replicaRepoReady = (condition.Status == metav1.ConditionTrue)
 	}
 
-	// get pod name and container name as needed to exec into the proper pod and create
-	// the pgBackRest backup
-	_, containerName, err := getPGBackRestExecSelector(postgresCluster, replicaCreateRepo)
-	if err != nil {
-		return errors.WithStack(err)
+	// TODO: Since we now only exec into the repo host when backing up to a local volume and
+	// run the backup in the job pod when backing up to a cloud-based repo, we should consider
+	// using a different value than the container name for the "pgbackrest-config" annotation
+	// that we attach to these backups
+	var containerName string
+	if replicaCreateRepo.Volume != nil {
+		containerName = naming.PGBackRestRepoContainerName
+	} else {
+		containerName = naming.ContainerDatabase
 	}
 
 	// determine if the dedicated repository host is ready using the repo host ready status
@@ -2591,10 +2591,10 @@ func (r *Reconciler) reconcileReplicaCreateBackup(ctx context.Context,
 		}
 	}
 
-	dedicatedEnabled := pgbackrest.RepoHostVolumeDefined(postgresCluster)
 	// return if no job has been created and the replica repo or the dedicated
 	// repo host is not ready
-	if job == nil && ((dedicatedEnabled && !dedicatedRepoReady) || !replicaRepoReady) {
+	if job == nil && ((pgbackrest.RepoHostVolumeDefined(postgresCluster) && !dedicatedRepoReady) ||
+		!replicaRepoReady) {
 		return nil
 	}
 
@@ -2619,11 +2619,8 @@ func (r *Reconciler) reconcileReplicaCreateBackup(ctx context.Context,
 	backupJob.ObjectMeta.Labels = labels
 	backupJob.ObjectMeta.Annotations = annotations
 
-	spec, err := generateBackupJobSpecIntent(ctx, postgresCluster, replicaCreateRepo,
+	spec := generateBackupJobSpecIntent(ctx, postgresCluster, replicaCreateRepo,
 		serviceAccount.GetName(), labels, annotations)
-	if err != nil {
-		return errors.WithStack(err)
-	}
 
 	backupJob.Spec = *spec
 
@@ -2805,27 +2802,6 @@ func (r *Reconciler) reconcileStanzaCreate(ctx context.Context,
 	return false, nil
 }
 
-// getPGBackRestExecSelector returns a selector and container name that allows the proper
-// Pod (along with a specific container within it) to be found within the Kubernetes
-// cluster as needed to exec into the container and run a pgBackRest command.
-func getPGBackRestExecSelector(postgresCluster *v1beta1.PostgresCluster,
-	repo v1beta1.PGBackRestRepo) (labels.Selector, string, error) {
-
-	var err error
-	var podSelector labels.Selector
-	var containerName string
-
-	if repo.Volume != nil {
-		podSelector = naming.PGBackRestDedicatedSelector(postgresCluster.GetName())
-		containerName = naming.PGBackRestRepoContainerName
-	} else {
-		podSelector, err = naming.AsSelector(naming.ClusterPrimary(postgresCluster.GetName()))
-		containerName = naming.ContainerDatabase
-	}
-
-	return podSelector, containerName, err
-}
-
 // getRepoHostStatus is responsible for returning the pgBackRest status for the
 // provided pgBackRest repository host
 func getRepoHostStatus(repoHost *appsv1.StatefulSet) *v1beta1.RepoHostStatus {
@@ -3070,11 +3046,8 @@ func (r *Reconciler) reconcilePGBackRestCronJob(
 	// set backup type (i.e. "full", "diff", "incr")
 	backupOpts := []string{"--type=" + backupType}
 
-	jobSpec, err := generateBackupJobSpecIntent(ctx, cluster, repo,
+	jobSpec := generateBackupJobSpecIntent(ctx, cluster, repo,
 		serviceAccount.GetName(), labels, annotations, backupOpts...)
-	if err != nil {
-		return errors.WithStack(err)
-	}
 
 	// Suspend cronjobs when shutdown or read-only. Any jobs that have already
 	// started will continue.
@@ -3107,7 +3080,7 @@ func (r *Reconciler) reconcilePGBackRestCronJob(
 
 	// set metadata
 	pgBackRestCronJob.SetGroupVersionKind(batchv1.SchemeGroupVersion.WithKind("CronJob"))
-	err = errors.WithStack(r.setControllerReference(cluster, pgBackRestCronJob))
+	err := errors.WithStack(r.setControllerReference(cluster, pgBackRestCronJob))
 
 	if err == nil {
 		err = r.apply(ctx, pgBackRestCronJob)