From 0a326e67d4d89b3625d6f5312a5f00753892fdb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Raphael=20Z=C3=B6llner?= Date: Thu, 31 Oct 2024 16:42:41 +0100 Subject: [PATCH 1/3] Attempt to retrieve manifest from local repo for manifest requests to proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Raphael Zöllner --- src/controller/proxy/controller.go | 111 ++++++++++++++++++----- src/controller/proxy/controller_test.go | 108 +++++++++++++++++++++- src/controller/proxy/local.go | 6 ++ src/server/middleware/repoproxy/proxy.go | 10 +- 4 files changed, 206 insertions(+), 29 deletions(-) diff --git a/src/controller/proxy/controller.go b/src/controller/proxy/controller.go index 7138b6f0b0e..06ac1b1ce79 100644 --- a/src/controller/proxy/controller.go +++ b/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:: or cache:manifest::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:: or cache:manifestlist::sha256:xxxx + // actual redis key format is cache:manifest:: or cache:manifest::sha256:xxxx return "manifestlist:" + repo + ":" + getReference(art) } diff --git a/src/controller/proxy/controller_test.go b/src/controller/proxy/controller_test.go index a25fe959a57..0e819b9179d 100644 --- a/src/controller/proxy/controller_test.go +++ b/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" diff --git a/src/controller/proxy/local.go b/src/controller/proxy/local.go index 67f6d5cbdc1..921e0bec193 100644 --- a/src/controller/proxy/local.go +++ b/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 diff --git a/src/server/middleware/repoproxy/proxy.go b/src/server/middleware/repoproxy/proxy.go index 641638766e7..baf7342be62 100644 --- a/src/server/middleware/repoproxy/proxy.go +++ b/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 } From d4399cc1fe45d127c1263b83c957e2319ed7b2d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Raphael=20Z=C3=B6llner?= Date: Tue, 5 Nov 2024 09:29:09 +0100 Subject: [PATCH 2/3] Revert comment for manifestListKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Raphael Zöllner --- src/controller/proxy/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/proxy/controller.go b/src/controller/proxy/controller.go index 06ac1b1ce79..cae6b941ac9 100644 --- a/src/controller/proxy/controller.go +++ b/src/controller/proxy/controller.go @@ -270,7 +270,7 @@ func manifestContentTypeKey(rep string, art lib.ArtifactInfo) string { } func manifestListKey(repo string, art lib.ArtifactInfo) string { - // actual redis key format is cache:manifest:: or cache:manifest::sha256:xxxx + // actual redis key format is cache:manifestlist:: or cache:manifestlist::sha256:xxxx return "manifestlist:" + repo + ":" + getReference(art) } From dc4b936eafc68a934054000961eb9bc9d5bfbae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Raphael=20Z=C3=B6llner?= Date: Tue, 5 Nov 2024 10:40:52 +0100 Subject: [PATCH 3/3] Revert adding unused manifestKey manifestContentTypeKey functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Raphael Zöllner --- src/controller/proxy/controller.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/controller/proxy/controller.go b/src/controller/proxy/controller.go index cae6b941ac9..ee6919b39eb 100644 --- a/src/controller/proxy/controller.go +++ b/src/controller/proxy/controller.go @@ -260,15 +260,6 @@ func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, 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:: or cache:manifest::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:: or cache:manifestlist::sha256:xxxx return "manifestlist:" + repo + ":" + getReference(art)