Fixes #36678 - Support Link header in docker v2 repo discovery #10695
Conversation
|
Issues: #36678 |
|
Can one of the admins verify this patch? |
|
[test katello] |
| @@ -42,6 +44,70 @@ def run(resume_point) | |||
|
|
|||
| private | |||
|
|
|||
| def parse_parameters(field_value) | |||
There was a problem hiding this comment.
I have a proposition for simplifying how we determine if the Link header needs to be used.
Looking at the spec from Docker for how a container registry should communicate pagination (https://docs.docker.com/registry/spec/api/#pagination), it says:
To get the next result set, a client would issue the request as follows, using the URL encoded in the described Link header:
...
The above process should then be repeated until the Link header is no longer set.
So, perhaps we can simply directly check for rel="next" ? Or, if we're being super careful, we could do:
link.include?('rel=\"next\"') || link.include?('rel="next"')The same could be said for anchor.
There was a problem hiding this comment.
Also I'm assuming the Link has a form like: "link"=>["<http://pulpcore-api/v2/container-image-name/tags/list?n=100&last=last-tag-name>; rel=\"next\""]
This is what it looked like when talking to Pulp, just for an example.
There was a problem hiding this comment.
The Docker spec at https://docs.docker.com/registry/spec/api/#pagination says:
If there are indeed more results, the URL for the next block is encoded in an RFC5988open_in_new Link header, as a "next" relation.
It is silent on whether a response could carry further, different Link header data. Therefore, I would assume that it is better that a client explicitly checks for a "next" link encoded as per the Link spec (RFC 8288 obsoleting RFC 5988), rather than extrapolating from example Link headers seen in the wild.
(Though I do understand the sentiment to keep the code simple. And I wasn't impressed that I apparently had to code this all up ad-hoc here, instead of being able to reuse some existing code.)
There was a problem hiding this comment.
Gotcha, so we want to be very careful about plucking out "rel=next". My intention is to see if we can switch the code from from a "parse char by char" style to a "reads like english(?) via ruby built-in methods" style. I'll add some suggestions with some neat Ruby methods.
There was a problem hiding this comment.
It is silent on whether a response could carry further, different Link header data. Therefore, I would assume that it is better that a client explicitly checks for a "next" link encoded as per the Link spec (RFC 8288 obsoleting RFC 5988), rather than extrapolating from example Link headers seen in the wild.
I understand 8288 obsoletes 5988 but isn't the docker spec saying it has to be in 5988 format ?
For example I ran the following curl command. By "next" link are ya referring to link: </v2/_catalog?last=f32%2Fflatpak-sdk&n=100>; rel="next" below ?
$ curl -i -H "Accept: application/json" -H "Content-type: application/json" "https://registry.fedoraproject.org/v2/_catalog"
HTTP/2 200
strict-transport-security: max-age=31536000; includeSubDomains; preload
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
referrer-policy: same-origin
content-type: application/json; charset=utf-8
docker-distribution-api-version: registry/2.0
link: </v2/_catalog?last=f32%2Fflatpak-sdk&n=100>; rel="next"
vary: Accept
date: Tue, 12 Sep 2023 19:26:23 GMT
content-length: 1533
apptime: D=161655
x-fedora-proxyserver: proxy10.iad2.fedoraproject.org
x-fedora-requestid: ZQC7X4t6dbwX308xkqp_NwAEQQo
server: Apache
{"repositories":["0ad","abiword","aisleriot","alligator","angelfish","aqualung","ardour5","ardour6","arianna","ark","armacycles-ad","artikulate","astromenace","atlantik","atomix","audacious","audacity","baobab","berusky","bijiben","bitstower-markets","blackbox-terminal","blinken","bomber","bovo","calindori","calligraplan","celestia","celestia-qt","cheese","compneuro","convertall","d-feet","d-spy","darktable","dconf-editor","devhelp","dialect","dolphin","dosbox","dosbox-staging","drawing","easytag","elisa-player","endless-sky","eog","eol/astromenace","eol/baobab","eol/meld","epiphany","evince","evolution","exaile","exfalso","extremetuxracer","f29/cassandra","f29/cockpit","f29/fedora-toolbox","f29/golang","f29/httpd","f29/mirrormanager2-mirrorlist","f29/nginx","f29/origin-base","f29/origin-cli","f29/origin-control-plane","f29/origin-docker-builder","f29/origin-egress-dns-proxy","f29/origin-egress-router","f29/origin-f5-router","f29/origin-hyperkube","f29/origin-hypershift","f29/origin-keepalived-ipfailover","f29/origin-nginx-router","f29/origin-recycler","f29/origin-template-service-broker","f29/origin-tests","f29/postgresql","f29/ruby","f29/s2i-base","f29/s2i-core","f30/fedora-toolbox","f30/flatpak-runtime","f30/flatpak-sdk","f30/mariadb","f30/s2i-base","f30/s2i-core","f31/fedora-toolbox","f31/flatpak-runtime","f31/flatpak-sdk","f31/mariadb","f31/postgresql","f31/python-classroom","f31/python3","f31/redis","f31/ruby","f31/s2i-base","f31/s2i-core","f32/fedora-toolbox","f32/flatpak-runtime","f32/flatpak-sdk"]}
There was a problem hiding this comment.
@parthaa yep, that's the Link we're talking about.
There was a problem hiding this comment.
I understand 8288 obsoletes 5988 but isn't the docker spec saying it has to be in 5988 format ?
But none of the differences between the two RFCs should be of practical concern for us here, see https://www.rfc-editor.org/rfc/rfc8288#appendix-C "Changes from RFC 5988".
|
Rubocop is failing: https://ci.theforeman.org/blue/organizations/jenkins/katello-pr-test/detail/katello-pr-test/13324/pipeline/118 You can check this locally by running the following in the Foreman dir: |
stbergmann
force-pushed
the
nextlink
branch
from
ffd5c1f to
acc9221
Compare
addressed with acc9221 now |
stbergmann
force-pushed
the
nextlink
branch
from
acc9221 to
04eecce
Compare
| @@ -42,6 +44,70 @@ def run(resume_point) | |||
|
|
|||
| private | |||
|
|
|||
| def parse_parameters(field_value) | |||
There was a problem hiding this comment.
Gotcha, so we want to be very careful about plucking out "rel=next". My intention is to see if we can switch the code from from a "parse char by char" style to a "reads like english(?) via ruby built-in methods" style. I'll add some suggestions with some neat Ruby methods.
stbergmann
force-pushed
the
nextlink
branch
from
04eecce to
4fa3cde
Compare
stbergmann
force-pushed
the
nextlink
branch
from
4fa3cde to
55c47a5
Compare
|
[test katello] |
There was a problem hiding this comment.
Code is looking fine to me! Tested, working with https://registry.fedoraproject.org/.
This test line will need updating to return an object with headers rather than a hash: https://github.com/Katello/katello/blob/master/test/lib/repo_discovery_test.rb#L103
...as e.g. needed by a Repository Discovery with custom container images URL <https://registry.fedoraproject.org/>, which only includes the first 100 results in the original response.
stbergmann
force-pushed
the
nextlink
branch
from
55c47a5 to
ed016e0
Compare
addressed with ed016e0 now (though that might not be the most idiomatic approach?) |
|
[test katello] |
...as e.g. needed by a Repository Discovery with custom container images URL https://registry.fedoraproject.org/, which only includes the first 100 results in the original response.