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

@@ -172,12 +172,18 @@ func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnno
 // reset to their current state according to the API server.
 func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error {
 	a := o.GetAnnotations()
+	first := true
 	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
+		if !first {
+			// only get after the first attempt to avoid resetting o, and with that avoid all changes
+			// to o to be forgotten until next reconcile.
+			nn := types.NamespacedName{Name: o.GetName()}
+			if err := u.client.Get(ctx, nn, o); err != nil {
+				return err
+			}
+			meta.AddAnnotations(o, a)
 		}
-		meta.AddAnnotations(o, a)
+		first = false
 		return u.client.Update(ctx, o)
 	})
 	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"
@@ -374,6 +376,10 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 			reason: "We should return any error we encounter getting the supplied object",
 			c: &test.MockClient{
 				MockGet: test.NewMockGetFn(errBoom),
+				MockUpdate: test.NewMockUpdateFn(kerrors.NewConflict(schema.GroupResource{
+					Group:    "foo.com",
+					Resource: "bars",
+				}, "abc", errBoom)),
 			},
 			args: args{
 				o: &fake.Managed{},