feat: add support for docker images #2714
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2714 +/- ##
==========================================
- Coverage 92.08% 92.01% -0.08%
==========================================
Files 169 170 +1
Lines 30038 30066 +28
==========================================
+ Hits 27660 27664 +4
- Misses 1759 1782 +23
- Partials 619 620 +1 ☔ View full report in Codecov by Sentry. |
|
Thank you for considering to support docker images! This looks very promising! As described in #724 (comment), we are using It's a bit surprising that the pushed artifact isn't listed in the Zot-UI. Even after pushing the artifact, the UI still shows |
|
Attaching and downloading/verifying cosign-attestations (see #724 (comment)) works fine: Or by digest: Again, the attestation artifacts are not shown in the UI, unfortunately. But everything else seems to work! |
|
Docker manifest lists (media type $ regctl image copy alpine localhost:5003/library/alpine
WARN[0006] Failed to push manifest err="failed to put manifest localhost:5003/library/alpine:latest: request failed: unexpected http status code: Unsupported Media Type [http 415]: {\"errors\":[{\"code\":\"MANIFEST_INVALID\",\"message\":\"manifest invalid\",\"detail\":{\"description\":\"During upload, manifests undergo several checks ensuring validity. If those checks fail, this error MAY be returned, unless a more specific error is included. The detail will contain information the failed validation.\",\"mediaType\":\"application/vnd.docker.distribution.manifest.list.v2+json\"}}]}" target="localhost:5003/library/alpine"
Manifests: 8/9 | Blobs: 28.060MB copied, 0.000B skipped | Elapsed: 6s
failed to put manifest localhost:5003/library/alpine:latest: request failed: unexpected http status code: Unsupported Media Type [http 415]: {"errors":[{"code":"MANIFEST_INVALID","message":"manifest invalid","detail":{"description":"During upload, manifests undergo several checks ensuring validity. If those checks fail, this error MAY be returned, unless a more specific error is included. The detail will contain information the failed validation.","mediaType":"application/vnd.docker.distribution.manifest.list.v2+json"}}]} |
^ feature-ask-creep as cautioned ... "Properly" supporting this means UI changes, sync/mirror changes, cve scan changes, scrub changes, etc ... So now our turn, when can we expect these additional PRs from the community :) |
rchincha
force-pushed
the
docker
branch
2 times, most recently
from
505af16
to
e6f5117
Compare
PR updated |
|
Just to be clear on expectations ... The plan is to initially include a config variable to support docker images for storage alone. |
|
https://github.com/dkowis/zot-builder I built the containers here using the same action as the regular zot repo. I haven't had a chance to try it yet, but I will soon. They're built to include this pull request. I am not quite skilled enough to tell it how to run every update to the pr |
I have pushed a docker buildx built container into zot. I can confirm that it doesn't show up in the UI anywhere, but a pull works just fine. |
^ a new "compat" config field is now added |
rchincha
force-pushed
the
docker
branch
4 times, most recently
from
54be4e4
to
9a64b2d
Compare
Issue project-zot#724 A new config section under "HTTP" called "Compat" is added which currently takes a list of possible compatible legacy media-types. Only "docker2s2" (Docker Manifest V2 Schema V2) is currently supported. Signed-off-by: Ramkumar Chinchani <[email protected]>
rchincha
requested review from
Andreea-Lupu,
shimish2 and
andaaron
as code owners
|
@andaaron let's merge this so community can at least start using zot as a docker registry. |
| @@ -62,10 +65,10 @@ func GetManifestDescByReference(index ispec.Index, reference string) (ispec.Desc | |||
| } | |||
|
|
|||
| func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaType string, body []byte, | |||
There was a problem hiding this comment.
Shouldn't we validate the docker manifest the same as ispec.MediaTypeImageManifest and the docker image list as a ispec.MediaTypeImageIndex in this function?
Theoretically we we should be able to unmarshal them, and use the references for dedupe, gc, and other essential features.
| return true | ||
| } | ||
| } | ||
|
|
||
| return mediaType == ispec.MediaTypeImageIndex || | ||
| mediaType == ispec.MediaTypeImageManifest | ||
| } |
There was a problem hiding this comment.
Given my other comment on this file is applied, the IsNonDistributable function should be modified to include the docker media types for layers.
| } | ||
| } | ||
|
|
||
| func (mc *MediaCompatibility) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
Is this code used? It has 0 coverage.
| DockerManifestV2SchemaV2 = "docker2s2" | ||
| ) | ||
|
|
||
| func (mc MediaCompatibility) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
Is this code used? It has 0 coverage.
There was a problem hiding this comment.
This file should have been updated to handle the docker media types when writing to the repo index.
Same as the other methods reading/updating the repo index.
I am thinking in particular about the docker manifest list, and that it needs to be unmarshaled into an oci Index struct in order to handle the references to the manifests inside. There are some recursive functions used.
That code is in pkg/storage/common/common.go but imported here.
This is just 1 example, there may be other such cases where we need to unmarshal the docker manifests/manifest lists.
There may be changes needed here and in pkg/storage/common/common.go
| FROM public.ecr.aws/docker/library/busybox:latest | ||
| RUN echo "hello world" > /testfile | ||
| EOF | ||
| docker build -f Dockerfile . -t localhost:${zot_port}/test |
There was a problem hiding this comment.
Can we make sure docker mediatypes are produced? The docker command may be an alias for some other tool, such as podman.
I think we can handle the search / CVE / UI changes in a separate PR, but the storage (GC/dedupe/scrub) changes should be included in this PR. |
Issue #724
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.