From bc74b1ee134da9aab2dd4733653f9fae4f1be7dd Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 13 Nov 2024 15:42:26 -0600 Subject: [PATCH 1/2] Remove server-side apply tests for old Kubernetes Kubernetes 1.22 and OpenShift 4.8 have been out of commission for some time now. --- internal/controller/postgrescluster/apply.go | 3 +- .../controller/postgrescluster/apply_test.go | 94 +------------------ .../postgrescluster/controller_test.go | 76 +++++---------- .../controller/postgrescluster/suite_test.go | 27 +----- 4 files changed, 29 insertions(+), 171 deletions(-) diff --git a/internal/controller/postgrescluster/apply.go b/internal/controller/postgrescluster/apply.go index 88659cf396..ab7c238716 100644 --- a/internal/controller/postgrescluster/apply.go +++ b/internal/controller/postgrescluster/apply.go @@ -54,8 +54,7 @@ func (r *Reconciler) apply(ctx context.Context, object client.Object) error { func applyServiceSpec( patch *kubeapi.JSON6902, actual, intent corev1.ServiceSpec, path ...string, ) { - // Service.Spec.Selector is not +mapType=atomic until Kubernetes 1.22. - // - https://issue.k8s.io/97970 + // Service.Spec.Selector cannot be unset; perhaps https://issue.k8s.io/117447 if !equality.Semantic.DeepEqual(actual.Selector, intent.Selector) { patch.Replace(append(path, "selector")...)(intent.Selector) } diff --git a/internal/controller/postgrescluster/apply_test.go b/internal/controller/postgrescluster/apply_test.go index e06f290590..413ffbd135 100644 --- a/internal/controller/postgrescluster/apply_test.go +++ b/internal/controller/postgrescluster/apply_test.go @@ -14,7 +14,6 @@ import ( "github.com/google/go-cmp/cmp" "gotest.tools/v3/assert" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -144,41 +143,6 @@ func TestServerSideApply(t *testing.T) { ) }) - t.Run("StatefulSetStatus", func(t *testing.T) { - constructor := func(name string) *appsv1.StatefulSet { - var sts appsv1.StatefulSet - sts.SetGroupVersionKind(appsv1.SchemeGroupVersion.WithKind("StatefulSet")) - sts.Namespace, sts.Name = ns.Name, name - sts.Spec.Selector = &metav1.LabelSelector{ - MatchLabels: map[string]string{"select": name}, - } - sts.Spec.Template.Labels = map[string]string{"select": name} - sts.Spec.Template.Spec.Containers = []corev1.Container{{Name: "test", Image: "test"}} - return &sts - } - - cc := client.WithFieldOwner(cc, t.Name()) - reconciler := Reconciler{Writer: cc} - upstream := constructor("status-upstream") - - // The structs defined in "k8s.io/api/apps/v1" marshal empty status fields. - switch { - case serverVersion.LessThan(version.MustParseGeneric("1.22")): - assert.ErrorContains(t, - cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership), - "field not declared in schema", - "expected https://issue.k8s.io/109210") - - default: - assert.NilError(t, - cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership)) - } - - // Our apply method generates the correct apply-patch. - again := constructor("status-local") - assert.NilError(t, reconciler.apply(ctx, again)) - }) - t.Run("ServiceSelector", func(t *testing.T) { constructor := func(name string) *corev1.Service { var service corev1.Service @@ -190,61 +154,6 @@ func TestServerSideApply(t *testing.T) { return &service } - t.Run("wrong-keys", func(t *testing.T) { - cc := client.WithFieldOwner(cc, t.Name()) - reconciler := Reconciler{Writer: cc} - - intent := constructor("some-selector") - intent.Spec.Selector = map[string]string{"k1": "v1"} - - // Create the Service. - before := intent.DeepCopy() - assert.NilError(t, - cc.Patch(ctx, before, client.Apply, client.ForceOwnership)) - - // Something external mucks it up. - assert.NilError(t, - cc.Patch(ctx, before, - client.RawPatch(client.Merge.Type(), []byte(`{"spec":{"selector":{"bad":"v2"}}}`)), - client.FieldOwner("wrong"))) - - // client.Apply cannot correct it in old versions of Kubernetes. - after := intent.DeepCopy() - assert.NilError(t, - cc.Patch(ctx, after, client.Apply, client.ForceOwnership)) - - switch { - case serverVersion.LessThan(version.MustParseGeneric("1.22")): - - assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector), - "expected https://issue.k8s.io/97970, got %v", after.Spec.Selector) - - default: - assert.DeepEqual(t, after.Spec.Selector, intent.Spec.Selector) - } - - // Our apply method corrects it. - again := intent.DeepCopy() - assert.NilError(t, reconciler.apply(ctx, again)) - assert.DeepEqual(t, again.Spec.Selector, intent.Spec.Selector) - - var count int - var managed *metav1.ManagedFieldsEntry - for i := range again.ManagedFields { - if again.ManagedFields[i].Manager == t.Name() { - count++ - managed = &again.ManagedFields[i] - } - } - - assert.Equal(t, count, 1, "expected manager once in %v", again.ManagedFields) - assert.Equal(t, managed.Operation, metav1.ManagedFieldsOperationApply) - - assert.Assert(t, managed.FieldsV1 != nil) - assert.Assert(t, strings.Contains(string(managed.FieldsV1.Raw), `"f:selector":{`), - "expected f:selector in %s", managed.FieldsV1.Raw) - }) - for _, tt := range []struct { name string selector map[string]string @@ -275,6 +184,9 @@ func TestServerSideApply(t *testing.T) { assert.NilError(t, cc.Patch(ctx, after, client.Apply, client.ForceOwnership)) + // Perhaps one of: + // - https://issue.k8s.io/117447 + // - https://github.com/kubernetes-sigs/structured-merge-diff/issues/259 assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector), "got %v", after.Spec.Selector) diff --git a/internal/controller/postgrescluster/controller_test.go b/internal/controller/postgrescluster/controller_test.go index a6f237b81a..987e5329ca 100644 --- a/internal/controller/postgrescluster/controller_test.go +++ b/internal/controller/postgrescluster/controller_test.go @@ -21,7 +21,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" - "k8s.io/apimachinery/pkg/util/version" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -342,59 +341,32 @@ spec: // // The "metadata.finalizers" field is also okay. // - https://book.kubebuilder.io/reference/using-finalizers.html - // - // NOTE(cbandy): Kubernetes prior to v1.16.10 and v1.17.6 does not track - // managed fields on the status subresource: https://issue.k8s.io/88901 - switch { - case suite.ServerVersion.LessThan(version.MustParseGeneric("1.22")): - - // Kubernetes 1.22 began tracking subresources in managed fields. - // - https://pr.k8s.io/100970 - Expect(existing.ManagedFields).To(ContainElement( - MatchFields(IgnoreExtras, Fields{ - "Manager": Equal(test.Owner), - "FieldsV1": PointTo(MatchAllFields(Fields{ - "Raw": WithTransform(func(in []byte) (out map[string]any) { - Expect(yaml.Unmarshal(in, &out)).To(Succeed()) - return out - }, MatchAllKeys(Keys{ - "f:metadata": MatchAllKeys(Keys{ - "f:finalizers": Not(BeZero()), - }), - "f:status": Not(BeZero()), - })), - })), - }), - ), `controller should manage only "finalizers" and "status"`) - - default: - Expect(existing.ManagedFields).To(ContainElements( - MatchFields(IgnoreExtras, Fields{ - "Manager": Equal(test.Owner), - "FieldsV1": PointTo(MatchAllFields(Fields{ - "Raw": WithTransform(func(in []byte) (out map[string]any) { - Expect(yaml.Unmarshal(in, &out)).To(Succeed()) - return out - }, MatchAllKeys(Keys{ - "f:metadata": MatchAllKeys(Keys{ - "f:finalizers": Not(BeZero()), - }), - })), + Expect(existing.ManagedFields).To(ContainElements( + MatchFields(IgnoreExtras, Fields{ + "Manager": Equal(test.Owner), + "FieldsV1": PointTo(MatchAllFields(Fields{ + "Raw": WithTransform(func(in []byte) (out map[string]any) { + Expect(yaml.Unmarshal(in, &out)).To(Succeed()) + return out + }, MatchAllKeys(Keys{ + "f:metadata": MatchAllKeys(Keys{ + "f:finalizers": Not(BeZero()), + }), })), - }), - MatchFields(IgnoreExtras, Fields{ - "Manager": Equal(test.Owner), - "FieldsV1": PointTo(MatchAllFields(Fields{ - "Raw": WithTransform(func(in []byte) (out map[string]any) { - Expect(yaml.Unmarshal(in, &out)).To(Succeed()) - return out - }, MatchAllKeys(Keys{ - "f:status": Not(BeZero()), - })), + })), + }), + MatchFields(IgnoreExtras, Fields{ + "Manager": Equal(test.Owner), + "FieldsV1": PointTo(MatchAllFields(Fields{ + "Raw": WithTransform(func(in []byte) (out map[string]any) { + Expect(yaml.Unmarshal(in, &out)).To(Succeed()) + return out + }, MatchAllKeys(Keys{ + "f:status": Not(BeZero()), })), - }), - ), `controller should manage only "finalizers" and "status"`) - } + })), + }), + ), `controller should manage only "finalizers" and "status"`) }) Specify("Patroni Distributed Configuration", func() { diff --git a/internal/controller/postgrescluster/suite_test.go b/internal/controller/postgrescluster/suite_test.go index ffb9d6f1eb..f109839028 100644 --- a/internal/controller/postgrescluster/suite_test.go +++ b/internal/controller/postgrescluster/suite_test.go @@ -6,18 +6,12 @@ package postgrescluster import ( "context" - "os" - "strings" "testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/util/version" - "k8s.io/client-go/discovery" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" "github.com/crunchydata/postgres-operator/internal/logging" "github.com/crunchydata/postgres-operator/internal/testing/require" @@ -25,11 +19,6 @@ import ( var suite struct { Client client.Client - Config *rest.Config - - ServerVersion *version.Version - - Manager manager.Manager } func TestAPIs(t *testing.T) { @@ -39,24 +28,10 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - if os.Getenv("KUBEBUILDER_ASSETS") == "" && !strings.EqualFold(os.Getenv("USE_EXISTING_CLUSTER"), "true") { - Skip("skipping") - } + suite.Client = require.Kubernetes(GinkgoT()) logging.SetLogSink(logging.Logrus(GinkgoWriter, "test", 1, 1)) log.SetLogger(logging.FromContext(context.Background())) - - By("bootstrapping test environment") - suite.Config, suite.Client = require.Kubernetes2(GinkgoT()) - - dc, err := discovery.NewDiscoveryClientForConfig(suite.Config) - Expect(err).ToNot(HaveOccurred()) - - server, err := dc.ServerVersion() - Expect(err).ToNot(HaveOccurred()) - - suite.ServerVersion, err = version.ParseGeneric(server.GitVersion) - Expect(err).ToNot(HaveOccurred()) }) var _ = AfterSuite(func() { From 02a345e951d74ee654d6816602f49241eef173e1 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 13 Nov 2024 17:12:48 -0600 Subject: [PATCH 2/2] Move server-side apply function to a shared package Multiple controllers had a method for this, but the implementations differed slightly. This combines their fixes and tests them in a single place. --- internal/bridge/crunchybridgecluster/apply.go | 33 ----------- .../bridge/crunchybridgecluster/postgres.go | 3 +- internal/controller/pgupgrade/apply.go | 31 ---------- .../pgupgrade/pgupgrade_controller.go | 4 +- internal/controller/postgrescluster/apply.go | 43 +------------- .../postgrescluster/controller_ref_manager.go | 8 +-- internal/controller/runtime/apply.go | 58 +++++++++++++++++++ .../apply_test.go | 33 +++++------ internal/controller/runtime/client.go | 1 + internal/controller/runtime/conversion.go | 4 +- .../{kubeapi => controller/runtime}/patch.go | 2 +- .../runtime}/patch_test.go | 2 +- .../controller/standalone_pgadmin/apply.go | 33 ----------- .../standalone_pgadmin/configmap.go | 3 +- .../controller/standalone_pgadmin/service.go | 3 +- .../standalone_pgadmin/statefulset.go | 3 +- .../controller/standalone_pgadmin/users.go | 3 +- .../controller/standalone_pgadmin/volume.go | 3 +- 18 files changed, 98 insertions(+), 172 deletions(-) delete mode 100644 internal/bridge/crunchybridgecluster/apply.go delete mode 100644 internal/controller/pgupgrade/apply.go create mode 100644 internal/controller/runtime/apply.go rename internal/controller/{postgrescluster => runtime}/apply_test.go (90%) rename internal/{kubeapi => controller/runtime}/patch.go (99%) rename internal/{kubeapi => controller/runtime}/patch_test.go (99%) delete mode 100644 internal/controller/standalone_pgadmin/apply.go diff --git a/internal/bridge/crunchybridgecluster/apply.go b/internal/bridge/crunchybridgecluster/apply.go deleted file mode 100644 index 850920fa83..0000000000 --- a/internal/bridge/crunchybridgecluster/apply.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. -// -// SPDX-License-Identifier: Apache-2.0 - -package crunchybridgecluster - -import ( - "context" - "reflect" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// apply sends an apply patch to object's endpoint in the Kubernetes API and -// 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 -// -// NOTE: This function is duplicated from a version in the postgrescluster package -func (r *CrunchyBridgeClusterReconciler) apply(ctx context.Context, object client.Object) error { - // Generate an apply-patch by comparing the object to its zero value. - zero := reflect.New(reflect.TypeOf(object).Elem()).Interface() - data, err := client.MergeFrom(zero.(client.Object)).Data(object) - apply := client.RawPatch(client.Apply.Type(), data) - - // Send the apply-patch with force=true. - if err == nil { - err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership) - } - - return err -} diff --git a/internal/bridge/crunchybridgecluster/postgres.go b/internal/bridge/crunchybridgecluster/postgres.go index 0aa09517d5..f8b8bf6b12 100644 --- a/internal/bridge/crunchybridgecluster/postgres.go +++ b/internal/bridge/crunchybridgecluster/postgres.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crunchydata/postgres-operator/internal/bridge" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -152,7 +153,7 @@ func (r *CrunchyBridgeClusterReconciler) reconcilePostgresRoleSecrets( roleSecrets[roleName], err = r.generatePostgresRoleSecret(cluster, role, clusterRole) } if err == nil { - err = errors.WithStack(r.apply(ctx, roleSecrets[roleName])) + err = errors.WithStack(runtime.Apply(ctx, r.Writer, roleSecrets[roleName])) } if err != nil { log.Error(err, "Issue creating role secret.") diff --git a/internal/controller/pgupgrade/apply.go b/internal/controller/pgupgrade/apply.go deleted file mode 100644 index c3e869eba6..0000000000 --- a/internal/controller/pgupgrade/apply.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. -// -// SPDX-License-Identifier: Apache-2.0 - -package pgupgrade - -import ( - "context" - "reflect" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// apply sends an apply patch to object's endpoint in the Kubernetes API and -// 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 { - // Generate an apply-patch by comparing the object to its zero value. - zero := reflect.New(reflect.TypeOf(object).Elem()).Interface() - data, err := client.MergeFrom(zero.(client.Object)).Data(object) - apply := client.RawPatch(client.Apply.Type(), data) - - // Send the apply-patch with force=true. - if err == nil { - err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership) - } - - return err -} diff --git a/internal/controller/pgupgrade/pgupgrade_controller.go b/internal/controller/pgupgrade/pgupgrade_controller.go index 653ea9e55e..f3385e9b37 100644 --- a/internal/controller/pgupgrade/pgupgrade_controller.go +++ b/internal/controller/pgupgrade/pgupgrade_controller.go @@ -453,7 +453,7 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // TODO: error from apply could mean that the job exists with a different spec. if err == nil && !upgradeJobComplete { - err = errors.WithStack(r.apply(ctx, + err = errors.WithStack(runtime.Apply(ctx, r.Writer, r.generateUpgradeJob(ctx, upgrade, world.ClusterPrimary, config.FetchKeyCommand(&world.Cluster.Spec)))) } @@ -464,7 +464,7 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err == nil && upgradeJobComplete && !removeDataJobsComplete { for _, sts := range world.ClusterReplicas { if err == nil { - err = r.apply(ctx, r.generateRemoveDataJob(ctx, upgrade, sts)) + err = runtime.Apply(ctx, r.Writer, r.generateRemoveDataJob(ctx, upgrade, sts)) } } } diff --git a/internal/controller/postgrescluster/apply.go b/internal/controller/postgrescluster/apply.go index ab7c238716..22aa1d3ce6 100644 --- a/internal/controller/postgrescluster/apply.go +++ b/internal/controller/postgrescluster/apply.go @@ -6,13 +6,10 @@ package postgrescluster import ( "context" - "reflect" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/crunchydata/postgres-operator/internal/kubeapi" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" ) // apply sends an apply patch to object's endpoint in the Kubernetes API and @@ -21,41 +18,5 @@ import ( // - https://docs.k8s.io/reference/using-api/server-side-apply/#managers // - https://docs.k8s.io/reference/using-api/server-side-apply/#conflicts func (r *Reconciler) apply(ctx context.Context, object client.Object) error { - // Generate an apply-patch by comparing the object to its zero value. - zero := reflect.New(reflect.TypeOf(object).Elem()).Interface() - data, err := client.MergeFrom(zero.(client.Object)).Data(object) - apply := client.RawPatch(client.Apply.Type(), data) - - // Keep a copy of the object before any API calls. - intent := object.DeepCopyObject() - patch := kubeapi.NewJSONPatch() - - // Send the apply-patch with force=true. - if err == nil { - err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership) - } - - // Some fields cannot be server-side applied correctly. When their outcome - // does not match the intent, send a json-patch to get really specific. - switch actual := object.(type) { - case *corev1.Service: - applyServiceSpec(patch, actual.Spec, intent.(*corev1.Service).Spec, "spec") - } - - // Send the json-patch when necessary. - if err == nil && !patch.IsEmpty() { - err = r.Writer.Patch(ctx, object, patch) - } - return err -} - -// applyServiceSpec is called by Reconciler.apply to work around issues -// with server-side apply. -func applyServiceSpec( - patch *kubeapi.JSON6902, actual, intent corev1.ServiceSpec, path ...string, -) { - // Service.Spec.Selector cannot be unset; perhaps https://issue.k8s.io/117447 - if !equality.Semantic.DeepEqual(actual.Selector, intent.Selector) { - patch.Replace(append(path, "selector")...)(intent.Selector) - } + return runtime.Apply(ctx, r.Writer, object) } diff --git a/internal/controller/postgrescluster/controller_ref_manager.go b/internal/controller/postgrescluster/controller_ref_manager.go index fc814259bf..e73b1701f1 100644 --- a/internal/controller/postgrescluster/controller_ref_manager.go +++ b/internal/controller/postgrescluster/controller_ref_manager.go @@ -16,7 +16,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/crunchydata/postgres-operator/internal/kubeapi" + "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/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -31,7 +31,7 @@ func (r *Reconciler) adoptObject(ctx context.Context, postgresCluster *v1beta1.P return err } - patchBytes, err := kubeapi.NewMergePatch(). + patchBytes, err := runtime.NewMergePatch(). Add("metadata", "ownerReferences")(obj.GetOwnerReferences()).Bytes() if err != nil { return err @@ -160,8 +160,8 @@ func (r *Reconciler) manageControllerRefs(ctx context.Context, func (r *Reconciler) releaseObject(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, obj client.Object) error { - // TODO create a strategic merge type in kubeapi instead of using Merge7386 - patch, err := kubeapi.NewMergePatch(). + // TODO create a strategic merge type instead of using Merge7386 + patch, err := runtime.NewMergePatch(). Add("metadata", "ownerReferences")([]map[string]string{{ "$patch": "delete", "uid": string(postgresCluster.GetUID()), diff --git a/internal/controller/runtime/apply.go b/internal/controller/runtime/apply.go new file mode 100644 index 0000000000..18926488e5 --- /dev/null +++ b/internal/controller/runtime/apply.go @@ -0,0 +1,58 @@ +// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package runtime + +import ( + "context" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Apply sends an apply patch with force=true using cc and updates object with any returned content. +// The client is responsible for setting fieldManager; see [client.WithFieldOwner]. +// +// - https://docs.k8s.io/reference/using-api/server-side-apply#managers +// - https://docs.k8s.io/reference/using-api/server-side-apply#conflicts +func Apply[ + // NOTE: This interface can go away following https://go.dev/issue/47487. + ClientPatch interface { + Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error + }, + T interface{ client.Object }, +](ctx context.Context, cc ClientPatch, object T) error { + // Generate an apply-patch by comparing the object to its zero value. + data, err := client.MergeFrom(*new(T)).Data(object) + apply := client.RawPatch(client.Apply.Type(), data) + + // Keep a copy of the object before any API calls. + intent := object.DeepCopyObject() + + // Send the apply-patch with force=true. + if err == nil { + err = cc.Patch(ctx, object, apply, client.ForceOwnership) + } + + // Some fields cannot be server-side applied correctly. + // When their outcome does not match the intent, send a json-patch to get really specific. + patch := NewJSONPatch() + + switch actual := any(object).(type) { + case *corev1.Service: + intent := intent.(*corev1.Service) + + // Service.Spec.Selector cannot be unset; perhaps https://issue.k8s.io/117447 + if !equality.Semantic.DeepEqual(actual.Spec.Selector, intent.Spec.Selector) { + patch.Replace("spec", "selector")(intent.Spec.Selector) + } + } + + // Send the json-patch when necessary. + if err == nil && !patch.IsEmpty() { + err = cc.Patch(ctx, object, patch) + } + return err +} diff --git a/internal/controller/postgrescluster/apply_test.go b/internal/controller/runtime/apply_test.go similarity index 90% rename from internal/controller/postgrescluster/apply_test.go rename to internal/controller/runtime/apply_test.go index 413ffbd135..c6f182ddeb 100644 --- a/internal/controller/postgrescluster/apply_test.go +++ b/internal/controller/runtime/apply_test.go @@ -2,10 +2,9 @@ // // SPDX-License-Identifier: Apache-2.0 -package postgrescluster +package runtime_test import ( - "context" "errors" "regexp" "strings" @@ -23,17 +22,18 @@ 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/testing/require" ) func TestServerSideApply(t *testing.T) { - ctx := context.Background() - cfg, cc := setupKubernetes(t) + ctx := t.Context() + config, base := require.Kubernetes2(t) require.ParallelCapacity(t, 0) - ns := setupNamespace(t, cc) + ns := require.Namespace(t, base) - dc, err := discovery.NewDiscoveryClientForConfig(cfg) + dc, err := discovery.NewDiscoveryClientForConfig(config) assert.NilError(t, err) server, err := dc.ServerVersion() @@ -43,8 +43,7 @@ func TestServerSideApply(t *testing.T) { assert.NilError(t, err) t.Run("ObjectMeta", func(t *testing.T) { - cc := client.WithFieldOwner(cc, t.Name()) - reconciler := Reconciler{Writer: cc} + cc := client.WithFieldOwner(base, t.Name()) constructor := func() *corev1.ConfigMap { var cm corev1.ConfigMap cm.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) @@ -78,17 +77,16 @@ func TestServerSideApply(t *testing.T) { assert.Assert(t, after.GetResourceVersion() == before.GetResourceVersion()) } - // Our apply method generates the correct apply-patch. + // Our [runtime.Apply] generates the correct apply-patch. again := constructor() - assert.NilError(t, reconciler.apply(ctx, again)) + assert.NilError(t, runtime.Apply(ctx, cc, again)) assert.Assert(t, again.GetResourceVersion() != "") assert.Assert(t, again.GetResourceVersion() == after.GetResourceVersion(), "expected to correctly no-op") }) t.Run("ControllerReference", func(t *testing.T) { - cc := client.WithFieldOwner(cc, t.Name()) - reconciler := Reconciler{Writer: cc} + cc := client.WithFieldOwner(base, t.Name()) // Setup two possible controllers. controller1 := new(corev1.ConfigMap) @@ -128,8 +126,8 @@ func TestServerSideApply(t *testing.T) { assert.Assert(t, len(status.ErrStatus.Details.Causes) != 0) assert.Equal(t, status.ErrStatus.Details.Causes[0].Field, "metadata.ownerReferences") - // Try to change the controller using our apply method. - err2 := reconciler.apply(ctx, applied) + // Try to change the controller using our [runtime.Apply]. + err2 := runtime.Apply(ctx, cc, applied) // Same result; patch not accepted. assert.DeepEqual(t, err1, err2, @@ -162,8 +160,7 @@ func TestServerSideApply(t *testing.T) { {"empty", make(map[string]string)}, } { t.Run(tt.name, func(t *testing.T) { - cc := client.WithFieldOwner(cc, t.Name()) - reconciler := Reconciler{Writer: cc} + cc := client.WithFieldOwner(base, t.Name()) intent := constructor(tt.name + "-selector") intent.Spec.Selector = tt.selector @@ -190,9 +187,9 @@ func TestServerSideApply(t *testing.T) { assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector), "got %v", after.Spec.Selector) - // Our apply method corrects it. + // Our [runtime.Apply] corrects it. again := intent.DeepCopy() - assert.NilError(t, reconciler.apply(ctx, again)) + assert.NilError(t, runtime.Apply(ctx, cc, again)) assert.Assert(t, equality.Semantic.DeepEqual(again.Spec.Selector, intent.Spec.Selector), "\n--- again.Spec.Selector\n+++ intent.Spec.Selector\n%v", diff --git a/internal/controller/runtime/client.go b/internal/controller/runtime/client.go index c41fe5a9c0..1bdbdddd14 100644 --- a/internal/controller/runtime/client.go +++ b/internal/controller/runtime/client.go @@ -76,6 +76,7 @@ func (fn ClientUpdate) Update(ctx context.Context, obj client.Object, opts ...cl return fn(ctx, obj, opts...) } +// WarningHandler implements [rest.WarningHandler] and [rest.WarningHandlerWithContext] as a single function. type WarningHandler func(ctx context.Context, code int, agent string, text string) func (fn WarningHandler) HandleWarningHeader(code int, agent string, text string) { diff --git a/internal/controller/runtime/conversion.go b/internal/controller/runtime/conversion.go index ae4495e865..57f7938f35 100644 --- a/internal/controller/runtime/conversion.go +++ b/internal/controller/runtime/conversion.go @@ -50,7 +50,7 @@ func FromUnstructuredObject[ FromUnstructured(object.UnstructuredContent(), result) } -// ToUnstructuredList returns a copy of list by marshaling through JSON. +// ToUnstructuredList returns a copy of list using reflection. func ToUnstructuredList(list client.ObjectList) (*unstructured.UnstructuredList, error) { content, err := runtime. DefaultUnstructuredConverter. @@ -61,7 +61,7 @@ func ToUnstructuredList(list client.ObjectList) (*unstructured.UnstructuredList, return result, err } -// ToUnstructuredObject returns a copy of object by marshaling through JSON. +// ToUnstructuredObject returns a copy of object using reflection. func ToUnstructuredObject(object client.Object) (*unstructured.Unstructured, error) { content, err := runtime. DefaultUnstructuredConverter. diff --git a/internal/kubeapi/patch.go b/internal/controller/runtime/patch.go similarity index 99% rename from internal/kubeapi/patch.go rename to internal/controller/runtime/patch.go index 95bcc9a6e1..955b93e1d4 100644 --- a/internal/kubeapi/patch.go +++ b/internal/controller/runtime/patch.go @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: Apache-2.0 -package kubeapi +package runtime import ( "strings" diff --git a/internal/kubeapi/patch_test.go b/internal/controller/runtime/patch_test.go similarity index 99% rename from internal/kubeapi/patch_test.go rename to internal/controller/runtime/patch_test.go index 05bd140066..07092be068 100644 --- a/internal/kubeapi/patch_test.go +++ b/internal/controller/runtime/patch_test.go @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: Apache-2.0 -package kubeapi +package runtime import ( "encoding/json" diff --git a/internal/controller/standalone_pgadmin/apply.go b/internal/controller/standalone_pgadmin/apply.go deleted file mode 100644 index 23df91192f..0000000000 --- a/internal/controller/standalone_pgadmin/apply.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2023 - 2025 Crunchy Data Solutions, Inc. -// -// SPDX-License-Identifier: Apache-2.0 - -package standalone_pgadmin - -import ( - "context" - "reflect" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// apply sends an apply patch to object's endpoint in the Kubernetes API and -// 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 -// -// TODO(tjmoore4): This function is duplicated from a version that takes a PostgresCluster object. -func (r *PGAdminReconciler) apply(ctx context.Context, object client.Object) error { - // Generate an apply-patch by comparing the object to its zero value. - zero := reflect.New(reflect.TypeOf(object).Elem()).Interface() - data, err := client.MergeFrom(zero.(client.Object)).Data(object) - apply := client.RawPatch(client.Apply.Type(), data) - - // Send the apply-patch with force=true. - if err == nil { - err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership) - } - - return err -} diff --git a/internal/controller/standalone_pgadmin/configmap.go b/internal/controller/standalone_pgadmin/configmap.go index d2378802c3..95c0bd9be5 100644 --- a/internal/controller/standalone_pgadmin/configmap.go +++ b/internal/controller/standalone_pgadmin/configmap.go @@ -19,6 +19,7 @@ import ( corev1 "k8s.io/api/core/v1" "github.com/crunchydata/postgres-operator/internal/collector" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -43,7 +44,7 @@ func (r *PGAdminReconciler) reconcilePGAdminConfigMap( err = errors.WithStack(r.setControllerReference(pgadmin, configmap)) } if err == nil { - err = errors.WithStack(r.apply(ctx, configmap)) + err = errors.WithStack(runtime.Apply(ctx, r.Writer, configmap)) } return configmap, err diff --git a/internal/controller/standalone_pgadmin/service.go b/internal/controller/standalone_pgadmin/service.go index 8f21da4765..43835b31d6 100644 --- a/internal/controller/standalone_pgadmin/service.go +++ b/internal/controller/standalone_pgadmin/service.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" + "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/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -100,7 +101,7 @@ func (r *PGAdminReconciler) reconcilePGAdminService( return err } - return errors.WithStack(r.apply(ctx, service)) + return errors.WithStack(runtime.Apply(ctx, r.Writer, service)) } // If we get here then ServiceName was not provided through the spec diff --git a/internal/controller/standalone_pgadmin/statefulset.go b/internal/controller/standalone_pgadmin/statefulset.go index 8e507acdad..a431ad5d3f 100644 --- a/internal/controller/standalone_pgadmin/statefulset.go +++ b/internal/controller/standalone_pgadmin/statefulset.go @@ -16,6 +16,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/collector" "github.com/crunchydata/postgres-operator/internal/controller/postgrescluster" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/util" @@ -55,7 +56,7 @@ func (r *PGAdminReconciler) reconcilePGAdminStatefulSet( if err := errors.WithStack(r.setControllerReference(pgadmin, sts)); err != nil { return err } - return errors.WithStack(r.apply(ctx, sts)) + return errors.WithStack(runtime.Apply(ctx, r.Writer, sts)) } // statefulset defines the StatefulSet needed to run pgAdmin. diff --git a/internal/controller/standalone_pgadmin/users.go b/internal/controller/standalone_pgadmin/users.go index 959437762f..e89705b633 100644 --- a/internal/controller/standalone_pgadmin/users.go +++ b/internal/controller/standalone_pgadmin/users.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "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/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -297,7 +298,7 @@ cd $PGADMIN_DIR err = errors.WithStack(r.setControllerReference(pgadmin, intentUserSecret)) if err == nil { - err = errors.WithStack(r.apply(ctx, intentUserSecret)) + err = errors.WithStack(runtime.Apply(ctx, r.Writer, intentUserSecret)) } return err diff --git a/internal/controller/standalone_pgadmin/volume.go b/internal/controller/standalone_pgadmin/volume.go index a3e26682ef..a4d0a5e13d 100644 --- a/internal/controller/standalone_pgadmin/volume.go +++ b/internal/controller/standalone_pgadmin/volume.go @@ -14,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -32,7 +33,7 @@ func (r *PGAdminReconciler) reconcilePGAdminDataVolume( if err == nil { err = r.handlePersistentVolumeClaimError(pgadmin, - errors.WithStack(r.apply(ctx, pvc))) + errors.WithStack(runtime.Apply(ctx, r.Writer, pvc))) } return pvc, err