From 54b924b13d9f627a86c3c028f822210ebce7f00d Mon Sep 17 00:00:00 2001 From: Jonathan Graniero Date: Tue, 13 May 2025 12:59:04 -0400 Subject: [PATCH 1/5] adding upsert for adopt-or-create --- pkg/runtime/reconciler.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index ff02467..622b485 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -333,13 +333,26 @@ func (r *resourceReconciler) handlePopulation( } populated := desired.DeepCopy() - err = populated.PopulateResourceFromAnnotation(adoptionFields) - // maybe don't return errors when it's adopt-or-create? + + // For adopt-or-create, we want to use the adoption fields as upsert fields + // rather than exact match fields // // TODO (michaelhtm) change PopulateResourceFromAnnotation to understand // adopt-or-create, and validate Spec fields are not nil... - if err != nil && adoptionPolicy != AdoptionPolicy_AdoptOrCreate { - return nil, err + + if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { + // For adopt-or-create, we want to be lenient about adoption field errors + // since they are used for upsert rather than exact matching + err = populated.PopulateResourceFromAnnotation(adoptionFields) + if err != nil { + rlog.Debug("Ignoring adoption field population error for adopt-or-create", "error", err) + } + } else { + // For regular adoption, maintain existing behavior + err = populated.PopulateResourceFromAnnotation(adoptionFields) + if err != nil { + return nil, err + } } return populated, nil @@ -490,8 +503,11 @@ func (r *resourceReconciler) Sync( } } else if !isReadOnly { if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { - // set adopt-or-create resource as managed before attempting - // update + // For adopt-or-create, we want to: + // 1. Mark the resource as managed and adopted + // 2. Update the resource with the desired state + // 3. Not require exact matching of adoption fields + rm.FilterSystemTags(latest) if err = r.setResourceManaged(ctx, rm, latest); err != nil { return latest, err } From e9ac7eabf8f9e0222a85dcc17d4ecbbd8f122ef7 Mon Sep 17 00:00:00 2001 From: Jonathan Graniero Date: Tue, 13 May 2025 13:17:15 -0400 Subject: [PATCH 2/5] upsert test --- pkg/runtime/reconciler_test.go | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index d5df314..9a2148d 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -457,6 +457,76 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) { rm.AssertNumberOfCalls(t, "Create", 0) } +func TestReconcilerAdoptOrCreateResource_Upsert(t *testing.T) { + require := require.New(t) + + ctx := context.TODO() + + desired, _, metaObj := resourceMocks() + desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() + metaObj.SetAnnotations(map[string]string{ + ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create", + ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}", + }) + + ids := &ackmocks.AWSResourceIdentifiers{} + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + + latest, latestRTObj, _ := resourceMocks() + latest.On("Identifiers").Return(ids) + latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) + latest.On("MetaObject").Return(metav1.ObjectMeta{ + Annotations: map[string]string{ + ackv1alpha1.AnnotationAdoptionPolicy: "adopt", + ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}", + }, + }) + latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) + latest.On( + "ReplaceConditions", + mock.AnythingOfType("[]*v1alpha1.Condition"), + ).Return() + + rm := &ackmocks.AWSResourceManager{} + rm.On("ResolveReferences", ctx, nil, desired).Return( + desired, false, nil, + ).Times(2) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) + rm.On("ReadOne", ctx, desired).Return( + latest, nil, + ).Once() + rm.On("Update", ctx, desired, latest, delta).Return( + latest, nil, + ).Once() + rm.On("IsSynced", ctx, latest).Return(true, nil) + rmf, rd := managedResourceManagerFactoryMocks(desired, latest) + + rm.On("LateInitialize", ctx, latest).Return(latest, nil) + rd.On("IsManaged", desired).Return(true) + rd.On("Delta", desired, latest).Return( + delta, + ).Once() + rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) + rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) + + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) + statusWriter := &ctrlrtclientmock.SubResourceWriter{} + kc.On("Status").Return(statusWriter) + kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) + statusWriter.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) + + // Test that we can update the resource even with non-matching adoption fields + _, err := r.Sync(ctx, rm, desired) + require.Nil(err) + rm.AssertNumberOfCalls(t, "ReadOne", 1) + rm.AssertCalled(t, "Update", ctx, desired, latest, delta) + rd.AssertCalled(t, "Delta", desired, latest) + rm.AssertNumberOfCalls(t, "Create", 0) +} + func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testing.T) { require := require.New(t) @@ -1331,6 +1401,8 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { rm.AssertCalled(t, "Update", ctx, desired, latest, delta) // No difference in desired, latest metadata and spec kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")) + // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus + kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } From 935fdf317c8838a41485428312e22ed7974eed7a Mon Sep 17 00:00:00 2001 From: Jonathan Graniero Date: Tue, 13 May 2025 15:16:19 -0400 Subject: [PATCH 3/5] update comments and deep copy desired for latest --- pkg/runtime/reconciler.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 622b485..ee41f9e 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -334,15 +334,11 @@ func (r *resourceReconciler) handlePopulation( populated := desired.DeepCopy() - // For adopt-or-create, we want to use the adoption fields as upsert fields - // rather than exact match fields - // - // TODO (michaelhtm) change PopulateResourceFromAnnotation to understand - // adopt-or-create, and validate Spec fields are not nil... - + // For adopt-or-create, we want to: + // 1. Strictly parse and validate adoption fields + // 2. If found, adopt the resource + // 3. Use spec values for updates if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { - // For adopt-or-create, we want to be lenient about adoption field errors - // since they are used for upsert rather than exact matching err = populated.PopulateResourceFromAnnotation(adoptionFields) if err != nil { rlog.Debug("Ignoring adoption field population error for adopt-or-create", "error", err) @@ -505,13 +501,15 @@ func (r *resourceReconciler) Sync( if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { // For adopt-or-create, we want to: // 1. Mark the resource as managed and adopted - // 2. Update the resource with the desired state + // 2. Update the resource with the desired state from spec // 3. Not require exact matching of adoption fields rm.FilterSystemTags(latest) if err = r.setResourceManaged(ctx, rm, latest); err != nil { return latest, err } r.rd.MarkAdopted(latest) + // Use the desired state from spec for updates + latest = desired.DeepCopy() } if latest, err = r.updateResource(ctx, rm, resolved, latest); err != nil { return latest, err From 4839be01359bfffb0ab8e3be203b3cb40c6e0c0c Mon Sep 17 00:00:00 2001 From: Jonathan Graniero Date: Tue, 13 May 2025 17:02:40 -0400 Subject: [PATCH 4/5] updating test to validate that the latest and desired are different --- pkg/runtime/reconciler_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 9a2148d..05d10ce 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -463,22 +463,34 @@ func TestReconcilerAdoptOrCreateResource_Upsert(t *testing.T) { ctx := context.TODO() desired, _, metaObj := resourceMocks() - desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() + desired.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return() metaObj.SetAnnotations(map[string]string{ ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create", ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}", }) + // Initialize the spec field in the desired object + metaObj.Object = map[string]interface{}{ + "spec": map[string]interface{}{ + "A": "val1", + }, + } ids := &ackmocks.AWSResourceIdentifiers{} delta := ackcompare.NewDelta() delta.Add("Spec.A", "val1", "val2") - latest, latestRTObj, _ := resourceMocks() + latest, latestRTObj, latestMetaObj := resourceMocks() latest.On("Identifiers").Return(ids) latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) + // Initialize the spec field in the latest object + latestMetaObj.Object = map[string]interface{}{ + "spec": map[string]interface{}{ + "A": "val2", + }, + } latest.On("MetaObject").Return(metav1.ObjectMeta{ Annotations: map[string]string{ - ackv1alpha1.AnnotationAdoptionPolicy: "adopt", + ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create", ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}", }, }) @@ -525,6 +537,10 @@ func TestReconcilerAdoptOrCreateResource_Upsert(t *testing.T) { rm.AssertCalled(t, "Update", ctx, desired, latest, delta) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertNumberOfCalls(t, "Create", 0) + + rm.AssertCalled(t, "Update", ctx, desired, latest, mock.MatchedBy(func(d *ackcompare.Delta) bool { + return d.DifferentAt("Spec.A") + })) } func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testing.T) { From b8ad462d014c163ddad221c74989613707609528 Mon Sep 17 00:00:00 2001 From: Jonathan Graniero Date: Wed, 14 May 2025 09:14:39 -0400 Subject: [PATCH 5/5] removed unnecessary lines from code review --- pkg/runtime/reconciler.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index ee41f9e..ee73c8f 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -499,17 +499,12 @@ func (r *resourceReconciler) Sync( } } else if !isReadOnly { if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { - // For adopt-or-create, we want to: - // 1. Mark the resource as managed and adopted - // 2. Update the resource with the desired state from spec - // 3. Not require exact matching of adoption fields - rm.FilterSystemTags(latest) + // set adopt-or-create resource as managed before attempting + // update if err = r.setResourceManaged(ctx, rm, latest); err != nil { return latest, err } r.rd.MarkAdopted(latest) - // Use the desired state from spec for updates - latest = desired.DeepCopy() } if latest, err = r.updateResource(ctx, rm, resolved, latest); err != nil { return latest, err