ociserver: "end-relative range not supported"

[tianon]

The OCI conformance tests are getting a (much needed, IMO) rewrite in opencontainers/distribution-spec#588, and I've been playing with them against a bare-bones ocimem implementation. There are a number of small gaps, but overall we're in pretty good shape (more in #45 (comment), for example). 🫶

One of the gaps introduced with recent new tests there relates to an (intentional?) design of the blob range interface here, which explicitly doesn't support Range: bytes=-<suffix-length>:

oci/ociregistry/ociserver/range.go

Line 66 in 3adeb86

return nil, errors.New("end-relative range not supported")

oci/ociregistry/ociserver/range.go

Lines 24 to 28 in 3adeb86

	// This adapted from the code in fs.go in the net/http package.
	// The main difference is that it's more limited because we
	// don't have access to the size before parsing the range,
	// (because otherwise we'd have to make an extra round trip
	// to fetch the size before making the actual request).

When looking at the documentation for the GetBlobRange interface function, it's clear that if this wasn't intentional by design, it's certainly a breaking change if we wanted to fix this in a way that shoves this detail down on the implementations (where it could be most efficiently implemented):

oci/ociregistry/interface.go

Lines 98 to 103 in 3adeb86

	// GetBlobRange is like GetBlob but asks to get only the given range of bytes from the blob,
	// starting at offset0, up to but not including offset1.
	// If offset1 is negative or exceeds the actual size of the blob, GetBlobRange will
	// return all the data starting from offset0.
	// The context also controls the lifetime of the returned BlobReader.
	GetBlobRange(ctx context.Context, repo string, digest Digest, offset0, offset1 int64) (BlobReader, error)

I see three major options here:

1. do nothing, and continue to consider this use case unsupported (not my favorite option, but a valid one 🖤)

2. make a breaking change to the interface, for example to say that offset0 might be negative which means offset1 is the amount of the end of the file to be returned (not great because it's a silent breaking change -- maybe there's a better way to skin the breaking change)

3. perform the "extra round trip" alluded to in the range.go documentation, but only for this use case:

   oci/ociregistry/ociserver/reader.go

   Lines 85 to 86 in 3adeb86

   	rng := ranges[0]
   	blob, err := r.backend.GetBlobRange(ctx, rreq.Repo, ociregistry.Digest(rreq.Digest), rng.start, rng.end)

   if rng.start is negative, we hit ResolveBlob to get Size and pre-calculate

   (great because it'll always work, not great because an implementation can't choose to make that more efficient, like if you're using an ociclient as an ociserver and each of these operations are a full round-trip, or even S3-backed and have to look up the size to then look up the range when a better implementation could just pass the Range forward as-is)

[tianon]

Oh, apparently we have an option 4, which is to respond with 200 and the full blob instead, although I'm not sure how that would fit into the interface as-defined.

FWIW, I implemented option 2 in my WIP branch and the changes necessary were surprisingly minimal: cue-labs:3adeb86...tianon:2e548aa#diff-8fde3fab589ad545112000688693d47ec93225a56c9a673cda51346870645829 (if it's too hard to pick out the related changes here, I'm happy to make a cleaner commit with only the changes relevant to this issue instead) -- the biggest problem is that it affects downstream consumers because they might get a -1 where they weren't expecting to ever have negative values and I don't know how horrible that is

(for what it's worth, with that branch as-is, my code and ocimem are passing 100% of the current tests from opencontainers/distribution-spec#588 again, including all the sha512 tests 🥳)