From ee4b16ccdbb1f848b5a320e4fa8d74eaa51cce7a Mon Sep 17 00:00:00 2001 From: stonezdj Date: Fri, 25 Sep 2020 14:23:13 +0800 Subject: [PATCH] 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 --- src/controller/proxy/controller.go | 44 ++++++++++++++++-------- src/controller/proxy/controller_test.go | 11 +++--- src/controller/proxy/remote.go | 14 +++++--- src/server/middleware/repoproxy/proxy.go | 12 ++++--- 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/src/controller/proxy/controller.go b/src/controller/proxy/controller.go index 7545bd7ab..28c7da424 100644 --- a/src/controller/proxy/controller.go +++ b/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) { - if len(art.Digest) == 0 { +func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, error) { + 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) - return a != nil, err + // Pull by digest + 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 { diff --git a/src/controller/proxy/controller_test.go b/src/controller/proxy/controller_test.go index 77ce351c2..92c98142c 100644 --- a/src/controller/proxy/controller_test.go +++ b/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.Assert().Nil(err) + p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(false, "", nil) + result, err := p.ctr.UseLocalManifest(ctx, art, p.remote) + p.Assert().True(errors.IsNotFoundErr(err)) p.Assert().False(result) } diff --git a/src/controller/proxy/remote.go b/src/controller/proxy/remote.go index db3a8d273..94197a8c3 100644 --- a/src/controller/proxy/remote.go +++ b/src/controller/proxy/remote.go @@ -23,12 +23,14 @@ import ( "io" ) -// remoteInterface defines operations related to remote repository under proxy -type remoteInterface interface { +// RemoteInterface defines operations related to remote repository under proxy +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 -func newRemoteHelper(regID int64) (*remoteHelper, error) { +// NewRemoteHelper create a remote interface +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) +} diff --git a/src/server/middleware/repoproxy/proxy.go b/src/server/middleware/repoproxy/proxy.go index dc7266d7d..1877e12d1 100644 --- a/src/server/middleware/repoproxy/proxy.go +++ b/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 { - man, err := ctl.ProxyManifest(ctx, p, art) +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, remote) if err != nil { return err }