clean up retryable/non-retryable errors in controllers

Use a custom internal error type to allow reconcile helpers to indicate
when they encounter errors that are non-retryable.

Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>

Author: BlaineEXE

controller/internal/reconciler/bucketclaim.go

@@ -18,6 +18,7 @@ package reconciler
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -33,6 +34,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
+	cosierr "sigs.k8s.io/container-object-storage-interface/internal/errors"
 	cosipredicate "sigs.k8s.io/container-object-storage-interface/internal/predicate"
 )
 
@@ -63,18 +65,18 @@ func (r *BucketClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
-	retryError, err := r.reconcile(ctx, logger, claim)
+	err := r.reconcile(ctx, logger, claim)
 	if err != nil {
 		// Record any error as a timestamped error in the status.
 		claim.Status.Error = cosiapi.NewTimestampedError(time.Now(), err.Error())
 		if updErr := r.Status().Update(ctx, claim); updErr != nil {
 			logger.Error(err, "failed to update BucketClaim status after reconcile error", "updateError", updErr)
 			// If status update fails, we must retry the error regardless of the reconcile return.
-			// The reconcile needs to run again to make sure the status is eventually be updated.
+			// The reconcile needs to run again to make sure the status is eventually updated.
 			return reconcile.Result{}, err
 		}
 
-		if !retryError {
+		if errors.Is(err, cosierr.NonRetryableError(nil)) {
 			return reconcile.Result{}, reconcile.TerminalError(err)
 		}
 		return reconcile.Result{}, err
@@ -112,31 +114,20 @@ func (r *BucketClaimReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-// Type definition enforces readability. true/false in code is hard to read/review/maintain.
-type retryErrorType bool
-
-const (
-	RetryError      retryErrorType = true  // DO retry the error.
-	DoNotRetryError retryErrorType = false // Do NOT retry the error.
-	NoError         retryErrorType = false // No error, don't retry. For readability.
-)
-
-func (r *BucketClaimReconciler) reconcile(
-	ctx context.Context, logger logr.Logger, claim *cosiapi.BucketClaim,
-) (retryErrorType, error) {
+func (r *BucketClaimReconciler) reconcile(ctx context.Context, logger logr.Logger, claim *cosiapi.BucketClaim) error {
 	bucketName, err := determineBucketName(claim)
 	if err != nil {
 		// Opinion: It is best to not apply a missing finalizer when boundBucketName is degraded
 		// (err returned here). When degraded, the user needs to delete and re-create the
 		// BucketClaim to fix the degradation, which requires the finalizer be absent.
 		logger.Error(err, "failed to determine Bucket name for claim")
-		return DoNotRetryError, err
+		return cosierr.NonRetryableError(err)
 	}
 
 	logger = logger.WithValues("bucketName", bucketName)
 
 	if claim.Spec.ExistingBucketName != "" {
-		return DoNotRetryError, fmt.Errorf("static provisioning is not yet supported") // TODO
+		return cosierr.NonRetryableError(fmt.Errorf("static provisioning is not yet supported")) // TODO
 	}
 
 	if !claim.GetDeletionTimestamp().IsZero() {
@@ -147,10 +138,10 @@ func (r *BucketClaimReconciler) reconcile(
 		ctrlutil.RemoveFinalizer(claim, cosiapi.ProtectionFinalizer)
 		if err := r.Update(ctx, claim); err != nil {
 			logger.Error(err, "failed to remove finalizer")
-			return RetryError, fmt.Errorf("failed to remove finalizer: %w", err)
+			return fmt.Errorf("failed to remove finalizer: %w", err)
 		}
 
-		return DoNotRetryError, fmt.Errorf("deletion is not yet implemented") // TODO
+		return cosierr.NonRetryableError(fmt.Errorf("deletion is not yet implemented")) // TODO
 	}
 
 	logger.V(1).Info("reconciling BucketClaim")
@@ -159,7 +150,7 @@ func (r *BucketClaimReconciler) reconcile(
 	if didAdd {
 		if err := r.Update(ctx, claim); err != nil {
 			logger.Error(err, "failed to add protection finalizer")
-			return RetryError, fmt.Errorf("failed to add protection finalizer: %w", err)
+			return fmt.Errorf("failed to add protection finalizer: %w", err)
 		}
 	}
 
@@ -168,7 +159,7 @@ func (r *BucketClaimReconciler) reconcile(
 		claim.Status.BoundBucketName = bucketName
 		if err := r.Status().Update(ctx, claim); err != nil {
 			logger.Error(err, "failed to bind BucketClaim to Bucket")
-			return RetryError, fmt.Errorf("failed to bind BucketClaim to Bucket: %w", err)
+			return fmt.Errorf("failed to bind BucketClaim to Bucket: %w", err)
 		}
 	}
 
@@ -180,14 +171,14 @@ func (r *BucketClaimReconciler) reconcile(
 	if err := r.Get(ctx, bucketNsName, bucket); err != nil {
 		if !kerrors.IsNotFound(err) {
 			logger.Error(err, "failed to determine if Bucket exists")
-			return RetryError, err
+			return err
 		}
 
 		// TODO: static provisioning: don't do this
 		logger.Info("creating intermediate Bucket")
-		_, retryErr, err := createIntermediateBucket(ctx, logger, r.Client, claim, bucketName)
+		_, err := createIntermediateBucket(ctx, logger, r.Client, claim, bucketName)
 		if err != nil {
-			return retryErr, err
+			return err
 		}
 	}
 
@@ -205,7 +196,7 @@ func (r *BucketClaimReconciler) reconcile(
 	//     1. Supported protocols
 	//     2. `ReadyToUse`
 
-	return NoError, nil
+	return nil
 }
 
 // Determine the bucket name that should go with the claim. No errors can be retried.
@@ -244,11 +235,11 @@ func createIntermediateBucket(
 	client client.Client,
 	claim *cosiapi.BucketClaim,
 	bucketName string,
-) (*cosiapi.Bucket, retryErrorType, error) {
+) (*cosiapi.Bucket, error) {
 	className := claim.Spec.BucketClassName
 	if className == "" {
 		logger.Error(nil, "BucketClaim cannot have empty bucketClassName")
-		return nil, DoNotRetryError, fmt.Errorf("BucketClaim cannot have empty bucketClassName")
+		return nil, cosierr.NonRetryableError(fmt.Errorf("BucketClaim cannot have empty bucketClassName"))
 	}
 
 	logger = logger.WithValues("bucketClassName", className)
@@ -265,10 +256,10 @@ func createIntermediateBucket(
 			// BucketClass reconciler that enqueues requests for BucketClaims that reference the
 			// class and don't yet have a bound Bucket.
 			logger.Error(err, "BucketClass not found")
-			return nil, RetryError, err
+			return nil, err
 		}
 		logger.Error(err, "failed to get BucketClass")
-		return nil, RetryError, err
+		return nil, err
 	}
 
 	logger.V(1).Info("using BucketClass for intermediate Bucket")
@@ -279,13 +270,13 @@ func createIntermediateBucket(
 		if kerrors.IsAlreadyExists(err) {
 			// Unlikely race condition. Error to allow the next reconcile to attempt to recover.
 			logger.Error(err, "intermediate Bucket already exists")
-			return nil, RetryError, err
+			return nil, err
 		}
 		logger.Error(err, "failed to create intermediate Bucket")
-		return nil, RetryError, err
+		return nil, err
 	}
 
-	return bucket, NoError, nil
+	return bucket, nil
 }
 
 func generateIntermediateBucket(

controller/internal/reconciler/bucketclaim_test.go

@@ -33,6 +33,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
+	cosierr "sigs.k8s.io/container-object-storage-interface/internal/errors"
 )
 
 func Test_determineBucketName(t *testing.T) {
@@ -162,7 +163,7 @@ func Test_createIntermediateBucket(t *testing.T) {
 		claim := baseClaim.DeepCopy()
 		client := newClient(baseClass.DeepCopy())
 
-		bucket, _, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
+		bucket, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
 		assert.NoError(t, err)
 
 		assert.Empty(t, bucket.Finalizers) // NO finalizers pre-applied
@@ -185,10 +186,10 @@ func Test_createIntermediateBucket(t *testing.T) {
 		claim := baseClaim.DeepCopy()
 		client := newClient() // no bucketclass exists
 
-		bucket, retryErr, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
+		bucket, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
 		assert.Error(t, err)
 		assert.ErrorContains(t, err, "s3-class") // the class name
-		assert.Equal(t, RetryError, retryErr)
+		assert.NotErrorIs(t, err, cosierr.NonRetryableError(nil))
 		assert.Nil(t, bucket)
 	})
 
@@ -197,9 +198,9 @@ func Test_createIntermediateBucket(t *testing.T) {
 		claim.Spec.BucketClassName = ""
 		client := newClient(baseClass.DeepCopy())
 
-		bucket, retryErr, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
+		bucket, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
 		assert.Error(t, err)
-		assert.Equal(t, DoNotRetryError, retryErr)
+		assert.ErrorIs(t, err, cosierr.NonRetryableError(nil))
 		assert.Nil(t, bucket)
 	})
 
@@ -213,9 +214,9 @@ func Test_createIntermediateBucket(t *testing.T) {
 		}
 		client := newClient(baseClass.DeepCopy(), raceBucket)
 
-		bucket, retryErr, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
+		bucket, err := createIntermediateBucket(ctx, nolog, client, claim, "bc-qwerty")
 		assert.Error(t, err)
-		assert.Equal(t, RetryError, retryErr)
+		assert.NotErrorIs(t, err, cosierr.NonRetryableError(nil))
 		assert.Nil(t, bucket)
 	})
 }

internal/errors/errors.go

@@ -0,0 +1,43 @@
+/*
+Copyright 2025 The Kubernetes 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.
+*/
+
+// COSI internal error types.
+package errors
+
+import "errors"
+
+// NonRetryableError is an error that should not be retried but still treated as a reconcile error.
+func NonRetryableError(wrapped error) error {
+	return &nonRetryableError{err: wrapped}
+}
+
+type nonRetryableError struct {
+	err error
+}
+
+func (te *nonRetryableError) Error() string {
+	return te.err.Error()
+}
+
+// This function will return nil if te.err is nil.
+func (te *nonRetryableError) Unwrap() error {
+	return te.err
+}
+
+func (te *nonRetryableError) Is(target error) bool {
+	tp := &nonRetryableError{}
+	return errors.As(target, &tp)
+}