From d00586de9f21e6e7d3736646b2f434e76a25f00a Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 12 Aug 2015 13:07:57 -0700 Subject: [PATCH 1/2] Use correct path for manifest revision path Unfortunately, the refactor used the incorrect path for manifest links within a repository. While this didn't stop the registry from working, it did break compatibility with 2.0 deployments for manifest fetches. Tests were added to ensure these are locked down to the appropriate paths. Signed-off-by: Stephen J Day --- registry/storage/linkedblobstore.go | 6 ++++- registry/storage/manifeststore_test.go | 34 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/registry/storage/linkedblobstore.go b/registry/storage/linkedblobstore.go index 2ba62a95..d8252e5d 100644 --- a/registry/storage/linkedblobstore.go +++ b/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. @@ -297,5 +301,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}) } diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index a4ce9149..0bb72fb0 100644 --- a/registry/storage/manifeststore_test.go +++ b/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) + } + } + +} From 06a098c632aee74619a06f88c23a06140f442a6f Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 12 Aug 2015 13:11:13 -0700 Subject: [PATCH 2/2] Maintain manifest link compatibility Unfortunately, the 2.1 releease has written manfiest links into the wrong directory. This doesn't affect new 2.1 deployments but fixing this to be 2.0 backwards compatible has broken 2.1.0 compatibility. To ensure we have compatibility between 2.0, 2.1.0 and future releases, we now check one of several locations to identify a manifest link. Signed-off-by: Stephen J Day --- registry/storage/linkedblobstore.go | 92 +++++++++++++++++++++-------- registry/storage/registry.go | 23 +++++--- registry/storage/signaturestore.go | 8 +-- registry/storage/tagstore.go | 7 +-- 4 files changed, 88 insertions(+), 42 deletions(-) diff --git a/registry/storage/linkedblobstore.go b/registry/storage/linkedblobstore.go index d8252e5d..dc670542 100644 --- a/registry/storage/linkedblobstore.go +++ b/registry/storage/linkedblobstore.go @@ -27,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{} @@ -217,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 } @@ -240,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 { @@ -280,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) - if err != nil { - return err +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 lbs.blobStore.driver.Delete(ctx, blobLinkPath) + 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 lbs.blobStore.readlink(ctx, blobLinkPath) } func (lbs *linkedBlobStatter) SetDescriptor(ctx context.Context, dgst digest.Digest, desc distribution.Descriptor) error { diff --git a/registry/storage/registry.go b/registry/storage/registry.go index c5058b80..b6e0ba4d 100644 --- a/registry/storage/registry.go +++ b/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, } } diff --git a/registry/storage/signaturestore.go b/registry/storage/signaturestore.go index 78fd2e6c..105d66f3 100644 --- a/registry/storage/signaturestore.go +++ b/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}, } } diff --git a/registry/storage/tagstore.go b/registry/storage/tagstore.go index a74d9b09..a7ca3301 100644 --- a/registry/storage/tagstore.go +++ b/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, }) - }, + }}, } - }