exporter: avoid descriptor annotations on docker manifests#1730
exporter: avoid descriptor annotations on docker manifests#1730AkihiroSuda merged 1 commit intomoby:masterfrom
Conversation
|
Looks like this breaks |
| delete(desc.Annotations, "containerd.io/uncompressed") | ||
| delete(desc.Annotations, "buildkit/createdat") | ||
| } else { | ||
| desc.Annotations = nil |
There was a problem hiding this comment.
@tonistiigi Cross-repo mount fails in the test. dockerPusher doesn't support it without containerd.io/distribution.source... Annotations.
--- FAIL: TestIntegration/TestLazyImagePush/worker=containerd-1.3 (1.26s)
client_test.go:2283:
Error Trace: client_test.go:2283
run.go:171
Error: Should be true
Test: TestIntegration/TestLazyImagePush/worker=containerd-1.3
Messages: unexpected error <nil>
Configuring the exporters used in TestLazyImagePush to push OCI image (with oci-mediatypes Attr or something) makes the test happy?
There was a problem hiding this comment.
@ktock Thanks for debug. Do you want to make a follow-up that makes oci-mediatypes=false and stargz invalid combination as they don't seem to be compatible then if annotation is required.
There was a problem hiding this comment.
@tonistiigi I'm willing to contribute to this issue, but this patch doesn't seem directly related to stargz config (actually, TestLazyImagePush isn't a test for stargz images). Discarding the descriptor annotations seem to end up disabling cross-repo mounts of any types of lazyrefs (including non-stargz layers).
Roughly reading through codes around exporter/pusher, I currently think we need changes on push handlers in util/push, based on the following understanding (please tell me if I'm missing something):
For non-lazy layers, even if the layer descriptors lost annotations (this occurs by this patch), containerd.io/distribution.source... are recovered from that layer's entry stored in the content store, during (util/push).annotateDistributionSourceHandler. But lazyrefs don't have these entries in the content store so this recovery doesn't work. So we might need additional logic something like recovering distribution annotations from the original manifest (not layers) blob.
cc: @sipsma
There was a problem hiding this comment.
@ktock You're right. We should make sure containerd.io/distribution.source do not end up in image manifests as well but if I remove them from here it would indeed disable cross-repo push as well. So we need some other way to pass annotations, not through the json.
sipsma
left a comment
sipsma
left a comment
There was a problem hiding this comment.
LGTM, one very minor nit, feel free to update if you agree but not a blocker in my mind at all
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
|
@tonistiigi Looks like it still persist (for Google Artifact Registry) with |
5 participants
Fixes docker/setup-buildx-action#29
Manifests with Docker media types do not define
Annotationsfor descriptor like oci spec does. Normally registries just ignore JSON keys they don't understand but GCR is picky and produces400error for such manifest. This PR makes sure that annotations are never in Docker manifests and also removes the internal annotations from OCI manifests as they do not add anything to the image context.@sipsma I believe this is a regression from
GetRemotechanges that now handle full descriptors.If you hit this another workaround is to switch to oci mediatypes.
-o type=image,oci-mediatypes=true,push=trueAlso found another GCR issue in containerd while debugging this. If you hit 401 specifically in GCR you are probably hitting containerd/containerd#4622
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com