Refactor reference parsing for oci-layout #3300
Conversation
jedevc
force-pushed
the
oci-layout-reference-parsing
branch
from
d8f24d5
to
4757e53
Compare
| storeID := parsed.Hostname() | ||
|
|
||
| rslvr = getOCILayoutResolver(storeID, sm, "", g) | ||
| rslvr = getOCILayoutResolver(parsed.Hostname(), sm, "", g) |
There was a problem hiding this comment.
I'm surprised of the use of Hostname() in here. Even if you consider this input same as the image reference it doesn't mean that Hostname() is the storeID. storeID is the name that isn't digest or tag in that case. The reason this works is because it uses the containerd reference pkg that only deals with full reference strings(not normalized/familiar). For that library alpine:latest is invalid and doesn't work. It needs to be converted to docker.io/library/alpine before what doesn't make sense for the OCI case.
So the current code relies on accidental side-effect on passing invalid parameters.
There was a problem hiding this comment.
Agreed, this is weird.
I've tried reworking this around, but the core of the issue comes in that because we prefix the dummy image name in the dockerfile frontend, we can't just take parsed.Locator which would feel more logical IMO.
Some ideas:
- We could change
ResolveImageConfigto take<store-id>@<digest>with no need for the dummy image name - but then the argument isn't an actual ref, which definitely feels confusing. - We could modify the Opts passed to
ResolveImageConfigto take a store id, so that then we have access to that, similar to the rest of the cases in this file where we callgetOCILayoutResolverwhere we just ignore the ref entirely and get the store id from another specified location.
I think there's also a level of confusion about the dummy name eventually going to be replaced with a name that we could do tag resolution with, but I think as we've discussed previously, we'd prefer to do all that resolution client-side (similar to how we do local cache, etc), instead of somehow forwarding the data of the image layout into buildkit.
Also, related to dummy names - we should try and select some better dummy names, since these will show up in the progress, so we should make an effort to make them as user-readable as well can.
Instead of using custom parsing mechansisms for references in oci-layout, we use containerd's reference.Parse or docker distribution's reference.Parse (depending on where we do the parsing, and what's consistent with the file where it's already done). These operations are neater than manually parsing, and have hopefully more consistent error messages, and better handling of labels (for if/when those are introduced). Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
oci-layout-reference-parsing
branch
from
4757e53
to
6340184
Compare
馃崚 Cherry-picked off of #3118 (since it seems unlikely we'll take the functionality in the way that it's implemented for v0.11)
This PR introduces improvements to the user-facing error messages of the dockerfile oci-layout functionality. Essentially, we achieve this by using all of containerd's/distribution's reference parsers instead of doing manual string manipulation.
The main internal-api change that this introduces is combining the reference with the digest together in the OCILayout llb - we can just pass the whole thing together, instead of needing to later reconstruct it back together.
Signed-off-by: Justin Chadwell me@jedevc.com