Skip to content

Commit f862748

Browse files
cbandycbandy
committed
Separate postgrescluster.Reconciler 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. This allows `make check` to cover 9% more of the "postgrescluster" package.
1 parent 743dc39 commit f862748

31 files changed

+415
-439
lines changed

‎cmd/postgres-operator/main.go‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ import (
3434
"github.com/crunchydata/postgres-operator/internal/initialize"
3535
"github.com/crunchydata/postgres-operator/internal/kubernetes"
3636
"github.com/crunchydata/postgres-operator/internal/logging"
37-
"github.com/crunchydata/postgres-operator/internal/naming"
3837
"github.com/crunchydata/postgres-operator/internal/registration"
3938
"github.com/crunchydata/postgres-operator/internal/tracing"
4039
"github.com/crunchydata/postgres-operator/internal/upgradecheck"
@@ -256,8 +255,8 @@ func main() {
256255
}
257256

258257
// add all PostgreSQL Operator controllers to the runtime manager
259-
addControllersToManager(manager, log, registrar)
260258
must(pgupgrade.ManagedReconciler(manager, registrar))
259+
must(postgrescluster.ManagedReconciler(manager, registrar))
261260
must(standalone_pgadmin.ManagedReconciler(manager))
262261
must(crunchybridgecluster.ManagedReconciler(manager, func() bridge.ClientInterface {
263262
return bridgeClient()
@@ -306,19 +305,3 @@ func main() {
306305
log.Info("shutdown complete")
307306
}
308307
}
309-
310-
// addControllersToManager adds all PostgreSQL Operator controllers to the provided controller
311-
// runtime manager.
312-
func addControllersToManager(mgr runtime.Manager, log logging.Logger, reg registration.Registration) {
313-
pgReconciler := &postgrescluster.Reconciler{
314-
Client: mgr.GetClient(),
315-
Owner: naming.ControllerPostgresCluster,
316-
Recorder: mgr.GetEventRecorderFor(naming.ControllerPostgresCluster),
317-
Registration: reg,
318-
}
319-
320-
if err := pgReconciler.SetupWithManager(mgr); err != nil {
321-
log.Error(err, "unable to create PostgresCluster controller")
322-
os.Exit(1)
323-
}
324-
}

‎internal/controller/postgrescluster/apply.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ import (
1616
)
1717

1818
// apply sends an apply patch to object's endpoint in the Kubernetes API and
19-
// updates object with any returned content. The fieldManager is set to
20-
// r.Owner and the force parameter is true.
19+
// updates object with any returned content. The fieldManager is set by
20+
// r.Writer and the force parameter is true.
2121
// - https://docs.k8s.io/reference/using-api/server-side-apply/#managers
2222
// - https://docs.k8s.io/reference/using-api/server-side-apply/#conflicts
2323
func (r *Reconciler) apply(ctx context.Context, object client.Object) error {
@@ -32,7 +32,7 @@ func (r *Reconciler) apply(ctx context.Context, object client.Object) error {
3232

3333
// Send the apply-patch with force=true.
3434
if err == nil {
35-
err = r.patch(ctx, object, apply, client.ForceOwnership)
35+
err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership)
3636
}
3737

3838
// Some fields cannot be server-side applied correctly. When their outcome
@@ -44,7 +44,7 @@ func (r *Reconciler) apply(ctx context.Context, object client.Object) error {
4444

4545
// Send the json-patch when necessary.
4646
if err == nil && !patch.IsEmpty() {
47-
err = r.patch(ctx, object, patch)
47+
err = r.Writer.Patch(ctx, object, patch)
4848
}
4949
return err
5050
}

‎internal/controller/postgrescluster/apply_test.go‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ func TestServerSideApply(t *testing.T) {
4444
assert.NilError(t, err)
4545

4646
t.Run("ObjectMeta", func(t *testing.T) {
47-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
47+
cc := client.WithFieldOwner(cc, t.Name())
48+
reconciler := Reconciler{Writer: cc}
4849
constructor := func() *corev1.ConfigMap {
4950
var cm corev1.ConfigMap
5051
cm.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))
@@ -55,15 +56,15 @@ func TestServerSideApply(t *testing.T) {
5556

5657
// Create the object.
5758
before := constructor()
58-
assert.NilError(t, cc.Patch(ctx, before, client.Apply, reconciler.Owner))
59+
assert.NilError(t, cc.Patch(ctx, before, client.Apply))
5960
assert.Assert(t, before.GetResourceVersion() != "")
6061

6162
// Allow the Kubernetes API clock to advance.
6263
time.Sleep(time.Second)
6364

6465
// client.Apply changes the ResourceVersion inadvertently.
6566
after := constructor()
66-
assert.NilError(t, cc.Patch(ctx, after, client.Apply, reconciler.Owner))
67+
assert.NilError(t, cc.Patch(ctx, after, client.Apply))
6768
assert.Assert(t, after.GetResourceVersion() != "")
6869

6970
switch {
@@ -87,7 +88,8 @@ func TestServerSideApply(t *testing.T) {
8788
})
8889

8990
t.Run("ControllerReference", func(t *testing.T) {
90-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
91+
cc := client.WithFieldOwner(cc, t.Name())
92+
reconciler := Reconciler{Writer: cc}
9193

9294
// Setup two possible controllers.
9395
controller1 := new(corev1.ConfigMap)
@@ -115,7 +117,7 @@ func TestServerSideApply(t *testing.T) {
115117
assert.NilError(t,
116118
controllerutil.SetControllerReference(controller2, applied, cc.Scheme()))
117119

118-
err1 := cc.Patch(ctx, applied, client.Apply, client.ForceOwnership, reconciler.Owner)
120+
err1 := cc.Patch(ctx, applied, client.Apply, client.ForceOwnership)
119121

120122
// Patch not accepted; the ownerReferences field is invalid.
121123
assert.Assert(t, apierrors.IsInvalid(err1), "got %#v", err1)
@@ -155,20 +157,21 @@ func TestServerSideApply(t *testing.T) {
155157
return &sts
156158
}
157159

158-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
160+
cc := client.WithFieldOwner(cc, t.Name())
161+
reconciler := Reconciler{Writer: cc}
159162
upstream := constructor("status-upstream")
160163

161164
// The structs defined in "k8s.io/api/apps/v1" marshal empty status fields.
162165
switch {
163166
case serverVersion.LessThan(version.MustParseGeneric("1.22")):
164167
assert.ErrorContains(t,
165-
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership, reconciler.Owner),
168+
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership),
166169
"field not declared in schema",
167170
"expected https://issue.k8s.io/109210")
168171

169172
default:
170173
assert.NilError(t,
171-
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership, reconciler.Owner))
174+
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership))
172175
}
173176

174177
// Our apply method generates the correct apply-patch.
@@ -188,15 +191,16 @@ func TestServerSideApply(t *testing.T) {
188191
}
189192

190193
t.Run("wrong-keys", func(t *testing.T) {
191-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
194+
cc := client.WithFieldOwner(cc, t.Name())
195+
reconciler := Reconciler{Writer: cc}
192196

193197
intent := constructor("some-selector")
194198
intent.Spec.Selector = map[string]string{"k1": "v1"}
195199

196200
// Create the Service.
197201
before := intent.DeepCopy()
198202
assert.NilError(t,
199-
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
203+
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))
200204

201205
// Something external mucks it up.
202206
assert.NilError(t,
@@ -207,7 +211,7 @@ func TestServerSideApply(t *testing.T) {
207211
// client.Apply cannot correct it in old versions of Kubernetes.
208212
after := intent.DeepCopy()
209213
assert.NilError(t,
210-
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
214+
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))
211215

212216
switch {
213217
case serverVersion.LessThan(version.MustParseGeneric("1.22")):
@@ -249,15 +253,16 @@ func TestServerSideApply(t *testing.T) {
249253
{"empty", make(map[string]string)},
250254
} {
251255
t.Run(tt.name, func(t *testing.T) {
252-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
256+
cc := client.WithFieldOwner(cc, t.Name())
257+
reconciler := Reconciler{Writer: cc}
253258

254259
intent := constructor(tt.name + "-selector")
255260
intent.Spec.Selector = tt.selector
256261

257262
// Create the Service.
258263
before := intent.DeepCopy()
259264
assert.NilError(t,
260-
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
265+
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))
261266

262267
// Something external mucks it up.
263268
assert.NilError(t,
@@ -268,7 +273,7 @@ func TestServerSideApply(t *testing.T) {
268273
// client.Apply cannot correct it.
269274
after := intent.DeepCopy()
270275
assert.NilError(t,
271-
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
276+
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))
272277

273278
assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector),
274279
"got %v", after.Spec.Selector)

‎internal/controller/postgrescluster/cluster_test.go‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -82,19 +82,20 @@ func TestCustomLabels(t *testing.T) {
8282
require.ParallelCapacity(t, 2)
8383

8484
reconciler := &Reconciler{
85-
Client: cc,
86-
Owner: client.FieldOwner(t.Name()),
87-
Recorder: new(record.FakeRecorder),
85+
Reader: cc,
86+
Recorder: new(record.FakeRecorder),
87+
StatusWriter: client.WithFieldOwner(cc, t.Name()).Status(),
88+
Writer: client.WithFieldOwner(cc, t.Name()),
8889
}
8990

9091
ns := setupNamespace(t, cc)
9192

9293
reconcileTestCluster := func(cluster *v1beta1.PostgresCluster) {
93-
assert.NilError(t, reconciler.Client.Create(ctx, cluster))
94+
assert.NilError(t, cc.Create(ctx, cluster))
9495
t.Cleanup(func() {
9596
// Remove finalizers, if any, so the namespace can terminate.
9697
assert.Check(t, client.IgnoreNotFound(
97-
reconciler.Client.Patch(ctx, cluster, client.RawPatch(
98+
cc.Patch(ctx, cluster, client.RawPatch(
9899
client.Merge.Type(), []byte(`{"metadata":{"finalizers":[]}}`)))))
99100
})
100101

@@ -168,7 +169,7 @@ func TestCustomLabels(t *testing.T) {
168169
for _, gvk := range gvks {
169170
uList := &unstructured.UnstructuredList{}
170171
uList.SetGroupVersionKind(gvk)
171-
assert.NilError(t, reconciler.Client.List(ctx, uList,
172+
assert.NilError(t, cc.List(ctx, uList,
172173
client.InNamespace(cluster.Namespace),
173174
client.MatchingLabelsSelector{Selector: selector}))
174175

@@ -216,7 +217,7 @@ func TestCustomLabels(t *testing.T) {
216217
for _, gvk := range gvks {
217218
uList := &unstructured.UnstructuredList{}
218219
uList.SetGroupVersionKind(gvk)
219-
assert.NilError(t, reconciler.Client.List(ctx, uList,
220+
assert.NilError(t, cc.List(ctx, uList,
220221
client.InNamespace(cluster.Namespace),
221222
client.MatchingLabelsSelector{Selector: selector}))
222223

@@ -263,7 +264,7 @@ func TestCustomLabels(t *testing.T) {
263264
for _, gvk := range gvks {
264265
uList := &unstructured.UnstructuredList{}
265266
uList.SetGroupVersionKind(gvk)
266-
assert.NilError(t, reconciler.Client.List(ctx, uList,
267+
assert.NilError(t, cc.List(ctx, uList,
267268
client.InNamespace(cluster.Namespace),
268269
client.MatchingLabelsSelector{Selector: selector}))
269270

@@ -298,7 +299,7 @@ func TestCustomLabels(t *testing.T) {
298299
for _, gvk := range gvks {
299300
uList := &unstructured.UnstructuredList{}
300301
uList.SetGroupVersionKind(gvk)
301-
assert.NilError(t, reconciler.Client.List(ctx, uList,
302+
assert.NilError(t, cc.List(ctx, uList,
302303
client.InNamespace(cluster.Namespace),
303304
client.MatchingLabelsSelector{Selector: selector}))
304305

@@ -320,19 +321,20 @@ func TestCustomAnnotations(t *testing.T) {
320321
require.ParallelCapacity(t, 2)
321322

322323
reconciler := &Reconciler{
323-
Client: cc,
324-
Owner: client.FieldOwner(t.Name()),
325-
Recorder: new(record.FakeRecorder),
324+
Reader: cc,
325+
Recorder: new(record.FakeRecorder),
326+
StatusWriter: client.WithFieldOwner(cc, t.Name()).Status(),
327+
Writer: client.WithFieldOwner(cc, t.Name()),
326328
}
327329

328330
ns := setupNamespace(t, cc)
329331

330332
reconcileTestCluster := func(cluster *v1beta1.PostgresCluster) {
331-
assert.NilError(t, reconciler.Client.Create(ctx, cluster))
333+
assert.NilError(t, cc.Create(ctx, cluster))
332334
t.Cleanup(func() {
333335
// Remove finalizers, if any, so the namespace can terminate.
334336
assert.Check(t, client.IgnoreNotFound(
335-
reconciler.Client.Patch(ctx, cluster, client.RawPatch(
337+
cc.Patch(ctx, cluster, client.RawPatch(
336338
client.Merge.Type(), []byte(`{"metadata":{"finalizers":[]}}`)))))
337339
})
338340

@@ -407,7 +409,7 @@ func TestCustomAnnotations(t *testing.T) {
407409
for _, gvk := range gvks {
408410
uList := &unstructured.UnstructuredList{}
409411
uList.SetGroupVersionKind(gvk)
410-
assert.NilError(t, reconciler.Client.List(ctx, uList,
412+
assert.NilError(t, cc.List(ctx, uList,
411413
client.InNamespace(cluster.Namespace),
412414
client.MatchingLabelsSelector{Selector: selector}))
413415

@@ -455,7 +457,7 @@ func TestCustomAnnotations(t *testing.T) {
455457
for _, gvk := range gvks {
456458
uList := &unstructured.UnstructuredList{}
457459
uList.SetGroupVersionKind(gvk)
458-
assert.NilError(t, reconciler.Client.List(ctx, uList,
460+
assert.NilError(t, cc.List(ctx, uList,
459461
client.InNamespace(cluster.Namespace),
460462
client.MatchingLabelsSelector{Selector: selector}))
461463

@@ -502,7 +504,7 @@ func TestCustomAnnotations(t *testing.T) {
502504
for _, gvk := range gvks {
503505
uList := &unstructured.UnstructuredList{}
504506
uList.SetGroupVersionKind(gvk)
505-
assert.NilError(t, reconciler.Client.List(ctx, uList,
507+
assert.NilError(t, cc.List(ctx, uList,
506508
client.InNamespace(cluster.Namespace),
507509
client.MatchingLabelsSelector{Selector: selector}))
508510

@@ -537,7 +539,7 @@ func TestCustomAnnotations(t *testing.T) {
537539
for _, gvk := range gvks {
538540
uList := &unstructured.UnstructuredList{}
539541
uList.SetGroupVersionKind(gvk)
540-
assert.NilError(t, reconciler.Client.List(ctx, uList,
542+
assert.NilError(t, cc.List(ctx, uList,
541543
client.InNamespace(cluster.Namespace),
542544
client.MatchingLabelsSelector{Selector: selector}))
543545

@@ -554,10 +556,7 @@ func TestCustomAnnotations(t *testing.T) {
554556
}
555557

556558
func TestGenerateClusterPrimaryService(t *testing.T) {
557-
_, cc := setupKubernetes(t)
558-
require.ParallelCapacity(t, 0)
559-
560-
reconciler := &Reconciler{Client: cc}
559+
reconciler := &Reconciler{}
561560

562561
cluster := &v1beta1.PostgresCluster{}
563562
cluster.Namespace = "ns2"
@@ -658,7 +657,7 @@ func TestReconcileClusterPrimaryService(t *testing.T) {
658657
_, cc := setupKubernetes(t)
659658
require.ParallelCapacity(t, 1)
660659

661-
reconciler := &Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
660+
reconciler := &Reconciler{Writer: client.WithFieldOwner(cc, t.Name())}
662661

663662
cluster := testCluster()
664663
cluster.Namespace = setupNamespace(t, cc).Name
@@ -676,10 +675,7 @@ func TestReconcileClusterPrimaryService(t *testing.T) {
676675
}
677676

678677
func TestGenerateClusterReplicaServiceIntent(t *testing.T) {
679-
_, cc := setupKubernetes(t)
680-
require.ParallelCapacity(t, 0)
681-
682-
reconciler := &Reconciler{Client: cc}
678+
reconciler := &Reconciler{}
683679

684680
cluster := &v1beta1.PostgresCluster{}
685681
cluster.Namespace = "ns1"

0 commit comments

Comments
 (0)