Skip to content

Commit bd59153

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 64a2e07 commit bd59153

31 files changed

+417
-441
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)
@@ -154,20 +156,21 @@ func TestServerSideApply(t *testing.T) {
154156
return &sts
155157
}
156158

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

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

168171
default:
169172
assert.NilError(t,
170-
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership, reconciler.Owner))
173+
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership))
171174
}
172175

173176
// Our apply method generates the correct apply-patch.
@@ -187,15 +190,16 @@ func TestServerSideApply(t *testing.T) {
187190
}
188191

189192
t.Run("wrong-keys", func(t *testing.T) {
190-
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
193+
cc := client.WithFieldOwner(cc, t.Name())
194+
reconciler := Reconciler{Writer: cc}
191195

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

195199
// Create the Service.
196200
before := intent.DeepCopy()
197201
assert.NilError(t,
198-
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
202+
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))
199203

200204
// Something external mucks it up.
201205
assert.NilError(t,
@@ -206,7 +210,7 @@ func TestServerSideApply(t *testing.T) {
206210
// client.Apply cannot correct it in old versions of Kubernetes.
207211
after := intent.DeepCopy()
208212
assert.NilError(t,
209-
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
213+
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))
210214

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

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

256261
// Create the Service.
257262
before := intent.DeepCopy()
258263
assert.NilError(t,
259-
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
264+
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))
260265

261266
// Something external mucks it up.
262267
assert.NilError(t,
@@ -267,7 +272,7 @@ func TestServerSideApply(t *testing.T) {
267272
// client.Apply cannot correct it.
268273
after := intent.DeepCopy()
269274
assert.NilError(t,
270-
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
275+
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))
271276

272277
assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector),
273278
"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)