Change the condition of LocalManifest

Compare the local digest and the remote digest when pull by tag
Use HEAD request (ManifestExist) instead of GET request (GetManifest) to avoid been throttled
For manifest list, it can avoid GET request because cached manifest list maybe different with the original manifest list
Make RemoteInterface public
Fixes #13112

Signed-off-by: stonezdj <stonezdj@gmail.com>

src/controller/proxy/controller.go

@@ -16,6 +16,7 @@ package proxy
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"strings"
 	"sync"
@@ -53,13 +54,13 @@ type Controller interface {
 	// UseLocalBlob check if the blob should use local copy
 	UseLocalBlob(ctx context.Context, art lib.ArtifactInfo) bool
 	// UseLocalManifest check manifest should use local copy
-	UseLocalManifest(ctx context.Context, art lib.ArtifactInfo) (bool, error)
+	UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, error)
 	// ProxyBlob proxy the blob request to the remote server, p is the proxy project
 	// art is the ArtifactInfo which includes the digest of the blob
 	ProxyBlob(ctx context.Context, p *models.Project, art lib.ArtifactInfo) (int64, io.ReadCloser, error)
 	// ProxyManifest proxy the manifest request to the remote server, p is the proxy project,
 	// art is the ArtifactInfo which includes the tag or digest of the manifest
-	ProxyManifest(ctx context.Context, p *models.Project, art lib.ArtifactInfo) (distribution.Manifest, error)
+	ProxyManifest(ctx context.Context, p *models.Project, art lib.ArtifactInfo, remote RemoteInterface) (distribution.Manifest, error)
 }
 type controller struct {
 	blobCtl     blob.Controller
@@ -93,22 +94,37 @@ func (c *controller) UseLocalBlob(ctx context.Context, art lib.ArtifactInfo) boo
 	return exist
 }
 
-func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo) (bool, error) {
+func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, error) {
-	if len(art.Digest) == 0 {
+	a, err := c.local.GetManifest(ctx, art)
+	if err != nil {
+		return false, err
+	}
+	if a == nil {
 		return false, nil
 	}
-	a, err := c.local.GetManifest(ctx, art)
+	// Pull by digest
-	return a != nil, err
+	if len(art.Digest) > 0 {
+		return true, nil
+	}
+	// Pull by tag
+	remoteRepo := getRemoteRepo(art)
+	exist, dig, err := remote.ManifestExist(remoteRepo, art.Tag) // HEAD
+	if err != nil {
+		return false, err
+	}
+	if !exist {
+		go func() {
+			c.local.DeleteManifest(remoteRepo, art.Tag)
+		}()
+		return false, errors.NotFoundError(fmt.Errorf("repo %v, tag %v not found", art.Repository, art.Tag))
+	}
+	return dig == a.Digest, nil // digest matches
 }
 
-func (c *controller) ProxyManifest(ctx context.Context, p *models.Project, art lib.ArtifactInfo) (distribution.Manifest, error) {
+func (c *controller) ProxyManifest(ctx context.Context, p *models.Project, art lib.ArtifactInfo, remote RemoteInterface) (distribution.Manifest, error) {
 	var man distribution.Manifest
 	remoteRepo := getRemoteRepo(art)
 	ref := getReference(art)
-	remote, err := newRemoteHelper(p.RegistryID)
-	if err != nil {
-		return man, err
-	}
 	man, dig, err := remote.Manifest(remoteRepo, ref)
 	if err != nil {
 		if errors.IsNotFoundErr(err) {
@@ -153,7 +169,7 @@ func (c *controller) ProxyManifest(ctx context.Context, p *models.Project, art l
 func (c *controller) ProxyBlob(ctx context.Context, p *models.Project, art lib.ArtifactInfo) (int64, io.ReadCloser, error) {
 	remoteRepo := getRemoteRepo(art)
 	log.Debugf("The blob doesn't exist, proxy the request to the target server, url:%v", remoteRepo)
-	rHelper, err := newRemoteHelper(p.RegistryID)
+	rHelper, err := NewRemoteHelper(p.RegistryID)
 	if err != nil {
 		return 0, nil, err
 	}
@@ -173,7 +189,7 @@ func (c *controller) ProxyBlob(ctx context.Context, p *models.Project, art lib.A
 	return size, bReader, nil
 }
 
-func (c *controller) putBlobToLocal(remoteRepo string, localRepo string, desc distribution.Descriptor, r remoteInterface) error {
+func (c *controller) putBlobToLocal(remoteRepo string, localRepo string, desc distribution.Descriptor, r RemoteInterface) error {
 	log.Debugf("Put blob to local registry!, sourceRepo:%v, localRepo:%v, digest: %v", remoteRepo, localRepo, desc.Digest)
 	_, bReader, err := r.BlobReader(remoteRepo, string(desc.Digest))
 	if err != nil {
@@ -185,7 +201,7 @@ func (c *controller) putBlobToLocal(remoteRepo string, localRepo string, desc di
 	return err
 }
 
-func (c *controller) waitAndPushManifest(ctx context.Context, remoteRepo string, man distribution.Manifest, art lib.ArtifactInfo, contType string, r remoteInterface) {
+func (c *controller) waitAndPushManifest(ctx context.Context, remoteRepo string, man distribution.Manifest, art lib.ArtifactInfo, contType string, r RemoteInterface) {
 	if contType == manifestlist.MediaTypeManifestList || contType == v1.MediaTypeImageIndex {
 		err := c.local.PushManifestList(ctx, art.Repository, getReference(art), man)
 		if err != nil {

src/controller/proxy/controller_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/goharbor/harbor/src/controller/artifact"
 	"github.com/goharbor/harbor/src/controller/blob"
 	"github.com/goharbor/harbor/src/lib"
+	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
 	"io"
@@ -88,7 +89,6 @@ func (l *localInterfaceMock) CheckDependencies(ctx context.Context, repo string,
 }
 
 func (l *localInterfaceMock) DeleteManifest(repo, ref string) {
-	panic("implement me")
 }
 
 type proxyControllerTestSuite struct {
@@ -116,7 +116,7 @@ func (p *proxyControllerTestSuite) TestUseLocalManifest_True() {
 	art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
 	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
 
-	result, err := p.ctr.UseLocalManifest(ctx, art)
+	result, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
 	p.Assert().Nil(err)
 	p.Assert().True(result)
 }
@@ -126,7 +126,7 @@ func (p *proxyControllerTestSuite) TestUseLocalManifest_False() {
 	dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
 	art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
 	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(nil, nil)
-	result, err := p.ctr.UseLocalManifest(ctx, art)
+	result, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
 	p.Assert().Nil(err)
 	p.Assert().False(result)
 }
@@ -135,8 +135,9 @@ func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_False() {
 	ctx := context.Background()
 	art := lib.ArtifactInfo{Repository: "library/hello-world", Tag: "latest"}
 	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
-	result, err := p.ctr.UseLocalManifest(ctx, art)
+	p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(false, "", nil)
-	p.Assert().Nil(err)
+	result, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
+	p.Assert().True(errors.IsNotFoundErr(err))
 	p.Assert().False(result)
 }
 

src/controller/proxy/remote.go

@@ -23,12 +23,14 @@ import (
 	"io"
 )
 
-// remoteInterface defines operations related to remote repository under proxy
+// RemoteInterface defines operations related to remote repository under proxy
-type remoteInterface interface {
+type RemoteInterface interface {
 	// BlobReader create a reader for remote blob
 	BlobReader(repo, dig string) (int64, io.ReadCloser, error)
 	// Manifest get manifest by reference
 	Manifest(repo string, ref string) (distribution.Manifest, string, error)
+	// ManifestExist checks manifest exist, if exist, return digest
+	ManifestExist(repo string, ref string) (bool, string, error)
 }
 
 // remoteHelper defines operations related to remote repository under proxy
@@ -38,8 +40,8 @@ type remoteHelper struct {
 	registryMgr registry.Manager
 }
 
-// newRemoteHelper create a remoteHelper interface
+// NewRemoteHelper create a remote interface
-func newRemoteHelper(regID int64) (*remoteHelper, error) {
+func NewRemoteHelper(regID int64) (RemoteInterface, error) {
 	r := &remoteHelper{
 		regID:       regID,
 		registryMgr: registry.NewDefaultManager()}
@@ -83,3 +85,7 @@ func (r *remoteHelper) BlobReader(repo, dig string) (int64, io.ReadCloser, error
 func (r *remoteHelper) Manifest(repo string, ref string) (distribution.Manifest, string, error) {
 	return r.registry.PullManifest(repo, ref)
 }
+
+func (r *remoteHelper) ManifestExist(repo string, ref string) (bool, string, error) {
+	return r.registry.ManifestExist(repo, ref)
+}

src/server/middleware/repoproxy/proxy.go

@@ -102,7 +102,11 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
 		next.ServeHTTP(w, r)
 		return nil
 	}
-	useLocal, err := proxyCtl.UseLocalManifest(ctx, art)
+	remote, err := proxy.NewRemoteHelper(p.RegistryID)
+	if err != nil {
+		return err
+	}
+	useLocal, err := proxyCtl.UseLocalManifest(ctx, art, remote)
 	if err != nil {
 		return err
 	}
@@ -111,7 +115,7 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
 		return nil
 	}
 	log.Debugf("the tag is %v, digest is %v", art.Tag, art.Digest)
-	err = proxyManifest(ctx, w, r, next, proxyCtl, p, art)
+	err = proxyManifest(ctx, w, proxyCtl, p, art, remote)
 	if err != nil {
 		if errors.IsNotFoundErr(err) {
 			return err
@@ -122,8 +126,8 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
 	return nil
 }
 
-func proxyManifest(ctx context.Context, w http.ResponseWriter, r *http.Request, next http.Handler, ctl proxy.Controller, p *models.Project, art lib.ArtifactInfo) error {
+func proxyManifest(ctx context.Context, w http.ResponseWriter, ctl proxy.Controller, p *models.Project, art lib.ArtifactInfo, remote proxy.RemoteInterface) error {
-	man, err := ctl.ProxyManifest(ctx, p, art)
+	man, err := ctl.ProxyManifest(ctx, p, art, remote)
 	if err != nil {
 		return err
 	}