Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add manifestcache push for tag and digest to local repository #21141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add manifestcache push for tag and digest to local repository #21141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/controller/proxy/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ func (l *localInterfaceMock) PushManifestList(ctx context.Context, repo string,
}

func (l *localInterfaceMock) CheckDependencies(ctx context.Context, repo string, man distribution.Manifest) []distribution.Descriptor {
panic("implement me")
args := l.Called(ctx, repo, man)
return args.Get(0).([]distribution.Descriptor)
}

func (l *localInterfaceMock) DeleteManifest(repo, ref string) {
Expand Down
29 changes: 25 additions & 4 deletions src/controller/proxy/manifestcache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package proxy

import (
"context"
"errors"
"fmt"
"strings"
"time"
Expand All @@ -28,7 +29,7 @@ import (

"github.com/goharbor/harbor/src/lib"
libCache "github.com/goharbor/harbor/src/lib/cache"
"github.com/goharbor/harbor/src/lib/errors"
libErrors "github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/lib/log"
)

Expand Down Expand Up @@ -142,7 +143,7 @@ func (m *ManifestListCache) push(ctx context.Context, repo, reference string, ma
}
}
if len(newMan.References()) == 0 {
return errors.New("manifest list doesn't contain any pushed manifest")
return libErrors.New("manifest list doesn't contain any pushed manifest")
}
_, pl, err := newMan.Payload()
if err != nil {
Expand Down Expand Up @@ -198,10 +199,30 @@ func (m *ManifestCache) CacheContent(ctx context.Context, remoteRepo string, man
}
}
}
err := m.local.PushManifest(art.Repository, getReference(art), man)

err := m.push(art, man)
if err != nil {
log.Errorf("failed to push manifest, tag: %v, error %v", art.Tag, err)
log.Errorf("error occured on manifest push to local: %v", err)
}
}

func (m *ManifestCache) push(art lib.ArtifactInfo, man distribution.Manifest) error {
Copy link
Contributor

@stonezdj stonezdj Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have replace the m.local.PushManifest(art.Repository, getReference(art), man) with the m.push(art, man),
the intention of this method is push the manifest to the proxy cache project if there is any tag exist in the art, current the art is parsing from the pull request, for a normal image pull request, it is pulled either by digest or by tag. it can't be both.
The previous getReference(art) will return tag if it pull by tag, and return digest if it is pull by digest.
Current code change has no difference with the previous implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have replace the m.local.PushManifest(art.Repository, getReference(art), man) with the m.push(art, man)
Yes my intention is to push the manifest to the local proxy cache project with any reference known (tag, digest or both).

for a normal image pull request, it is pulled either by digest or by tag. it can't be both.
I agree the GET manifest request either uses a tag or digest as reference, but during the storage of the proxied manifest the digest might be looked up in the remote repository and the manifest is then only stored under the digest in the local proxy cache project.

The previous getReference(art) will return tag if it pull by tag, and return digest if it is pull by digest.
I think there are cases where art will contain both a digest and tag here. In this case the getReference(art) will favor digest over tag, ultimately leading to the manifest being stored in the local proxy cache project only under the digest.

This happens during the proxy controller's ProxyManifest call.

For a manifest pull referencing a tag the digest will be looked up in the remote repository.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L224

Then the digest will be added to the copied artInfo which should until then contain the tag parsed from the the original pull by tag.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L249

This artInfo containing both tag and digest will then be used in the original call to m.local.PushManifest(art.Repository, getReference(art), man), where getReference(art) leads to the digest being favored over the tag.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L251C1-L251C69
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L318
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/manifestcache.go#L201
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L327-L330

My proposal is to push the manifest with both references (digest and tag) if both are known. This should lead to subsequent manifest pulls referencing the tag being served from the local proxy cache project if the digest has not changed to the remote manifest digest for said tag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @stonezdj,

are there any concerns left, or could I provide anything else?

errs := []error{}
if len(art.Digest) > 0 {
err := m.local.PushManifest(art.Repository, art.Digest, man)
if err != nil {
log.Errorf("failed to push manifest referencing digest, tag: %v, digest: %v, error %v", art.Tag, art.Digest, err)
errs = append(errs, err)
}
}
if len(art.Tag) > 0 {
err := m.local.PushManifest(art.Repository, art.Tag, man)
if err != nil {
log.Errorf("failed to push manifest referencing tag, tag: %v, digest: %v, error %v", art.Tag, art.Digest, err)
errs = append(errs, err)
}
}
return errors.Join(errs...)
}

func (m *ManifestCache) putBlobToLocal(remoteRepo string, localRepo string, desc distribution.Descriptor, r RemoteInterface) error {
Expand Down
109 changes: 104 additions & 5 deletions src/controller/proxy/manifestcache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,41 @@ import (
"github.com/goharbor/harbor/src/controller/artifact"
"github.com/goharbor/harbor/src/lib"
"github.com/goharbor/harbor/src/testing/mock"

v1 "github.com/opencontainers/image-spec/specs-go/v1"
)

const ociManifest = `{
"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"
}
}`

type CacheTestSuite struct {
suite.Suite
mHandler *ManifestListCache
local localInterfaceMock
mCache *ManifestCache
mListCache *ManifestListCache
local localInterfaceMock
}

func (suite *CacheTestSuite) SetupSuite() {
suite.local = localInterfaceMock{}
suite.mHandler = &ManifestListCache{local: &suite.local}
suite.mListCache = &ManifestListCache{local: &suite.local}
suite.mCache = &ManifestCache{local: &suite.local}
}

func (suite *CacheTestSuite) TearDownSuite() {
Expand Down Expand Up @@ -89,7 +113,7 @@ func (suite *CacheTestSuite) TestUpdateManifestList() {
suite.local.On("GetManifest", ctx, artInfo1).Return(ar, nil)
suite.local.On("GetManifest", ctx, mock.Anything).Return(nil, nil)

newMan, err := suite.mHandler.updateManifestList(ctx, "library/hello-world", manList)
newMan, err := suite.mListCache.updateManifestList(ctx, "library/hello-world", manList)
suite.Require().Nil(err)
suite.Assert().Equal(len(newMan.References()), 1)
}
Expand Down Expand Up @@ -147,10 +171,85 @@ func (suite *CacheTestSuite) TestPushManifestList() {
suite.local.On("PushManifest", repo, originDigest, mock.Anything).Return(fmt.Errorf("wrong digest"))
suite.local.On("PushManifest", repo, mock.Anything, mock.Anything).Return(nil)

err = suite.mHandler.push(ctx, "library/hello-world", string(originDigest), manList)
err = suite.mListCache.push(ctx, "library/hello-world", string(originDigest), manList)
suite.Require().Nil(err)
}

func (suite *CacheTestSuite) TestManifestCache_CacheContent() {
defer suite.local.AssertExpectations(suite.T())

manifest := ociManifest
man, desc, err := distribution.UnmarshalManifest(v1.MediaTypeImageManifest, []byte(manifest))
suite.Require().NoError(err)

ctx := context.Background()
repo := "library/hello-world"

artInfo := lib.ArtifactInfo{
Repository: repo,
Digest: string(desc.Digest),
Tag: "latest",
}

suite.local.On("CheckDependencies", ctx, artInfo.Repository, man).Once().Return([]distribution.Descriptor{})
suite.local.On("PushManifest", artInfo.Repository, artInfo.Digest, man).Once().Return(nil)
suite.local.On("PushManifest", artInfo.Repository, artInfo.Tag, man).Once().Return(nil)

suite.mCache.CacheContent(ctx, repo, man, artInfo, nil, "")
}

func (suite *CacheTestSuite) TestManifestCache_push_succeeds() {
defer suite.local.AssertExpectations(suite.T())

manifest := ociManifest
man, desc, err := distribution.UnmarshalManifest(v1.MediaTypeImageManifest, []byte(manifest))
suite.Require().NoError(err)

repo := "library/hello-world"

artInfo := lib.ArtifactInfo{
Repository: repo,
Digest: string(desc.Digest),
Tag: "latest",
}

suite.local.On("PushManifest", artInfo.Repository, artInfo.Digest, man).Once().Return(nil)
suite.local.On("PushManifest", artInfo.Repository, artInfo.Tag, man).Once().Return(nil)

err = suite.mCache.push(artInfo, man)
suite.Assert().NoError(err)
}

func (suite *CacheTestSuite) TestManifestCache_push_fails() {
defer suite.local.AssertExpectations(suite.T())

manifest := ociManifest
man, desc, err := distribution.UnmarshalManifest(v1.MediaTypeImageManifest, []byte(manifest))
suite.Require().NoError(err)

repo := "library/hello-world"

artInfo := lib.ArtifactInfo{
Repository: repo,
Digest: string(desc.Digest),
Tag: "latest",
}

digestErr := fmt.Errorf("error during manifest push referencing digest")
tagErr := fmt.Errorf("error during manifest push referencing tag")
suite.local.On("PushManifest", artInfo.Repository, artInfo.Digest, man).Once().Return(digestErr)
suite.local.On("PushManifest", artInfo.Repository, artInfo.Tag, man).Once().Return(tagErr)

err = suite.mCache.push(artInfo, man)
suite.Assert().Error(err)
wrappedErr, isWrappedErr := err.(interface{ Unwrap() []error })
suite.Assert().True(isWrappedErr)
errs := wrappedErr.Unwrap()
suite.Assert().Len(errs, 2)
suite.Assert().Contains(errs, digestErr)
suite.Assert().Contains(errs, tagErr)
}

func TestCacheTestSuite(t *testing.T) {
suite.Run(t, &CacheTestSuite{})
}