[Net-5510][Net-5455]: CRD controller should only patch the finalizer …

…in the metadata of a CRD, rather than the whole object (#3362)

Finalizer patcher

Co-authored-by: Derek Menteer <[email protected]>

.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.
+```

control-plane/config-entries/controllers/configentry_controller.go

@@ -31,15 +31,21 @@ import (
 const (
 	FinalizerName                = "finalizers.consul.hashicorp.com"
 	ConsulAgentError             = "ConsulAgentError"
+	ConsulPatchError             = "ConsulPatchError"
 	ExternallyManagedConfigError = "ExternallyManagedConfigError"
 	MigrationFailedError         = "MigrationFailedError"
 )
 
 // 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,

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

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

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
+}

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)
+		})
+	}
+}

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

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

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

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

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

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

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

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

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

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