Merge pull request #220 from michaelhtm/revertandfix

Revert "ensure status is patched on adoption (#218)"

Author: a-hilaly

pkg/errors/error.go

@@ -123,9 +123,4 @@ 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{}

pkg/runtime/reconciler.go

@@ -505,19 +505,21 @@ func (r *resourceReconciler) Sync(
 	ctx context.Context,
 	rm acktypes.AWSResourceManager,
 	desired acktypes.AWSResource,
-) (latest acktypes.AWSResource, err error) {
+) (acktypes.AWSResource, error) {
+	var 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)
 
@@ -526,7 +528,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 latest, err
+		return nil, err
 	}
 	if !needAdoption {
 		adoptionPolicy = ""
@@ -541,9 +543,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 {
-		latest = ackcondition.WithReferencesResolvedCondition(latest, err)
-		return latest, err
+		return ackcondition.WithReferencesResolvedCondition(desired, err), err
 	}
 	if hasReferences {
 		resolved = ackcondition.WithReferencesResolvedCondition(resolved, err)
@@ -552,7 +554,7 @@ func (r *resourceReconciler) Sync(
 	if needAdoption {
 		populated, err := r.handlePopulation(ctx, desired)
 		if err != nil {
-			return latest, err
+			return nil, err
 		}
 		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
 			// here we assume the spec fields are provided in the spec.
@@ -577,25 +579,25 @@ func (r *resourceReconciler) Sync(
 	rlog.Exit("rm.ReadOne", err)
 	if err != nil {
 		if err != ackerr.NotFound {
-			return resolved, err
+			return latest, err
 		}
 		if adoptionPolicy == AdoptionPolicy_Adopt || isAdopted {
-			return resolved, ackerr.AdoptedResourceNotFound
+			return nil, ackerr.AdoptedResourceNotFound
 		}
 		if isReadOnly {
-			return resolved, ackerr.ReadOnlyResourceNotFound
+			return nil, ackerr.ReadOnlyResourceNotFound
 		}
 		if latest, err = r.createResource(ctx, rm, resolved); err != nil {
-			return resolved, err
+			return latest, err
 		}
 	} else if adoptionPolicy == AdoptionPolicy_Adopt {
 		rm.FilterSystemTags(latest, r.cfg.ResourceTagKeys)
 		if err = r.setResourceManagedAndAdopted(ctx, rm, latest); err != nil {
-			return resolved, err
+			return latest, err
 		}
 		latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest)
 		if err != nil {
-			return resolved, err
+			return latest, err
 		}
 	} else if isReadOnly {
 		delta := r.rd.Delta(desired, latest)
@@ -623,11 +625,10 @@ 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.
-	lateInitialized, err := r.lateInitializeResource(ctx, rm, latest)
-	if err != nil {
+	if latest, err = r.lateInitializeResource(ctx, rm, latest); err != nil {
 		return latest, err
 	}
-	return lateInitialized, nil
+	return latest, nil
 }
 
 // resetConditions strips the supplied resource of all objects in its
@@ -669,13 +670,6 @@ 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 {
@@ -694,7 +688,7 @@ func (r *resourceReconciler) ensureConditions(
 
 		if reconcileErr != nil {
 			condReason = reconcileErr.Error()
-			if ackerr.IsTerminalError(reconcileErr) {
+			if reconcileErr == ackerr.Terminal {
 				// 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
@@ -1325,7 +1319,6 @@ 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
@@ -1342,11 +1335,10 @@ func (r *resourceReconciler) HandleReconcileError(
 		// there is a more robust way to handle failures in the patch operation
 		_ = r.patchResourceStatus(ctx, desired, latest)
 	}
-
-	if ackerr.IsTerminalError(err) {
-		rlog.Info("resource is terminal")
+	if err == nil || err == ackerr.Terminal {
 		return ctrlrt.Result{}, nil
 	}
+	rlog := ackrtlog.FromContext(ctx)
 
 	var requeueNeededAfter *requeue.RequeueNeededAfter
 	if errors.As(err, &requeueNeededAfter) {

pkg/runtime/reconciler_test.go

@@ -252,8 +252,8 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) {
 	latest, latestRTObj, _ := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 
-	desired.On("Conditions").Return([]*ackv1alpha1.Condition{})
-	desired.On(
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.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, desired).Return(false, nil)
+	rm.On("IsSynced", ctx, latest).Return(false, nil)
 	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 	rd.On("IsManaged", desired).Return(false).Twice()
 	rd.On("IsManaged", desired).Return(true)
@@ -1495,13 +1495,12 @@ 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,
@@ -1515,7 +1514,6 @@ 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) {
@@ -1605,7 +1603,7 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, _, _ := resourceMocks()
-	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
@@ -1633,35 +1631,30 @@ 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(
 		desired, true, resolveReferenceError,
 	)
 	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)
-	rm.On("IsSynced", ctx, desired).Return(true, nil)
+	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())
 
 	r, kc, scmd := reconcilerMocks(rmf)
 	rm.On("EnsureTags", ctx, desired, scmd).Return(nil)
@@ -1697,7 +1690,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, _, _ := resourceMocks()
-	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return().Once()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
@@ -1711,8 +1704,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.
-	desired.On("Conditions").Return([]*ackv1alpha1.Condition{})
-	desired.On(
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
 		"ReplaceConditions",
 		mock.AnythingOfType("[]*v1alpha1.Condition"),
 	).Return().Run(func(args mock.Arguments) {
@@ -1722,11 +1715,10 @@ 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.ConditionUnknown, cond.Status)
-		assert.Equal(t, ackcondition.UnknownSyncedMessage, *cond.Message)
+		assert.Equal(t, corev1.ConditionFalse, cond.Status)
+		assert.Equal(t, ackcondition.NotSyncedMessage, *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)
@@ -1746,7 +1738,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, desired).Return(false, nil)
+	rm.On("IsSynced", ctx, latest).Return(true, nil)
 	rd.On("Delta", latest, latest).Return(ackcompare.NewDelta())
 
 	r, kc, scmd := reconcilerMocks(rmf)

pkg/runtime/util.go

@@ -208,30 +208,30 @@ func NeedAdoption(res acktypes.AWSResource) bool {
 }
 
 func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) {
-	fields, exists := getAdoptionFields(res)
-	if !exists {
-		return nil, fmt.Errorf("%s is not defined", ackv1alpha1.AnnotationAdoptionFields)
-	}
+	fields := getAdoptionFields(res)
 
-	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, bool) {
+func getAdoptionFields(res acktypes.AWSResource) string {
 	mo := res.MetaObject()
 	if mo == nil {
 		// Should never happen... if it does, it's buggy code.
 		panic("ExtractRequiredFields received resource with nil RuntimeObject")
 	}
 
-	fields, ok := mo.GetAnnotations()[ackv1alpha1.AnnotationAdoptionFields]
-
-	return fields, ok
+	for k, v := range mo.GetAnnotations() {
+		if k == ackv1alpha1.AnnotationAdoptionFields {
+			return v
+		}
+	}
+	return ""
 }
 
 // patchObject performs a patch operation using context.WithoutCancel to prevent