Better support for non-deterministic external-names by updating critical annotations

pkg/reconciler/managed/api.go

@@ -277,15 +277,25 @@ 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.
-// 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.
+// Only annotations will be updated as part of this operation, other fields of the
+// supplied Object will not be modified.
 func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error {
 	a := o.GetAnnotations()
 	err := retry.OnError(retry.DefaultRetry, func(err error) bool {
 		return !errors.Is(err, context.Canceled)
 	}, func() error {
-		err := u.client.Update(ctx, o)
+		patchMap := map[string]interface{}{
+			"metadata": map[string]any{
+				"annotations": a,
+			},
+		}
+
+		patchData, err := json.Marshal(patchMap)
+		if err != nil {
+			return err
+		}
+
+		err = u.client.Patch(ctx, o, client.RawPatch(types.MergePatchType, patchData), client.FieldOwner(fieldOwnerAPISimpleRefResolver), client.ForceOwnership)
 		if kerrors.IsConflict(err) {
 			if getErr := u.client.Get(ctx, client.ObjectKeyFromObject(o), o); getErr != nil {
 				return getErr

pkg/reconciler/managed/api_test.go

@@ -543,12 +543,12 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		o   client.Object
 	}
 
-	setLabels := func(obj client.Object) error {
-		obj.SetLabels(map[string]string{"getcalled": "true"})
+	setAnnotations := func(obj client.Object) error {
+		obj.SetAnnotations(map[string]string{"getcalled": "true"})
 		return nil
 	}
 	objectReturnedByGet := &fake.LegacyManaged{}
-	setLabels(objectReturnedByGet)
+	setAnnotations(objectReturnedByGet)
 
 	cases := map[string]struct {
 		reason string
@@ -559,8 +559,8 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		"UpdateConflictGetError": {
 			reason: "We should return any error we encounter getting the supplied object",
 			c: &test.MockClient{
-				MockGet: test.NewMockGetFn(errBoom, setLabels),
-				MockUpdate: test.NewMockUpdateFn(kerrors.NewConflict(schema.GroupResource{
+				MockGet: test.NewMockGetFn(errBoom, setAnnotations),
+				MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{
 					Group:    "foo.com",
 					Resource: "bars",
 				}, "abc", errBoom)),
@@ -576,8 +576,8 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		"UpdateError": {
 			reason: "We should return any error we encounter updating the supplied object",
 			c: &test.MockClient{
-				MockGet:    test.NewMockGetFn(nil, setLabels),
-				MockUpdate: test.NewMockUpdateFn(errBoom),
+				MockGet:   test.NewMockGetFn(nil, setAnnotations),
+				MockPatch: test.NewMockPatchFn(errBoom),
 			},
 			args: args{
 				o: &fake.LegacyManaged{},
@@ -590,8 +590,8 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
 		"SuccessfulGetAfterAConflict": {
 			reason: "A successful get after a conflict should not hide the conflict error and prevent retries",
 			c: &test.MockClient{
-				MockGet: test.NewMockGetFn(nil, setLabels),
-				MockUpdate: test.NewMockUpdateFn(kerrors.NewConflict(schema.GroupResource{
+				MockGet: test.NewMockGetFn(nil, setAnnotations),
+				MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{
 					Group:    "foo.com",
 					Resource: "bars",
 				}, "abc", errBoom)),
@@ -610,8 +610,8 @@ 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, setLabels),
-				MockUpdate: test.NewMockUpdateFn(errBoom),
+				MockGet:   test.NewMockGetFn(nil, setAnnotations),
+				MockPatch: test.NewMockPatchFn(errBoom),
 			},
 			args: args{
 				o: &fake.LegacyManaged{},

pkg/reconciler/managed/reconciler.go

@@ -1408,6 +1408,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
 	}
 
+	if observation.ResourceExists {
+		// When a resource exists or is just created, it might have received
+		// a non-deterministic external name after its creation, which we need to persist.
+		// We do this by updating the critical annotations.
+		// This is needed because some resources might not receive an external-name directly 
+		// after the creation, but later as part of an asynchronous process.
+		// When Crossplane supports asynchronous creation of resources natively, this logic
+		// might not be needed anymore and can be revisited. 
+		if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil {
+			log.Debug(errUpdateManagedAnnotations, "error", err)
+			record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations)))
+			return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedAnnotations)
+		}
+	}
+
 	if observation.ResourceLateInitialized && policy.ShouldLateInitialize() {
 		// Note that this update may reset any pending updates to the status of
 		// the managed resource from when it was observed above. This is because