From 8dfcfd1ef3a8356140b3c09281c047540734e9c2 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Wed, 25 Jun 2025 18:03:45 +0200 Subject: [PATCH 01/10] Update critical annotations when a resource exists Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/reconciler.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 188835416..c68107ca2 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -1410,6 +1410,17 @@ 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, which we need to persist. + // We do this by updating the critical annotations. + 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 From b57908c36d348564c7022cbe9da2abcaac98490b Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Wed, 25 Jun 2025 19:12:19 +0200 Subject: [PATCH 02/10] Ensure only annotations are updated As the UpdateCriticialAnnotations function is now not exclusively called in the creation process, we have to ensure no other fields like the spec are updated, so we don't interfer with the normal LateInitialize logic Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api.go | 18 ++++-- pkg/reconciler/managed/api_test.go | 12 ++-- .../managed/reconciler_legacy_test.go | 58 ++++++++++++++++--- 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 7bf6a1514..4b12fe3a0 100644 --- a/pkg/reconciler/managed/api.go +++ b/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 diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index c1d5952fd..3ba8f9a59 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -568,7 +568,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { 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{ + MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{ Group: "foo.com", Resource: "bars", }, "abc", errBoom)), @@ -584,8 +584,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, setLabels), + MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ o: &fake.LegacyManaged{}, @@ -599,7 +599,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { 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{ + MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{ Group: "foo.com", Resource: "bars", }, "abc", errBoom)), @@ -618,8 +618,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, setLabels), + MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ o: &fake.LegacyManaged{}, diff --git a/pkg/reconciler/managed/reconciler_legacy_test.go b/pkg/reconciler/managed/reconciler_legacy_test.go index 72695fd2a..3fa227e33 100644 --- a/pkg/reconciler/managed/reconciler_legacy_test.go +++ b/pkg/reconciler/managed/reconciler_legacy_test.go @@ -348,7 +348,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -759,6 +760,7 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(errBoom), + MockPatch: test.NewMockPatchFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -807,6 +809,7 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -858,6 +861,7 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -907,6 +911,7 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -969,6 +974,7 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -1009,6 +1015,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -1044,6 +1051,7 @@ func TestReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) @@ -1085,7 +1093,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1126,7 +1135,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1170,7 +1180,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1208,7 +1219,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1250,7 +1262,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42)) @@ -1294,7 +1307,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -1351,7 +1365,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1395,7 +1410,8 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: legacyManagedMockGetFn(nil, 42), + MockGet: legacyManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1447,6 +1463,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "true"}) @@ -1477,6 +1494,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{}) @@ -1513,6 +1531,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "false"}) @@ -1560,6 +1579,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return errBoom }), @@ -1581,6 +1601,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionCreate}) @@ -1611,6 +1632,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionCreate}) @@ -1644,6 +1666,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -1678,6 +1701,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) @@ -1724,6 +1748,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) @@ -1775,6 +1800,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) @@ -1828,6 +1854,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -1870,6 +1897,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -1912,6 +1940,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(errBoom), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve, xpv2.ManagementActionLateInitialize, xpv2.ManagementActionCreate, xpv2.ManagementActionDelete}) @@ -1963,6 +1992,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -2014,6 +2044,7 @@ func TestReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -2066,6 +2097,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(errBoom), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve, xpv2.ManagementActionUpdate, xpv2.ManagementActionCreate, xpv2.ManagementActionDelete}) @@ -2115,6 +2147,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -2145,6 +2178,7 @@ func TestReconciler(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -2643,6 +2677,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), @@ -2681,6 +2716,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), @@ -2720,6 +2756,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), @@ -2758,6 +2795,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { Client: &test.MockClient{ MockGet: legacyManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), @@ -2803,6 +2841,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), @@ -2847,6 +2886,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) { return nil }), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, Scheme: fake.SchemeWith(&fake.LegacyManaged{}), From 64183fc530e05e4483da7fc946661f374fbe5260 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Wed, 25 Jun 2025 19:37:27 +0200 Subject: [PATCH 03/10] Update test assumption to update annotations Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index 3ba8f9a59..a26fcc199 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -551,12 +551,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 @@ -567,7 +567,7 @@ 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), + MockGet: test.NewMockGetFn(errBoom, setAnnotations), MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{ Group: "foo.com", Resource: "bars", @@ -584,7 +584,7 @@ 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), + MockGet: test.NewMockGetFn(nil, setAnnotations), MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ @@ -598,7 +598,7 @@ 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), + MockGet: test.NewMockGetFn(nil, setAnnotations), MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{ Group: "foo.com", Resource: "bars", @@ -618,7 +618,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, setLabels), + MockGet: test.NewMockGetFn(nil, setAnnotations), MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ From d1ce6f1d5cbfdbbfe4761fb0ba7354dba9c57a4c Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Thu, 26 Jun 2025 09:31:44 +0200 Subject: [PATCH 04/10] Add more details to comment Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/reconciler.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index c68107ca2..3ab828f3c 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -1412,8 +1412,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if observation.ResourceExists { // When a resource exists or is just created, it might have received - // a non-deterministic external name, which we need to persist. + // 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))) From a3516dbc12566f30570508d50c45305418f2e679 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Thu, 9 Oct 2025 10:06:54 +0200 Subject: [PATCH 05/10] Add MockPatch to modern test case Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/reconciler_modern_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/reconciler/managed/reconciler_modern_test.go b/pkg/reconciler/managed/reconciler_modern_test.go index 3e980dd58..f48075d51 100644 --- a/pkg/reconciler/managed/reconciler_modern_test.go +++ b/pkg/reconciler/managed/reconciler_modern_test.go @@ -1781,6 +1781,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) From fd69032c06682155fa5822354bc9b347abf74933 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Fri, 10 Oct 2025 16:17:10 +0200 Subject: [PATCH 06/10] Add all missing mocks Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- .../managed/reconciler_modern_test.go | 78 +++++++++++++++---- 1 file changed, 63 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/managed/reconciler_modern_test.go b/pkg/reconciler/managed/reconciler_modern_test.go index f48075d51..ab94da952 100644 --- a/pkg/reconciler/managed/reconciler_modern_test.go +++ b/pkg/reconciler/managed/reconciler_modern_test.go @@ -98,6 +98,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetDeletionTimestamp(&now) @@ -139,6 +140,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetDeletionTimestamp(&now) @@ -176,6 +178,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), }, Scheme: fake.SchemeWith(&fake.ModernManaged{}), }, @@ -192,7 +195,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -228,6 +232,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), }, Scheme: fake.SchemeWith(&fake.ModernManaged{}), @@ -263,6 +268,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) meta.SetExternalCreatePending(want, now.Time) @@ -292,7 +298,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -322,7 +329,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, got client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileConnect)).WithObservedGeneration(42)) @@ -352,7 +360,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -393,7 +402,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileObserve)).WithObservedGeneration(42)) @@ -699,7 +709,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -733,7 +744,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -764,6 +776,7 @@ func TestModernReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -812,6 +825,7 @@ func TestModernReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -864,6 +878,7 @@ func TestModernReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -913,6 +928,7 @@ func TestModernReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), MockUpdate: test.NewMockUpdateFn(nil), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) meta.SetExternalCreatePending(want, time.Now()) @@ -974,6 +990,7 @@ func TestModernReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1014,6 +1031,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1050,6 +1068,7 @@ func TestModernReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1091,7 +1110,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1132,7 +1152,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1176,7 +1197,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1214,7 +1236,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), @@ -1256,7 +1279,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42)) @@ -1300,7 +1324,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileError(errBoom).WithObservedGeneration(42)) @@ -1357,7 +1382,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1401,7 +1427,8 @@ func TestModernReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: modernManagedMockGetFn(nil, 42), + MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42)) @@ -1453,6 +1480,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "true"}) @@ -1483,6 +1511,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{}) @@ -1519,6 +1548,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "false"}) @@ -1566,6 +1596,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return errBoom }), @@ -1587,6 +1618,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionCreate}) @@ -1617,6 +1649,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionCreate}) @@ -1650,6 +1683,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -1684,6 +1718,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) @@ -1730,6 +1765,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve}) @@ -1834,6 +1870,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1876,6 +1913,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1918,6 +1956,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -1970,6 +2009,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -2021,6 +2061,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll}) @@ -2072,6 +2113,7 @@ func TestModernReconciler(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newModernManaged(42) @@ -2594,6 +2636,7 @@ func TestReconcilerChangeLogs(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, @@ -2632,6 +2675,7 @@ func TestReconcilerChangeLogs(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, @@ -2671,6 +2715,7 @@ func TestReconcilerChangeLogs(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, @@ -2709,6 +2754,7 @@ func TestReconcilerChangeLogs(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: modernManagedMockGetFn(nil, 42), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, @@ -2754,6 +2800,7 @@ func TestReconcilerChangeLogs(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, @@ -2798,6 +2845,7 @@ func TestReconcilerChangeLogs(t *testing.T) { return nil }), + MockPatch: test.NewMockPatchFn(nil), MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { return nil }), }, From eb17cb344646cbe51f98caa6c7cbcbab5ae47a7a Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Wed, 3 Dec 2025 16:44:59 +0100 Subject: [PATCH 07/10] Add critical annotations handling in UpdateCriticalAnnotations method Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 4b12fe3a0..b902d2b06 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -55,6 +55,15 @@ const ( errUpdateCriticalAnnotations = "cannot update critical annotations" ) +var ( + criticalAnnotations = []string{ + meta.AnnotationKeyExternalCreateFailed, + meta.AnnotationKeyExternalCreatePending, + meta.AnnotationKeyExternalCreateSucceeded, + meta.AnnotationKeyExternalName, + } +) + // NameAsExternalName writes the name of the managed resource to // the external name annotation field in order to be used as name of // the external resource in provider. @@ -280,7 +289,18 @@ func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnno // 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() + a := make(map[string]string) + for _, k := range criticalAnnotations { + if v, ok := o.GetAnnotations()[k]; ok { + a[k] = v + } + } + + if len(a) == 0 { + // No critical annotations to update. + return nil + } + err := retry.OnError(retry.DefaultRetry, func(err error) bool { return !errors.Is(err, context.Canceled) }, func() error { From 5035730afd8d94f3c1f79fe51bac02cef0b0e7d7 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:15:45 +0200 Subject: [PATCH 08/10] Set a critical annotation Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api_test.go | 32 ++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index a26fcc199..5a8450418 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -552,7 +552,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { } setAnnotations := func(obj client.Object) error { - obj.SetAnnotations(map[string]string{"getcalled": "true"}) + obj.SetAnnotations(map[string]string{"crossplane.io/external-name": "my-external-name"}) return nil } objectReturnedByGet := &fake.LegacyManaged{} @@ -574,7 +574,11 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { }, "abc", errBoom)), }, args: args{ - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, want: want{ err: errors.Wrap(errBoom, errUpdateCriticalAnnotations), @@ -588,11 +592,19 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, want: want{ err: errors.Wrap(errBoom, errUpdateCriticalAnnotations), - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, }, "SuccessfulGetAfterAConflict": { @@ -605,7 +617,11 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { }, "abc", errBoom)), }, args: args{ - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, want: want{ err: errors.Wrap(kerrors.NewConflict(schema.GroupResource{ @@ -622,7 +638,11 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { MockPatch: test.NewMockPatchFn(errBoom), }, args: args{ - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, want: want{ err: errors.Wrap(errBoom, errUpdateCriticalAnnotations), From 7e15f7f3b0a7970faf794a5b20e49b91baa50579 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:28:23 +0200 Subject: [PATCH 09/10] apply formatting Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api.go | 3 ++- pkg/reconciler/managed/api_test.go | 2 +- pkg/reconciler/managed/reconciler.go | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index b902d2b06..01fcb0cc8 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -55,6 +55,7 @@ const ( errUpdateCriticalAnnotations = "cannot update critical annotations" ) +//nolint:gochecknoglobals // this is a list of critical annotations that should be retried when updating, and is not expected to be modified at runtime. var ( criticalAnnotations = []string{ meta.AnnotationKeyExternalCreateFailed, @@ -304,7 +305,7 @@ func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx contex err := retry.OnError(retry.DefaultRetry, func(err error) bool { return !errors.Is(err, context.Canceled) }, func() error { - patchMap := map[string]interface{}{ + patchMap := map[string]any{ "metadata": map[string]any{ "annotations": a, }, diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index 5a8450418..d97d11a45 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -600,7 +600,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { }, want: want{ err: errors.Wrap(errBoom, errUpdateCriticalAnnotations), - o: &fake.LegacyManaged{ + o: &fake.LegacyManaged{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, }, diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 3ab828f3c..60b3aaabf 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -1414,10 +1414,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // 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 + // 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. + // 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))) From 5fbe26160881a226e2b0cb009cb2d5a077c327a7 Mon Sep 17 00:00:00 2001 From: twobiers <22715034+twobiers@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:31:27 +0200 Subject: [PATCH 10/10] fix test assertion Signed-off-by: twobiers <22715034+twobiers@users.noreply.github.com> --- pkg/reconciler/managed/api_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index d97d11a45..ee0269cc3 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -646,7 +646,11 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { }, want: want{ err: errors.Wrap(errBoom, errUpdateCriticalAnnotations), - o: &fake.LegacyManaged{}, + o: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"crossplane.io/external-name": "my-external-name"}, + }, + }, }, }, }