proxy: Make `OpenImageOptional` work with `oci-archive:` #2114

cgwalters · 2023-09-15T12:55:13Z

By adding the standard ENOENT to our list of errors.

I hit this while working on
coreos/rpm-ostree#4598
which is a tool that builds base images and wants to query if the image exists beforehand.

cgwalters · 2023-09-15T12:56:01Z

cmd/skopeo/proxy.go

@@ -235,7 +235,8 @@ func isDockerManifestUnknownError(err error) bool {
 // TODO drive this into containers/image properly
 func isNotFoundImageError(err error) bool {
 	return isDockerManifestUnknownError(err) ||
-		errors.Is(err, ocilayout.ImageNotFoundError{})
+		errors.Is(err, ocilayout.ImageNotFoundError{}) ||
+		errors.Is(err, os.ErrNotExist)


OK self-review...perhaps the real fix is in c/image to map enoent there to ocilayout.ImageNotFoundError ?

The documentation says

// ImageNotFoundError is used when the OCI structure, in principle, exists and seems valid enough, // but nothing matches the “image” part of the provided reference.

mtrmac

My first impression is to wonder why this needs special handling.

Obviously “an image not found on the registry” is something that can only be detected by talking to the registry with full credentials, i.e. inside Skopeo. Similarly “a tag not found inside an OCI archive” requires an OCI archive implementation.

But if the archive just doesn’t exist… it might make sense for Skopeo to treat that uniformly, for callers that provide an user-specified transport:image syntax without understanding the details of that… and then we get to discuss semantics of what “uniformly” means.

But rpm-ostree compose image seems to have its own format=oci option, so the transport:image value is not user-provided.

Maybe a different way to ask this: Would we be expected to similarly return 0 for docker://nxdomain.invalid/something?) If not, what’s the difference?

Right now the documentation of OpenImageOptional says “If the image does not exist, zero
is returned.”.

What is the criteria for classifying errors as “image does not exist” vs. “other error opening an image”? Does the documentation of OpenImageOptional need clarifying, or updating?

*shrug* rpm-ostree is the only known user, so pragmatically speaking it gets to shape the proxy API however it is useful for rpm-ostree; but I’d like the Skopeo-repo documentation of that API to be reasonably clear about what are the properties that need maintaining. (“The ostree-rs-ext tests continue to pass” is one way to document those properties, I guess. I find that inelegant but I can live with that.)

mtrmac · 2023-09-15T13:46:18Z

cmd/skopeo/proxy.go

@@ -235,7 +235,8 @@ func isDockerManifestUnknownError(err error) bool {
 // TODO drive this into containers/image properly
 func isNotFoundImageError(err error) bool {
 	return isDockerManifestUnknownError(err) ||
-		errors.Is(err, ocilayout.ImageNotFoundError{})
+		errors.Is(err, ocilayout.ImageNotFoundError{}) ||
+		errors.Is(err, os.ErrNotExist)


The documentation says

// ImageNotFoundError is used when the OCI structure, in principle, exists and seems valid enough, // but nothing matches the “image” part of the provided reference.

mtrmac · 2023-09-15T14:03:48Z

cmd/skopeo/proxy.go

@@ -235,7 +235,8 @@ func isDockerManifestUnknownError(err error) bool {
 // TODO drive this into containers/image properly
 func isNotFoundImageError(err error) bool {
 	return isDockerManifestUnknownError(err) ||
-		errors.Is(err, ocilayout.ImageNotFoundError{})
+		errors.Is(err, ocilayout.ImageNotFoundError{}) ||
+		errors.Is(err, os.ErrNotExist)


This is not all that precise — e.g. I think it matches if /var/tmp does not exist.

(See the top-level comment first — what is the condition isNotFoundImageError should actually be checking for? Assuming that being precise about it matters at all…)

cgwalters · 2023-09-15T14:42:25Z

Maybe a different way to ask this: Would we be expected to similarly return 0 for docker://nxdomain.invalid/something?) If not, what’s the difference?
Right now the documentation of OpenImageOptional says “If the image does not exist, zero
is returned.”.
What is the criteria for classifying errors as “image does not exist” vs. “other error opening an image”?

For registries, it's really that the server returns HTTP 404 and that's how it works now. Giving something that doesn't expose the registry API (for example) we correctly error out:

$ sudo env rpm-ostree compose image --initialize-mode if-not-exists --format=registry  fedora-tier-1.yaml example.com/blah
error: Failed to invoke skopeo proxy method OpenImageOptional: remote error: pinging container registry example.com: StatusCode: 404, <!doctype html>
<html>
<head>
    <title>Example D...

Other cases like failure to make a TCP connection, DNS doesn't resolve all should error out. Only the "HTTP server returned 404" or semantically equivalent should return (in Rust terms) Ok(None).

This is not all that precise — e.g. I think it matches if /var/tmp does not exist.

Yes, agree. In this scenario, that's suboptimal but still okay. The case I care about most are registry and oci (which actually has a distinct bug).

cgwalters · 2023-09-15T17:00:04Z

How about this variant instead? (requires a patch to c/image)

From 63f4e4665b74e2d0d7b58e4fffa7fe665b85590f Mon Sep 17 00:00:00 2001
From: Colin Walters <[email protected]>
Date: Fri, 15 Sep 2023 08:53:45 -0400
Subject: [PATCH] proxy: Make `OpenImageOptional` work with `oci-archive:`

By adding the standard ENOENT to our list of errors.

I hit this while working on
https://github.com/coreos/rpm-ostree/pull/4598
which is a tool that builds base images and wants to query
if the image exists beforehand.

Signed-off-by: Colin Walters <[email protected]>
---
 cmd/skopeo/proxy.go                                  | 12 +++++++++++-
 integration/proxy_test.go                            |  7 +++++++
 .../containers/image/v5/oci/archive/oci_src.go       |  7 ++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go
index aab85365..40c8e85f 100644
--- a/cmd/skopeo/proxy.go
+++ b/cmd/skopeo/proxy.go
@@ -73,6 +73,7 @@ import (
 
 	"github.com/containers/image/v5/image"
 	"github.com/containers/image/v5/manifest"
+	ociarchive "github.com/containers/image/v5/oci/archive"
 	ocilayout "github.com/containers/image/v5/oci/layout"
 	"github.com/containers/image/v5/pkg/blobinfocache"
 	"github.com/containers/image/v5/transports"
@@ -229,13 +230,22 @@ func isDockerManifestUnknownError(err error) bool {
 	return ec.ErrorCode() == dockerdistributionapi.ErrorCodeManifestUnknown
 }
 
+func isOciImageNotFound(err error) bool {
+	var lerr ocilayout.ImageNotFoundError
+	if errors.As(err, &lerr) {
+		return true
+	}
+	var aerr ociarchive.ImageNotFoundError
+	return errors.As(err, &aerr)
+}
+
 // isNotFoundImageError heuristically attempts to determine whether an error
 // is saying the remote source couldn't find the image (as opposed to an
 // authentication error, an I/O error etc.)
 // TODO drive this into containers/image properly
 func isNotFoundImageError(err error) bool {
 	return isDockerManifestUnknownError(err) ||
-		errors.Is(err, ocilayout.ImageNotFoundError{})
+		isOciImageNotFound(err)
 }
 
 func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBuf replyBuf, retErr error) {
diff --git a/integration/proxy_test.go b/integration/proxy_test.go
index 826128da..28949eff 100644
--- a/integration/proxy_test.go
+++ b/integration/proxy_test.go
@@ -354,4 +354,11 @@ func (s *proxySuite) TestProxy() {
 		err = fmt.Errorf("Testing optional image %s: %v", knownNotExtantImage, err)
 	}
 	assert.NoError(t, err)
+
+	nonExistentArchive := "oci-archive:/enoent"
+	err = runTestOpenImageOptionalNotFound(p, nonExistentArchive)
+	if err != nil {
+		err = fmt.Errorf("Testing optional image %s: %v", nonExistentArchive, err)
+	}
+	assert.NoError(t, err)
 }
diff --git a/vendor/github.com/containers/image/v5/oci/archive/oci_src.go b/vendor/github.com/containers/image/v5/oci/archive/oci_src.go
index 6c9ee334..819659e7 100644
--- a/vendor/github.com/containers/image/v5/oci/archive/oci_src.go
+++ b/vendor/github.com/containers/image/v5/oci/archive/oci_src.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 
 	"github.com/containers/image/v5/internal/imagesource"
 	"github.com/containers/image/v5/internal/imagesource/impl"
@@ -41,7 +42,11 @@ type ociArchiveImageSource struct {
 func newImageSource(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (private.ImageSource, error) {
 	tempDirRef, err := createUntarTempDir(sys, ref)
 	if err != nil {
-		return nil, fmt.Errorf("creating temp directory: %w", err)
+		if errors.Is(err, fs.ErrNotExist) {
+			return nil, ImageNotFoundError{ref: ref}
+		} else {
+			return nil, fmt.Errorf("creating temp directory: %w", err)
+		}
 	}
 
 	unpackedSrc, err := tempDirRef.ociRefExtracted.NewImageSource(ctx, sys)
-- 
2.40.1

cgwalters · 2023-09-15T17:02:51Z

(not going to lie, it took me at least 20 minutes of confusion to realize that there's two copies of ImageNotFoundError in c/image and that the previous code was always buggy and also needed to use errors.As)

cgwalters · 2023-09-15T17:06:42Z

Tangential to this of course is my very strong opinion that downcasting and parsing errors should be an extremely rare event in code, and in e.g. languages like Rust the right thing to do is exactly how we model this API in the Rust bindings:

fn open_image_optional(&self, imgref: &str) -> Result<Option<OpenedImage>>

The common "not found" case is the Option - it's not an error! More on this in e.g. https://users.rust-lang.org/t/why-i-use-anyhow-error-even-in-libraries/68592

cgwalters · 2023-09-15T17:09:58Z

But rpm-ostree compose image seems to have its own format=oci option, so the transport:image value is not user-provided.

Yes, they are distinct options but they are very much intended to exactly map to the c/image transports and work the same way from a user PoV. (I just take the liberty to add a nicer registry alias instead of docker:// which just looks weird/legacy IMO)

mtrmac · 2023-09-15T17:39:48Z

How about this variant instead? (requires a patch to c/image)

Still proxy: Make OpenImageOptional work with oci-archive: #2114 (comment) . Introducing a new error type would maintain the existing API. (And introducing a new precisely-targeted error is, I think, the cleanest solution.)

@@ -41,7 +42,11 @@ type ociArchiveImageSource struct {
 func newImageSource(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (private.ImageSource, error) {
 	tempDirRef, err := createUntarTempDir(sys, ref)
 	if err != nil {
-		return nil, fmt.Errorf("creating temp directory: %w", err)
+		if errors.Is(err, fs.ErrNotExist) {
+			return nil, ImageNotFoundError{ref: ref}
+		} else {
+			return nil, fmt.Errorf("creating temp directory: %w", err)
+		}

This still matches the /var/tmp case; pushing the detection code down inside createUntarTempDir would allow precisely targeting the right os.Open.

cgwalters · 2023-09-15T17:43:03Z

Introducing a new error type would maintain the existing API. (And introducing a new precisely-targeted error is, I think the cleanest solution.)

Hmm. So with oci: transports, we can distinguish between "oci directory exists, has index.json etc." and "oci directory doesn't exist". But with oci-archive there's no such thing...either the file exists, or it doesn't - and ISTM that for oci-archive, this does semantically map to ImageNotFoundError. Right?

mtrmac · 2023-09-15T17:48:09Z

The common "not found" case is the Option - it's not an error!

(Many things in Go are awkward and unsafe, with no sum types, no optionality… The complete lack of compiler support for errors, and things like errors.Is vs. errors.As, with no compile-time checking, is also just sad.)

I think that works for a frequent expected cause of a specific operation; the c/image “transport” abstraction makes this harder (in a sense, that’s exactly what this PR is hitting — after abstracting to the NewImageSource interface method we can no longer discuss what exactly failed / how exactly it failed, without talking about concepts specific to a particular transport).

c/image can just about manage to provide a common types.ImageReference / transport.ImageName concept, but I think providing a common “image container” that can deal with registries, c/storage, oci-archive (a filesystem of dir: subdirectories?) is just not feasible. And “image not found in a container” vs “container not found at all” is that kind of a situation.

Ultimately, API stability does not allow us to change NewImageSource return values anyway…

By adding the standard ENOENT to our list of errors. I hit this while working on coreos/rpm-ostree#4598 which is a tool that builds base images and wants to query if the image exists beforehand. Signed-off-by: Colin Walters <[email protected]>

mtrmac · 2023-09-15T17:49:33Z

Hmm. So with oci: transports, we can distinguish between "oci directory exists, has index.json etc." and "oci directory doesn't exist". But with oci-archive there's no such thing...either the file exists, or it doesn't

No, oci-archive can also contain multiple images. So, oci-archive:existing-path:non-matching-reference is the current ImageNotFoundError, and oci-archive:/dev/null/this/does/not/exist:whatever is a different failure mode.

cgwalters · 2023-09-15T17:57:58Z

No, oci-archive can also contain multiple images. So, oci-archive:existing-path:non-matching-reference is the current ImageNotFoundError, and oci-archive:/dev/null/this/does/not/exist:whatever is a different failure mode.

Thanks, I didn't know that. But...I guess then I'm arguing that at least for all use cases I can think of, I really do want to treat "oci-archive file doesn't exist" here the same as ImageNotFoundError.

Unlike oci directories which require initialization, to me kind of the point of oci-archive is that it's just a single file that you can just...manage as a file. Requiring users of rpm-ostree compose image to create an "empty" oci-archive file would just be weird and defeat the purpose of the ergonomics here.

cgwalters · 2023-09-15T18:00:11Z

I've updated the main PR here so far to:

In c/image:

Move the error check further down
Necessarily reorder the open invocation first

In skopeo:

Add a test case for the "parent directory doesn't exist" correctly erroring

This is for containers/skopeo#2114 which is in turn a dependency of coreos/rpm-ostree#4598 Basically I want to map ENOENT to `ImageNotFound`, because the build tooling wants to treat "target image is not present" differently from "DNS lookup failed" or "we got EPERM". There's a bit of code motion here because we need to move the `os.Open()` call before creating a temporary directory. Signed-off-by: Colin Walters <[email protected]>

This is for containers/skopeo#2114 which is in turn a dependency of coreos/rpm-ostree#4598 Basically I want to map ENOENT to `ImageNotFound`, because the build tooling wants to treat "target image is not present" differently from "DNS lookup failed" or "we got EPERM". There's a bit of code motion here because we need to move the `os.Open()` call before creating a temporary directory.

cgwalters · 2023-09-18T14:04:41Z

Moved the c/image changes to containers/image#2123

This is for containers/skopeo#2114 which is in turn a dependency of coreos/rpm-ostree#4598 Basically I want to map ENOENT to a clear error, because the build tooling wants to treat "target image is not present" differently from "DNS lookup failed" or "we got EPERM". There's a bit of code motion here because we need to move the `os.Open()` call before creating a temporary directory. Signed-off-by: Colin Walters <[email protected]>

mtrmac · 2023-09-18T14:41:25Z

Requiring users of rpm-ostree compose image to create an "empty" oci-archive file would just be weird and defeat the purpose of the ergonomics here.

(The archives are not editable that way — they can either be read or written. But I do think that when reading a multi-image archive, differentiating the two failures should be possible.)

This is for containers/skopeo#2114 which is in turn a dependency of coreos/rpm-ostree#4598 Basically I want to map ENOENT to a clear error, because the build tooling wants to treat "target image is not present" differently from "DNS lookup failed" or "we got EPERM". There's a bit of code motion here because we need to move the `os.Open()` call before creating a temporary directory. Signed-off-by: Colin Walters <[email protected]>

github-actions · 2023-11-16T00:00:36Z

A friendly reminder that this PR had no activity for 30 days.

mtrmac · 2024-06-24T15:41:32Z

@cgwalters the c/image error type PR has merged; do you want to update this?

cgwalters force-pushed the open-optional-ociarchive branch from acaf245 to 21aadaf Compare September 15, 2023 12:55

cgwalters commented Sep 15, 2023

View reviewed changes

cgwalters mentioned this pull request Sep 15, 2023

compose-image: Add --initialize-mode coreos/rpm-ostree#4598

Merged

mtrmac reviewed Sep 15, 2023

View reviewed changes

cgwalters force-pushed the open-optional-ociarchive branch from 21aadaf to b80b222 Compare September 15, 2023 17:49

cgwalters mentioned this pull request Sep 18, 2023

ociarchive: Add new ArchiveFileNotFoundError containers/image#2123

Merged

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Oct 16, 2023

github-actions bot added the stale-pr label Nov 16, 2023

cgwalters mentioned this pull request Feb 26, 2024

main: return different exit code when an input is not found #2236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy: Make `OpenImageOptional` work with `oci-archive:` #2114

proxy: Make `OpenImageOptional` work with `oci-archive:` #2114

cgwalters commented Sep 15, 2023

cgwalters Sep 15, 2023

mtrmac Sep 15, 2023

mtrmac left a comment

mtrmac Sep 15, 2023

mtrmac Sep 15, 2023

cgwalters commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023

cgwalters commented Sep 15, 2023

cgwalters commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023

mtrmac commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023

mtrmac commented Sep 15, 2023

mtrmac commented Sep 15, 2023

cgwalters commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023

cgwalters commented Sep 18, 2023

mtrmac commented Sep 18, 2023

github-actions bot commented Nov 16, 2023

mtrmac commented Jun 24, 2024

proxy: Make OpenImageOptional work with oci-archive: #2114

Are you sure you want to change the base?

proxy: Make OpenImageOptional work with oci-archive: #2114

Conversation

cgwalters commented Sep 15, 2023

cgwalters Sep 15, 2023

Choose a reason for hiding this comment

mtrmac Sep 15, 2023

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Sep 15, 2023

Choose a reason for hiding this comment

mtrmac Sep 15, 2023

Choose a reason for hiding this comment

cgwalters commented Sep 15, 2023 • edited Loading

cgwalters commented Sep 15, 2023

cgwalters commented Sep 15, 2023

cgwalters commented Sep 15, 2023 • edited Loading

cgwalters commented Sep 15, 2023

mtrmac commented Sep 15, 2023 • edited Loading

cgwalters commented Sep 15, 2023

mtrmac commented Sep 15, 2023

mtrmac commented Sep 15, 2023

cgwalters commented Sep 15, 2023 • edited Loading

cgwalters commented Sep 15, 2023

cgwalters commented Sep 18, 2023

mtrmac commented Sep 18, 2023

github-actions bot commented Nov 16, 2023

mtrmac commented Jun 24, 2024

proxy: Make `OpenImageOptional` work with `oci-archive:` #2114

proxy: Make `OpenImageOptional` work with `oci-archive:` #2114

cgwalters commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023 •

edited

Loading

mtrmac commented Sep 15, 2023 •

edited

Loading

cgwalters commented Sep 15, 2023 •

edited

Loading