Merge pull request distribution#864 from stevvooe/use-correct-manifest-link

registry/storage: use correct manifest link

Author: Richard Scothern

registry/storage/linkedblobstore.go

@@ -11,6 +11,10 @@ import (
 	"github.com/docker/distribution/uuid"
 )
 
+// linkPathFunc describes a function that can resolve a link based on the
+// repository name and digest.
+type linkPathFunc func(pm *pathMapper, name string, dgst digest.Digest) (string, error)
+
 // linkedBlobStore provides a full BlobService that namespaces the blobs to a
 // given repository. Effectively, it manages the links in a given repository
 // that grant access to the global blob store.
@@ -23,11 +27,13 @@ type linkedBlobStore struct {
 	deleteEnabled          bool
 	resumableDigestEnabled bool
 
-	// linkPath allows one to control the repository blob link set to which
-	// the blob store dispatches. This is required because manifest and layer
-	// blobs have not yet been fully merged. At some point, this functionality
-	// should be removed an the blob links folder should be merged.
-	linkPath func(pm *pathMapper, name string, dgst digest.Digest) (string, error)
+	// linkPathFns specifies one or more path functions allowing one to
+	// control the repository blob link set to which the blob store
+	// dispatches. This is required because manifest and layer blobs have not
+	// yet been fully merged. At some point, this functionality should be
+	// removed an the blob links folder should be merged. The first entry is
+	// treated as the "canonical" link location and will be used for writes.
+	linkPathFns []linkPathFunc
 }
 
 var _ distribution.BlobStore = &linkedBlobStore{}
@@ -213,13 +219,16 @@ func (lbs *linkedBlobStore) linkBlob(ctx context.Context, canonical distribution
 	// Don't make duplicate links.
 	seenDigests := make(map[digest.Digest]struct{}, len(dgsts))
 
+	// only use the first link
+	linkPathFn := lbs.linkPathFns[0]
+
 	for _, dgst := range dgsts {
 		if _, seen := seenDigests[dgst]; seen {
 			continue
 		}
 		seenDigests[dgst] = struct{}{}
 
-		blobLinkPath, err := lbs.linkPath(lbs.pm, lbs.repository.Name(), dgst)
+		blobLinkPath, err := linkPathFn(lbs.pm, lbs.repository.Name(), dgst)
 		if err != nil {
 			return err
 		}
@@ -236,33 +245,43 @@ type linkedBlobStatter struct {
 	*blobStore
 	repository distribution.Repository
 
-	// linkPath allows one to control the repository blob link set to which
-	// the blob store dispatches. This is required because manifest and layer
-	// blobs have not yet been fully merged. At some point, this functionality
-	// should be removed an the blob links folder should be merged.
-	linkPath func(pm *pathMapper, name string, dgst digest.Digest) (string, error)
+	// linkPathFns specifies one or more path functions allowing one to
+	// control the repository blob link set to which the blob store
+	// dispatches. This is required because manifest and layer blobs have not
+	// yet been fully merged. At some point, this functionality should be
+	// removed an the blob links folder should be merged. The first entry is
+	// treated as the "canonical" link location and will be used for writes.
+	linkPathFns []linkPathFunc
 }
 
 var _ distribution.BlobDescriptorService = &linkedBlobStatter{}
 
 func (lbs *linkedBlobStatter) Stat(ctx context.Context, dgst digest.Digest) (distribution.Descriptor, error) {
-	blobLinkPath, err := lbs.linkPath(lbs.pm, lbs.repository.Name(), dgst)
-	if err != nil {
-		return distribution.Descriptor{}, err
-	}
+	var (
+		resolveErr error
+		target     digest.Digest
+	)
+
+	// try the many link path functions until we get success or an error that
+	// is not PathNotFoundError.
+	for _, linkPathFn := range lbs.linkPathFns {
+		var err error
+		target, err = lbs.resolveWithLinkFunc(ctx, dgst, linkPathFn)
+
+		if err == nil {
+			break // success!
+		}
 
-	target, err := lbs.blobStore.readlink(ctx, blobLinkPath)
-	if err != nil {
 		switch err := err.(type) {
 		case driver.PathNotFoundError:
-			return distribution.Descriptor{}, distribution.ErrBlobUnknown
+			resolveErr = distribution.ErrBlobUnknown // move to the next linkPathFn, saving the error
 		default:
 			return distribution.Descriptor{}, err
 		}
+	}
 
-		// TODO(stevvooe): For backwards compatibility with data in "_layers", we
-		// need to hit layerLinkPath, as well. Or, somehow migrate to the new path
-		// layout.
+	if resolveErr != nil {
+		return distribution.Descriptor{}, resolveErr
 	}
 
 	if target != dgst {
@@ -276,13 +295,38 @@ func (lbs *linkedBlobStatter) Stat(ctx context.Context, dgst digest.Digest) (dis
 	return lbs.blobStore.statter.Stat(ctx, target)
 }
 
-func (lbs *linkedBlobStatter) Clear(ctx context.Context, dgst digest.Digest) error {
-	blobLinkPath, err := lbs.linkPath(lbs.pm, lbs.repository.Name(), dgst)
+func (lbs *linkedBlobStatter) Clear(ctx context.Context, dgst digest.Digest) (err error) {
+	// clear any possible existence of a link described in linkPathFns
+	for _, linkPathFn := range lbs.linkPathFns {
+		blobLinkPath, err := linkPathFn(lbs.pm, lbs.repository.Name(), dgst)
+		if err != nil {
+			return err
+		}
+
+		err = lbs.blobStore.driver.Delete(ctx, blobLinkPath)
+		if err != nil {
+			switch err := err.(type) {
+			case driver.PathNotFoundError:
+				continue // just ignore this error and continue
+			default:
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+// resolveTargetWithFunc allows us to read a link to a resource with different
+// linkPathFuncs to let us try a few different paths before returning not
+// found.
+func (lbs *linkedBlobStatter) resolveWithLinkFunc(ctx context.Context, dgst digest.Digest, linkPathFn linkPathFunc) (digest.Digest, error) {
+	blobLinkPath, err := linkPathFn(lbs.pm, lbs.repository.Name(), dgst)
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	return lbs.blobStore.driver.Delete(ctx, blobLinkPath)
+	return lbs.blobStore.readlink(ctx, blobLinkPath)
 }
 
 func (lbs *linkedBlobStatter) SetDescriptor(ctx context.Context, dgst digest.Digest, desc distribution.Descriptor) error {
@@ -297,5 +341,5 @@ func blobLinkPath(pm *pathMapper, name string, dgst digest.Digest) (string, erro
 
 // manifestRevisionLinkPath provides the path to the manifest revision link.
 func manifestRevisionLinkPath(pm *pathMapper, name string, dgst digest.Digest) (string, error) {
-	return pm.path(layerLinkPathSpec{name: name, digest: dgst})
+	return pm.path(manifestRevisionLinkPathSpec{name: name, revision: dgst})
 }

registry/storage/manifeststore_test.go

@@ -362,3 +362,37 @@ func TestManifestStorage(t *testing.T) {
 		t.Errorf("Unexpected success deleting while disabled")
 	}
 }
+
+// TestLinkPathFuncs ensures that the link path functions behavior are locked
+// down and implemented as expected.
+func TestLinkPathFuncs(t *testing.T) {
+	for _, testcase := range []struct {
+		repo       string
+		digest     digest.Digest
+		linkPathFn linkPathFunc
+		expected   string
+	}{
+		{
+			repo:       "foo/bar",
+			digest:     "sha256:deadbeaf",
+			linkPathFn: blobLinkPath,
+			expected:   "/docker/registry/v2/repositories/foo/bar/_layers/sha256/deadbeaf/link",
+		},
+		{
+			repo:       "foo/bar",
+			digest:     "sha256:deadbeaf",
+			linkPathFn: manifestRevisionLinkPath,
+			expected:   "/docker/registry/v2/repositories/foo/bar/_manifests/revisions/sha256/deadbeaf/link",
+		},
+	} {
+		p, err := testcase.linkPathFn(defaultPathMapper, testcase.repo, testcase.digest)
+		if err != nil {
+			t.Fatalf("unexpected error calling linkPathFn(pm, %q, %q): %v", testcase.repo, testcase.digest, err)
+		}
+
+		if p != testcase.expected {
+			t.Fatalf("incorrect path returned: %q != %q", p, testcase.expected)
+		}
+	}
+
+}

registry/storage/registry.go

@@ -108,6 +108,13 @@ func (repo *repository) Name() string {
 // may be context sensitive in the future. The instance should be used similar
 // to a request local.
 func (repo *repository) Manifests(ctx context.Context, options ...distribution.ManifestServiceOption) (distribution.ManifestService, error) {
+	manifestLinkPathFns := []linkPathFunc{
+		// NOTE(stevvooe): Need to search through multiple locations since
+		// 2.1.0 unintentionally linked into  _layers.
+		manifestRevisionLinkPath,
+		blobLinkPath,
+	}
+
 	ms := &manifestStore{
 		ctx:        ctx,
 		repository: repo,
@@ -120,14 +127,14 @@ func (repo *repository) Manifests(ctx context.Context, options ...distribution.M
 				repository:    repo,
 				deleteEnabled: repo.registry.deleteEnabled,
 				blobAccessController: &linkedBlobStatter{
-					blobStore:  repo.blobStore,
-					repository: repo,
-					linkPath:   manifestRevisionLinkPath,
+					blobStore:   repo.blobStore,
+					repository:  repo,
+					linkPathFns: manifestLinkPathFns,
 				},
 
 				// TODO(stevvooe): linkPath limits this blob store to only
 				// manifests. This instance cannot be used for blob checks.
-				linkPath: manifestRevisionLinkPath,
+				linkPathFns: manifestLinkPathFns,
 			},
 		},
 		tagStore: &tagStore{
@@ -153,9 +160,9 @@ func (repo *repository) Manifests(ctx context.Context, options ...distribution.M
 // to a request local.
 func (repo *repository) Blobs(ctx context.Context) distribution.BlobStore {
 	var statter distribution.BlobDescriptorService = &linkedBlobStatter{
-		blobStore:  repo.blobStore,
-		repository: repo,
-		linkPath:   blobLinkPath,
+		blobStore:   repo.blobStore,
+		repository:  repo,
+		linkPathFns: []linkPathFunc{blobLinkPath},
 	}
 
 	if repo.descriptorCache != nil {
@@ -171,7 +178,7 @@ func (repo *repository) Blobs(ctx context.Context) distribution.BlobStore {
 
 		// TODO(stevvooe): linkPath limits this blob store to only layers.
 		// This instance cannot be used for manifest checks.
-		linkPath:      blobLinkPath,
+		linkPathFns:   []linkPathFunc{blobLinkPath},
 		deleteEnabled: repo.registry.deleteEnabled,
 	}
 }

registry/storage/signaturestore.go

@@ -132,10 +132,10 @@ func (s *signatureStore) linkedBlobStore(ctx context.Context, revision digest.Di
 		repository: s.repository,
 		blobStore:  s.blobStore,
 		blobAccessController: &linkedBlobStatter{
-			blobStore:  s.blobStore,
-			repository: s.repository,
-			linkPath:   linkpath,
+			blobStore:   s.blobStore,
+			repository:  s.repository,
+			linkPathFns: []linkPathFunc{linkpath},
 		},
-		linkPath: linkpath,
+		linkPathFns: []linkPathFunc{linkpath},
 	}
 }

registry/storage/tagstore.go

@@ -122,7 +122,7 @@ func (ts *tagStore) delete(tag string) error {
 	return ts.blobStore.driver.Delete(ts.ctx, tagPath)
 }
 
-// namedBlobStore returns the namedBlobStore for the named tag, allowing one
+// linkedBlobStore returns the linkedBlobStore for the named tag, allowing one
 // to index manifest blobs by tag name. While the tag store doesn't map
 // precisely to the linked blob store, using this ensures the links are
 // managed via the same code path.
@@ -131,13 +131,12 @@ func (ts *tagStore) linkedBlobStore(ctx context.Context, tag string) *linkedBlob
 		blobStore:  ts.blobStore,
 		repository: ts.repository,
 		ctx:        ctx,
-		linkPath: func(pm *pathMapper, name string, dgst digest.Digest) (string, error) {
+		linkPathFns: []linkPathFunc{func(pm *pathMapper, name string, dgst digest.Digest) (string, error) {
 			return pm.path(manifestTagIndexEntryLinkPathSpec{
 				name:     name,
 				tag:      tag,
 				revision: dgst,
 			})
-		},
+		}},
 	}
-
 }