fix HandleReconcileError not detecting wrapped errors when using ErrorWithMetrics

Author: zac-nixon

pkg/runtime/reconcile.go

@@ -3,26 +3,41 @@ package runtime
 import (
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
+	errmetrics "sigs.k8s.io/aws-load-balancer-controller/pkg/error"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 // HandleReconcileError will handle errors from reconcile handlers, which respects runtime errors.
-func HandleReconcileError(err error, log logr.Logger) (ctrl.Result, error) {
-	if err == nil {
+func HandleReconcileError(inputErr error, log logr.Logger) (ctrl.Result, error) {
+	resolvedErr := handleNestedError(inputErr)
+	if resolvedErr == nil {
 		return ctrl.Result{}, nil
 	}
 
 	var requeueNeededAfter *RequeueNeededAfter
-	if errors.As(err, &requeueNeededAfter) {
+	if errors.As(resolvedErr, &requeueNeededAfter) {
 		log.V(1).Info("requeue after", "duration", requeueNeededAfter.Duration(), "reason", requeueNeededAfter.Reason())
 		return ctrl.Result{RequeueAfter: requeueNeededAfter.Duration()}, nil
 	}
 
 	var requeueNeeded *RequeueNeeded
-	if errors.As(err, &requeueNeeded) {
+	if errors.As(resolvedErr, &requeueNeeded) {
 		log.V(1).Info("requeue", "reason", requeueNeeded.Reason())
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	return ctrl.Result{}, err
+	return ctrl.Result{}, inputErr
+}
+
+func handleNestedError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	var wrappedError *errmetrics.ErrorWithMetrics
+	if errors.As(err, &wrappedError) {
+		return wrappedError.Err
+	}
+
+	return err
 }

pkg/runtime/reconcile_test.go

@@ -1,6 +1,7 @@
 package runtime
 
 import (
+	errmetrics "sigs.k8s.io/aws-load-balancer-controller/pkg/error"
 	"testing"
 	"time"
 
@@ -15,6 +16,12 @@ func TestHandleReconcileError(t *testing.T) {
 	type args struct {
 		err error
 	}
+
+	otherErrType := errors.New("some error")
+	wrappedOtherErrorType := &errmetrics.ErrorWithMetrics{
+		Err:          otherErrType,
+		ResourceType: "foo",
+	}
 	tests := []struct {
 		name    string
 		args    args
@@ -29,6 +36,14 @@ func TestHandleReconcileError(t *testing.T) {
 			want:    ctrl.Result{},
 			wantErr: nil,
 		},
+		{
+			name: "input err is nil",
+			args: args{
+				err: &errmetrics.ErrorWithMetrics{},
+			},
+			want:    ctrl.Result{},
+			wantErr: nil,
+		},
 		{
 			name: "input err is RequeueNeededAfter",
 			args: args{
@@ -49,20 +64,53 @@ func TestHandleReconcileError(t *testing.T) {
 			},
 			wantErr: nil,
 		},
+		{
+			name: "input err is ErrorWithMetrics and is RequeueNeededAfter",
+			args: args{
+				err: &errmetrics.ErrorWithMetrics{
+					Err: NewRequeueNeededAfter("some error", 3*time.Second),
+				},
+			},
+			want: ctrl.Result{
+				RequeueAfter: 3 * time.Second,
+			},
+			wantErr: nil,
+		},
+		{
+			name: "input err is ErrorWithMetrics and is RequeueNeeded",
+			args: args{
+				err: &errmetrics.ErrorWithMetrics{
+					Err: NewRequeueNeeded("some error"),
+				},
+			},
+			want: ctrl.Result{
+				Requeue: true,
+			},
+			wantErr: nil,
+		},
 		{
 			name: "input err is other error type",
 			args: args{
-				err: errors.New("some error"),
+				err: otherErrType,
+			},
+			want:    ctrl.Result{},
+			wantErr: otherErrType,
+		},
+		{
+			name: "input err is ErrorWithMetrics with other error type",
+			args: args{
+				err: wrappedOtherErrorType,
 			},
 			want:    ctrl.Result{},
-			wantErr: errors.New("some error"),
+			wantErr: wrappedOtherErrorType,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := HandleReconcileError(tt.args.err, logr.New(&log.NullLogSink{}))
 			if tt.wantErr != nil {
 				assert.EqualError(t, err, tt.wantErr.Error())
+				assert.Equal(t, err, tt.wantErr)
 			} else {
 				assert.NoError(t, err)
 				assert.Equal(t, tt.want, got)