reconciler/managed: avoid temporary data loss to managed on annotatio…

…n update

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

pkg/reconciler/managed/api.go

@@ -22,6 +22,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
@@ -167,18 +168,19 @@ func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnno
 
 // UpdateCriticalAnnotations updates (i.e. persists) the annotations of the
 // supplied Object. It retries in the face of any API server error several times
-// in order to ensure annotations that contain critical state are persisted. Any
-// pending changes to the supplied Object's spec, status, or other metadata are
-// reset to their current state according to the API server.
+// in order to ensure annotations that contain critical state are persisted.
+// Pending changes to the supplied Object's spec, status, or other metadata
+// might get reset to their current state according to the API server, e.g. in
+// case of a conflict error.
 func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error {
 	a := o.GetAnnotations()
 	err := retry.OnError(retry.DefaultRetry, resource.IsAPIError, func() error {
-		nn := types.NamespacedName{Name: o.GetName()}
-		if err := u.client.Get(ctx, nn, o); err != nil {
-			return err
+		err := u.client.Update(ctx, o)
+		if kerrors.IsConflict(err) {
+			err = u.client.Get(ctx, types.NamespacedName{Name: o.GetName()}, o)
+			meta.AddAnnotations(o, a)
 		}
-		meta.AddAnnotations(o, a)
-		return u.client.Update(ctx, o)
+		return err
 	})
 	return errors.Wrap(err, errUpdateCriticalAnnotations)
 }

pkg/reconciler/managed/api_test.go

@@ -21,8 +21,10 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	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"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
@@ -364,26 +366,37 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		o   client.Object
 	}
 
+	setAnnotation := func(obj client.Object) error {
+		obj.SetAnnotations(map[string]string{"getcalled": "true"})
+		return nil
+	}
+
 	cases := map[string]struct {
-		reason string
-		c      client.Client
-		args   args
-		want   error
+		reason        string
+		c             *test.MockClient
+		args          args
+		want          error
+		wantGetCalled bool
 	}{
-		"GetError": {
+		"UpdateConflictGetError": {
 			reason: "We should return any error we encounter getting the supplied object",
 			c: &test.MockClient{
-				MockGet: test.NewMockGetFn(errBoom),
+				MockGet: test.NewMockGetFn(errBoom, setAnnotation),
+				MockUpdate: test.NewMockUpdateFn(kerrors.NewConflict(schema.GroupResource{
+					Group:    "foo.com",
+					Resource: "bars",
+				}, "abc", errBoom)),
 			},
 			args: args{
 				o: &fake.Managed{},
 			},
-			want: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+			want:          errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+			wantGetCalled: true,
 		},
 		"UpdateError": {
 			reason: "We should return any error we encounter updating the supplied object",
 			c: &test.MockClient{
-				MockGet:    test.NewMockGetFn(nil),
+				MockGet:    test.NewMockGetFn(nil, setAnnotation),
 				MockUpdate: test.NewMockUpdateFn(errBoom),
 			},
 			args: args{
@@ -394,7 +407,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		"Success": {
 			reason: "We should return without error if we successfully update our annotations",
 			c: &test.MockClient{
-				MockGet:    test.NewMockGetFn(nil),
+				MockGet:    test.NewMockGetFn(nil, setAnnotation),
 				MockUpdate: test.NewMockUpdateFn(errBoom),
 			},
 			args: args{
@@ -411,6 +424,10 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 			if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" {
 				t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff)
 			}
+			getCalled := tc.args.o.GetAnnotations()["getcalled"] == "true"
+			if getCalled != tc.wantGetCalled {
+				t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...) calling get: -want %v, +got %v", tc.reason, tc.wantGetCalled, getCalled)
+			}
 		})
 	}
 }