From 671184e9100ada8c195fd5974749aba66c28cc2f Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 2 May 2023 12:09:48 -0400 Subject: [PATCH 1/2] Remove ManifestBuilder interface Defining an interface on the implementer side is generally not best practice in Go code. There is no code in the distribution module which consumes a ManifestBuilder value so there is no need to define the interface in the distribution module. Export the concrete ManifestBuilder types and modify the constructors to return concrete values. Co-authored-by: Sebastiaan van Stijn Signed-off-by: Cory Snider --- manifest/ocischema/builder.go | 2 +- manifest/schema2/builder.go | 14 +++++++------- manifests.go | 21 --------------------- registry/storage/manifeststore_test.go | 2 +- testutil/manifests.go | 26 ++++++++++++++++---------- 5 files changed, 25 insertions(+), 40 deletions(-) diff --git a/manifest/ocischema/builder.go b/manifest/ocischema/builder.go index 7d99c184f..736910a9c 100644 --- a/manifest/ocischema/builder.go +++ b/manifest/ocischema/builder.go @@ -32,7 +32,7 @@ type Builder struct { // NewManifestBuilder is used to build new manifests for the current schema // version. It takes a BlobService so it can publish the configuration blob // as part of the Build process, and annotations. -func NewManifestBuilder(bs distribution.BlobService, configJSON []byte, annotations map[string]string) distribution.ManifestBuilder { +func NewManifestBuilder(bs distribution.BlobService, configJSON []byte, annotations map[string]string) *Builder { mb := &Builder{ bs: bs, configJSON: make([]byte, len(configJSON)), diff --git a/manifest/schema2/builder.go b/manifest/schema2/builder.go index 585741d03..97cf12a0e 100644 --- a/manifest/schema2/builder.go +++ b/manifest/schema2/builder.go @@ -6,8 +6,8 @@ import ( "github.com/distribution/distribution/v3" ) -// builder is a type for constructing manifests. -type builder struct { +// Builder is a type for constructing manifests. +type Builder struct { // configDescriptor is used to describe configuration configDescriptor distribution.Descriptor @@ -22,8 +22,8 @@ type builder struct { // NewManifestBuilder is used to build new manifests for the current schema // version. It takes a BlobService so it can publish the configuration blob // as part of the Build process. -func NewManifestBuilder(configDescriptor distribution.Descriptor, configJSON []byte) distribution.ManifestBuilder { - mb := &builder{ +func NewManifestBuilder(configDescriptor distribution.Descriptor, configJSON []byte) *Builder { + mb := &Builder{ configDescriptor: configDescriptor, configJSON: make([]byte, len(configJSON)), } @@ -33,7 +33,7 @@ func NewManifestBuilder(configDescriptor distribution.Descriptor, configJSON []b } // Build produces a final manifest from the given references. -func (mb *builder) Build(ctx context.Context) (distribution.Manifest, error) { +func (mb *Builder) Build(ctx context.Context) (distribution.Manifest, error) { m := Manifest{ Versioned: SchemaVersion, Layers: make([]distribution.Descriptor, len(mb.dependencies)), @@ -46,12 +46,12 @@ func (mb *builder) Build(ctx context.Context) (distribution.Manifest, error) { } // AppendReference adds a reference to the current ManifestBuilder. -func (mb *builder) AppendReference(d distribution.Describable) error { +func (mb *Builder) AppendReference(d distribution.Describable) error { mb.dependencies = append(mb.dependencies, d.Descriptor()) return nil } // References returns the current references added to this builder. -func (mb *builder) References() []distribution.Descriptor { +func (mb *Builder) References() []distribution.Descriptor { return mb.dependencies } diff --git a/manifests.go b/manifests.go index f38d3ce8b..04558c654 100644 --- a/manifests.go +++ b/manifests.go @@ -26,27 +26,6 @@ type Manifest interface { Payload() (mediaType string, payload []byte, err error) } -// ManifestBuilder creates a manifest allowing one to include dependencies. -// Instances can be obtained from a version-specific manifest package. Manifest -// specific data is passed into the function which creates the builder. -type ManifestBuilder interface { - // Build creates the manifest from his builder. - Build(ctx context.Context) (Manifest, error) - - // References returns a list of objects which have been added to this - // builder. The dependencies are returned in the order they were added, - // which should be from base to head. - References() []Descriptor - - // AppendReference includes the given object in the manifest after any - // existing dependencies. If the add fails, such as when adding an - // unsupported dependency, an error may be returned. - // - // The destination of the reference is dependent on the manifest type and - // the dependency type. - AppendReference(dependency Describable) error -} - // ManifestService describes operations on manifests. type ManifestService interface { // Exists returns true if the manifest exists. diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index 1b071df25..2594e2cbf 100644 --- a/registry/storage/manifeststore_test.go +++ b/registry/storage/manifeststore_test.go @@ -595,7 +595,7 @@ func TestIndexManifestStorageWithSelectivePlatforms(t *testing.T) { // createRandomImage builds an image manifest and store it and its layers in the registry func createRandomImage(t *testing.T, testname string, imageMediaType string, blobStore distribution.BlobStore) (distribution.Manifest, error) { builder := ocischema.NewManifestBuilder(blobStore, []byte{}, map[string]string{}) - err := builder.(*ocischema.Builder).SetMediaType(imageMediaType) + err := builder.SetMediaType(imageMediaType) if err != nil { t.Fatal(err) } diff --git a/testutil/manifests.go b/testutil/manifests.go index 0bcc4ddba..271e0ac37 100644 --- a/testutil/manifests.go +++ b/testutil/manifests.go @@ -1,7 +1,6 @@ package testutil import ( - "context" "fmt" "github.com/distribution/distribution/v3" @@ -51,7 +50,18 @@ func MakeSchema2Manifest(repository distribution.Repository, digests []digest.Di return nil, fmt.Errorf("unexpected error storing content in blobstore: %v", err) } builder := schema2.NewManifestBuilder(d, configJSON) - return makeManifest(ctx, builder, digests) + for _, dgst := range digests { + if err := builder.AppendReference(distribution.Descriptor{Digest: dgst}); err != nil { + return nil, fmt.Errorf("unexpected error building schema2 manifest: %v", err) + } + } + + mfst, err := builder.Build(ctx) + if err != nil { + return nil, fmt.Errorf("unexpected error generating schema2 manifest: %v", err) + } + + return mfst, nil } func MakeOCIManifest(repository distribution.Repository, digests []digest.Digest) (distribution.Manifest, error) { @@ -61,19 +71,15 @@ func MakeOCIManifest(repository distribution.Repository, digests []digest.Digest var configJSON []byte builder := ocischema.NewManifestBuilder(blobStore, configJSON, make(map[string]string)) - return makeManifest(ctx, builder, digests) -} - -func makeManifest(ctx context.Context, builder distribution.ManifestBuilder, digests []digest.Digest) (distribution.Manifest, error) { - for _, digest := range digests { - if err := builder.AppendReference(distribution.Descriptor{Digest: digest}); err != nil { - return nil, fmt.Errorf("unexpected error building manifest: %v", err) + for _, dgst := range digests { + if err := builder.AppendReference(distribution.Descriptor{Digest: dgst}); err != nil { + return nil, fmt.Errorf("unexpected error building OCI manifest: %v", err) } } mfst, err := builder.Build(ctx) if err != nil { - return nil, fmt.Errorf("unexpected error generating manifest: %v", err) + return nil, fmt.Errorf("unexpected error generating OCI manifest: %v", err) } return mfst, nil From f1c8c414080c0cec46355c02cd42a1b5304b6788 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 17:53:17 +0200 Subject: [PATCH 2/2] Descriptor: do not implement Describable interface Commit cb6f0023500c3d2afb8c9f3ee4a0097526192156 implemented a generic Manifest interface to represent manifests in the registry and remove references to schema specific manifests. As part of this refactor, the Describable interface was introduced, which allowed for a single ManifestBuilder interface to handle both schema1 and schema2 manifests. Implementations of Describable are generally objects which can be described, not simply descriptors, but for convenience, this interface was also implemented on Descriptor in 2ff77c00bad887928be04367f0dd58f6aed5b756. This interface served its purpose, but no longer needed for most cases; schema2 (and OCI) descriptors do not need this method, making it only needed for `schema1.Reference`, which is now deprecated. Requiring this interface to be implemented limits interoperability between distribution's Descriptor and the OCI Descriptor types, which are identical in every other way, except for the presence of the Describable interface. This patch: - Removes the `Descriptor.Descriptor()` method (no longer implementing the `Describable` interface). - Updates ManifestBuilder interface and implementations to accept either - Updates ManifestBuilder interface and implementations to accept a `Descriptor`. After this patch, the caller is responsible for changing a describable type into a descriptor; builder.AppendReference(describable.Descriptor()) Signed-off-by: Sebastiaan van Stijn --- blobs.go | 9 --------- manifest/ocischema/builder.go | 4 ++-- manifest/schema2/builder.go | 4 ++-- manifests.go | 6 +++++- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/blobs.go b/blobs.go index 3f81f35cb..000430b14 100644 --- a/blobs.go +++ b/blobs.go @@ -85,15 +85,6 @@ type Descriptor struct { // depend on the simplicity of this type. } -// Descriptor returns the descriptor, to make it satisfy the Describable -// interface. Note that implementations of Describable are generally objects -// which can be described, not simply descriptors; this exception is in place -// to make it more convenient to pass actual descriptors to functions that -// expect Describable objects. -func (d Descriptor) Descriptor() Descriptor { - return d -} - // BlobStatter makes blob descriptors available by digest. The service may // provide a descriptor of a different digest if the provided digest is not // canonical. diff --git a/manifest/ocischema/builder.go b/manifest/ocischema/builder.go index 736910a9c..204f2ee10 100644 --- a/manifest/ocischema/builder.go +++ b/manifest/ocischema/builder.go @@ -96,8 +96,8 @@ func (mb *Builder) Build(ctx context.Context) (distribution.Manifest, error) { } // AppendReference adds a reference to the current ManifestBuilder. -func (mb *Builder) AppendReference(d distribution.Describable) error { - mb.layers = append(mb.layers, d.Descriptor()) +func (mb *Builder) AppendReference(ref distribution.Descriptor) error { + mb.layers = append(mb.layers, ref) return nil } diff --git a/manifest/schema2/builder.go b/manifest/schema2/builder.go index 97cf12a0e..d432ad60b 100644 --- a/manifest/schema2/builder.go +++ b/manifest/schema2/builder.go @@ -46,8 +46,8 @@ func (mb *Builder) Build(ctx context.Context) (distribution.Manifest, error) { } // AppendReference adds a reference to the current ManifestBuilder. -func (mb *Builder) AppendReference(d distribution.Describable) error { - mb.dependencies = append(mb.dependencies, d.Descriptor()) +func (mb *Builder) AppendReference(ref distribution.Descriptor) error { + mb.dependencies = append(mb.dependencies, ref) return nil } diff --git a/manifests.go b/manifests.go index 04558c654..97b246d20 100644 --- a/manifests.go +++ b/manifests.go @@ -48,8 +48,12 @@ type ManifestEnumerator interface { Enumerate(ctx context.Context, ingester func(digest.Digest) error) error } -// Describable is an interface for descriptors +// Describable is an interface for descriptors. +// +// Implementations of Describable are generally objects which can be +// described, not simply descriptors. type Describable interface { + // Descriptor returns the descriptor. Descriptor() Descriptor }