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.

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

oci buildcache: handle pagination of tags #43136

Merged

Conversation

scottwittenburg
Copy link
Contributor

Setting up an example ci pipeline on gitlab.com, I discovered that registry.gitlab.com only returns 100 tags at a time, so my oci binary cache index only ever contained the first 100 specs alphabetically.

This PR updates the mock oci registry to paginate results as described in the oci distribution spec, and adds a list_tags method to the oci.py module to handle the paginated results.

@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality tests General test capability(ies) labels Mar 12, 2024
@scottwittenburg
Copy link
Contributor Author

fyi @zackgalbreath

@alecbcs
Copy link
Member

alecbcs commented Mar 12, 2024

cc @haampie

lib/spack/spack/oci/oci.py Outdated Show resolved Hide resolved
lib/spack/spack/oci/oci.py Outdated Show resolved Hide resolved
@haampie
Copy link
Member

haampie commented Mar 12, 2024

I would follow the spec strictly even if inefficient. There's no mention of "MUST respond with Link header". That's just a gitlab implementation detail. So send another request unconditionally? (They do refer to a Link header but don't talk about pagination -- you might wanna file an issue in the spec because it's imprecise?) Edit: see comment below, the spec is broken.

Notice that gitlab is not following the OCI spec, they are supposed to return all.

It could be that their registry is a Docker registry, which may or may not behave differently from an OCI registry. If that is the case, then check if you can infer from the response headers that it is a Docker not OCI registry, and only then use their Link response header if it is documented (where potentially you can assume that it does not follow the Link header RFC either...)

@haampie haampie self-assigned this Mar 12, 2024
lib/spack/spack/oci/oci.py Outdated Show resolved Hide resolved
@haampie
Copy link
Member

haampie commented Mar 12, 2024

Incredible: opencontainers/distribution-spec#461 (comment) "missed in review" how can you miss basic things like this in a spec.

See opencontainers/distribution-spec#470.

@scottwittenburg
Copy link
Contributor Author

Well, now I've caught up a bit on the discussion regarding how the spec handles tag listing. Maybe we should wait to proceed with this until that gets ironed out?

@haampie
Copy link
Member

haampie commented Mar 12, 2024

So my TL;DR to see if we're on the same page:

The OCI spec (accidentally) omits a key part abou pagination in case of many tags. Right now it says the registry should not do pagination unless the user sets a limit. The current Spack implementation conforms to the spec (we don't set a limit).

In practice registries implement pagination by default because they wanna avoid possible denial of service attacks with large requests.

How they implement it is registry specific: some (like whatever gitlab uses) set Link response headers, others (quay.io) add additional info to the JSON response.


A strategy that still conforms to the spec is to request with n=100, stop if < 100 results, or continue with n=100&last=... if not. That does not rely on undocumented Link: ... headers. The assumption is that no registry implements automatic pagination with fewer than 100 items max. However, it also assumes that all major registries implement this part of the spec, which has to be verified (quay, dockerhub, github packages, gitlab, others?)

@scottwittenburg
Copy link
Contributor Author

@haampie Following up on our slack conversation this morning:

TL;DR

gitlab, github, and dockerhub all use Link headers for pagination, while quay doesn't attempt to imlement the OCI spec at all. And my guess is the distribution spec will be updated to correct the oversight, and the Link headers will eventually be documented. Unless you object, my vote is to fix up the issues you pointed out with this PR and go with the Link header approach. But your suggested strategy that does not rely on undocumented features seems like it would work too, and if you have a strong preference for that, I'm happy to go that way instead.

Some extra details

I couldn't find any evidence that quay.io actually implements the oci distribution spec, to me it appears they have their own custom rest api, which as you mentioned, returns json objects for the list tags functionality, where each response looks like:

{
    tags: [{...}, ...],
    page: 1,
    has_additional: true,
}

So I don't think we should take inspiration from quay.io here, since it seems the odd one out. But let me know if you disagree.

However, I tested gitlab, dockerhub, and github container registry, and discovered they all use Link headers to do pagination. For gitlab and ghcr, I was able to find images with enough tags to exceed the default limit (meaning I didn't have to put n=10 in my query). While they both sent a Link header indicating the last returned tag, gitlab included their default value for n (100) in the uri, while ghcr put n=0. The ghcr behavior there isn't super helpful for clients, but I don't think this PR would be affected by it, since I wasn't using the n returned in the Link header anyway. Also, I noticed gitlab sorted the tags alphabetically, as required by the spec, while ghcr returned them in some arbitrary order of their own.

For dockerhub, I learned you can't actually use the registry api to query tags for the "official" images, like e.g. python. If you try to do that, you might lose your afternoon debugging why their authentication doesn't work: you request the resource, you get a 401 response with a www-authenticate header, you use the contents of that to request a token, you receive the token, and then when you use it to authorize your original resource request, you get another 401 response with a www-authenticate header with an extra error:insufficient_scope. Which is frustrating. But it turns out that's what they do for the official images, and once I knew that was the problem I tried with pytorch/pytorch, and it worked, though it doesn't have enough tags to exceed dockerhub's single response tag limit. But I could at least verify they use Link headers by passing n=10 in my query.

If you know of a "non-official" image on dockerhub with a lot of tags, I'd like to list the tags and see what I get without a limiting query, but I think at this point, it's clear some of the major OCI registries use Link header for pagination.

@haampie
Copy link
Member

haampie commented Mar 13, 2024

Okay, I'm fine with the Link response header then. Do the registries follow https://www.rfc-editor.org/rfc/rfc5988.html? Do they encode URLs?

Also add a test

@scottwittenburg
Copy link
Contributor Author

Do the registries follow https://www.rfc-editor.org/rfc/rfc5988.html? Do they encode URLs?

To me it looks like not, unless curl -v --raw is processing the headers before printing them out. Here is how I have been doing my testing, and some sample output:

< HTTP/2 200 
< content-type: application/json
< docker-distribution-api-version: registry/2.0
< link: </v2/spack/github-actions-buildcache/tags/list?last=libunistring-1.1-k46tvbaphmkbookxr5krcmw6koqjlvob.spack&n=10>; rel="next"
< date: Wed, 13 Mar 2024 18:49:13 GMT
< content-length: 603
< x-github-request-id: 915A:1788:27E0D5:2FFC2A:65F1F529
< 
{"name":"spack/github-actions-buildcache","tags":["libmd-1.0.4-gczlerelbro64t2ygs35n7drc3qyuwie.spack","util-macros-1.19.3-3azfa2rh2pv6by7hqenafdsbgwrckrzr.spack","xz-5.4.1-upooqgvsr6el4sjlpa6kmvpc4prr6gke.spack","tar-1.34-j7motvrkm6uiewr5lcdxjpzoyeodl2ib.spack","krb5-1.20.1-j4fzaidhwwwwsu56a3au4khwgaaott5g.spack","libmd-1.0.4-itaxviw36lee7jym6obpehtqtdstdadl.spack","util-linux-uuid-2.38.1-3g7ojlkxwwtz42bbutocm55xc3vlx6be.spack","util-macros-1.19.3-bvdqy2zk2kv7mb7qmhikyssuaykshuir.spack","tar-1.34-lta4rocxkw5f4vyql7ck3sw7zgc6pulf.spack","libunistring-1.1-k46tvbaphmkbookxr5krcmw6koqjlvob.spack"]}

@haampie
Copy link
Member

haampie commented Mar 14, 2024

Hm okay. Can run unquote regardless, it's a no-op in that case. In principle it's not necessary because /v2/<name>/tags/list?=last=<tag> cannot contain a > with valid image name and tag, but still, better safe than sorry.

From the docs notice we claim to support quay.io, so need to deal with it separately. Maybe take inspiration from here, it's also link header based.

@haampie
Copy link
Member

haampie commented May 10, 2024

Any news here @scottwittenburg?

@scottwittenburg
Copy link
Contributor Author

I didn't have the time to add support for quay.io at the time, so I set this aside. I'm hoping to pick it up again in the next week or so.

@haampie
Copy link
Member

haampie commented May 17, 2024

I need this myself for https://github.com/spack/github-actions-buildcache, so I've pushed a change for improved Link: parsing. It's not exactly following RFC 5988, but does to a reasonable extent.

@haampie haampie added this to the v0.22.1 milestone May 17, 2024
haampie
haampie previously approved these changes May 17, 2024
Copy link
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-approving, can you confirm this is OK for you @scottwittenburg?

@haampie haampie merged commit b33e2d0 into spack:develop May 18, 2024
34 of 35 checks passed
haampie added a commit that referenced this pull request May 18, 2024
This fixes an issue where ghcr, gitlab and possibly other container registries paginate tags by default, which violates the OCI spec v1.0, but is common practice (the spec was broken itself). After this commit, you can create build cache indices of > 100 specs on ghcr.

Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>
@haampie haampie mentioned this pull request May 18, 2024
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands core PR affects Spack core functionality fetching tests General test capability(ies) utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants