From 6e1adb13f2df746d4ebf6f5e32d5cafeb0590a54 Mon Sep 17 00:00:00 2001 From: TJ Moore Date: Tue, 14 Jan 2025 17:46:41 -0500 Subject: [PATCH] Reconcile a Service Account for the pgBackRest Repo Host Currently, the pgBackRest repo host uses the 'default' service account. However, EKS's IAM role integration requires a specific annotation to enable this feature. This change adds a new SA for the repo host to allow PGO to reconcile a SA with this annotation, thus allowing the IAM integration to work as expected. fixes #4006 Issue: PGO-2123 --- .../controller/postgrescluster/pgbackrest.go | 49 ++++++++++++++++--- .../postgrescluster/pgbackrest_test.go | 46 +++++++++++++++-- internal/naming/names.go | 9 ++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index d0f2232472..ae68864598 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -122,9 +122,9 @@ type RepoResources struct { // strategy. func (r *Reconciler) applyRepoHostIntent(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoHostName string, repoResources *RepoResources, - observedInstances *observedInstances) (*appsv1.StatefulSet, error) { + observedInstances *observedInstances, saName string) (*appsv1.StatefulSet, error) { - repo, err := r.generateRepoHostIntent(ctx, postgresCluster, repoHostName, repoResources, observedInstances) + repo, err := r.generateRepoHostIntent(ctx, postgresCluster, repoHostName, repoResources, observedInstances, saName) if err != nil { return nil, err } @@ -567,7 +567,7 @@ func (r *Reconciler) setScheduledJobStatus(ctx context.Context, // as needed to create and reconcile a pgBackRest dedicated repository host within the kubernetes // cluster. func (r *Reconciler) generateRepoHostIntent(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - repoHostName string, repoResources *RepoResources, observedInstances *observedInstances, + repoHostName string, repoResources *RepoResources, observedInstances *observedInstances, saName string, ) (*appsv1.StatefulSet, error) { annotations := naming.Merge( @@ -681,6 +681,8 @@ func (r *Reconciler) generateRepoHostIntent(ctx context.Context, postgresCluster repo.Spec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster) + repo.Spec.Template.Spec.ServiceAccountName = saName + pgbackrest.AddServerToRepoPod(ctx, postgresCluster, &repo.Spec.Template.Spec) if pgbackrest.RepoHostVolumeDefined(postgresCluster) { @@ -1380,10 +1382,18 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, return result, nil } + // reconcile the RBAC required to run the pgBackRest Repo Host + repoHostSA, err := r.reconcileRepoHostRBAC(ctx, postgresCluster) + if err != nil { + log.Error(err, "unable to reconcile pgBackRest repo host RBAC") + result.Requeue = true + return result, nil + } + var repoHost *appsv1.StatefulSet var repoHostName string // reconcile the pgbackrest repository host - repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances) + repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances, repoHostSA.GetName()) if err != nil { log.Error(err, "unable to reconcile pgBackRest repo host") result.Requeue = true @@ -2118,12 +2128,39 @@ func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, return sa, nil } +// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={create,patch} + +// reconcileRepoHostRBAC reconciles the ServiceAccount for the pgBackRest repo host +func (r *Reconciler) reconcileRepoHostRBAC(ctx context.Context, + postgresCluster *v1beta1.PostgresCluster) (*corev1.ServiceAccount, error) { + + sa := &corev1.ServiceAccount{ObjectMeta: naming.RepoHostRBAC(postgresCluster)} + sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) + + if err := r.setControllerReference(postgresCluster, sa); err != nil { + return nil, errors.WithStack(err) + } + + sa.Annotations = naming.Merge(postgresCluster.Spec.Metadata.GetAnnotationsOrNil(), + postgresCluster.Spec.Backups.PGBackRest.Metadata.GetAnnotationsOrNil()) + sa.Labels = naming.Merge(postgresCluster.Spec.Metadata.GetLabelsOrNil(), + postgresCluster.Spec.Backups.PGBackRest.Metadata.GetLabelsOrNil(), + naming.PGBackRestLabels(postgresCluster.GetName())) + + if err := r.apply(ctx, sa); err != nil { + return nil, errors.WithStack(err) + } + + return sa, nil +} + // reconcileDedicatedRepoHost is responsible for reconciling a pgBackRest dedicated repository host // StatefulSet according to a specific PostgresCluster custom resource. func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoResources *RepoResources, - observedInstances *observedInstances) (*appsv1.StatefulSet, error) { + observedInstances *observedInstances, + saName string) (*appsv1.StatefulSet, error) { log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") @@ -2164,7 +2201,7 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, } repoHostName := repoResources.hosts[0].Name repoHost, err := r.applyRepoHostIntent(ctx, postgresCluster, repoHostName, repoResources, - observedInstances) + observedInstances, saName) if err != nil { log.Error(err, "reconciling repository host") return nil, err diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index 5b024af643..b3934d0fd1 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -328,6 +328,8 @@ schedulerName: default-scheduler securityContext: fsGroup: 26 fsGroupChangePolicy: OnRootMismatch +serviceAccount: hippocluster-repohost +serviceAccountName: hippocluster-repohost shareProcessNamespace: true terminationGracePeriodSeconds: 30 tolerations: @@ -724,6 +726,42 @@ func TestReconcilePGBackRestRBAC(t *testing.T) { assert.Assert(t, foundSubject) } +func TestReconcileRepoHostRBAC(t *testing.T) { + // Garbage collector cleans up test resources before the test completes + if strings.EqualFold(os.Getenv("USE_EXISTING_CLUSTER"), "true") { + t.Skip("USE_EXISTING_CLUSTER: Test fails due to garbage collection") + } + + ctx := context.Background() + _, tClient := setupKubernetes(t) + require.ParallelCapacity(t, 0) + + r := &Reconciler{Client: tClient, Owner: client.FieldOwner(t.Name())} + + clusterName := "hippocluster" + clusterUID := "hippouid" + + ns := setupNamespace(t, tClient) + + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{ + Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: false}}, + } + + serviceAccount, err := r.reconcileRepoHostRBAC(ctx, postgresCluster) + assert.NilError(t, err) + assert.Assert(t, serviceAccount != nil) + + // verify the service account has been created + sa := &corev1.ServiceAccount{} + err = tClient.Get(ctx, types.NamespacedName{ + Name: naming.RepoHostRBAC(postgresCluster).Name, + Namespace: postgresCluster.GetNamespace(), + }, sa) + assert.NilError(t, err) +} + func TestReconcileStanzaCreate(t *testing.T) { cfg, tClient := setupKubernetes(t) require.ParallelCapacity(t, 0) @@ -2672,12 +2710,12 @@ func TestGenerateRepoHostIntent(t *testing.T) { t.Run("empty", func(t *testing.T) { _, err := r.generateRepoHostIntent(ctx, &v1beta1.PostgresCluster{}, "", &RepoResources{}, - &observedInstances{}) + &observedInstances{}, "") assert.NilError(t, err) }) cluster := &v1beta1.PostgresCluster{} - sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, &observedInstances{}) + sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, &observedInstances{}, "") assert.NilError(t, err) t.Run("ServiceAccount", func(t *testing.T) { @@ -2698,7 +2736,7 @@ func TestGenerateRepoHostIntent(t *testing.T) { }, } observed := &observedInstances{forCluster: []*Instance{{Pods: []*corev1.Pod{{}}}}} - sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, observed) + sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, observed, "") assert.NilError(t, err) assert.Equal(t, *sts.Spec.Replicas, int32(1)) }) @@ -2710,7 +2748,7 @@ func TestGenerateRepoHostIntent(t *testing.T) { }, } observed := &observedInstances{forCluster: []*Instance{{}}} - sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, observed) + sts, err := r.generateRepoHostIntent(ctx, cluster, "", &RepoResources{}, observed, "") assert.NilError(t, err) assert.Equal(t, *sts.Spec.Replicas, int32(0)) }) diff --git a/internal/naming/names.go b/internal/naming/names.go index b07c5b1a59..fc310d837f 100644 --- a/internal/naming/names.go +++ b/internal/naming/names.go @@ -490,6 +490,15 @@ func PGBackRestRBAC(cluster *v1beta1.PostgresCluster) metav1.ObjectMeta { } } +// RepoHostRBAC returns the ObjectMeta necessary to lookup the ServiceAccount for +// the pgBackRest Repo Host +func RepoHostRBAC(cluster *v1beta1.PostgresCluster) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Namespace: cluster.Namespace, + Name: cluster.Name + "-repohost", + } +} + // PGBackRestRepoVolume returns the ObjectMeta for a pgBackRest repository volume func PGBackRestRepoVolume(cluster *v1beta1.PostgresCluster, repoName string) metav1.ObjectMeta {