From b2b3f86039b3936710632d50157fa21db2d89836 Mon Sep 17 00:00:00 2001 From: Bracken Dawson Date: Mon, 15 Feb 2021 18:36:19 +0000 Subject: [PATCH] Remove workaround from 2.1.1 for faulty 2.1.0 manifest links This reverts commit 06a098c632aee74619a06f88c23a06140f442a6f This changes the function of linkedBlobStatter.Clear(). It was either removing the first of two possible manifest links or returning nil if none were found. Now it once again it removes only the valid manifest link or returns an error if none are found. Signed-off-by: Bracken Dawson --- registry/storage/linkedblobstore.go | 87 +++++++---------------------- registry/storage/registry.go | 23 +++----- registry/storage/tagstore.go | 17 +++--- 3 files changed, 37 insertions(+), 90 deletions(-) diff --git a/registry/storage/linkedblobstore.go b/registry/storage/linkedblobstore.go index 89573ddc..40338d89 100644 --- a/registry/storage/linkedblobstore.go +++ b/registry/storage/linkedblobstore.go @@ -32,13 +32,11 @@ type linkedBlobStore struct { deleteEnabled bool resumableDigestEnabled bool - // 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 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 + // 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 linkPathFunc // linkDirectoryPathSpec locates the root directories in which one might find links linkDirectoryPathSpec pathSpec @@ -338,16 +336,13 @@ 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 := linkPathFn(lbs.repository.Named().Name(), dgst) + blobLinkPath, err := lbs.linkPath(lbs.repository.Named().Name(), dgst) if err != nil { return err } @@ -364,46 +359,31 @@ type linkedBlobStatter struct { *blobStore repository distribution.Repository - // 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 + // 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 linkPathFunc } var _ distribution.BlobDescriptorService = &linkedBlobStatter{} func (lbs *linkedBlobStatter) Stat(ctx context.Context, dgst digest.Digest) (distribution.Descriptor, error) { - var ( - found bool - 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 { - found = true - break // success! - } + blobLinkPath, err := lbs.linkPath(lbs.repository.Named().Name(), dgst) + if err != nil { + return distribution.Descriptor{}, err + } + target, err := lbs.blobStore.readlink(ctx, blobLinkPath) + if err != nil { switch err := err.(type) { case driver.PathNotFoundError: - // do nothing, just move to the next linkPathFn + return distribution.Descriptor{}, distribution.ErrBlobUnknown default: return distribution.Descriptor{}, err } } - if !found { - return distribution.Descriptor{}, distribution.ErrBlobUnknown - } - if target != dgst { // Track when we are doing cross-digest domain lookups. ie, sha512 to sha256. dcontext.GetLogger(ctx).Warnf("looking up blob with canonical target: %v -> %v", dgst, target) @@ -416,37 +396,12 @@ func (lbs *linkedBlobStatter) Stat(ctx context.Context, dgst digest.Digest) (dis } 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.repository.Named().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.repository.Named().Name(), dgst) + blobLinkPath, err := lbs.linkPath(lbs.repository.Named().Name(), dgst) if err != nil { - return "", err + return err } - return lbs.blobStore.readlink(ctx, blobLinkPath) + return lbs.blobStore.driver.Delete(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 45939083..c49b6dbd 100644 --- a/registry/storage/registry.go +++ b/registry/storage/registry.go @@ -215,19 +215,12 @@ func (repo *repository) Tags(ctx context.Context) distribution.TagService { // 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, - } - manifestDirectoryPathSpec := manifestRevisionsPathSpec{name: repo.name.Name()} var statter distribution.BlobDescriptorService = &linkedBlobStatter{ - blobStore: repo.blobStore, - repository: repo, - linkPathFns: manifestLinkPathFns, + blobStore: repo.blobStore, + repository: repo, + linkPath: manifestRevisionLinkPath, } if repo.registry.blobDescriptorServiceFactory != nil { @@ -243,7 +236,7 @@ func (repo *repository) Manifests(ctx context.Context, options ...distribution.M // TODO(stevvooe): linkPath limits this blob store to only // manifests. This instance cannot be used for blob checks. - linkPathFns: manifestLinkPathFns, + linkPath: manifestRevisionLinkPath, linkDirectoryPathSpec: manifestDirectoryPathSpec, } @@ -306,9 +299,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, - linkPathFns: []linkPathFunc{blobLinkPath}, + blobStore: repo.blobStore, + repository: repo, + linkPath: blobLinkPath, } if repo.descriptorCache != nil { @@ -329,7 +322,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. - linkPathFns: []linkPathFunc{blobLinkPath}, + linkPath: blobLinkPath, linkDirectoryPathSpec: layersPathSpec{name: repo.name.Name()}, deleteEnabled: repo.registry.deleteEnabled, resumableDigestEnabled: repo.resumableDigestEnabled, diff --git a/registry/storage/tagstore.go b/registry/storage/tagstore.go index 9c16330b..1d1dd8bb 100644 --- a/registry/storage/tagstore.go +++ b/registry/storage/tagstore.go @@ -124,14 +124,13 @@ func (ts *tagStore) linkedBlobStore(ctx context.Context, tag string) *linkedBlob blobStore: ts.blobStore, repository: ts.repository, ctx: ctx, - linkPathFns: []linkPathFunc{func(name string, dgst digest.Digest) (string, error) { + linkPath: func(name string, dgst digest.Digest) (string, error) { return pathFor(manifestTagIndexEntryLinkPathSpec{ name: name, tag: tag, revision: dgst, }) - - }}, + }, } } @@ -185,13 +184,13 @@ func (ts *tagStore) ManifestDigests(ctx context.Context, tag string) ([]digest.D lbs := &linkedBlobStore{ blobStore: ts.blobStore, blobAccessController: &linkedBlobStatter{ - blobStore: ts.blobStore, - repository: ts.repository, - linkPathFns: []linkPathFunc{manifestRevisionLinkPath}, + blobStore: ts.blobStore, + repository: ts.repository, + linkPath: manifestRevisionLinkPath, }, - repository: ts.repository, - ctx: ctx, - linkPathFns: []linkPathFunc{tagLinkPath}, + repository: ts.repository, + ctx: ctx, + linkPath: tagLinkPath, linkDirectoryPathSpec: manifestTagIndexPathSpec{ name: ts.repository.Named().Name(), tag: tag,