Merge pull request #13162 from stonezdj/200921_rating_limit

Change the condition of LocalManifest

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) {
@@ -154,7 +170,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
 	}
@@ -174,7 +190,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 {
@@ -186,7 +202,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
 	}