Skip to content
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.

Sign up for GitHub

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

exporter: avoid descriptor annotations on docker manifests #1730

Merged
merged 1 commit into from
Oct 20, 2020
Merged

exporter: avoid descriptor annotations on docker manifests #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion exporter/containerimage/export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ func (e *imageExporterInstance) Export(ctx context.Context, src exporter.Source,
}
}
if e.push {
annotations := map[digest.Digest]map[string]string{}
mprovider := contentutil.NewMultiProvider(e.opt.ImageWriter.ContentStore())
if src.Ref != nil {
remote, err := src.Ref.GetRemote(ctx, false, e.layerCompression)
Expand All @@ -247,6 +248,7 @@ func (e *imageExporterInstance) Export(ctx context.Context, src exporter.Source,
}
for _, desc := range remote.Descriptors {
mprovider.Add(desc.Digest, remote.Provider)
addAnnotations(annotations, desc)
}
}
if len(src.Refs) > 0 {
Expand All @@ -257,11 +259,12 @@ func (e *imageExporterInstance) Export(ctx context.Context, src exporter.Source,
}
for _, desc := range remote.Descriptors {
mprovider.Add(desc.Digest, remote.Provider)
addAnnotations(annotations, desc)
}
}
}

if err := push.Push(ctx, e.opt.SessionManager, sessionID, mprovider, e.opt.ImageWriter.ContentStore(), desc.Digest, targetName, e.insecure, e.opt.RegistryHosts, e.pushByDigest); err != nil {
if err := push.Push(ctx, e.opt.SessionManager, sessionID, mprovider, e.opt.ImageWriter.ContentStore(), desc.Digest, targetName, e.insecure, e.opt.RegistryHosts, e.pushByDigest, annotations); err != nil {
return nil, err
}
}
Expand Down Expand Up @@ -357,3 +360,17 @@ func getLayers(ctx context.Context, descs []ocispec.Descriptor, manifest ocispec
}
return layers, nil
}

func addAnnotations(m map[digest.Digest]map[string]string, desc ocispec.Descriptor) {
if desc.Annotations == nil {
return
}
a, ok := m[desc.Digest]
if !ok {
m[desc.Digest] = desc.Annotations
return
}
for k, v := range desc.Annotations {
a[k] = v
}
}
13 changes: 13 additions & 0 deletions exporter/containerimage/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"strings"
"time"

"github.com/containerd/containerd/content"
Expand Down Expand Up @@ -235,6 +236,18 @@ func (ic *ImageWriter) commitDistributionManifest(ctx context.Context, ref cache
}

for i, desc := range remote.Descriptors {
// oci supports annotations but don't export internal annotations
if oci {
delete(desc.Annotations, "containerd.io/uncompressed")
delete(desc.Annotations, "buildkit/createdat")
for k := range desc.Annotations {
if strings.HasPrefix(k, "containerd.io/distribution.source.") {
delete(desc.Annotations, k)
}
}
} else {
desc.Annotations = nil
Copy link
Collaborator

@ktock ktock Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>

https://github.com/containerd/containerd/blob/caad53dba7dbedbb5cdb95c54ec46c2d751a53e6/remotes/docker/pusher.go#L131

Configuring the exporters used in TestLazyImagePush to push OCI image (with oci-mediatypes Attr or something) makes the test happy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
mfst.Layers = append(mfst.Layers, desc)
labels[fmt.Sprintf("containerd.io/gc.ref.content.%d", i+1)] = desc.Digest.String()
}
Expand Down
20 changes: 17 additions & 3 deletions util/push/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"github.com/sirupsen/logrus"
)

func Push(ctx context.Context, sm *session.Manager, sid string, provider content.Provider, manager content.Manager, dgst digest.Digest, ref string, insecure bool, hosts docker.RegistryHosts, byDigest bool) error {
func Push(ctx context.Context, sm *session.Manager, sid string, provider content.Provider, manager content.Manager, dgst digest.Digest, ref string, insecure bool, hosts docker.RegistryHosts, byDigest bool, annotations map[digest.Digest]map[string]string) error {
desc := ocispec.Descriptor{
Digest: dgst,
}
Expand Down Expand Up @@ -87,7 +87,7 @@ func Push(ctx context.Context, sm *session.Manager, sid string, provider content
}

handlers := append([]images.Handler{},
images.HandlerFunc(annotateDistributionSourceHandler(manager, childrenHandler(provider))),
images.HandlerFunc(annotateDistributionSourceHandler(manager, annotations, childrenHandler(provider))),
filterHandler,
dedupeHandler(pushUpdateSourceHandler),
)
Expand Down Expand Up @@ -121,7 +121,7 @@ func Push(ctx context.Context, sm *session.Manager, sid string, provider content
return mfstDone(nil)
}

func annotateDistributionSourceHandler(manager content.Manager, f images.HandlerFunc) func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
func annotateDistributionSourceHandler(manager content.Manager, annotations map[digest.Digest]map[string]string, f images.HandlerFunc) func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
return func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
children, err := f(ctx, desc)
if err != nil {
Expand All @@ -138,6 +138,20 @@ func annotateDistributionSourceHandler(manager content.Manager, f images.Handler

for i := range children {
child := children[i]

if m, ok := annotations[child.Digest]; ok {
for k, v := range m {
if !strings.HasPrefix(k, "containerd.io/distribution.source.") {
continue
}
if child.Annotations == nil {
child.Annotations = map[string]string{}
}
child.Annotations[k] = v
}
}
children[i] = child

info, err := manager.Info(ctx, child.Digest)
if errors.Is(err, errdefs.ErrNotFound) {
continue
Expand Down