docker/reference: add IsFullIdentifier

[vrothberg]

containers/common/libimage is in need to check whether a given input
looks like a full image ID (i.e., a 64-byte hex string). Since
compiling regexes has a negative impact on init- and run-time, let's
expose a function to check whether a given string matches the already
compiled regex in docker/reference.

Signed-off-by: Valentin Rothberg vrothberg@redhat.com

@mtrmac PTAL
I want it for optimizing the fixes for containers/podman#12761.

[mtrmac]

I realize I’m probably being a pest, still… what’s the full story here?

(Conceptually, I’d expect a “parse input of $syntax” function that is the ~authoritative implementation of $syntax; not an API of “try parsing the input as $syntax1; if it failed, try $syntax2; if it failed, try $syntax3”. This new function could well be a part of the first “authoritative parser” implementation, it just raises my suspicion :) )

Specifically WRT containers/podman#12761 , compare

image/storage/storage_transport.go

Line 145 in 6ab45a6

} else if img, err := store.Image(possibleID); err == nil && img != nil && len(possibleID) >= minimumTruncatedIDLength && strings.HasPrefix(img.ID, possibleID) {

and

image/storage/storage_transport.go

Line 160 in 6ab45a6

if id == "" && len(ref) >= minimumTruncatedIDLength && !strings.ContainsAny(ref, "@:") {

enforcing a minimumTruncatedIDLength (OTOH the contents check is fairly weak, and using a stricter regex might be valuable). That’s, AFAICT, not the case in lookupImageInLocalStorage, where store.Image() accepts an unambiguous prefix of any length.

I’m also not sure how detecting a full identifier changes handling of short identifiers, perhaps that’s only tangentially related?

Is this just a locally-scoped change to modify the one

                if strings.HasPrefix(name, "sha256:") || (len(name) == 64 && !strings.ContainsAny(name, "/.:@")) {

check in Runtime.Pull? Consider ParseAnyReference — that would work fine for the ID syntaxes, OTOH the Named returned would not be usable, so that’s probably not better.

(Completely unrelated; The HasPrefix(name, "sha256:") check seems to be always false.)

---

Implementation looks OK, of course. From a maintenance POV, I’d prefer adding it to a separate file (regexp-additions.go)? so that the correspondence with distribution/distribution/reference is clear and easier to maintain — OTOH the test is really much better when integrated into the existing one, and placing the test and the function differently is not all that attractive.

[vrothberg]

I realize I’m probably being a pest, still… what’s the full story here?

🤣

I’m also not sure how detecting a full identifier changes handling of short identifiers, perhaps that’s only tangentially related?

I want it for an optimization. containers/podman#12761 happens because we're doing a lookupImageInLocalStorage() with the input before doing the short-name expansion. I want to keep looking up an image with it's full ID as fast as possible (i.e., immediately) without it being subject to short-name expansion with N look ups. In order to do that, I need check whether the input is a 64-byte hex string.

Here's the PR for c/common that should make my intention clearer: containers/common#939

[vrothberg]

There's another issue with tagging that I noticed:
podman tag $image $64-byte-hex does not error out but should -.-

[mtrmac]

containers/podman#12761 happens because we're doing a lookupImageInLocalStorage() with the input before doing the short-name expansion. I want to keep looking up an image with it's full ID as fast as possible (i.e., immediately) without it being subject to short-name expansion with N look ups. In order to do that, I need check whether the input is a 64-byte hex string.

OK, that’s actually getting us closer to a consistent parser — where the 64-byte hex strings are only ever interpreted one way. Nice.

LGTM; consider moving it to a separate file, but that’s quite a weak preference.

[vrothberg]

LGTM; consider moving it to a separate file, but that’s quite a weak preference.

Thanks! Done ✔️

[mtrmac]

Looks like CI is frequently broken now…

[vrothberg]

Looks like CI is frequently broken now…

--- FAIL: TestReadKeyring (0.00s)
keyring_test.go:185: expected to read 1 userkeyring, but get 2

I saw this one a number of times this week.

[mtrmac]

Looks like CI is frequently broken now…

#1477 , for the record.

[vrothberg]

green

[TomSweeneyRedHat]

@vrothberg you may need to rebase
changes LGTM otherwise