Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ use controller-runtime Terminal error instead of custom Unrecoverable error #405

Merged
merged 1 commit into from
Sep 18, 2024
Merged

✨ use controller-runtime Terminal error instead of custom Unrecoverable error #405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions internal/controllers/core/clustercatalog_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package core

import (
"context" // #nosec
"errors"
"fmt"
"time"

Expand All @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
12 changes: 0 additions & 12 deletions internal/errors/unrecoverable.go
View file
Open in desktop

This file was deleted.

18 changes: 9 additions & 9 deletions internal/source/containers_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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))
}

//////////////////////////////////////////////////////
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
24 changes: 12 additions & 12 deletions internal/source/containers_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
Expand Down