Skip to content

Commit 79724c6

Browse files
michaelhtmmichaelhtm
authored
ensure status is patched on adoption (#218)
Issue [#2682](aws-controllers-k8s/community#2682) ## Changes Made ### reconciler.go: - **Improved error handling**: Initialize `latest` with `desired.DeepCopy()` to ensure consistent resource state is returned on errors - **Fixed return values**: Use named return parameters and return appropriate resource state in all error paths - **Enhanced late initialization**: Separate late initialization result handling to prevent state loss - **Cleaner variable management**: Removed redundant variable declarations ### util.go: - **Fixed adoption fields extraction**: Return proper error when annotation is missing instead of empty string - **Simplified field lookup**: Use direct map access instead of iteration for better performance - **Corrected JSON unmarshaling**: Fix pointer usage in unmarshaling operation ### reconciler_test.go: - **Updated test mocks**: Align mock expectations with new resource state handling - **Fixed condition assertions**: Correct expected condition states for error scenarios - **Enhanced error path testing**: Better validation of resource states during error conditions ## Impact - Ensures consistent resource state is always returned from reconciler operations - Improves error handling reliability and debugging capabilities - Fixes potential nil pointer issues in error scenarios - Better test coverage alignment with actual implementation behavior By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 981220e commit 79724c6

File tree

4 files changed

+79
-58
lines changed

4 files changed

+79
-58
lines changed

‎pkg/errors/error.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,4 +123,9 @@ func (e TerminalError) Unwrap() error {
123123
return e.err
124124
}
125125

126+
func IsTerminalError(err error) bool {
127+
var terminalErr *TerminalError
128+
return err == Terminal || errors.As(err, &terminalErr)
129+
}
130+
126131
var _ error = &TerminalError{}

‎pkg/runtime/reconciler.go‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -505,21 +505,19 @@ func (r *resourceReconciler) Sync(
505505
ctx context.Context,
506506
rm acktypes.AWSResourceManager,
507507
desired acktypes.AWSResource,
508-
) (acktypes.AWSResource, error) {
509-
var err error
508+
) (latest acktypes.AWSResource, err error) {
510509
rlog := ackrtlog.FromContext(ctx)
511510
exit := rlog.Trace("r.Sync")
512511
defer func() {
513512
exit(err)
514513
}()
515514

516-
var latest acktypes.AWSResource // the newly created or mutated resource
517-
518515
r.resetConditions(ctx, desired)
519516
defer func() {
520517
r.ensureConditions(ctx, rm, latest, err)
521518
}()
522519

520+
latest = desired.DeepCopy()
523521
isAdopted := IsAdopted(desired)
524522
rlog.WithValues("is_adopted", isAdopted)
525523

@@ -528,7 +526,7 @@ func (r *resourceReconciler) Sync(
528526
needAdoption := NeedAdoption(desired) && !r.rd.IsManaged(desired) && r.cfg.FeatureGates.IsEnabled(featuregate.ResourceAdoption)
529527
adoptionPolicy, err := GetAdoptionPolicy(desired)
530528
if err != nil {
531-
return nil, err
529+
return latest, err
532530
}
533531
if !needAdoption {
534532
adoptionPolicy = ""
@@ -543,9 +541,9 @@ func (r *resourceReconciler) Sync(
543541
rlog.Enter("rm.ResolveReferences")
544542
resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired)
545543
rlog.Exit("rm.ResolveReferences", err)
546-
// TODO (michaelhtm): ignore error only for `adopt` or `read-only`
547544
if err != nil && (adoptionPolicy == "" || adoptionPolicy == AdoptionPolicy_AdoptOrCreate) && !isReadOnly {
548-
return ackcondition.WithReferencesResolvedCondition(desired, err), err
545+
latest = ackcondition.WithReferencesResolvedCondition(latest, err)
546+
return latest, err
549547
}
550548
if hasReferences {
551549
resolved = ackcondition.WithReferencesResolvedCondition(resolved, err)
@@ -554,7 +552,7 @@ func (r *resourceReconciler) Sync(
554552
if needAdoption {
555553
populated, err := r.handlePopulation(ctx, desired)
556554
if err != nil {
557-
return nil, err
555+
return latest, err
558556
}
559557
if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
560558
// here we assume the spec fields are provided in the spec.
@@ -579,25 +577,25 @@ func (r *resourceReconciler) Sync(
579577
rlog.Exit("rm.ReadOne", err)
580578
if err != nil {
581579
if err != ackerr.NotFound {
582-
return latest, err
580+
return resolved, err
583581
}
584582
if adoptionPolicy == AdoptionPolicy_Adopt || isAdopted {
585-
return nil, ackerr.AdoptedResourceNotFound
583+
return resolved, ackerr.AdoptedResourceNotFound
586584
}
587585
if isReadOnly {
588-
return nil, ackerr.ReadOnlyResourceNotFound
586+
return resolved, ackerr.ReadOnlyResourceNotFound
589587
}
590588
if latest, err = r.createResource(ctx, rm, resolved); err != nil {
591-
return latest, err
589+
return resolved, err
592590
}
593591
} else if adoptionPolicy == AdoptionPolicy_Adopt {
594592
rm.FilterSystemTags(latest, r.cfg.ResourceTagKeys)
595593
if err = r.setResourceManagedAndAdopted(ctx, rm, latest); err != nil {
596-
return latest, err
594+
return resolved, err
597595
}
598596
latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest)
599597
if err != nil {
600-
return latest, err
598+
return resolved, err
601599
}
602600
} else if isReadOnly {
603601
delta := r.rd.Delta(desired, latest)
@@ -625,10 +623,11 @@ func (r *resourceReconciler) Sync(
625623
}
626624
// Attempt to late initialize the resource. If there are no fields to
627625
// late initialize, this operation will be a no-op.
628-
if latest, err = r.lateInitializeResource(ctx, rm, latest); err != nil {
626+
lateInitialized, err := r.lateInitializeResource(ctx, rm, latest)
627+
if err != nil {
629628
return latest, err
630629
}
631-
return latest, nil
630+
return lateInitialized, nil
632631
}
633632

634633
// resetConditions strips the supplied resource of all objects in its
@@ -670,6 +669,13 @@ func (r *resourceReconciler) ensureConditions(
670669
exit(err)
671670
}()
672671

672+
if ackcondition.Terminal(res) == nil {
673+
if ackerr.IsTerminalError(reconcileErr) {
674+
condReason := reconcileErr.Error()
675+
ackcondition.SetTerminal(res, corev1.ConditionTrue, &condReason, nil)
676+
}
677+
}
678+
673679
// If the ACK.ResourceSynced condition is not set using the custom hooks,
674680
// determine the Synced condition using "rm.IsSynced" method
675681
if ackcondition.Synced(res) == nil {
@@ -688,7 +694,7 @@ func (r *resourceReconciler) ensureConditions(
688694

689695
if reconcileErr != nil {
690696
condReason = reconcileErr.Error()
691-
if reconcileErr == ackerr.Terminal {
697+
if ackerr.IsTerminalError(reconcileErr) {
692698
// A terminal condition is a stable state for a resource.
693699
// Terminal conditions indicate that without changes to the
694700
// desired state of a resource, the resource's desired state
@@ -1319,6 +1325,7 @@ func (r *resourceReconciler) HandleReconcileError(
13191325
latest acktypes.AWSResource,
13201326
err error,
13211327
) (ctrlrt.Result, error) {
1328+
rlog := ackrtlog.FromContext(ctx)
13221329
if ackcompare.IsNotNil(latest) {
13231330
// The reconciliation loop may have returned an error, but if latest is
13241331
// not nil, there may be some changes available in the CR's Status
@@ -1335,10 +1342,11 @@ func (r *resourceReconciler) HandleReconcileError(
13351342
// there is a more robust way to handle failures in the patch operation
13361343
_ = r.patchResourceStatus(ctx, desired, latest)
13371344
}
1338-
if err == nil || err == ackerr.Terminal {
1345+
1346+
if ackerr.IsTerminalError(err) {
1347+
rlog.Info("resource is terminal")
13391348
return ctrlrt.Result{}, nil
13401349
}
1341-
rlog := ackrtlog.FromContext(ctx)
13421350

13431351
var requeueNeededAfter *requeue.RequeueNeededAfter
13441352
if errors.As(err, &requeueNeededAfter) {

‎pkg/runtime/reconciler_test.go‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,8 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) {
252252
latest, latestRTObj, _ := resourceMocks()
253253
latest.On("Identifiers").Return(ids)
254254

255-
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
256-
latest.On(
255+
desired.On("Conditions").Return([]*ackv1alpha1.Condition{})
256+
desired.On(
257257
"ReplaceConditions",
258258
mock.AnythingOfType("[]*v1alpha1.Condition"),
259259
).Return()
@@ -270,7 +270,7 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) {
270270
rm.On("Create", ctx, desired).Return(
271271
latest, awsError{},
272272
)
273-
rm.On("IsSynced", ctx, latest).Return(false, nil)
273+
rm.On("IsSynced", ctx, desired).Return(false, nil)
274274
rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
275275
rd.On("IsManaged", desired).Return(false).Twice()
276276
rd.On("IsManaged", desired).Return(true)
@@ -1495,12 +1495,13 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
14951495

14961496
desired, _, _ := resourceMocks()
14971497
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
1498-
1498+
14991499
ids := &ackmocks.AWSResourceIdentifiers{}
15001500
ids.On("ARN").Return(&arn)
1501-
1501+
15021502
latest, _, _ := resourceMocks()
15031503
latest.On("Identifiers").Return(ids)
1504+
desired.On("DeepCopy").Return(latest)
15041505

15051506
terminalCondition := ackv1alpha1.Condition{
15061507
Type: ackv1alpha1.ConditionTypeTerminal,
@@ -1514,6 +1515,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
15141515
// These calls will be made from ensureConditions method, which sets
15151516
// ACK.ResourceSynced condition correctly
15161517
latest.On("Conditions").Return([]*ackv1alpha1.Condition{&terminalCondition})
1518+
desired.On("Conditions").Return([]*ackv1alpha1.Condition{&terminalCondition})
15171519

15181520
// Verify once when the terminal condition is set
15191521
latest.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return([]*ackv1alpha1.Condition{&terminalCondition}).Run(func(args mock.Arguments) {
@@ -1603,7 +1605,7 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) {
16031605
delta.Add("Spec.A", "val1", "val2")
16041606

16051607
desired, _, _ := resourceMocks()
1606-
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
1608+
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once()
16071609

16081610
ids := &ackmocks.AWSResourceIdentifiers{}
16091611
ids.On("ARN").Return(&arn)
@@ -1631,30 +1633,35 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) {
16311633
assert.Equal(t, corev1.ConditionUnknown, cond.Status)
16321634
assert.Equal(t, ackcondition.FailedReferenceResolutionMessage, *cond.Message)
16331635
assert.Equal(t, resolveReferenceError.Error(), *cond.Reason)
1634-
})
1636+
}).Once()
1637+
desired.On(
1638+
"ReplaceConditions",
1639+
mock.AnythingOfType("[]*v1alpha1.Condition"),
1640+
).Return().Run(func(args mock.Arguments) {
1641+
conditions := args.Get(0).([]*ackv1alpha1.Condition)
1642+
assert.Equal(t, 1, len(conditions))
1643+
cond := conditions[0]
1644+
assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type)
1645+
// The non-terminal reconciler error causes the ReferencesResolved
1646+
// condition to be Unknown
1647+
assert.Equal(t, corev1.ConditionUnknown, cond.Status)
1648+
assert.Equal(t, ackcondition.UnknownSyncedMessage, *cond.Message)
1649+
assert.Equal(t, resolveReferenceError.Error(), *cond.Reason)
1650+
}).Once()
1651+
desired.On(
1652+
"ReplaceConditions",
1653+
mock.AnythingOfType("[]*v1alpha1.Condition"),
1654+
).Return()
16351655

16361656
rm := &ackmocks.AWSResourceManager{}
16371657
rm.On("ResolveReferences", ctx, nil, desired).Return(
16381658
desired, true, resolveReferenceError,
16391659
)
16401660
rm.On("ClearResolvedReferences", desired).Return(desired)
16411661
rm.On("ClearResolvedReferences", latest).Return(latest)
1642-
rm.On("ReadOne", ctx, desired).Return(
1643-
latest, nil,
1644-
)
1645-
rm.On("Update", ctx, desired, latest, delta).Return(
1646-
latest, nil,
1647-
)
16481662

16491663
rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
1650-
rd.On("Delta", desired, latest).Return(
1651-
delta,
1652-
).Once()
1653-
rd.On("Delta", desired, latest).Return(ackcompare.NewDelta())
1654-
1655-
rm.On("LateInitialize", ctx, latest).Return(latest, nil)
1656-
rm.On("IsSynced", ctx, latest).Return(true, nil)
1657-
rd.On("Delta", latest, latest).Return(ackcompare.NewDelta())
1664+
rm.On("IsSynced", ctx, desired).Return(true, nil)
16581665

16591666
r, kc, scmd := reconcilerMocks(rmf)
16601667
rm.On("EnsureTags", ctx, desired, scmd).Return(nil)
@@ -1690,7 +1697,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) {
16901697
delta.Add("Spec.A", "val1", "val2")
16911698

16921699
desired, _, _ := resourceMocks()
1693-
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
1700+
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once()
16941701

16951702
ids := &ackmocks.AWSResourceIdentifiers{}
16961703
ids.On("ARN").Return(&arn)
@@ -1704,8 +1711,8 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) {
17041711
// manager has not set any Conditions on the resource, that at least an
17051712
// ACK.ResourceSynced condition with status Unknown will be set on the
17061713
// resource.
1707-
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
1708-
latest.On(
1714+
desired.On("Conditions").Return([]*ackv1alpha1.Condition{})
1715+
desired.On(
17091716
"ReplaceConditions",
17101717
mock.AnythingOfType("[]*v1alpha1.Condition"),
17111718
).Return().Run(func(args mock.Arguments) {
@@ -1715,10 +1722,11 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) {
17151722
assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type)
17161723
// The non-terminal reconciler error causes the ResourceSynced
17171724
// condition to be False
1718-
assert.Equal(t, corev1.ConditionFalse, cond.Status)
1719-
assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message)
1725+
assert.Equal(t, corev1.ConditionUnknown, cond.Status)
1726+
assert.Equal(t, ackcondition.UnknownSyncedMessage, *cond.Message)
17201727
assert.Equal(t, ensureControllerTagsError.Error(), *cond.Reason)
1721-
})
1728+
}).Once()
1729+
desired.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return()
17221730

17231731
rm := &ackmocks.AWSResourceManager{}
17241732
rm.On("ResolveReferences", ctx, nil, desired).Return(desired, false, nil)
@@ -1738,7 +1746,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) {
17381746
rd.On("Delta", desired, latest).Return(ackcompare.NewDelta())
17391747

17401748
rm.On("LateInitialize", ctx, latest).Return(latest, nil)
1741-
rm.On("IsSynced", ctx, latest).Return(true, nil)
1749+
rm.On("IsSynced", ctx, desired).Return(false, nil)
17421750
rd.On("Delta", latest, latest).Return(ackcompare.NewDelta())
17431751

17441752
r, kc, scmd := reconcilerMocks(rmf)

‎pkg/runtime/util.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -208,30 +208,30 @@ func NeedAdoption(res acktypes.AWSResource) bool {
208208
}
209209

210210
func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) {
211-
fields := getAdoptionFields(res)
211+
fields, exists := getAdoptionFields(res)
212+
if !exists {
213+
return nil, fmt.Errorf("%s is not defined", ackv1alpha1.AnnotationAdoptionFields)
214+
}
212215

213-
extractedFields := &map[string]string{}
214-
err := json.Unmarshal([]byte(fields), extractedFields)
216+
extractedFields := map[string]string{}
217+
err := json.Unmarshal([]byte(fields), &extractedFields)
215218
if err != nil {
216219
return nil, err
217220
}
218221

219-
return *extractedFields, nil
222+
return extractedFields, nil
220223
}
221224

222-
func getAdoptionFields(res acktypes.AWSResource) string {
225+
func getAdoptionFields(res acktypes.AWSResource) (string, bool) {
223226
mo := res.MetaObject()
224227
if mo == nil {
225228
// Should never happen... if it does, it's buggy code.
226229
panic("ExtractRequiredFields received resource with nil RuntimeObject")
227230
}
228231

229-
for k, v := range mo.GetAnnotations() {
230-
if k == ackv1alpha1.AnnotationAdoptionFields {
231-
return v
232-
}
233-
}
234-
return ""
232+
fields, ok := mo.GetAnnotations()[ackv1alpha1.AnnotationAdoptionFields]
233+
234+
return fields, ok
235235
}
236236

237237
// patchObject performs a patch operation using context.WithoutCancel to prevent

0 commit comments

Comments
 (0)