terraform: controller timeout, cleanup the last error and fix returne…

…d refresh result in ongoing state

Signed-off-by: Muvaffak Onus <me@muvaf.com>

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/spf13/afero v1.6.0
 	go.uber.org/multierr v1.7.0 // indirect
 	golang.org/x/tools v0.1.5
+	k8s.io/api v0.21.3
 	k8s.io/apimachinery v0.21.3
 	sigs.k8s.io/controller-runtime v0.9.6
 )

pkg/controller/external.go

@@ -32,17 +32,16 @@ import (
 )
 
 const (
-	errUnexpectedObject    = "the managed resource is not a Terraformed resource"
-	errGetTerraformSetup   = "cannot get terraform setup"
-	errGetWorkspace        = "cannot get a terraform workspace for resource"
-	errRefresh             = "cannot run refresh"
-	errRefreshAttributes   = "refresh returned empty attributes"
-	errPlan                = "cannot run plan"
-	errLastOperationFailed = "the last operation failed"
-	errStartAsyncApply     = "cannot start async apply"
-	errStartAsyncDestroy   = "cannot start async destroy"
-	errApply               = "cannot apply"
-	errDestroy             = "cannot destroy"
+	errUnexpectedObject  = "the managed resource is not a Terraformed resource"
+	errGetTerraformSetup = "cannot get terraform setup"
+	errGetWorkspace      = "cannot get a terraform workspace for resource"
+	errRefresh           = "cannot run refresh"
+	errPlan              = "cannot run plan"
+	errStartAsyncApply   = "cannot start async apply"
+	errStartAsyncDestroy = "cannot start async destroy"
+	errApply             = "cannot apply"
+	errDestroy           = "cannot destroy"
+	errStatusUpdate      = "cannot update status of custom resource"
 )
 
 // Option allows you to configure Connector.
@@ -122,19 +121,24 @@ func (e *external) Observe(ctx context.Context, mg xpresource.Managed) (managed.
 
 	res, err := e.workspace.Refresh(ctx)
 	if err != nil {
-		return managed.ExternalObservation{}, errors.Wrap(xpresource.Ignore(terraform.IsNotFound, err), errRefresh)
+		return managed.ExternalObservation{}, errors.Wrap(err, errRefresh)
 	}
-	if res.IsDestroying || res.IsApplying {
+	if e.async {
+		tr.SetConditions(resource.LastOperationCondition(res.LastOperationError))
+		if err := e.kube.Status().Update(ctx, tr); err != nil {
+			return managed.ExternalObservation{}, errors.Wrap(err, errStatusUpdate)
+		}
+	}
+	switch {
+	case res.IsApplying, res.IsDestroying:
 		return managed.ExternalObservation{
 			ResourceExists:   true,
 			ResourceUpToDate: true,
 		}, nil
-	}
-	if res.LastOperationError != nil {
-		return managed.ExternalObservation{}, errors.Wrap(res.LastOperationError, errLastOperationFailed)
-	}
-	if res.State.GetAttributes() == nil {
-		return managed.ExternalObservation{}, errors.New(errRefreshAttributes)
+	case !res.Exists:
+		return managed.ExternalObservation{
+			ResourceExists: false,
+		}, nil
 	}
 
 	// No operation was in progress, our observation completed successfully, and

pkg/controller/external_test.go

@@ -18,24 +18,19 @@ package controller
 
 import (
 	"context"
-
-	"github.com/crossplane-contrib/terrajet/pkg/resource/json"
-
-	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
-
-	"github.com/crossplane-contrib/terrajet/pkg/resource"
-
-	"sigs.k8s.io/controller-runtime/pkg/client"
-
 	"testing"
 
+	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	xpresource "github.com/crossplane/crossplane-runtime/pkg/resource"
 	xpfake "github.com/crossplane/crossplane-runtime/pkg/resource/fake"
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/crossplane-contrib/terrajet/pkg/resource"
 	"github.com/crossplane-contrib/terrajet/pkg/resource/fake"
+	"github.com/crossplane-contrib/terrajet/pkg/resource/json"
 	"github.com/crossplane-contrib/terrajet/pkg/terraform"
 )
 
@@ -173,8 +168,10 @@ func TestConnect(t *testing.T) {
 
 func TestObserve(t *testing.T) {
 	type args struct {
-		w   Workspace
-		obj xpresource.Managed
+		w     Workspace
+		kube  client.Client
+		async bool
+		obj   xpresource.Managed
 	}
 	type want struct {
 		obs managed.ExternalObservation
@@ -213,7 +210,7 @@ func TestObserve(t *testing.T) {
 				obj: &fake.Terraformed{},
 				w: WorkspaceFns{
 					RefreshFn: func(_ context.Context) (terraform.RefreshResult, error) {
-						return terraform.RefreshResult{}, terraform.NewNotFound()
+						return terraform.RefreshResult{Exists: false}, nil
 					},
 				},
 			},
@@ -238,35 +235,56 @@ func TestObserve(t *testing.T) {
 			},
 		},
 		"LastOperationFailed": {
-			reason: "It should report if the last operation failed",
+			reason: "It should report the last operation error without failing",
 			args: args{
-				obj: &fake.Terraformed{},
+				async: true,
+				obj: &fake.Terraformed{
+					MetadataProvider: fake.MetadataProvider{
+						IDField: "id",
+					},
+				},
+				kube: &test.MockClient{
+					MockStatusUpdate: test.NewMockStatusUpdateFn(nil),
+				},
 				w: WorkspaceFns{
 					RefreshFn: func(_ context.Context) (terraform.RefreshResult, error) {
 						return terraform.RefreshResult{
+							State:              exampleState,
 							LastOperationError: errBoom,
 						}, nil
 					},
+					PlanFn: func(_ context.Context) (terraform.PlanResult, error) {
+						return terraform.PlanResult{UpToDate: true}, nil
+					},
 				},
 			},
 			want: want{
-				err: errors.Wrap(errBoom, errLastOperationFailed),
+				obs: managed.ExternalObservation{
+					ResourceExists:          true,
+					ResourceUpToDate:        true,
+					ResourceLateInitialized: true,
+				},
 			},
 		},
-		"EmptyAttributes": {
-			reason: "We should report if refresh returns an empty state in a non-error case",
+		"StatusUpdateFailed": {
+			reason: "It should fail if status cannot be updated",
 			args: args{
-				obj: &fake.Terraformed{},
+				async: true,
+				obj:   &fake.Terraformed{},
+				kube: &test.MockClient{
+					MockStatusUpdate: test.NewMockStatusUpdateFn(errBoom),
+				},
 				w: WorkspaceFns{
 					RefreshFn: func(_ context.Context) (terraform.RefreshResult, error) {
 						return terraform.RefreshResult{
-							State: json.NewStateV4(),
+							State:              exampleState,
+							LastOperationError: errBoom,
 						}, nil
 					},
 				},
 			},
 			want: want{
-				err: errors.New(errRefreshAttributes),
+				err: errors.Wrap(errBoom, errStatusUpdate),
 			},
 		},
 		"PlanFailed": {
@@ -280,7 +298,8 @@ func TestObserve(t *testing.T) {
 				w: WorkspaceFns{
 					RefreshFn: func(_ context.Context) (terraform.RefreshResult, error) {
 						return terraform.RefreshResult{
-							State: exampleState,
+							Exists: true,
+							State:  exampleState,
 						}, nil
 					},
 					PlanFn: func(_ context.Context) (terraform.PlanResult, error) {
@@ -321,7 +340,7 @@ func TestObserve(t *testing.T) {
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			e := &external{workspace: tc.w}
+			e := &external{workspace: tc.w, kube: tc.kube, async: tc.async}
 			_, err := e.Observe(context.TODO(), tc.args.obj)
 			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
 				t.Errorf("\n%s\nObserve(...): -want error, +got error:\n%s", tc.reason, diff)

pkg/pipeline/templates/controller.go.tmpl

@@ -5,6 +5,8 @@
 package {{ .Package }}
 
 import (
+	"time"
+
 	"k8s.io/client-go/util/workqueue"
 	ctrl "sigs.k8s.io/controller-runtime"
 
@@ -33,6 +35,7 @@ func Setup(mgr ctrl.Manager, l logging.Logger, rl workqueue.RateLimiter, s terra
 		managed.WithLogger(l.WithValues("controller", name)),
 		managed.WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name))),
 		managed.WithFinalizer(terraform.NewWorkspaceFinalizer(ws, xpresource.NewAPIFinalizer(mgr.GetClient(), managed.FinalizerName))),
+		managed.WithTimeout(3*time.Minute),
 		{{- if .DisableNameInitializer }}
 		managed.WithInitializers(),
 		{{- end}}

pkg/resource/conditions.go

@@ -0,0 +1,72 @@
+/*
+Copyright 2021 The Crossplane Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package resource
+
+import (
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	tferrors "github.com/crossplane-contrib/terrajet/pkg/terraform/errors"
+)
+
+// Condition constants.
+const (
+	TypeAsyncOperation = "AsyncOperation"
+
+	ReasonApplyFailure   xpv1.ConditionReason = "ApplyFailure"
+	ReasonDestroyFailure xpv1.ConditionReason = "DestroyFailure"
+	ReasonSuccess        xpv1.ConditionReason = "Success"
+)
+
+// LastOperationCondition returns the condition depending on the content
+// of the error.
+func LastOperationCondition(err error) xpv1.Condition {
+	switch {
+	case err == nil:
+		return xpv1.Condition{
+			Type:               TypeAsyncOperation,
+			Status:             corev1.ConditionTrue,
+			LastTransitionTime: metav1.Now(),
+			Reason:             ReasonSuccess,
+		}
+	case tferrors.IsApplyFailed(err):
+		return xpv1.Condition{
+			Type:               TypeAsyncOperation,
+			Status:             corev1.ConditionFalse,
+			LastTransitionTime: metav1.Now(),
+			Reason:             ReasonApplyFailure,
+			Message:            err.Error(),
+		}
+	case tferrors.IsDestroyFailed(err):
+		return xpv1.Condition{
+			Type:               TypeAsyncOperation,
+			Status:             corev1.ConditionFalse,
+			LastTransitionTime: metav1.Now(),
+			Reason:             ReasonDestroyFailure,
+			Message:            err.Error(),
+		}
+	default:
+		return xpv1.Condition{
+			Type:               "Unknown",
+			Status:             corev1.ConditionFalse,
+			LastTransitionTime: metav1.Now(),
+			Reason:             "Unknown",
+			Message:            err.Error(),
+		}
+	}
+}

pkg/terraform/errors/errors.go

@@ -0,0 +1,55 @@
+/*
+Copyright 2021 The Crossplane Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package errors
+
+type applyFailed struct {
+	log string
+}
+
+func (a *applyFailed) Error() string {
+	return a.log
+}
+
+// NewApplyFailed returns a new apply failure error with given logs.
+func NewApplyFailed(log string) error {
+	return &applyFailed{log: log}
+}
+
+// IsApplyFailed returns whether error is due to failure of an apply operation.
+func IsApplyFailed(err error) bool {
+	_, ok := err.(*applyFailed)
+	return ok
+}
+
+type destroyFailed struct {
+	log string
+}
+
+func (a *destroyFailed) Error() string {
+	return a.log
+}
+
+// NewDestroyFailed returns a new destroy failure error with given logs.
+func NewDestroyFailed(log string) error {
+	return &destroyFailed{log: log}
+}
+
+// IsDestroyFailed returns whether error is due to failure of a destroy operation.
+func IsDestroyFailed(err error) bool {
+	_, ok := err.(*destroyFailed)
+	return ok
+}