From 962f047e4b856510f8f835b88209688d84bb053c Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 14 Nov 2023 10:34:20 -0600 Subject: [PATCH] Finalizer patcher Co-authored-by: Derek Menteer --- .changelog/3362.txt | 3 + .../controllers/configentry_controller.go | 26 +++-- .../controlplanerequestlimit_controller.go | 3 + .../exportedservices_controller.go | 3 + .../controllers/finalizer_patch.go | 78 +++++++++++++++ .../controllers/finalizer_patch_test.go | 97 +++++++++++++++++++ .../controllers/ingressgateway_controller.go | 1 + .../controllers/jwtprovider_controller.go | 3 + .../controllers/mesh_controller.go | 3 + .../controllers/proxydefaults_controller.go | 3 + .../controllers/samenessgroups_controller.go | 3 + .../controllers/servicedefaults_controller.go | 3 + .../serviceintentions_controller.go | 3 + .../controllers/serviceresolver_controller.go | 3 + .../controllers/servicerouter_controller.go | 3 + .../controllers/servicesplitter_controller.go | 3 + .../terminatinggateway_controller.go | 3 + 17 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 .changelog/3362.txt create mode 100644 control-plane/config-entries/controllers/finalizer_patch.go create mode 100644 control-plane/config-entries/controllers/finalizer_patch_test.go diff --git a/.changelog/3362.txt b/.changelog/3362.txt new file mode 100644 index 0000000000..82a7e56f0a --- /dev/null +++ b/.changelog/3362.txt @@ -0,0 +1,3 @@ +```release-note:bug-fix +crd-controllers: When the CRD controller reconciles a config entry CRD, only patch finalizers in the metadata of the CRD, rather than updating the entire entry, which causes changes to the CRD spec (such as adding in unspecified zero values) when using a Kubernetes client such as kubectl with `replace` mode. +``` diff --git a/control-plane/config-entries/controllers/configentry_controller.go b/control-plane/config-entries/controllers/configentry_controller.go index f1a4e316e8..03d30c0aa1 100644 --- a/control-plane/config-entries/controllers/configentry_controller.go +++ b/control-plane/config-entries/controllers/configentry_controller.go @@ -31,6 +31,7 @@ import ( const ( FinalizerName = "finalizers.consul.hashicorp.com" ConsulAgentError = "ConsulAgentError" + ConsulPatchError = "ConsulPatchError" ExternallyManagedConfigError = "ExternallyManagedConfigError" MigrationFailedError = "MigrationFailedError" ) @@ -38,8 +39,13 @@ const ( // Controller is implemented by CRD-specific controllers. It is used by // ConfigEntryController to abstract CRD-specific controllers. type Controller interface { - // Update updates the state of the whole object. - Update(context.Context, client.Object, ...client.UpdateOption) error + // AddFinalizersPatch creates a patch with the original finalizers with new ones appended to the end. + AddFinalizersPatch(obj client.Object, finalizers ...string) *FinalizerPatch + // RemoveFinalizersPatch creates a patch to remove a set of finalizers, while preserving the order. + RemoveFinalizersPatch(obj client.Object, finalizers ...string) *FinalizerPatch + // Patch patches the object. This should only ever be used for updating the metadata of an object, and not object + // spec or status. Updating the spec could have unintended consequences such as defaulting zero values. + Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error // UpdateStatus updates the state of just the object's status. UpdateStatus(context.Context, client.Object, ...client.SubResourceUpdateOption) error // Get retrieves an obj for the given object key from the Kubernetes Cluster. @@ -125,7 +131,13 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont // then let's add the finalizer and update the object. This is equivalent // registering our finalizer. if !containsString(configEntry.GetFinalizers(), FinalizerName) { - configEntry.AddFinalizer(FinalizerName) + addPatch := crdCtrl.AddFinalizersPatch(configEntry, FinalizerName) + err := crdCtrl.Patch(ctx, configEntry, addPatch) + if err != nil { + return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulPatchError, + fmt.Errorf("adding finalizer: %w", err)) + } + if err := r.syncUnknown(ctx, crdCtrl, configEntry); err != nil { return ctrl.Result{}, err } @@ -159,8 +171,8 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont } } // remove our finalizer from the list and update it. - configEntry.RemoveFinalizer(FinalizerName) - if err := crdCtrl.Update(ctx, configEntry); err != nil { + removePatch := crdCtrl.RemoveFinalizersPatch(configEntry, FinalizerName) + if err := crdCtrl.Patch(ctx, configEntry, removePatch); err != nil { return ctrl.Result{}, err } logger.Info("finalizer removed") @@ -355,7 +367,9 @@ func (r *ConfigEntryController) syncSuccessful(ctx context.Context, updater Cont func (r *ConfigEntryController) syncUnknown(ctx context.Context, updater Controller, configEntry common.ConfigEntryResource) error { configEntry.SetSyncedCondition(corev1.ConditionUnknown, "", "") - return updater.Update(ctx, configEntry) + timeNow := metav1.NewTime(time.Now()) + configEntry.SetLastSyncedTime(&timeNow) + return updater.UpdateStatus(ctx, configEntry) } func (r *ConfigEntryController) syncUnknownWithError(ctx context.Context, diff --git a/control-plane/config-entries/controllers/controlplanerequestlimit_controller.go b/control-plane/config-entries/controllers/controlplanerequestlimit_controller.go index f5afbdcc48..d5a41cf8ec 100644 --- a/control-plane/config-entries/controllers/controlplanerequestlimit_controller.go +++ b/control-plane/config-entries/controllers/controlplanerequestlimit_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ControlPlaneRequestLimitController)(nil) + // ControlPlaneRequestLimitController reconciles a ControlPlaneRequestLimit object. type ControlPlaneRequestLimitController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/exportedservices_controller.go b/control-plane/config-entries/controllers/exportedservices_controller.go index 2e82ed0eae..28d0f9c807 100644 --- a/control-plane/config-entries/controllers/exportedservices_controller.go +++ b/control-plane/config-entries/controllers/exportedservices_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ExportedServicesController)(nil) + // ExportedServicesController reconciles a ExportedServices object. type ExportedServicesController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/finalizer_patch.go b/control-plane/config-entries/controllers/finalizer_patch.go new file mode 100644 index 0000000000..c0974edae0 --- /dev/null +++ b/control-plane/config-entries/controllers/finalizer_patch.go @@ -0,0 +1,78 @@ +package controllers + +import ( + "encoding/json" + + jsonpatch "github.com/evanphx/json-patch" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type FinalizerPatcher struct{} + +type FinalizerPatch struct { + NewFinalizers []string +} + +// Type implements client.Patch. Since this patch is used for a custom CRD, Kubernetes does not allow the more advanced +// StrategicMergePatch. Therefore, this patcher will replace the entire list of finalizers with the new list, rather +// than adding/removing individual entries. +// +// This can result in a small race condition where we could overwrite recently modified finalizers (either modified by a +// user or another controller process). Before the addition of this finalizer patcher implementation, this race +// condition still existed, but applied to the entirety of the CRD because we used to update the entire CRD rather than +// just the finalizer, so this reduces the surface area of the race condition. Generally we should not expect users or +// other controllers to be touching the finalizers of consul-k8s managed CRDs. +func (fp *FinalizerPatch) Type() types.PatchType { + return types.MergePatchType +} + +var _ client.Patch = (*FinalizerPatch)(nil) + +func (f *FinalizerPatcher) AddFinalizersPatch(oldObj client.Object, addFinalizers ...string) *FinalizerPatch { + output := make([]string, 0, len(addFinalizers)) + existing := make(map[string]bool) + for _, f := range oldObj.GetFinalizers() { + existing[f] = true + output = append(output, f) + } + for _, f := range addFinalizers { + if !existing[f] { + output = append(output, f) + } + } + return &FinalizerPatch{ + NewFinalizers: output, + } +} + +func (f *FinalizerPatcher) RemoveFinalizersPatch(oldObj client.Object, removeFinalizers ...string) *FinalizerPatch { + output := make([]string, 0) + remove := make(map[string]bool) + for _, f := range removeFinalizers { + remove[f] = true + } + for _, f := range oldObj.GetFinalizers() { + if !remove[f] { + output = append(output, f) + } + } + return &FinalizerPatch{ + NewFinalizers: output, + } +} + +// Data implements client.Patch. +func (fp *FinalizerPatch) Data(obj client.Object) ([]byte, error) { + newData, err := json.Marshal(map[string]any{ + "metadata": map[string]any{ + "finalizers": fp.NewFinalizers, + }, + }) + if err != nil { + return nil, err + } + + p, err := jsonpatch.CreateMergePatch([]byte(`{}`), newData) + return p, err +} diff --git a/control-plane/config-entries/controllers/finalizer_patch_test.go b/control-plane/config-entries/controllers/finalizer_patch_test.go new file mode 100644 index 0000000000..290d9c6437 --- /dev/null +++ b/control-plane/config-entries/controllers/finalizer_patch_test.go @@ -0,0 +1,97 @@ +package controllers + +import ( + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" +) + +func TestFinalizersPatcher(t *testing.T) { + cases := []struct { + name string + oldObject client.Object + addFinalizers []string + removeFinalizers []string + expectedFinalizerPatch *FinalizerPatch + op string + }{ + { + name: "adds finalizers at the end and keeps the original list in order", + oldObject: &v1alpha1.ServiceResolver{ + ObjectMeta: v1.ObjectMeta{ + Finalizers: []string{ + "a", + "b", + "c", + }, + }, + }, + addFinalizers: []string{"d", "e"}, + expectedFinalizerPatch: &FinalizerPatch{ + NewFinalizers: []string{"a", "b", "c", "d", "e"}, + }, + }, + { + name: "adds finalizers when original list is empty", + oldObject: &v1alpha1.ServiceResolver{ + ObjectMeta: v1.ObjectMeta{ + Finalizers: []string{}, + }, + }, + addFinalizers: []string{"d", "e"}, + expectedFinalizerPatch: &FinalizerPatch{ + NewFinalizers: []string{"d", "e"}, + }, + }, + { + name: "removes finalizers keeping the original list in order", + oldObject: &v1alpha1.ServiceResolver{ + ObjectMeta: v1.ObjectMeta{ + Finalizers: []string{ + "a", + "b", + "c", + "d", + }, + }, + }, + removeFinalizers: []string{"b"}, + expectedFinalizerPatch: &FinalizerPatch{ + NewFinalizers: []string{"a", "c", "d"}, + }, + }, + { + name: "removes all finalizers if specified", + oldObject: &v1alpha1.ServiceResolver{ + ObjectMeta: v1.ObjectMeta{ + Finalizers: []string{ + "a", + "b", + }, + }, + }, + removeFinalizers: []string{"a", "b"}, + expectedFinalizerPatch: &FinalizerPatch{ + NewFinalizers: []string{}, + }, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f := FinalizerPatcher{} + var patch *FinalizerPatch + + if len(c.addFinalizers) > 0 { + patch = f.AddFinalizersPatch(c.oldObject, c.addFinalizers...) + } else if len(c.removeFinalizers) > 0 { + patch = f.RemoveFinalizersPatch(c.oldObject, c.removeFinalizers...) + } + + require.Equal(t, c.expectedFinalizerPatch, patch) + }) + } +} diff --git a/control-plane/config-entries/controllers/ingressgateway_controller.go b/control-plane/config-entries/controllers/ingressgateway_controller.go index 5bcb39bc2a..563b9aa606 100644 --- a/control-plane/config-entries/controllers/ingressgateway_controller.go +++ b/control-plane/config-entries/controllers/ingressgateway_controller.go @@ -17,6 +17,7 @@ import ( // IngressGatewayController is the controller for IngressGateway resources. type IngressGatewayController struct { + FinalizerPatcher client.Client Log logr.Logger Scheme *runtime.Scheme diff --git a/control-plane/config-entries/controllers/jwtprovider_controller.go b/control-plane/config-entries/controllers/jwtprovider_controller.go index 157f4bc855..39e76d502c 100644 --- a/control-plane/config-entries/controllers/jwtprovider_controller.go +++ b/control-plane/config-entries/controllers/jwtprovider_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*JWTProviderController)(nil) + // JWTProviderController reconciles a JWTProvider object. type JWTProviderController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/mesh_controller.go b/control-plane/config-entries/controllers/mesh_controller.go index ba78f0c144..7cd6997fc6 100644 --- a/control-plane/config-entries/controllers/mesh_controller.go +++ b/control-plane/config-entries/controllers/mesh_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*MeshController)(nil) + // MeshController reconciles a Mesh object. type MeshController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/proxydefaults_controller.go b/control-plane/config-entries/controllers/proxydefaults_controller.go index 882843bf9e..c96c5968d3 100644 --- a/control-plane/config-entries/controllers/proxydefaults_controller.go +++ b/control-plane/config-entries/controllers/proxydefaults_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ProxyDefaultsController)(nil) + // ProxyDefaultsController reconciles a ProxyDefaults object. type ProxyDefaultsController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/samenessgroups_controller.go b/control-plane/config-entries/controllers/samenessgroups_controller.go index 815b7e8ab8..c34b017761 100644 --- a/control-plane/config-entries/controllers/samenessgroups_controller.go +++ b/control-plane/config-entries/controllers/samenessgroups_controller.go @@ -16,9 +16,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*SamenessGroupController)(nil) + // SamenessGroupController reconciles a SamenessGroups object. type SamenessGroupController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/servicedefaults_controller.go b/control-plane/config-entries/controllers/servicedefaults_controller.go index 0496788bb3..69e001a741 100644 --- a/control-plane/config-entries/controllers/servicedefaults_controller.go +++ b/control-plane/config-entries/controllers/servicedefaults_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ServiceDefaultsController)(nil) + // ServiceDefaultsController is the controller for ServiceDefaults resources. type ServiceDefaultsController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/serviceintentions_controller.go b/control-plane/config-entries/controllers/serviceintentions_controller.go index 4461a82dd6..e20cf34317 100644 --- a/control-plane/config-entries/controllers/serviceintentions_controller.go +++ b/control-plane/config-entries/controllers/serviceintentions_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ServiceIntentionsController)(nil) + // ServiceIntentionsController reconciles a ServiceIntentions object. type ServiceIntentionsController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/serviceresolver_controller.go b/control-plane/config-entries/controllers/serviceresolver_controller.go index cfc5c31ca3..5bbc9c9f11 100644 --- a/control-plane/config-entries/controllers/serviceresolver_controller.go +++ b/control-plane/config-entries/controllers/serviceresolver_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ServiceResolverController)(nil) + // ServiceResolverController is the controller for ServiceResolver resources. type ServiceResolverController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/servicerouter_controller.go b/control-plane/config-entries/controllers/servicerouter_controller.go index a0b3bc0581..6ea87b590a 100644 --- a/control-plane/config-entries/controllers/servicerouter_controller.go +++ b/control-plane/config-entries/controllers/servicerouter_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ServiceRouterController)(nil) + // ServiceRouterController is the controller for ServiceRouter resources. type ServiceRouterController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/servicesplitter_controller.go b/control-plane/config-entries/controllers/servicesplitter_controller.go index b733fb9cb5..07e2603833 100644 --- a/control-plane/config-entries/controllers/servicesplitter_controller.go +++ b/control-plane/config-entries/controllers/servicesplitter_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*ServiceSplitterController)(nil) + // ServiceSplitterReconciler reconciles a ServiceSplitter object. type ServiceSplitterController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController diff --git a/control-plane/config-entries/controllers/terminatinggateway_controller.go b/control-plane/config-entries/controllers/terminatinggateway_controller.go index d73d4e043c..c68db70538 100644 --- a/control-plane/config-entries/controllers/terminatinggateway_controller.go +++ b/control-plane/config-entries/controllers/terminatinggateway_controller.go @@ -15,9 +15,12 @@ import ( consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" ) +var _ Controller = (*TerminatingGatewayController)(nil) + // TerminatingGatewayController is the controller for TerminatingGateway resources. type TerminatingGatewayController struct { client.Client + FinalizerPatcher Log logr.Logger Scheme *runtime.Scheme ConfigEntryController *ConfigEntryController