diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 0a17820..7c5053d 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -123,4 +123,9 @@ func (e TerminalError) Unwrap() error { return e.err } +func IsTerminalError(err error) bool { + var terminalErr *TerminalError + return err == Terminal || errors.As(err, &terminalErr) +} + var _ error = &TerminalError{} diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 2abf836..e41c59c 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -505,21 +505,19 @@ func (r *resourceReconciler) Sync( ctx context.Context, rm acktypes.AWSResourceManager, desired acktypes.AWSResource, -) (acktypes.AWSResource, error) { - var err error +) (latest acktypes.AWSResource, err error) { rlog := ackrtlog.FromContext(ctx) exit := rlog.Trace("r.Sync") defer func() { exit(err) }() - var latest acktypes.AWSResource // the newly created or mutated resource - r.resetConditions(ctx, desired) defer func() { r.ensureConditions(ctx, rm, latest, err) }() + latest = desired.DeepCopy() isAdopted := IsAdopted(desired) rlog.WithValues("is_adopted", isAdopted) @@ -528,7 +526,7 @@ func (r *resourceReconciler) Sync( needAdoption := NeedAdoption(desired) && !r.rd.IsManaged(desired) && r.cfg.FeatureGates.IsEnabled(featuregate.ResourceAdoption) adoptionPolicy, err := GetAdoptionPolicy(desired) if err != nil { - return nil, err + return latest, err } if !needAdoption { adoptionPolicy = "" @@ -543,9 +541,9 @@ func (r *resourceReconciler) Sync( rlog.Enter("rm.ResolveReferences") resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired) rlog.Exit("rm.ResolveReferences", err) - // TODO (michaelhtm): ignore error only for `adopt` or `read-only` if err != nil && (adoptionPolicy == "" || adoptionPolicy == AdoptionPolicy_AdoptOrCreate) && !isReadOnly { - return ackcondition.WithReferencesResolvedCondition(desired, err), err + latest = ackcondition.WithReferencesResolvedCondition(latest, err) + return latest, err } if hasReferences { resolved = ackcondition.WithReferencesResolvedCondition(resolved, err) @@ -554,7 +552,7 @@ func (r *resourceReconciler) Sync( if needAdoption { populated, err := r.handlePopulation(ctx, desired) if err != nil { - return nil, err + return latest, err } if adoptionPolicy == AdoptionPolicy_AdoptOrCreate { // here we assume the spec fields are provided in the spec. @@ -579,25 +577,25 @@ func (r *resourceReconciler) Sync( rlog.Exit("rm.ReadOne", err) if err != nil { if err != ackerr.NotFound { - return latest, err + return resolved, err } if adoptionPolicy == AdoptionPolicy_Adopt || isAdopted { - return nil, ackerr.AdoptedResourceNotFound + return resolved, ackerr.AdoptedResourceNotFound } if isReadOnly { - return nil, ackerr.ReadOnlyResourceNotFound + return resolved, ackerr.ReadOnlyResourceNotFound } if latest, err = r.createResource(ctx, rm, resolved); err != nil { - return latest, err + return resolved, err } } else if adoptionPolicy == AdoptionPolicy_Adopt { rm.FilterSystemTags(latest, r.cfg.ResourceTagKeys) if err = r.setResourceManagedAndAdopted(ctx, rm, latest); err != nil { - return latest, err + return resolved, err } latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest) if err != nil { - return latest, err + return resolved, err } } else if isReadOnly { delta := r.rd.Delta(desired, latest) @@ -625,10 +623,11 @@ func (r *resourceReconciler) Sync( } // Attempt to late initialize the resource. If there are no fields to // late initialize, this operation will be a no-op. - if latest, err = r.lateInitializeResource(ctx, rm, latest); err != nil { + lateInitialized, err := r.lateInitializeResource(ctx, rm, latest) + if err != nil { return latest, err } - return latest, nil + return lateInitialized, nil } // resetConditions strips the supplied resource of all objects in its @@ -670,6 +669,13 @@ func (r *resourceReconciler) ensureConditions( exit(err) }() + if ackcondition.Terminal(res) == nil { + if ackerr.IsTerminalError(reconcileErr) { + condReason := reconcileErr.Error() + ackcondition.SetTerminal(res, corev1.ConditionTrue, &condReason, nil) + } + } + // If the ACK.ResourceSynced condition is not set using the custom hooks, // determine the Synced condition using "rm.IsSynced" method if ackcondition.Synced(res) == nil { @@ -688,7 +694,7 @@ func (r *resourceReconciler) ensureConditions( if reconcileErr != nil { condReason = reconcileErr.Error() - if reconcileErr == ackerr.Terminal { + if ackerr.IsTerminalError(reconcileErr) { // A terminal condition is a stable state for a resource. // Terminal conditions indicate that without changes to the // desired state of a resource, the resource's desired state @@ -1319,6 +1325,7 @@ func (r *resourceReconciler) HandleReconcileError( latest acktypes.AWSResource, err error, ) (ctrlrt.Result, error) { + rlog := ackrtlog.FromContext(ctx) if ackcompare.IsNotNil(latest) { // The reconciliation loop may have returned an error, but if latest is // not nil, there may be some changes available in the CR's Status @@ -1335,10 +1342,11 @@ func (r *resourceReconciler) HandleReconcileError( // there is a more robust way to handle failures in the patch operation _ = r.patchResourceStatus(ctx, desired, latest) } - if err == nil || err == ackerr.Terminal { + + if ackerr.IsTerminalError(err) { + rlog.Info("resource is terminal") return ctrlrt.Result{}, nil } - rlog := ackrtlog.FromContext(ctx) var requeueNeededAfter *requeue.RequeueNeededAfter if errors.As(err, &requeueNeededAfter) { diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 9dd9cac..ceb855e 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -252,8 +252,8 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) { latest, latestRTObj, _ := resourceMocks() latest.On("Identifiers").Return(ids) - latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) - latest.On( + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On( "ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition"), ).Return() @@ -270,7 +270,7 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) { rm.On("Create", ctx, desired).Return( latest, awsError{}, ) - rm.On("IsSynced", ctx, latest).Return(false, nil) + rm.On("IsSynced", ctx, desired).Return(false, nil) rmf, rd := managedResourceManagerFactoryMocks(desired, latest) rd.On("IsManaged", desired).Return(false).Twice() rd.On("IsManaged", desired).Return(true) @@ -1495,12 +1495,13 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { desired, _, _ := resourceMocks() desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() - + ids := &ackmocks.AWSResourceIdentifiers{} ids.On("ARN").Return(&arn) - + latest, _, _ := resourceMocks() latest.On("Identifiers").Return(ids) + desired.On("DeepCopy").Return(latest) terminalCondition := ackv1alpha1.Condition{ Type: ackv1alpha1.ConditionTypeTerminal, @@ -1514,6 +1515,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { // These calls will be made from ensureConditions method, which sets // ACK.ResourceSynced condition correctly latest.On("Conditions").Return([]*ackv1alpha1.Condition{&terminalCondition}) + desired.On("Conditions").Return([]*ackv1alpha1.Condition{&terminalCondition}) // Verify once when the terminal condition is set 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) { delta.Add("Spec.A", "val1", "val2") desired, _, _ := resourceMocks() - desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() + desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once() ids := &ackmocks.AWSResourceIdentifiers{} ids.On("ARN").Return(&arn) @@ -1631,7 +1633,25 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { assert.Equal(t, corev1.ConditionUnknown, cond.Status) assert.Equal(t, ackcondition.FailedReferenceResolutionMessage, *cond.Message) assert.Equal(t, resolveReferenceError.Error(), *cond.Reason) - }) + }).Once() + desired.On( + "ReplaceConditions", + mock.AnythingOfType("[]*v1alpha1.Condition"), + ).Return().Run(func(args mock.Arguments) { + conditions := args.Get(0).([]*ackv1alpha1.Condition) + assert.Equal(t, 1, len(conditions)) + cond := conditions[0] + assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type) + // The non-terminal reconciler error causes the ReferencesResolved + // condition to be Unknown + assert.Equal(t, corev1.ConditionUnknown, cond.Status) + assert.Equal(t, ackcondition.UnknownSyncedMessage, *cond.Message) + assert.Equal(t, resolveReferenceError.Error(), *cond.Reason) + }).Once() + desired.On( + "ReplaceConditions", + mock.AnythingOfType("[]*v1alpha1.Condition"), + ).Return() rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( @@ -1639,22 +1659,9 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) - rm.On("ReadOne", ctx, desired).Return( - latest, nil, - ) - rm.On("Update", ctx, desired, latest, delta).Return( - latest, nil, - ) rmf, rd := managedResourceManagerFactoryMocks(desired, latest) - rd.On("Delta", desired, latest).Return( - delta, - ).Once() - rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) - - rm.On("LateInitialize", ctx, latest).Return(latest, nil) - rm.On("IsSynced", ctx, latest).Return(true, nil) - rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) + rm.On("IsSynced", ctx, desired).Return(true, nil) r, kc, scmd := reconcilerMocks(rmf) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) @@ -1690,7 +1697,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { delta.Add("Spec.A", "val1", "val2") desired, _, _ := resourceMocks() - desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() + desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once() ids := &ackmocks.AWSResourceIdentifiers{} ids.On("ARN").Return(&arn) @@ -1704,8 +1711,8 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { // manager has not set any Conditions on the resource, that at least an // ACK.ResourceSynced condition with status Unknown will be set on the // resource. - latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) - latest.On( + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On( "ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition"), ).Return().Run(func(args mock.Arguments) { @@ -1715,10 +1722,11 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type) // The non-terminal reconciler error causes the ResourceSynced // condition to be False - assert.Equal(t, corev1.ConditionFalse, cond.Status) - assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message) + assert.Equal(t, corev1.ConditionUnknown, cond.Status) + assert.Equal(t, ackcondition.UnknownSyncedMessage, *cond.Message) assert.Equal(t, ensureControllerTagsError.Error(), *cond.Reason) - }) + }).Once() + desired.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return() rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return(desired, false, nil) @@ -1738,7 +1746,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) rm.On("LateInitialize", ctx, latest).Return(latest, nil) - rm.On("IsSynced", ctx, latest).Return(true, nil) + rm.On("IsSynced", ctx, desired).Return(false, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) diff --git a/pkg/runtime/util.go b/pkg/runtime/util.go index 81faca5..cd27f1f 100644 --- a/pkg/runtime/util.go +++ b/pkg/runtime/util.go @@ -208,30 +208,30 @@ func NeedAdoption(res acktypes.AWSResource) bool { } func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) { - fields := getAdoptionFields(res) + fields, exists := getAdoptionFields(res) + if !exists { + return nil, fmt.Errorf("%s is not defined", ackv1alpha1.AnnotationAdoptionFields) + } - extractedFields := &map[string]string{} - err := json.Unmarshal([]byte(fields), extractedFields) + extractedFields := map[string]string{} + err := json.Unmarshal([]byte(fields), &extractedFields) if err != nil { return nil, err } - return *extractedFields, nil + return extractedFields, nil } -func getAdoptionFields(res acktypes.AWSResource) string { +func getAdoptionFields(res acktypes.AWSResource) (string, bool) { mo := res.MetaObject() if mo == nil { // Should never happen... if it does, it's buggy code. panic("ExtractRequiredFields received resource with nil RuntimeObject") } - for k, v := range mo.GetAnnotations() { - if k == ackv1alpha1.AnnotationAdoptionFields { - return v - } - } - return "" + fields, ok := mo.GetAnnotations()[ackv1alpha1.AnnotationAdoptionFields] + + return fields, ok } // patchObject performs a patch operation using context.WithoutCancel to prevent