use controller-runtime Terminal error instead of custom Unrecoverable…

… error (#405)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

internal/controllers/core/clustercatalog_controller.go

@@ -18,7 +18,6 @@ package core
 
 import (
 	"context" // #nosec
-	"errors"
 	"fmt"
 	"time"
 
@@ -33,7 +32,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/operator-framework/catalogd/api/core/v1alpha1"
-	catalogderrors "github.com/operator-framework/catalogd/internal/errors"
 	"github.com/operator-framework/catalogd/internal/source"
 	"github.com/operator-framework/catalogd/internal/storage"
 )
@@ -75,11 +73,6 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	reconciledCatsrc := existingCatsrc.DeepCopy()
 	res, reconcileErr := r.reconcile(ctx, reconciledCatsrc)
 
-	if errors.As(reconcileErr, &catalogderrors.Unrecoverable{}) {
-		l.Error(reconcileErr, "unrecoverable reconcile error")
-		reconcileErr = nil
-	}
-
 	// Update the status subresource before updating the main object. This is
 	// necessary because, in many cases, the main object update will remove the
 	// finalizer, which will cause the core Kubernetes deletion logic to

internal/source/containers_image.go

@@ -26,9 +26,9 @@ import (
 	"github.com/opencontainers/go-digest"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1"
-	catalogderrors "github.com/operator-framework/catalogd/internal/errors"
 )
 
 const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
@@ -46,7 +46,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
 	}
 
 	if catalog.Spec.Source.Image == nil {
-		return nil, catalogderrors.NewUnrecoverable(fmt.Errorf("error parsing catalog, catalog %s has a nil image source", catalog.Name))
+		return nil, reconcile.TerminalError(fmt.Errorf("error parsing catalog, catalog %s has a nil image source", catalog.Name))
 	}
 
 	//////////////////////////////////////////////////////
@@ -191,7 +191,7 @@ func (i *ContainersImageRegistry) unpackPath(catalogName string, digest digest.D
 func resolveReferences(ctx context.Context, ref string, sourceContext *types.SystemContext) (reference.Named, reference.Canonical, bool, error) {
 	imgRef, err := reference.ParseNamed(ref)
 	if err != nil {
-		return nil, nil, false, catalogderrors.NewUnrecoverable(fmt.Errorf("error parsing image reference %q: %w", ref, err))
+		return nil, nil, false, reconcile.TerminalError(fmt.Errorf("error parsing image reference %q: %w", ref, err))
 	}
 
 	canonicalRef, isCanonical, err := resolveCanonicalRef(ctx, imgRef, sourceContext)
@@ -208,7 +208,7 @@ func resolveCanonicalRef(ctx context.Context, imgRef reference.Named, imageCtx *
 
 	srcRef, err := docker.NewReference(imgRef)
 	if err != nil {
-		return nil, false, catalogderrors.NewUnrecoverable(fmt.Errorf("error creating reference: %w", err))
+		return nil, false, reconcile.TerminalError(fmt.Errorf("error creating reference: %w", err))
 	}
 
 	imgSrc, err := srcRef.NewImageSource(ctx, imageCtx)
@@ -269,8 +269,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 	if !ok {
 		// If the spec is a tagged ref, retries could end up resolving a new digest, where the label
 		// might show up. If the spec is canonical, no amount of retries will make the label appear.
-		// Therefore, we treat the error as unrecoverable if the reference from the spec is canonical.
-		return wrapUnrecoverable(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
+		// Therefore, we treat the error as terminal if the reference from the spec is canonical.
+		return wrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
 	}
 
 	if err := os.MkdirAll(unpackPath, 0700); err != nil {
@@ -402,9 +402,9 @@ func deleteRecursive(root string) error {
 	return os.RemoveAll(root)
 }
 
-func wrapUnrecoverable(err error, isUnrecoverable bool) error {
-	if !isUnrecoverable {
+func wrapTerminal(err error, isTerminal bool) error {
+	if !isTerminal {
 		return err
 	}
-	return catalogderrors.NewUnrecoverable(err)
+	return reconcile.TerminalError(err)
 }

internal/source/containers_image_test.go

@@ -24,9 +24,9 @@ import (
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/operator-framework/catalogd/api/core/v1alpha1"
-	catalogderrors "github.com/operator-framework/catalogd/internal/errors"
 	"github.com/operator-framework/catalogd/internal/source"
 )
 
@@ -39,7 +39,7 @@ func TestImageRegistry(t *testing.T) {
 		// points to the registry created for the test
 		catalog             *v1alpha1.ClusterCatalog
 		wantErr             bool
-		unrecoverable       bool
+		terminal            bool
 		image               v1.Image
 		digestAlreadyExists bool
 		oldDigestExists     bool
@@ -60,9 +60,9 @@ func TestImageRegistry(t *testing.T) {
 					},
 				},
 			},
-			wantErr:       true,
-			unrecoverable: true,
-			refType:       "tag",
+			wantErr:  true,
+			terminal: true,
+			refType:  "tag",
 		},
 		{
 			name: ".spec.source.image.ref is unparsable",
@@ -79,9 +79,9 @@ func TestImageRegistry(t *testing.T) {
 					},
 				},
 			},
-			wantErr:       true,
-			unrecoverable: true,
-			refType:       "tag",
+			wantErr:  true,
+			terminal: true,
+			refType:  "tag",
 		},
 		{
 			name: "tag based, image is missing required label",
@@ -123,8 +123,8 @@ func TestImageRegistry(t *testing.T) {
 					},
 				},
 			},
-			wantErr:       true,
-			unrecoverable: true,
+			wantErr:  true,
+			terminal: true,
 			image: func() v1.Image {
 				img, err := random.Image(20, 3)
 				if err != nil {
@@ -408,8 +408,8 @@ func TestImageRegistry(t *testing.T) {
 				}
 			} else {
 				assert.Error(t, err)
-				isUnrecov := errors.As(err, &catalogderrors.Unrecoverable{})
-				assert.Equal(t, tt.unrecoverable, isUnrecov, "expected unrecoverable %v, got %v", tt.unrecoverable, isUnrecov)
+				isTerminal := errors.Is(err, reconcile.TerminalError(nil))
+				assert.Equal(t, tt.terminal, isTerminal, "expected terminal %v, got %v", tt.terminal, isTerminal)
 			}
 
 			assert.NoError(t, imgReg.Cleanup(ctx, tt.catalog))