Cache the error from the last asynchronous reconciliation to return i…

…t in the next asynchronous reconciliation for the Terraform plugin SDK & framework based external clients. - Set the "Synced" status condition to "False" in the async CallbackFn to immediately update it when the async operation fails. - Set the "Synced" status condition to "True" when the async operation succeeds, or when the external client's Observe call reveals an up-to-date external resource which is not scheduled for deletion. Signed-off-by: Alper Rifat Ulucinar <[email protected]>
crossplane · Apr 24, 2024 · 0ae1a67 · 0ae1a67
1 parent 4c67d8e
commit 0ae1a67
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 9 deletions.
diff --git a/pkg/controller/api.go b/pkg/controller/api.go
@@ -27,6 +27,13 @@ const (
 	errReconcileRequestFmt = "cannot request the reconciliation of the resource %s/%s after an async %s"
 )
 
+// crossplane-runtime error constants
+const (
+	errXPReconcileCreate = "create failed"
+	errXPReconcileUpdate = "update failed"
+	errXPReconcileDelete = "delete failed"
+)
+
 const (
 	rateLimiterCallback = "asyncCallback"
 )
@@ -119,6 +126,20 @@ func (ac *APICallbacks) callbackFn(name, op string) terraform.CallbackFn {
 		// to do so. So we keep the `LastAsyncOperation` condition.
 		// TODO: move this to the `Synced` condition.
 		tr.SetConditions(resource.LastAsyncOperationCondition(err))
+		if err != nil {
+			wrapMsg := ""
+			switch op {
+			case "create":
+				wrapMsg = errXPReconcileCreate
+			case "update":
+				wrapMsg = errXPReconcileUpdate
+			case "destroy":
+				wrapMsg = errXPReconcileDelete
+			}
+			tr.SetConditions(xpv1.ReconcileError(errors.Wrap(err, wrapMsg)))
+		} else {
+			tr.SetConditions(xpv1.ReconcileSuccess())
+		}
 		if ac.enableStatusUpdates {
 			tr.SetConditions(resource.AsyncOperationFinishedCondition())
 		}

diff --git a/pkg/controller/external_async_tfpluginfw.go b/pkg/controller/external_async_tfpluginfw.go
@@ -7,6 +7,7 @@ package controller
 import (
 	"context"
 
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
@@ -124,6 +125,7 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Observe(ctx context.Contex
 	// not scheduled to be deleted.
 	if err == nil && o.ResourceExists && o.ResourceUpToDate && !meta.WasDeleted(mg) {
 		mg.(resource.Terraformed).SetConditions(resource.LastAsyncOperationCondition(nil))
+		mg.(resource.Terraformed).SetConditions(xpv1.ReconcileSuccess())
 	}
 	return o, err
 }
@@ -149,7 +151,7 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Create(_ context.Context,
 		}
 	}()
 
-	return managed.ExternalCreation{}, nil
+	return managed.ExternalCreation{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context, mg xpresource.Managed) (managed.ExternalUpdate, error) {
@@ -173,7 +175,7 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context,
 		}
 	}()
 
-	return managed.ExternalUpdate{}, nil
+	return managed.ExternalUpdate{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginFrameworkAsyncExternalClient) Delete(_ context.Context, mg xpresource.Managed) error {
@@ -200,5 +202,5 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Delete(_ context.Context,
 		}
 	}()
 
-	return nil
+	return n.opTracker.LastOperation.Error()
 }
diff --git a/pkg/controller/external_async_tfpluginsdk.go b/pkg/controller/external_async_tfpluginsdk.go
@@ -8,6 +8,7 @@ import (
 	"context"
 	"time"
 
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
@@ -129,6 +130,7 @@ func (n *terraformPluginSDKAsyncExternal) Observe(ctx context.Context, mg xpreso
 	// not scheduled to be deleted.
 	if err == nil && o.ResourceExists && o.ResourceUpToDate && !meta.WasDeleted(mg) {
 		mg.(resource.Terraformed).SetConditions(resource.LastAsyncOperationCondition(nil))
+		mg.(resource.Terraformed).SetConditions(xpv1.ReconcileSuccess())
 	}
 	return o, err
 }
@@ -154,7 +156,7 @@ func (n *terraformPluginSDKAsyncExternal) Create(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return managed.ExternalCreation{}, nil
+	return managed.ExternalCreation{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginSDKAsyncExternal) Update(_ context.Context, mg xpresource.Managed) (managed.ExternalUpdate, error) {
@@ -178,7 +180,7 @@ func (n *terraformPluginSDKAsyncExternal) Update(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return managed.ExternalUpdate{}, nil
+	return managed.ExternalUpdate{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginSDKAsyncExternal) Delete(_ context.Context, mg xpresource.Managed) error {
@@ -205,5 +207,5 @@ func (n *terraformPluginSDKAsyncExternal) Delete(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return nil
+	return n.opTracker.LastOperation.Error()
 }
diff --git a/pkg/terraform/operation.go b/pkg/terraform/operation.go
@@ -43,14 +43,26 @@ func (o *Operation) MarkEnd() {
 	o.endTime = &now
 }
 
-// Flush cleans the operation information.
+// Flush cleans the operation information including the registered error from
+// the last reconciliation.
+// Deprecated: Please use Clear, which allows optionally preserving the error
+// from the last reconciliation to implement proper SYNC status condition for
+// the asynchronous external clients.
 func (o *Operation) Flush() {
+	o.Clear(false)
+}
+
+// Clear clears the operation information optionally preserving the last
+// registered error from the last reconciliation.
+func (o *Operation) Clear(preserveError bool) {
 	o.mu.Lock()
 	defer o.mu.Unlock()
 	o.Type = ""
 	o.startTime = nil
 	o.endTime = nil
-	o.err = nil
+	if !preserveError {
+		o.err = nil
+	}
 }
 
 // IsEnded returns whether the operation has ended, regardless of its result.

diff --git a/pkg/terraform/operation_test.go b/pkg/terraform/operation_test.go
@@ -8,9 +8,11 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/pkg/errors"
 )
 
 func TestOperation(t *testing.T) {
+	testErr := errors.New("test error")
 	type args struct {
 		calls func(o *Operation)
 	}
@@ -54,13 +56,46 @@ func TestOperation(t *testing.T) {
 			args: args{
 				calls: func(o *Operation) {
 					o.MarkStart("type")
+					o.SetError(testErr)
 					o.MarkEnd()
 					o.Flush()
 				},
 			},
 			want: want{
 				checks: func(o *Operation) bool {
-					return o.Type == "" && o.startTime == nil && o.endTime == nil
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && o.err == nil
+				},
+				result: true,
+			},
+		},
+		"ClearedIncludingErrors": {
+			args: args{
+				calls: func(o *Operation) {
+					o.MarkStart("type")
+					o.SetError(testErr)
+					o.MarkEnd()
+					o.Clear(false)
+				},
+			},
+			want: want{
+				checks: func(o *Operation) bool {
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && o.err == nil
+				},
+				result: true,
+			},
+		},
+		"ClearedPreservingErrors": {
+			args: args{
+				calls: func(o *Operation) {
+					o.MarkStart("type")
+					o.SetError(testErr)
+					o.MarkEnd()
+					o.Clear(true)
+				},
+			},
+			want: want{
+				checks: func(o *Operation) bool {
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && errors.Is(o.err, testErr)
 				},
 				result: true,
 			},