pkg/resource: make patching applicator consistent and deprecate updat…

…e applicator

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

pkg/resource/api.go

@@ -18,11 +18,9 @@ package resource
 
 import (
 	"context"
-	"encoding/json"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -33,6 +31,12 @@ import (
 // Error strings.
 const (
 	errUpdateObject = "cannot update object"
+
+	// taken from k8s.io/apiserver. Not crucial to match, but for uniformity it
+	// better should.
+	// TODO(sttts): import from k8s.io/apiserver/pkg/registry/generic/registry when
+	//              kube has updated otel dependencies post-1.28.
+	errOptimisticLock = "the object has been modified; please apply your changes to the latest version and try again"
 )
 
 // An APIPatchingApplicator applies changes to an object by either creating or
@@ -50,41 +54,53 @@ func NewAPIPatchingApplicator(c client.Client) *APIPatchingApplicator {
 // Apply changes to the supplied object. The object will be created if it does
 // not exist, or patched if it does. If the object does exist, it will only be
 // patched if the passed object has the same or an empty resource version.
-func (a *APIPatchingApplicator) Apply(ctx context.Context, o client.Object, ao ...ApplyOption) error {
-	m, ok := o.(metav1.Object)
-	if !ok {
-		return errors.New("cannot access object metadata")
-	}
-
-	if m.GetName() == "" && m.GetGenerateName() != "" {
-		return errors.Wrap(a.client.Create(ctx, o), "cannot create object")
+func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { //nolint:gocyclo // the logic here is crucial and deserves to stay in one method
+	if obj.GetName() == "" && obj.GetGenerateName() != "" {
+		return a.client.Create(ctx, obj)
 	}
 
-	desired := o.DeepCopyObject()
-
-	err := a.client.Get(ctx, types.NamespacedName{Name: m.GetName(), Namespace: m.GetNamespace()}, o)
+	current := obj.DeepCopyObject().(client.Object)
+	err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current)
 	if kerrors.IsNotFound(err) {
 		// TODO(negz): Apply ApplyOptions here too?
-		return errors.Wrap(a.client.Create(ctx, o), "cannot create object")
+		return a.client.Create(ctx, obj)
 	}
 	if err != nil {
-		return errors.Wrap(err, "cannot get object")
+		return err
 	}
 
-	for _, fn := range ao {
-		if err := fn(ctx, o, desired); err != nil {
+	// Note: this check would ideally not be necessary if the Apply signature
+	// had a current object that we could use for the diff. But we have no
+	// current and for consistency of the patch it matters that the object we
+	// get above is the one that was originally used.
+	if obj.GetResourceVersion() != "" && obj.GetResourceVersion() != current.GetResourceVersion() {
+		gvr, err := groupResource(a.client, obj)
+		if err != nil {
 			return err
 		}
+		return kerrors.NewConflict(gvr, current.GetName(), errors.New(errOptimisticLock))
 	}
 
-	// TODO(negz): Allow callers to override the kind of patch used.
-	return errors.Wrap(a.client.Patch(ctx, o, &patch{desired}), "cannot patch object")
-}
+	for _, fn := range ao {
+		if err := fn(ctx, current, obj); err != nil {
+			return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj))
+		}
+	}
 
-type patch struct{ from runtime.Object }
+	return a.client.Patch(ctx, obj, client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{}))
+}
 
-func (p *patch) Type() types.PatchType                { return types.MergePatchType }
-func (p *patch) Data(_ client.Object) ([]byte, error) { return json.Marshal(p.from) }
+func groupResource(c client.Client, o client.Object) (schema.GroupResource, error) {
+	gvk, err := c.GroupVersionKindFor(o)
+	if err != nil {
+		return schema.GroupResource{}, errors.Wrapf(err, "cannot determine group version kind of %T", o)
+	}
+	m, err := c.RESTMapper().RESTMapping(gvk.GroupKind(), gvk.Version)
+	if err != nil {
+		return schema.GroupResource{}, errors.Wrapf(err, "cannot determine group resource of %v", gvk)
+	}
+	return m.Resource.GroupResource(), nil
+}
 
 // An APIUpdatingApplicator applies changes to an object by either creating or
 // updating it in a Kubernetes API server.
@@ -94,40 +110,37 @@ type APIUpdatingApplicator struct {
 
 // NewAPIUpdatingApplicator returns an Applicator that applies changes to an
 // object by either creating or updating it in a Kubernetes API server.
+//
+// Deprecated: Use NewAPIPatchingApplicator instead. The updating applicator
+// can lead to data-loss if the Golang types in this process are not up-to-date.
 func NewAPIUpdatingApplicator(c client.Client) *APIUpdatingApplicator {
 	return &APIUpdatingApplicator{client: c}
 }
 
 // Apply changes to the supplied object. The object will be created if it does
 // not exist, or updated if it does.
-func (a *APIUpdatingApplicator) Apply(ctx context.Context, o client.Object, ao ...ApplyOption) error {
-	m, ok := o.(Object)
-	if !ok {
-		return errors.New("cannot access object metadata")
+func (a *APIUpdatingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error {
+	if obj.GetName() == "" && obj.GetGenerateName() != "" {
+		return a.client.Create(ctx, obj)
 	}
 
-	if m.GetName() == "" && m.GetGenerateName() != "" {
-		return errors.Wrap(a.client.Create(ctx, o), "cannot create object")
-	}
-
-	current := o.DeepCopyObject().(client.Object)
-
-	err := a.client.Get(ctx, types.NamespacedName{Name: m.GetName(), Namespace: m.GetNamespace()}, current)
+	current := obj.DeepCopyObject().(client.Object)
+	err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current)
 	if kerrors.IsNotFound(err) {
 		// TODO(negz): Apply ApplyOptions here too?
-		return errors.Wrap(a.client.Create(ctx, m), "cannot create object")
+		return a.client.Create(ctx, obj)
 	}
 	if err != nil {
-		return errors.Wrap(err, "cannot get object")
+		return err
 	}
 
 	for _, fn := range ao {
-		if err := fn(ctx, current, m); err != nil {
-			return err
+		if err := fn(ctx, current, obj); err != nil {
+			return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj))
 		}
 	}
 
-	return errors.Wrap(a.client.Update(ctx, m), "cannot update object")
+	return a.client.Update(ctx, obj)
 }
 
 // An APIFinalizer adds and removes finalizers to and from a resource.