Attempt to retrieve manifest from local repo for manifest requests to proxy

src/controller/proxy/controller.go

@@ -58,7 +58,7 @@ 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, remote RemoteInterface) (bool, *ManifestList, error)
+	UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, Manifests, 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 *proModels.Project, art lib.ArtifactInfo) (int64, io.ReadCloser, error)
@@ -142,18 +142,56 @@ func (c *controller) UseLocalBlob(ctx context.Context, art lib.ArtifactInfo) boo
 	return exist
 }
 
+// Manifests is an interface implemented by Manifest and ManifestList
+type Manifests interface {
+	Content() []byte
+	Digest() string
+	ContentType() string
+}
+
+// Manifest ...
+type Manifest struct {
+	content     []byte
+	digest      string
+	contentType string
+}
+
+func (m *Manifest) Content() []byte {
+	return m.content
+}
+
+func (m *Manifest) Digest() string {
+	return m.digest
+}
+
+func (m *Manifest) ContentType() string {
+	return m.contentType
+}
+
 // ManifestList ...
 type ManifestList struct {
-	Content     []byte
-	Digest      string
-	ContentType string
+	content     []byte
+	digest      string
+	contentType string
+}
+
+func (m *ManifestList) Content() []byte {
+	return m.content
+}
+
+func (m *ManifestList) Digest() string {
+	return m.digest
+}
+
+func (m *ManifestList) ContentType() string {
+	return m.contentType
 }
 
 // UseLocalManifest check if these manifest could be found in local registry,
 // the return error should be nil when it is not found in local and need to delegate to remote registry
 // the return error should be NotFoundError when it is not found in remote registry
 // the error will be captured by framework and return 404 to client
-func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, *ManifestList, error) {
+func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, Manifests, error) {
 	a, err := c.local.GetManifest(ctx, art)
 	if err != nil {
 		return false, nil, err
@@ -175,35 +213,64 @@ func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo,
 		return false, nil, errors.NotFoundError(fmt.Errorf("repo %v, tag %v not found", art.Repository, art.Tag))
 	}
 
+	// Use digest to retrieve manifest, digest is more stable than tag, because tag could be updated
+	// Pass digest to local repo
+	// Pass digest to the cache key
+	if len(art.Digest) == 0 {
+		art.Digest = string(desc.Digest)
+	}
+
+	// attempt to retrieve manifest from local repo
+	man, dig, err := c.local.PullManifest(art.Repository, art.Digest)
+	if err == nil {
+		log.Debugf("Got the manifest for repo: %v with digest: %v", art.Repository, dig)
+		mediaType, payload, err := man.Payload()
+		if err != nil {
+			log.Errorf("Failed to get payload for manifest with digest: %v, error: %v", dig, err)
+		}
+		return true, &Manifest{content: payload, digest: dig, contentType: mediaType}, nil
+	}
+	log.Errorf("Failed to get manifest from local repo, error: %v", err)
+
 	var content []byte
 	var contentType string
 	if c.cache == nil {
 		return a != nil && string(desc.Digest) == a.Digest, nil, nil // digest matches
 	}
-	// Pass digest to the cache key, digest is more stable than tag, because tag could be updated
-	if len(art.Digest) == 0 {
-		art.Digest = string(desc.Digest)
-	}
+
+	// attempt to retrieve manifest list from cache
 	err = c.cache.Fetch(ctx, manifestListKey(art.Repository, art), &content)
-	if err != nil {
-		if errors.Is(err, cache.ErrNotFound) {
-			log.Debugf("Digest is not found in manifest list cache, key=cache:%v", manifestListKey(art.Repository, art))
-		} else {
-			log.Errorf("Failed to get manifest list from cache, error: %v", err)
+	if err == nil {
+		// manifest list was found in cache
+		err = c.cache.Fetch(ctx, manifestListContentTypeKey(art.Repository, art), &contentType)
+		if err != nil {
+			log.Debugf("failed to get the manifest list content type, not use local. error:%v", err)
+			return false, nil, nil
 		}
-		return a != nil && string(desc.Digest) == a.Digest, nil, nil
+		log.Debugf("Get the manifest list with key=cache:%v", manifestListKey(art.Repository, art))
+		return true, &ManifestList{content, string(desc.Digest), contentType}, nil
 	}
-	err = c.cache.Fetch(ctx, manifestListContentTypeKey(art.Repository, art), &contentType)
-	if err != nil {
-		log.Debugf("failed to get the manifest list content type, not use local. error:%v", err)
-		return false, nil, nil
+	if errors.Is(err, cache.ErrNotFound) {
+		log.Debugf("Digest is not found in manifest list cache, key=cache:%v", manifestListKey(art.Repository, art))
+	} else {
+		log.Errorf("Failed to get manifest list from cache, error: %v", err)
 	}
-	log.Debugf("Get the manifest list with key=cache:%v", manifestListKey(art.Repository, art))
-	return true, &ManifestList{content, string(desc.Digest), contentType}, nil
+
+	// neither manifest was found in local repo nor manifest list was found in cache
+	return a != nil && string(desc.Digest) == a.Digest, nil, nil
+}
+
+func manifestKey(repo string, art lib.ArtifactInfo) string {
+	// actual redis key format is cache:manifest:<repo name>:<tag> or cache:manifest:<repo name>:sha256:xxxx
+	return "manifest:" + repo + ":" + getReference(art)
+}
+
+func manifestContentTypeKey(rep string, art lib.ArtifactInfo) string {
+	return manifestKey(rep, art) + ":contenttype"
 }
 
 func manifestListKey(repo string, art lib.ArtifactInfo) string {
-	// actual redis key format is cache:manifestlist:<repo name>:<tag> or cache:manifestlist:<repo name>:sha256:xxxx
+	// actual redis key format is cache:manifest:<repo name>:<tag> or cache:manifest:<repo name>:sha256:xxxx
 	return "manifestlist:" + repo + ":" + getReference(art)
 }
 

src/controller/proxy/controller_test.go

@@ -16,6 +16,7 @@ package proxy
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"testing"
 
@@ -31,6 +32,8 @@ import (
 	"github.com/goharbor/harbor/src/lib/errors"
 	proModels "github.com/goharbor/harbor/src/pkg/project/models"
 	testproxy "github.com/goharbor/harbor/src/testing/controller/proxy"
+	"github.com/goharbor/harbor/src/testing/lib/cache"
+	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 type localInterfaceMock struct {
@@ -64,6 +67,17 @@ func (l *localInterfaceMock) PushBlob(localRepo string, desc distribution.Descri
 	panic("implement me")
 }
 
+func (l *localInterfaceMock) PullManifest(repo string, ref string) (distribution.Manifest, string, error) {
+	args := l.Called(repo, ref)
+
+	var d distribution.Manifest
+	if arg := args.Get(0); arg != nil {
+		d = arg.(distribution.Manifest)
+	}
+
+	return d, args.String(1), args.Error(2)
+}
+
 func (l *localInterfaceMock) PushManifest(repo string, tag string, manifest distribution.Manifest) error {
 	args := l.Called(repo, tag, manifest)
 	return args.Error(0)
@@ -84,7 +98,7 @@ type proxyControllerTestSuite struct {
 	suite.Suite
 	local  *localInterfaceMock
 	remote *testproxy.RemoteInterface
-	ctr    Controller
+	ctr    *controller
 	proj   *proModels.Project
 }
 
@@ -114,12 +128,15 @@ func (p *proxyControllerTestSuite) TestUseLocalManifest_False() {
 	ctx := context.Background()
 	dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
 	desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
-	art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
+	repo := "library/hello-world"
+	art := lib.ArtifactInfo{Repository: repo, Digest: dig}
 	p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, desc, nil)
 	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(nil, nil)
+	p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(nil, "", fmt.Errorf("could not pull manifest"))
 	result, _, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
 	p.Assert().Nil(err)
 	p.Assert().False(result)
+	p.local.AssertExpectations(p.T())
 }
 
 func (p *proxyControllerTestSuite) TestUseLocalManifest_429() {
@@ -157,6 +174,93 @@ func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_False() {
 	p.Assert().False(result)
 }
 
+func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_LocalRepoTrueManifest() {
+	manifest := `{ 
+		"schemaVersion": 2,
+		"mediaType": "application/vnd.oci.image.manifest.v1+json",
+		"config": {
+			 "mediaType": "application/vnd.example.config.v1+json",
+			 "digest": "sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
+			 "size": 123
+		},
+		"layers": [
+			 {
+					"mediaType": "application/vnd.example.data.v1.tar+gzip",
+					"digest": "sha256:e258d248fda94c63753607f7c4494ee0fcbe92f1a76bfdac795c9d84101eb317",
+					"size": 1234
+			 }
+		],
+		"annotations": {
+			 "com.example.key1": "value1"
+		}
+	}`
+	man, desc, err := distribution.UnmarshalManifest(v1.MediaTypeImageManifest, []byte(manifest))
+	p.Require().NoError(err)
+	mediaType, payload, err := man.Payload()
+	p.Require().NoError(err)
+
+	ctx := context.Background()
+	repo := "library/hello-world"
+	art := lib.ArtifactInfo{Repository: repo, Tag: "latest"}
+	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
+	p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, &desc, nil)
+	p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(man, string(desc.Digest), nil)
+
+	result, manifests, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
+
+	p.Assert().NoError(err)
+	p.Assert().True(result)
+	p.Assert().NotNil(manifests)
+	p.Assert().Equal(mediaType, manifests.ContentType())
+	p.Assert().Equal(string(desc.Digest), manifests.Digest())
+	p.Assert().Equal(payload, manifests.Content())
+
+	p.local.AssertExpectations(p.T())
+}
+
+func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_CacheTrueManifestList() {
+	c := cache.NewCache(p.T())
+	p.ctr.cache = c
+
+	ctx := context.Background()
+	repo := "library/hello-world"
+	art := lib.ArtifactInfo{Repository: repo, Tag: "latest"}
+	dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
+	desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
+	content := "some content"
+	contentType := "some content type"
+	p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
+	p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, desc, nil)
+	p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(nil, "", fmt.Errorf("could not pull manifest"))
+	artInfoWithDigest := art
+	artInfoWithDigest.Digest = dig
+	c.On("Fetch", mock.Anything, manifestListKey(art.Repository, artInfoWithDigest), mock.Anything).
+		Times(1).
+		Run(func(args mock.Arguments) {
+			ct := args.Get(2).(*[]byte)
+			*ct = []byte(content)
+		}).
+		Return(nil)
+	c.On("Fetch", mock.Anything, manifestListContentTypeKey(art.Repository, artInfoWithDigest), mock.Anything).
+		Times(1).
+		Run(func(args mock.Arguments) {
+			ct := args.Get(2).(*string)
+			*ct = contentType
+		}).
+		Return(nil)
+
+	result, manifests, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
+
+	p.Assert().NoError(err)
+	p.Assert().True(result)
+	p.Assert().NotNil(manifests)
+	p.Assert().Equal(contentType, manifests.ContentType())
+	p.Assert().Equal(string(desc.Digest), manifests.Digest())
+	p.Assert().Equal([]byte(content), manifests.Content())
+
+	p.local.AssertExpectations(p.T())
+}
+
 func (p *proxyControllerTestSuite) TestUseLocalBlob_True() {
 	ctx := context.Background()
 	dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"

src/controller/proxy/local.go

@@ -43,6 +43,8 @@ type localInterface interface {
 	GetManifest(ctx context.Context, art lib.ArtifactInfo) (*artifact.Artifact, error)
 	// PushBlob push blob to local repo
 	PushBlob(localRepo string, desc distribution.Descriptor, bReader io.ReadCloser) error
+	// PullManifest pulls manifest from local repo, ref can be digest or tag
+	PullManifest(repo string, ref string) (distribution.Manifest, string, error)
 	// PushManifest push manifest to local repo, ref can be digest or tag
 	PushManifest(repo string, ref string, manifest distribution.Manifest) error
 	// CheckDependencies check if the manifest's dependency is ready
@@ -109,6 +111,10 @@ func (l *localHelper) PushBlob(localRepo string, desc distribution.Descriptor, b
 	return err
 }
 
+func (l *localHelper) PullManifest(repo string, ref string) (distribution.Manifest, string, error) {
+	return l.registry.PullManifest(repo, ref)
+}
+
 func (l *localHelper) PushManifest(repo string, ref string, manifest distribution.Manifest) error {
 	// Make sure there is only one go routing to push current artName to local repo
 	artName := repo + ":" + ref

src/server/middleware/repoproxy/proxy.go

@@ -185,12 +185,12 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
 	}
 	if useLocal {
 		if man != nil {
-			w.Header().Set(contentLength, fmt.Sprintf("%v", len(man.Content)))
-			w.Header().Set(contentType, man.ContentType)
-			w.Header().Set(dockerContentDigest, man.Digest)
-			w.Header().Set(etag, man.Digest)
+			w.Header().Set(contentLength, fmt.Sprintf("%v", len(man.Content())))
+			w.Header().Set(contentType, man.ContentType())
+			w.Header().Set(dockerContentDigest, man.Digest())
+			w.Header().Set(etag, man.Digest())
 			if r.Method == http.MethodGet {
-				_, err = w.Write(man.Content)
+				_, err = w.Write(man.Content())
 				if err != nil {
 					return err
 				}