From 9c81ed02c3b521bea5c56ddc2698f37a788b176f Mon Sep 17 00:00:00 2001 From: Steven Locke Date: Fri, 5 Feb 2021 15:20:33 -0800 Subject: [PATCH] Support non-distributable layers in Write and MultiWrite (#930) * Support non-distributable layers in Write and MultiWrite Signed-off-by: Dennis Leon * Rename the func option to WithNonDistributable * Also update the docstring * WithNondistributable directly accepts *options --- pkg/v1/remote/multi_write.go | 25 +++++------ pkg/v1/remote/multi_write_test.go | 41 +++++++++++++++++ pkg/v1/remote/options.go | 25 ++++++++--- pkg/v1/remote/write.go | 3 +- pkg/v1/remote/write_test.go | 74 ++++++++++++++++++++++++++++++- 5 files changed, 145 insertions(+), 23 deletions(-) diff --git a/pkg/v1/remote/multi_write.go b/pkg/v1/remote/multi_write.go index 803c22939..796b22481 100644 --- a/pkg/v1/remote/multi_write.go +++ b/pkg/v1/remote/multi_write.go @@ -45,23 +45,27 @@ func MultiWrite(m map[name.Reference]Taggable, options ...Option) error { } } + o, err := makeOptions(repo, options...) + if err != nil { + return err + } + // Collect unique blobs (layers and config blobs). blobs := map[v1.Hash]v1.Layer{} newManifests := []map[name.Reference]Taggable{} // Separate originally requested images and indexes, so we can push images first. images, indexes := map[name.Reference]Taggable{}, map[name.Reference]Taggable{} - var err error for ref, i := range m { if img, ok := i.(v1.Image); ok { images[ref] = i - if err := addImageBlobs(img, blobs); err != nil { + if err := addImageBlobs(img, blobs, o.allowNondistributableArtifacts); err != nil { return err } continue } if idx, ok := i.(v1.ImageIndex); ok { indexes[ref] = i - newManifests, err = addIndexBlobs(idx, blobs, repo, newManifests, 0) + newManifests, err = addIndexBlobs(idx, blobs, repo, newManifests, 0, o.allowNondistributableArtifacts) if err != nil { return err } @@ -70,10 +74,6 @@ func MultiWrite(m map[name.Reference]Taggable, options ...Option) error { return fmt.Errorf("pushable resource was not Image or ImageIndex: %T", i) } - o, err := makeOptions(repo, options...) - if err != nil { - return err - } // Determine if any of the layers are Mountable, because if so we need // to request Pull scope too. ls := []v1.Layer{} @@ -161,7 +161,7 @@ func MultiWrite(m map[name.Reference]Taggable, options ...Option) error { // addIndexBlobs adds blobs to the set of blobs we intend to upload, and // returns the latest copy of the ordered collection of manifests to upload. -func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repository, newManifests []map[name.Reference]Taggable, lvl int) ([]map[name.Reference]Taggable, error) { +func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repository, newManifests []map[name.Reference]Taggable, lvl int, allowNondistributableArtifacts bool) ([]map[name.Reference]Taggable, error) { if lvl > len(newManifests)-1 { newManifests = append(newManifests, map[name.Reference]Taggable{}) } @@ -177,7 +177,7 @@ func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repo if err != nil { return nil, err } - newManifests, err = addIndexBlobs(idx, blobs, repo, newManifests, lvl+1) + newManifests, err = addIndexBlobs(idx, blobs, repo, newManifests, lvl+1, allowNondistributableArtifacts) if err != nil { return nil, err } @@ -189,7 +189,7 @@ func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repo if err != nil { return nil, err } - if err := addImageBlobs(img, blobs); err != nil { + if err := addImageBlobs(img, blobs, allowNondistributableArtifacts); err != nil { return nil, err } @@ -202,7 +202,7 @@ func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repo return newManifests, nil } -func addImageBlobs(img v1.Image, blobs map[v1.Hash]v1.Layer) error { +func addImageBlobs(img v1.Image, blobs map[v1.Hash]v1.Layer, allowNondistributableArtifacts bool) error { ls, err := img.Layers() if err != nil { return err @@ -219,8 +219,7 @@ func addImageBlobs(img v1.Image, blobs map[v1.Hash]v1.Layer) error { if err != nil { return err } - if !mt.IsDistributable() { - // TODO(jonjohnsonjr): Add "allow-nondistributable-artifacts" option. + if !mt.IsDistributable() && !allowNondistributableArtifacts { continue } diff --git a/pkg/v1/remote/multi_write_test.go b/pkg/v1/remote/multi_write_test.go index 2e6fef7f8..6abe8b685 100644 --- a/pkg/v1/remote/multi_write_test.go +++ b/pkg/v1/remote/multi_write_test.go @@ -107,6 +107,47 @@ func TestMultiWrite(t *testing.T) { } } +func TestMultiWriteWithNondistributableLayer(t *testing.T) { + // Create a random image. + img1, err := random.Image(1024, 2) + if err != nil { + t.Fatal("random.Image:", err) + } + + // Create another image that's based on the first. + rl, err := random.Layer(1024, types.OCIRestrictedLayer) + if err != nil { + t.Fatal("random.Layer:", err) + } + img, err := mutate.AppendLayers(img1, rl) + if err != nil { + t.Fatal("mutate.AppendLayers:", err) + } + + // Set up a fake registry. + s := httptest.NewServer(registry.New()) + defer s.Close() + u, err := url.Parse(s.URL) + if err != nil { + t.Fatal(err) + } + + // Write the image. + tag1 := mustNewTag(t, u.Host+"/repo:tag1") + if err := MultiWrite(map[name.Reference]Taggable{tag1: img}, WithNondistributable); err != nil { + t.Error("Write:", err) + } + + // Check that tagged image is present. + got, err := Image(tag1) + if err != nil { + t.Error(err) + } + if err := validate.Image(got); err != nil { + t.Error("Validate() =", err) + } +} + // TestMultiWrite_Deep tests that a deeply nested tree of manifest lists gets // pushed in the correct order (i.e., each level in sequence). func TestMultiWrite_Deep(t *testing.T) { diff --git a/pkg/v1/remote/options.go b/pkg/v1/remote/options.go index 5544fabf9..04ff50827 100644 --- a/pkg/v1/remote/options.go +++ b/pkg/v1/remote/options.go @@ -29,13 +29,14 @@ import ( type Option func(*options) error type options struct { - auth authn.Authenticator - keychain authn.Keychain - transport http.RoundTripper - platform v1.Platform - context context.Context - jobs int - userAgent string + auth authn.Authenticator + keychain authn.Keychain + transport http.RoundTripper + platform v1.Platform + context context.Context + jobs int + userAgent string + allowNondistributableArtifacts bool } var defaultPlatform = v1.Platform{ @@ -173,3 +174,13 @@ func WithUserAgent(ua string) Option { return nil } } + +// WithNondistributable includes non-distributable (foreign) layers +// when writing images, see: +// https://github.com/opencontainers/image-spec/blob/master/layer.md#non-distributable-layers +// +// The default behaviour is to skip these layers +func WithNondistributable(o *options) error { + o.allowNondistributableArtifacts = true + return nil +} diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go index d850203d5..55fc1afe7 100644 --- a/pkg/v1/remote/write.go +++ b/pkg/v1/remote/write.go @@ -77,8 +77,7 @@ func Write(ref name.Reference, img v1.Image, options ...Option) error { if err != nil { return err } - if !mt.IsDistributable() { - // TODO(jonjohnsonjr): Add "allow-nondistributable-artifacts" option. + if !mt.IsDistributable() && !o.allowNondistributableArtifacts { continue } diff --git a/pkg/v1/remote/write_test.go b/pkg/v1/remote/write_test.go index a79d2ebb6..caf0ec1e8 100644 --- a/pkg/v1/remote/write_test.go +++ b/pkg/v1/remote/write_test.go @@ -1276,7 +1276,7 @@ func (l *fakeForeignLayer) Uncompressed() (io.ReadCloser, error) { return nil, nil } -func TestSkipForeignLayers(t *testing.T) { +func TestSkipForeignLayersByDefault(t *testing.T) { // Set up an image with a foreign layer. base := setupImage(t) img, err := mutate.AppendLayers(base, &fakeForeignLayer{t: t}) @@ -1302,6 +1302,78 @@ func TestSkipForeignLayers(t *testing.T) { } } +func TestWriteForeignLayerIfOptionSet(t *testing.T) { + // Set up an image with a foreign layer. + base := setupImage(t) + foreignLayer, err := random.Layer(1024, types.DockerForeignLayer) + if err != nil { + t.Fatal("random.Layer:", err) + } + img, err := mutate.AppendLayers(base, foreignLayer) + if err != nil { + t.Fatal(err) + } + + expectedRepo := "write/time" + headPathPrefix := fmt.Sprintf("/v2/%s/blobs/", expectedRepo) + initiatePath := fmt.Sprintf("/v2/%s/blobs/uploads/", expectedRepo) + manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo) + uploadPath := "/upload" + commitPath := "/commit" + var numUploads int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodHead && strings.HasPrefix(r.URL.Path, headPathPrefix) && r.URL.Path != initiatePath { + http.Error(w, "NotFound", http.StatusNotFound) + return + } + switch r.URL.Path { + case "/v2/": + w.WriteHeader(http.StatusOK) + case initiatePath: + if r.Method != http.MethodPost { + t.Errorf("Method; got %v, want %v", r.Method, http.MethodPost) + } + w.Header().Set("Location", uploadPath) + http.Error(w, "Accepted", http.StatusAccepted) + case uploadPath: + if r.Method != http.MethodPatch { + t.Errorf("Method; got %v, want %v", r.Method, http.MethodPatch) + } + atomic.AddInt32(&numUploads, 1) + w.Header().Set("Location", commitPath) + http.Error(w, "Created", http.StatusCreated) + case commitPath: + http.Error(w, "Created", http.StatusCreated) + case manifestPath: + if r.Method != http.MethodPut { + t.Errorf("Method; got %v, want %v", r.Method, http.MethodPut) + } + http.Error(w, "Created", http.StatusCreated) + default: + t.Fatalf("Unexpected path: %v", r.URL.Path) + } + })) + defer server.Close() + u, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("url.Parse(%v) = %v", server.URL, err) + } + tag, err := name.NewTag(fmt.Sprintf("%s/%s:latest", u.Host, expectedRepo), name.WeakValidation) + if err != nil { + t.Fatalf("NewTag() = %v", err) + } + + if err := Write(tag, img, WithNondistributable); err != nil { + t.Errorf("Write: %v", err) + } + + // 1 random layer, 1 foreign layer, 1 image config blob + wantUploads := int32(1 + 1 + 1) + if numUploads != wantUploads { + t.Fatalf("Write uploaded %d blobs, want %d", numUploads, wantUploads) + } +} + func TestTag(t *testing.T) { idx := setupIndex(t, 3) // Set up a fake registry.