Separate PGAdminReconciler client concerns

We want to be mindful of our interactions with the Kubernetes API, and
these interfaces will help keep functions focused. These interfaces are
also narrower than client.Reader and client.Writer and may help us keep
RBAC markers accurate.

A new constructor populates these fields with a single client.Client.
The client.WithFieldOwner constructor allows us to drop our Owner field
and patch method.

Author: cbandy

cmd/postgres-operator/main.go

@@ -250,6 +250,7 @@ func main() {
 
 	// add all PostgreSQL Operator controllers to the runtime manager
 	addControllersToManager(manager, log, registrar)
+	must(standalone_pgadmin.ManagedReconciler(manager))
 
 	if features.Enabled(feature.BridgeIdentifiers) {
 		url := os.Getenv("PGO_BRIDGE_URL")
@@ -329,17 +330,6 @@ func addControllersToManager(mgr runtime.Manager, log logging.Logger, reg regist
 		os.Exit(1)
 	}
 
-	pgAdminReconciler := &standalone_pgadmin.PGAdminReconciler{
-		Client:   mgr.GetClient(),
-		Owner:    naming.ControllerPGAdmin,
-		Recorder: mgr.GetEventRecorderFor(naming.ControllerPGAdmin),
-	}
-
-	if err := pgAdminReconciler.SetupWithManager(mgr); err != nil {
-		log.Error(err, "unable to create PGAdmin controller")
-		os.Exit(1)
-	}
-
 	constructor := func() bridge.ClientInterface {
 		client := bridge.NewClient(os.Getenv("PGO_BRIDGE_URL"), versionString)
 		client.Transport = otelTransportWrapper()(http.DefaultTransport)

internal/controller/standalone_pgadmin/apply.go

@@ -11,23 +11,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// patch sends patch to object's endpoint in the Kubernetes API and updates
-// object with any returned content. The fieldManager is set to r.Owner, but
-// can be overridden in options.
-// - https://docs.k8s.io/reference/using-api/server-side-apply/#managers
-//
-// TODO(tjmoore4): This function is duplicated from a version that takes a PostgresCluster object.
-func (r *PGAdminReconciler) patch(
-	ctx context.Context, object client.Object,
-	patch client.Patch, options ...client.PatchOption,
-) error {
-	options = append([]client.PatchOption{r.Owner}, options...)
-	return r.Patch(ctx, object, patch, options...)
-}
-
 // apply sends an apply patch to object's endpoint in the Kubernetes API and
-// updates object with any returned content. The fieldManager is set to
-// r.Owner and the force parameter is true.
+// updates object with any returned content. The fieldManager is set by
+// r.Writer and the force parameter is true.
 // - https://docs.k8s.io/reference/using-api/server-side-apply/#managers
 // - https://docs.k8s.io/reference/using-api/server-side-apply/#conflicts
 //
@@ -40,7 +26,7 @@ func (r *PGAdminReconciler) apply(ctx context.Context, object client.Object) err
 
 	// Send the apply-patch with force=true.
 	if err == nil {
-		err = r.patch(ctx, object, apply, client.ForceOwnership)
+		err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership)
 	}
 
 	return err

internal/controller/standalone_pgadmin/controller.go

@@ -6,6 +6,7 @@ package standalone_pgadmin
 
 import (
 	"context"
+	"errors"
 	"io"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -20,18 +21,30 @@ import (
 
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/logging"
+	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/tracing"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
 // PGAdminReconciler reconciles a PGAdmin object
 type PGAdminReconciler struct {
-	client.Client
-	Owner   client.FieldOwner
 	PodExec func(
 		ctx context.Context, namespace, pod, container string,
 		stdin io.Reader, stdout, stderr io.Writer, command ...string,
 	) error
+
+	Reader interface {
+		Get(context.Context, client.ObjectKey, client.Object, ...client.GetOption) error
+		List(context.Context, client.ObjectList, ...client.ListOption) error
+	}
+	Writer interface {
+		Delete(context.Context, client.Object, ...client.DeleteOption) error
+		Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error
+	}
+	StatusWriter interface {
+		Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error
+	}
+
 	Recorder record.EventRecorder
 }
 
@@ -42,19 +55,21 @@ type PGAdminReconciler struct {
 //+kubebuilder:rbac:groups="",resources="configmaps",verbs={list,watch}
 //+kubebuilder:rbac:groups="apps",resources="statefulsets",verbs={list,watch}
 
-// SetupWithManager sets up the controller with the Manager.
-//
-// TODO(tjmoore4): This function is duplicated from a version that takes a PostgresCluster object.
-func (r *PGAdminReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	if r.PodExec == nil {
-		var err error
-		r.PodExec, err = runtime.NewPodExecutor(mgr.GetConfig())
-		if err != nil {
-			return err
-		}
+// ManagedReconciler creates a [PGAdminReconciler] and adds it to m.
+func ManagedReconciler(m ctrl.Manager) error {
+	exec, err := runtime.NewPodExecutor(m.GetConfig())
+	kubernetes := client.WithFieldOwner(m.GetClient(), naming.ControllerPGAdmin)
+	recorder := m.GetEventRecorderFor(naming.ControllerPGAdmin)
+
+	reconciler := &PGAdminReconciler{
+		PodExec:      exec,
+		Reader:       kubernetes,
+		Recorder:     recorder,
+		StatusWriter: kubernetes.Status(),
+		Writer:       kubernetes,
 	}
 
-	return ctrl.NewControllerManagedBy(mgr).
+	return errors.Join(err, ctrl.NewControllerManagedBy(m).
 		For(&v1beta1.PGAdmin{}).
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.PersistentVolumeClaim{}).
@@ -64,16 +79,16 @@ func (r *PGAdminReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(
 			v1beta1.NewPostgresCluster(),
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, cluster client.Object) []ctrl.Request {
-				return runtime.Requests(r.findPGAdminsForPostgresCluster(ctx, cluster)...)
+				return runtime.Requests(reconciler.findPGAdminsForPostgresCluster(ctx, cluster)...)
 			}),
 		).
 		Watches(
 			&corev1.Secret{},
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, secret client.Object) []ctrl.Request {
-				return runtime.Requests(r.findPGAdminsForSecret(ctx, client.ObjectKeyFromObject(secret))...)
+				return runtime.Requests(reconciler.findPGAdminsForSecret(ctx, client.ObjectKeyFromObject(secret))...)
 			}),
 		).
-		Complete(r)
+		Complete(reconciler))
 }
 
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={get}
@@ -89,7 +104,7 @@ func (r *PGAdminReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	defer span.End()
 
 	pgAdmin := &v1beta1.PGAdmin{}
-	if err := r.Get(ctx, req.NamespacedName, pgAdmin); err != nil {
+	if err := r.Reader.Get(ctx, req.NamespacedName, pgAdmin); err != nil {
 		// NotFound cannot be fixed by requeuing so ignore it. During background
 		// deletion, we receive delete events from pgadmin's dependents after
 		// pgadmin is deleted.
@@ -100,7 +115,7 @@ func (r *PGAdminReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	before := pgAdmin.DeepCopy()
 	defer func() {
 		if !equality.Semantic.DeepEqual(before.Status, pgAdmin.Status) {
-			statusErr := r.Status().Patch(ctx, pgAdmin, client.MergeFrom(before), r.Owner)
+			statusErr := r.StatusWriter.Patch(ctx, pgAdmin, client.MergeFrom(before))
 			if statusErr != nil {
 				log.Error(statusErr, "Patching PGAdmin status")
 			}
@@ -166,7 +181,7 @@ func (r *PGAdminReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 func (r *PGAdminReconciler) setControllerReference(
 	owner *v1beta1.PGAdmin, controlled client.Object,
 ) error {
-	return controllerutil.SetControllerReference(owner, controlled, r.Scheme())
+	return controllerutil.SetControllerReference(owner, controlled, runtime.Scheme)
 }
 
 // deleteControlled safely deletes object when it is controlled by pgAdmin.
@@ -178,7 +193,7 @@ func (r *PGAdminReconciler) deleteControlled(
 		version := object.GetResourceVersion()
 		exactly := client.Preconditions{UID: &uid, ResourceVersion: &version}
 
-		return r.Delete(ctx, object, exactly)
+		return r.Writer.Delete(ctx, object, exactly)
 	}
 
 	return nil

internal/controller/standalone_pgadmin/controller_test.go

@@ -24,7 +24,7 @@ func TestDeleteControlled(t *testing.T) {
 	require.ParallelCapacity(t, 1)
 
 	ns := setupNamespace(t, cc)
-	reconciler := PGAdminReconciler{Client: cc}
+	reconciler := PGAdminReconciler{Writer: cc}
 
 	pgadmin := new(v1beta1.PGAdmin)
 	pgadmin.Namespace = ns.Name

internal/controller/standalone_pgadmin/related.go

@@ -30,7 +30,7 @@ func (r *PGAdminReconciler) findPGAdminsForPostgresCluster(
 	// namespace, we can configure the [manager.Manager] field indexer and pass a
 	// [fields.Selector] here.
 	// - https://book.kubebuilder.io/reference/watching-resources/externally-managed.html
-	if r.List(ctx, &pgadmins, &client.ListOptions{
+	if r.Reader.List(ctx, &pgadmins, &client.ListOptions{
 		Namespace: cluster.GetNamespace(),
 	}) == nil {
 		for i := range pgadmins.Items {
@@ -64,7 +64,7 @@ func (r *PGAdminReconciler) findPGAdminsForSecret(
 	// namespace, we can configure the [manager.Manager] field indexer and pass a
 	// [fields.Selector] here.
 	// - https://book.kubebuilder.io/reference/watching-resources/externally-managed.html
-	if err := r.List(ctx, &pgadmins, &client.ListOptions{
+	if err := r.Reader.List(ctx, &pgadmins, &client.ListOptions{
 		Namespace: secret.Namespace,
 	}); err == nil {
 		for i := range pgadmins.Items {
@@ -93,7 +93,7 @@ func (r *PGAdminReconciler) getClustersForPGAdmin(
 	for _, serverGroup := range pgAdmin.Spec.ServerGroups {
 		var cluster v1beta1.PostgresCluster
 		if serverGroup.PostgresClusterName != "" {
-			err = r.Get(ctx, client.ObjectKey{
+			err = r.Reader.Get(ctx, client.ObjectKey{
 				Name:      serverGroup.PostgresClusterName,
 				Namespace: pgAdmin.GetNamespace(),
 			}, &cluster)
@@ -104,7 +104,7 @@ func (r *PGAdminReconciler) getClustersForPGAdmin(
 		}
 		if selector, err = naming.AsSelector(serverGroup.PostgresClusterSelector); err == nil {
 			var list v1beta1.PostgresClusterList
-			err = r.List(ctx, &list,
+			err = r.Reader.List(ctx, &list,
 				client.InNamespace(pgAdmin.Namespace),
 				client.MatchingLabelsSelector{Selector: selector},
 			)

internal/controller/standalone_pgadmin/related_test.go

@@ -22,7 +22,7 @@ func TestFindPGAdminsForSecret(t *testing.T) {
 	require.ParallelCapacity(t, 0)
 
 	ns := setupNamespace(t, tClient)
-	reconciler := &PGAdminReconciler{Client: tClient}
+	reconciler := &PGAdminReconciler{Reader: tClient}
 
 	secret1 := &corev1.Secret{}
 	secret1.Namespace = ns.Name

internal/controller/standalone_pgadmin/service.go

@@ -36,7 +36,7 @@ func (r *PGAdminReconciler) reconcilePGAdminService(
 	// need to delete any existing service(s). At the start of every reconcile
 	// get all services that match the current pgAdmin labels.
 	services := corev1.ServiceList{}
-	if err := r.List(ctx, &services,
+	if err := r.Reader.List(ctx, &services,
 		client.InNamespace(pgadmin.Namespace),
 		client.MatchingLabels{
 			naming.LabelStandalonePGAdmin: pgadmin.Name,
@@ -62,7 +62,7 @@ func (r *PGAdminReconciler) reconcilePGAdminService(
 	if pgadmin.Spec.ServiceName != "" {
 		// Look for an existing service with name ServiceName in the namespace
 		existingService := &corev1.Service{}
-		err := r.Get(ctx, types.NamespacedName{
+		err := r.Reader.Get(ctx, types.NamespacedName{
 			Name:      pgadmin.Spec.ServiceName,
 			Namespace: pgadmin.GetNamespace(),
 		}, existingService)

internal/controller/standalone_pgadmin/statefulset.go

@@ -34,7 +34,7 @@ func (r *PGAdminReconciler) reconcilePGAdminStatefulSet(
 	// When we delete the StatefulSet, we will leave its Pods in place. They will be claimed by
 	// the StatefulSet that gets created in the next reconcile.
 	existing := &appsv1.StatefulSet{}
-	if err := errors.WithStack(r.Get(ctx, client.ObjectKeyFromObject(sts), existing)); err != nil {
+	if err := errors.WithStack(r.Reader.Get(ctx, client.ObjectKeyFromObject(sts), existing)); err != nil {
 		if !apierrors.IsNotFound(err) {
 			return err
 		}
@@ -47,7 +47,7 @@ func (r *PGAdminReconciler) reconcilePGAdminStatefulSet(
 			exactly := client.Preconditions{UID: &uid, ResourceVersion: &version}
 			propagate := client.PropagationPolicy(metav1.DeletePropagationOrphan)
 
-			return errors.WithStack(client.IgnoreNotFound(r.Delete(ctx, existing, exactly, propagate)))
+			return errors.WithStack(client.IgnoreNotFound(r.Writer.Delete(ctx, existing, exactly, propagate)))
 		}
 	}
 

internal/controller/standalone_pgadmin/statefulset_test.go

@@ -29,8 +29,8 @@ func TestReconcilePGAdminStatefulSet(t *testing.T) {
 	require.ParallelCapacity(t, 1)
 
 	reconciler := &PGAdminReconciler{
-		Client: cc,
-		Owner:  client.FieldOwner(t.Name()),
+		Reader: cc,
+		Writer: client.WithFieldOwner(cc, t.Name()),
 	}
 
 	ns := setupNamespace(t, cc)

internal/controller/standalone_pgadmin/users.go

@@ -53,7 +53,7 @@ func (r *PGAdminReconciler) reconcilePGAdminUsers(ctx context.Context, pgadmin *
 	pod := &corev1.Pod{ObjectMeta: naming.StandalonePGAdmin(pgadmin)}
 	pod.Name += "-0"
 
-	err := errors.WithStack(r.Get(ctx, client.ObjectKeyFromObject(pod), pod))
+	err := errors.WithStack(r.Reader.Get(ctx, client.ObjectKeyFromObject(pod), pod))
 	if err != nil {
 		return client.IgnoreNotFound(err)
 	}
@@ -136,7 +136,7 @@ func (r *PGAdminReconciler) writePGAdminUsers(ctx context.Context, pgadmin *v1be
 
 	existingUserSecret := &corev1.Secret{ObjectMeta: naming.StandalonePGAdmin(pgadmin)}
 	err := errors.WithStack(
-		r.Get(ctx, client.ObjectKeyFromObject(existingUserSecret), existingUserSecret))
+		r.Reader.Get(ctx, client.ObjectKeyFromObject(existingUserSecret), existingUserSecret))
 	if client.IgnoreNotFound(err) != nil {
 		return err
 	}
@@ -186,7 +186,7 @@ cd $PGADMIN_DIR
 			Name:      user.PasswordRef.Name,
 		}}
 		err := errors.WithStack(
-			r.Get(ctx, client.ObjectKeyFromObject(userPasswordSecret), userPasswordSecret))
+			r.Reader.Get(ctx, client.ObjectKeyFromObject(userPasswordSecret), userPasswordSecret))
 		if err != nil {
 			log.Error(err, "Could not get user password secret")
 			continue