-
Notifications
You must be signed in to change notification settings - Fork 232
Update tag list conformance test to verify sort order #583
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
Conversation
|
Oh, that's exciting:
|
Ah, this is slightly more complicated than what I've implemented. Moving to draft for now (however the CI failure is a real actual bug, I think). |
|
Implementation now follows the spec more correctly, although I did not add any uppercase tags to test it with. That's probably worth doing too. (and again, that CI failure is a legit spec compliance issue) |
|
An edge case the spec is not currently clear on is what to do with |
|
Are there registries where this passes? If everyone is running a |
|
I did some historical diving, and distribution/distribution@aebe850 is the first time the original spec we're based on mentioned "lexical" ordering, however that was for the This series was included in registry (distribution) v2.1.0, but digging into the code I get down to https://github.com/distribution/distribution/blob/9ca7921603852314b18a6ecc19f91806935f34bd/registry/storage/tagstore.go#L29 being the "original" implementation of said "ordering", which I believe is shelling out to the drivers like S3, Azure, etc and just passing their sorting forward verbatim, so I don't think the sorting was actually implemented intentionally, but more documented as an incidental detail of the means of implementation (which is totally fair, since I don't think they were actually trying to make a perfect spec that other people could implement servers for in a 100% compatible and reliable way - I see that original spec as more of documentation for implementing sane client code, but that's not really the point here). Scratching the surface via Git Blame on the current codebase, distribution/distribution@4da2712 is extremely relevant, but 2021 is way too recent (especially given the "last" parameter being defined in the API back in 2015), however it is pretty surprising to see a simple "Tianon, stawp, what's the takeaway"I'm not a distribution-spec maintainer, but I'm pretty well convinced that No matter what, I think we really ought to update the spec language here in some way to clarify what the intent is (especially since we've got several popular registries that do not implement the spec language as-is, now including the original "reference" implementation). A conservative change might be to keep the current sorting language but somehow make it clear that a given registry MUST be deterministic, because otherwise the pagination A more proactive change would be to codify Go's |
|
I guess an even simpler alternative that would increase the number of compliant registries would be to remove the language about ordering entirely, but I don't know what the implications of that would be. I guess it could be downgraded to a SHOULD? No matter what, we ought to fix the spec, the test, or both. |
|
I guess I should either update this implementation to use |
|
Given how many registries are doing an ascii sort, my preference is to adjust the spec wording to match the implementations. Loosening or removing the wording to account for any sort order the registry wants to implement would mean client logic to list tags starting at some given point (e.g. at the start of semver named tags) would not be reliable on OCI conformant registries. I suspect the reason this hasn't come up before is because so many repositories default to lower case tags. Until I tried pushing a tag with upper case characters, it wasn't obvious whether the sort order was lexical. |
|
Yeah, that makes sense to me! For now, I've updated the test here to verify that the tag order is either lexical or "asciibetical" ( The CI failure is still a legitimate bug (because the tested registry does not appear to sort tags at all, which is problematic): |
|
I've added a new commit with a bare minimum change to the spec that makes it clear |
> If the list is not empty, the tags MUST be in lexical order (i.e. case-insensitive alphanumeric order). This update explicitly allows either lexical *or* "asciibetical", as many existing registries have already implemented this requirement via `sort.Strings`. This also adds uppercase versions of the `testN` tags to help test "case-insensitive alphanumeric order" (the edge cases that matter for sorted order). Signed-off-by: Tianon Gravi <[email protected]>
Signed-off-by: Tianon Gravi <[email protected]>
jdolitsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this LGTM but appears to fail in CI since we are using zot. @rchincha - does zot sort tags in any way?
Tags in zot are sorted in lexical order |
|
https://github.com/project-zot/zot/blob/552242f558af313dbd58a0927bbe54340fad57c1/pkg/api/routes.go#L367 appears to be where sorting is applied (which isn't lexical, but also isn't what I'm seeing in CI here and should still pass this test, so more digging required) |
|
Ah, that wasn't applied unconditionally until project-zot/zot@24e37eb which was in 2.0.0-rc7 and OCI is still testing rc6. |
This should fix the tag listing API to be sorted all the time (not lexical, but the updated language/test accounts for that): project-zot/zot@24e37eb Signed-off-by: Tianon Gravi <[email protected]>
|
I updated zot to 2.0.4 so that it includes that I considered updating to the latest latest zot (2.1.x), but figured a more conservative update was safer here. |
|
In case it's helpful, https://github.com/quay/quay/blob/2172c6bd4610937ea6c2861756ef2e0f2f05e1c7/data/model/oci/tag.py#L142 seems to be where Quay has implemented sorting for the tag listing API, and it appears to be using a standard "order by" in whatever database backend they've got, which is going to be ASCIIbetical 10 times out of 10 also. 😄 ❤️ |
sudo-bmitch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
rchincha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Just to add another data point here, Docker Hub is also currently using "asciibetical" sorting (not lexical): $ crane ls tianon/test | grep -i ascii
ASCIIBETICAL
AsCiIbEtIcAl
aScIiBeTiCaL
asciibetical |
Uh oh!
There was an error while loading. Please reload this page.