Separate PGUpgradeReconciler 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

@@ -257,6 +257,7 @@ func main() {
 
 	// add all PostgreSQL Operator controllers to the runtime manager
 	addControllersToManager(manager, log, registrar)
+	must(pgupgrade.ManagedReconciler(manager, registrar))
 	must(standalone_pgadmin.ManagedReconciler(manager))
 	must(crunchybridgecluster.ManagedReconciler(manager, func() bridge.ClientInterface {
 		return bridgeClient()
@@ -320,16 +321,4 @@ func addControllersToManager(mgr runtime.Manager, log logging.Logger, reg regist
 		log.Error(err, "unable to create PostgresCluster controller")
 		os.Exit(1)
 	}
-
-	upgradeReconciler := &pgupgrade.PGUpgradeReconciler{
-		Client:       mgr.GetClient(),
-		Owner:        naming.ControllerPGUpgrade,
-		Recorder:     mgr.GetEventRecorderFor(naming.ControllerPGUpgrade),
-		Registration: reg,
-	}
-
-	if err := upgradeReconciler.SetupWithManager(mgr); err != nil {
-		log.Error(err, "unable to create PGUpgrade controller")
-		os.Exit(1)
-	}
 }

internal/controller/pgupgrade/apply.go

@@ -11,21 +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
-func (r *PGUpgradeReconciler) patch(
-	ctx context.Context, object client.Object,
-	patch client.Patch, options ...client.PatchOption,
-) error {
-	options = append([]client.PatchOption{r.Owner}, options...)
-	return r.Client.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
 func (r *PGUpgradeReconciler) apply(ctx context.Context, object client.Object) error {
@@ -36,7 +24,7 @@ func (r *PGUpgradeReconciler) apply(ctx context.Context, object client.Object) e
 
 	// 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/pgupgrade/pgupgrade_controller.go

@@ -21,6 +21,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/config"
 	"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/registration"
 	"github.com/crunchydata/postgres-operator/internal/tracing"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
@@ -32,29 +33,49 @@ const (
 
 // PGUpgradeReconciler reconciles a PGUpgrade object
 type PGUpgradeReconciler struct {
-	Client client.Client
-	Owner  client.FieldOwner
-
 	Recorder     record.EventRecorder
 	Registration registration.Registration
+
+	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
+	}
 }
 
 //+kubebuilder:rbac:groups="batch",resources="jobs",verbs={list,watch}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={list,watch}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={list,watch}
 
-// SetupWithManager sets up the controller with the Manager.
-func (r *PGUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewControllerManagedBy(mgr).
+// ManagedReconciler creates a [PGUpgradeReconciler] and adds it to m.
+func ManagedReconciler(m ctrl.Manager, r registration.Registration) error {
+	kubernetes := client.WithFieldOwner(m.GetClient(), naming.ControllerPGUpgrade)
+	recorder := m.GetEventRecorderFor(naming.ControllerPGUpgrade)
+
+	reconciler := &PGUpgradeReconciler{
+		Reader:       kubernetes,
+		Recorder:     recorder,
+		Registration: r,
+		StatusWriter: kubernetes.Status(),
+		Writer:       kubernetes,
+	}
+
+	return ctrl.NewControllerManagedBy(m).
 		For(&v1beta1.PGUpgrade{}).
 		Owns(&batchv1.Job{}).
 		Watches(
 			v1beta1.NewPostgresCluster(),
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, cluster client.Object) []ctrl.Request {
-				return runtime.Requests(r.findUpgradesForPostgresCluster(ctx, client.ObjectKeyFromObject(cluster))...)
+				return runtime.Requests(reconciler.findUpgradesForPostgresCluster(ctx, client.ObjectKeyFromObject(cluster))...)
 			}),
 		).
-		Complete(r)
+		Complete(reconciler)
 }
 
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={list}
@@ -70,7 +91,7 @@ func (r *PGUpgradeReconciler) findUpgradesForPostgresCluster(
 	// namespace, we can configure the [ctrl.Manager] field indexer and pass a
 	// [fields.Selector] here.
 	// - https://book.kubebuilder.io/reference/watching-resources/externally-managed.html
-	if r.Client.List(ctx, &upgrades, &client.ListOptions{
+	if r.Reader.List(ctx, &upgrades, &client.ListOptions{
 		Namespace: cluster.Namespace,
 	}) == nil {
 		for i := range upgrades.Items {
@@ -107,14 +128,14 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// copy before returning from its cache.
 	// - https://github.com/kubernetes-sigs/controller-runtime/issues/1235
 	upgrade := &v1beta1.PGUpgrade{}
-	err = r.Client.Get(ctx, req.NamespacedName, upgrade)
+	err = r.Reader.Get(ctx, req.NamespacedName, upgrade)
 
 	if err == nil {
 		// Write any changes to the upgrade status on the way out.
 		before := upgrade.DeepCopy()
 		defer func() {
 			if !equality.Semantic.DeepEqual(before.Status, upgrade.Status) {
-				status := r.Client.Status().Patch(ctx, upgrade, client.MergeFrom(before), r.Owner)
+				status := r.StatusWriter.Patch(ctx, upgrade, client.MergeFrom(before))
 
 				if err == nil && status != nil {
 					err = status
@@ -409,7 +430,7 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 				// - https://kubernetes.io/docs/concepts/workloads/controllers/job/
 				// - https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/batch/job/strategy.go#L58
 				propagate := client.PropagationPolicy(metav1.DeletePropagationBackground)
-				err = client.IgnoreNotFound(r.Client.Delete(ctx, object, exactly, propagate))
+				err = client.IgnoreNotFound(r.Writer.Delete(ctx, object, exactly, propagate))
 			}
 		}
 
@@ -424,7 +445,7 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			// Set the pgBackRest status for bootstrapping
 			patch.Status.PGBackRest.Repos = []v1beta1.RepoStatus{}
 
-			err = r.Client.Status().Patch(ctx, patch, client.MergeFrom(world.Cluster), r.Owner)
+			err = r.StatusWriter.Patch(ctx, patch, client.MergeFrom(world.Cluster))
 		}
 
 		return ctrl.Result{}, err
@@ -461,7 +482,7 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			uid := object.GetUID()
 			version := object.GetResourceVersion()
 			exactly := client.Preconditions{UID: &uid, ResourceVersion: &version}
-			err = client.IgnoreNotFound(r.Client.Delete(ctx, object, exactly))
+			err = client.IgnoreNotFound(r.Writer.Delete(ctx, object, exactly))
 		}
 
 		// Requeue to verify that Patroni endpoints are deleted

internal/controller/pgupgrade/utils.go

@@ -12,6 +12,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -29,7 +30,7 @@ func (r *PGUpgradeReconciler) setControllerReference(
 	owner *v1beta1.PGUpgrade, controlled client.Object,
 ) {
 	if metav1.GetControllerOf(controlled) != nil {
-		panic(controllerutil.SetControllerReference(owner, controlled, r.Client.Scheme()))
+		panic(controllerutil.SetControllerReference(owner, controlled, runtime.Scheme))
 	}
 
 	controlled.SetOwnerReferences(append(

internal/controller/pgupgrade/world.go

@@ -39,7 +39,7 @@ func (r *PGUpgradeReconciler) observeWorld(
 
 	cluster := v1beta1.NewPostgresCluster()
 	err := errors.WithStack(
-		r.Client.Get(ctx, client.ObjectKey{
+		r.Reader.Get(ctx, client.ObjectKey{
 			Namespace: upgrade.Namespace,
 			Name:      upgrade.Spec.PostgresClusterName,
 		}, cluster))
@@ -48,7 +48,7 @@ func (r *PGUpgradeReconciler) observeWorld(
 	if err == nil {
 		var endpoints corev1.EndpointsList
 		err = errors.WithStack(
-			r.Client.List(ctx, &endpoints,
+			r.Reader.List(ctx, &endpoints,
 				client.InNamespace(upgrade.Namespace),
 				client.MatchingLabelsSelector{Selector: selectCluster},
 			))
@@ -58,7 +58,7 @@ func (r *PGUpgradeReconciler) observeWorld(
 	if err == nil {
 		var jobs batchv1.JobList
 		err = errors.WithStack(
-			r.Client.List(ctx, &jobs,
+			r.Reader.List(ctx, &jobs,
 				client.InNamespace(upgrade.Namespace),
 				client.MatchingLabelsSelector{Selector: selectCluster},
 			))
@@ -70,7 +70,7 @@ func (r *PGUpgradeReconciler) observeWorld(
 	if err == nil {
 		var statefulsets appsv1.StatefulSetList
 		err = errors.WithStack(
-			r.Client.List(ctx, &statefulsets,
+			r.Reader.List(ctx, &statefulsets,
 				client.InNamespace(upgrade.Namespace),
 				client.MatchingLabelsSelector{Selector: selectCluster},
 			))