fixes ImagePruner error wrapping

This removes ErrImage wrapping / detection from the acceptable error
path in favor of directly inspecting the errors returned. However, we
keep ErrImage wrapping in place so that we can ensure that the image
name is associated with the error.

Author: cheesesashimi

pkg/controller/build/imagepruner/errors.go

@@ -45,10 +45,6 @@ func IsImageNotFoundErr(err error) bool {
 		return false
 	}
 
-	if !isErrImage(err) {
-		return false
-	}
-
 	if isMaskedHTTP404(err) {
 		return true
 	}
@@ -68,10 +64,6 @@ func IsAccessDeniedErr(err error) bool {
 		return false
 	}
 
-	if !isErrImage(err) {
-		return false
-	}
-
 	if isAccessDeniedErrorCode(err) {
 		return true
 	}
@@ -225,61 +217,35 @@ func isTolerableUnexpectedHTTPStatusError(err error) bool {
 
 // ErrImage holds and wraps an error related to a specific image.
 type ErrImage struct {
-	msg string
-	img string
-	err error
-}
-
-// newErrImageWithMessage constructs a new ErrImage instance with a custom message,
-// image pullspec, and wrapped error.
-func newErrImageWithMessage(msg, img string, err error) error {
-	return &ErrImage{msg: msg, img: img, err: err}
+	image string
+	err   error
 }
 
 // newErrImage constructs a new ErrImage instance with an image pullspec and
-// wrapped error, without a custom message.
+// wrapped error.
 func newErrImage(img string, err error) error {
-	return &ErrImage{img: img, err: err}
+	return &ErrImage{image: img, err: err}
 }
 
-// Image returns the image pullspec that caused the error.
+// Returns the image embedded within the ErrImage struct.
 func (e *ErrImage) Image() string {
-	return e.img
+	return e.image
 }
 
 // Error implements the error interface, providing a formatted error string
-// including the message (if present), image (if present), and the wrapped error's string.
+// including the image (if present), and the wrapped error's string.
 func (e *ErrImage) Error() string {
-	if e.msg != "" && e.img != "" {
-		// If both the message and image are not empty, include both.
-		return fmt.Sprintf("%s: image %q: %s", e.msg, e.img, e.err.Error())
+	// If the image is defined and not contained within the underlying error, inject it here.
+	if e.image != "" && !strings.Contains(e.err.Error(), e.image) {
+		return fmt.Sprintf("error occurred with image %q: %s", e.image, e.err.Error())
 	}
 
-	if e.msg == "" && e.img != "" {
-		// If the message is empty and the image is not, only include the image.
-		return fmt.Sprintf("image %q: %s", e.img, e.err.Error())
-	}
-
-	// If neither the message nor the image is populated, just return the error
-	// string as-is.
+	// If the image is undefined or is already present in the error, return the
+	// error as-is.
 	return e.err.Error()
 }
 
 // Unwrap implements the Unwrap interface, allowing the nested error to be surfaced.
 func (e *ErrImage) Unwrap() error {
 	return e.err
 }
-
-// isErrImage determines whether the given error is an instance of the ErrImage
-// type defined above.
-func isErrImage(err error) bool {
-	if err == nil {
-		return false
-	}
-
-	// Any errors related to the actual image registry query are wrapped in an
-	// ErrImage instance. This allows us to easily identify intolerable errors
-	// such as not being able to write the authfile or certs, etc.
-	var errImage *ErrImage
-	return errors.As(err, &errImage)
-}

pkg/controller/build/imagepruner/imageinspect.go

@@ -49,12 +49,21 @@ func NewImageInspectorDeleter() ImageInspectorDeleter {
 
 // ImageInspect uses the provided system context to inspect the provided image pullspec.
 func (i *imageInspectorImpl) ImageInspect(ctx context.Context, sysCtx *types.SystemContext, image string) (*types.ImageInspectInfo, *digest.Digest, error) {
-	return imageInspect(ctx, sysCtx, image)
+	info, digest, err := imageInspect(ctx, sysCtx, image)
+	if err == nil {
+		return info, digest, nil
+	}
+
+	return nil, nil, newErrImage(image, err)
 }
 
 // DeleteImage uses the provided system context to delete the provided image pullspec.
 func (i *imageInspectorImpl) DeleteImage(ctx context.Context, sysCtx *types.SystemContext, image string) error {
-	return deleteImage(ctx, sysCtx, image)
+	if err := deleteImage(ctx, sysCtx, image); err != nil {
+		return newErrImage(image, err)
+	}
+
+	return nil
 }
 
 // deleteImage attempts to delete the specified image with retries,
@@ -74,8 +83,9 @@ func deleteImage(ctx context.Context, sysCtx *types.SystemContext, imageName str
 	if err := retry.IfNecessary(ctx, func() error {
 		return ref.DeleteImage(ctx, sysCtx)
 	}, &retryOpts); err != nil {
-		return newErrImage(imageName, err)
+		return err
 	}
+
 	return nil
 }
 
@@ -96,7 +106,7 @@ func imageInspect(ctx context.Context, sysCtx *types.SystemContext, imageName st
 	}
 	src, err := imageutils.GetImageSourceFromReference(ctx, sysCtx, ref, &retryOpts)
 	if err != nil {
-		return nil, nil, fmt.Errorf("error getting image source for %s: %w", imageName, err)
+		return nil, nil, newErrImage(imageName, fmt.Errorf("error getting image source for %s: %w", imageName, err))
 	}
 	defer src.Close()
 
@@ -112,20 +122,20 @@ func imageInspect(ctx context.Context, sysCtx *types.SystemContext, imageName st
 	// get the digest here because it's not part of the image inspection
 	digest, err := manifest.Digest(rawManifest)
 	if err != nil {
-		return nil, nil, fmt.Errorf("error retrieving image digest: %q: %w", imageName, err)
+		return nil, nil, fmt.Errorf("error retrieving image digest: %w", err)
 	}
 
 	img, err := image.FromUnparsedImage(ctx, sysCtx, unparsedInstance)
 	if err != nil {
-		return nil, nil, newErrImage(imageName, fmt.Errorf("error parsing manifest for image: %w", err))
+		return nil, nil, fmt.Errorf("error parsing manifest for image: %w", err)
 	}
 
 	var imgInspect *types.ImageInspectInfo
 	if err := retry.IfNecessary(ctx, func() error {
 		imgInspect, err = img.Inspect(ctx)
 		return err
 	}, &retryOpts); err != nil {
-		return nil, nil, newErrImage(imageName, err)
+		return nil, nil, err
 	}
 
 	return imgInspect, &digest, nil