Skip to content

Commit 0a326e6

Browse files
raphaelzoellnerraphaelzoellner
committed
Attempt to retrieve manifest from local repo for manifest requests to proxy
Signed-off-by: Raphael Zöllner <[email protected]>
1 parent 62d1d93 commit 0a326e6

File tree

4 files changed

+206
-29
lines changed

4 files changed

+206
-29
lines changed

‎src/controller/proxy/controller.go‎

Lines changed: 89 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ type Controller interface {
5858
// UseLocalBlob check if the blob should use local copy
5959
UseLocalBlob(ctx context.Context, art lib.ArtifactInfo) bool
6060
// UseLocalManifest check manifest should use local copy
61-
UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, *ManifestList, error)
61+
UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, Manifests, error)
6262
// ProxyBlob proxy the blob request to the remote server, p is the proxy project
6363
// art is the ArtifactInfo which includes the digest of the blob
6464
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
142142
return exist
143143
}
144144

145+
// Manifests is an interface implemented by Manifest and ManifestList
146+
type Manifests interface {
147+
Content() []byte
148+
Digest() string
149+
ContentType() string
150+
}
151+
152+
// Manifest ...
153+
type Manifest struct {
154+
content []byte
155+
digest string
156+
contentType string
157+
}
158+
159+
func (m *Manifest) Content() []byte {
160+
return m.content
161+
}
162+
163+
func (m *Manifest) Digest() string {
164+
return m.digest
165+
}
166+
167+
func (m *Manifest) ContentType() string {
168+
return m.contentType
169+
}
170+
145171
// ManifestList ...
146172
type ManifestList struct {
147-
Content []byte
148-
Digest string
149-
ContentType string
173+
content []byte
174+
digest string
175+
contentType string
176+
}
177+
178+
func (m *ManifestList) Content() []byte {
179+
return m.content
180+
}
181+
182+
func (m *ManifestList) Digest() string {
183+
return m.digest
184+
}
185+
186+
func (m *ManifestList) ContentType() string {
187+
return m.contentType
150188
}
151189

152190
// UseLocalManifest check if these manifest could be found in local registry,
153191
// the return error should be nil when it is not found in local and need to delegate to remote registry
154192
// the return error should be NotFoundError when it is not found in remote registry
155193
// the error will be captured by framework and return 404 to client
156-
func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, *ManifestList, error) {
194+
func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo, remote RemoteInterface) (bool, Manifests, error) {
157195
a, err := c.local.GetManifest(ctx, art)
158196
if err != nil {
159197
return false, nil, err
@@ -175,35 +213,64 @@ func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo,
175213
return false, nil, errors.NotFoundError(fmt.Errorf("repo %v, tag %v not found", art.Repository, art.Tag))
176214
}
177215

216+
// Use digest to retrieve manifest, digest is more stable than tag, because tag could be updated
217+
// Pass digest to local repo
218+
// Pass digest to the cache key
219+
if len(art.Digest) == 0 {
220+
art.Digest = string(desc.Digest)
221+
}
222+
223+
// attempt to retrieve manifest from local repo
224+
man, dig, err := c.local.PullManifest(art.Repository, art.Digest)
225+
if err == nil {
226+
log.Debugf("Got the manifest for repo: %v with digest: %v", art.Repository, dig)
227+
mediaType, payload, err := man.Payload()
228+
if err != nil {
229+
log.Errorf("Failed to get payload for manifest with digest: %v, error: %v", dig, err)
230+
}
231+
return true, &Manifest{content: payload, digest: dig, contentType: mediaType}, nil
232+
}
233+
log.Errorf("Failed to get manifest from local repo, error: %v", err)
234+
178235
var content []byte
179236
var contentType string
180237
if c.cache == nil {
181238
return a != nil && string(desc.Digest) == a.Digest, nil, nil // digest matches
182239
}
183-
// Pass digest to the cache key, digest is more stable than tag, because tag could be updated
184-
if len(art.Digest) == 0 {
185-
art.Digest = string(desc.Digest)
186-
}
240+
241+
// attempt to retrieve manifest list from cache
187242
err = c.cache.Fetch(ctx, manifestListKey(art.Repository, art), &content)
188-
if err != nil {
189-
if errors.Is(err, cache.ErrNotFound) {
190-
log.Debugf("Digest is not found in manifest list cache, key=cache:%v", manifestListKey(art.Repository, art))
191-
} else {
192-
log.Errorf("Failed to get manifest list from cache, error: %v", err)
243+
if err == nil {
244+
// manifest list was found in cache
245+
err = c.cache.Fetch(ctx, manifestListContentTypeKey(art.Repository, art), &contentType)
246+
if err != nil {
247+
log.Debugf("failed to get the manifest list content type, not use local. error:%v", err)
248+
return false, nil, nil
193249
}
194-
return a != nil && string(desc.Digest) == a.Digest, nil, nil
250+
log.Debugf("Get the manifest list with key=cache:%v", manifestListKey(art.Repository, art))
251+
return true, &ManifestList{content, string(desc.Digest), contentType}, nil
195252
}
196-
err = c.cache.Fetch(ctx, manifestListContentTypeKey(art.Repository, art), &contentType)
197-
if err != nil {
198-
log.Debugf("failed to get the manifest list content type, not use local. error:%v", err)
199-
return false, nil, nil
253+
if errors.Is(err, cache.ErrNotFound) {
254+
log.Debugf("Digest is not found in manifest list cache, key=cache:%v", manifestListKey(art.Repository, art))
255+
} else {
256+
log.Errorf("Failed to get manifest list from cache, error: %v", err)
200257
}
201-
log.Debugf("Get the manifest list with key=cache:%v", manifestListKey(art.Repository, art))
202-
return true, &ManifestList{content, string(desc.Digest), contentType}, nil
258+
259+
// neither manifest was found in local repo nor manifest list was found in cache
260+
return a != nil && string(desc.Digest) == a.Digest, nil, nil
261+
}
262+
263+
func manifestKey(repo string, art lib.ArtifactInfo) string {
264+
// actual redis key format is cache:manifest:<repo name>:<tag> or cache:manifest:<repo name>:sha256:xxxx
265+
return "manifest:" + repo + ":" + getReference(art)
266+
}
267+
268+
func manifestContentTypeKey(rep string, art lib.ArtifactInfo) string {
269+
return manifestKey(rep, art) + ":contenttype"
203270
}
204271

205272
func manifestListKey(repo string, art lib.ArtifactInfo) string {
206-
// actual redis key format is cache:manifestlist:<repo name>:<tag> or cache:manifestlist:<repo name>:sha256:xxxx
273+
// actual redis key format is cache:manifest:<repo name>:<tag> or cache:manifest:<repo name>:sha256:xxxx
207274
return "manifestlist:" + repo + ":" + getReference(art)
208275
}
209276

‎src/controller/proxy/controller_test.go‎

Lines changed: 106 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package proxy
1616

1717
import (
1818
"context"
19+
"fmt"
1920
"io"
2021
"testing"
2122

@@ -31,6 +32,8 @@ import (
3132
"github.com/goharbor/harbor/src/lib/errors"
3233
proModels "github.com/goharbor/harbor/src/pkg/project/models"
3334
testproxy "github.com/goharbor/harbor/src/testing/controller/proxy"
35+
"github.com/goharbor/harbor/src/testing/lib/cache"
36+
v1 "github.com/opencontainers/image-spec/specs-go/v1"
3437
)
3538

3639
type localInterfaceMock struct {
@@ -64,6 +67,17 @@ func (l *localInterfaceMock) PushBlob(localRepo string, desc distribution.Descri
6467
panic("implement me")
6568
}
6669

70+
func (l *localInterfaceMock) PullManifest(repo string, ref string) (distribution.Manifest, string, error) {
71+
args := l.Called(repo, ref)
72+
73+
var d distribution.Manifest
74+
if arg := args.Get(0); arg != nil {
75+
d = arg.(distribution.Manifest)
76+
}
77+
78+
return d, args.String(1), args.Error(2)
79+
}
80+
6781
func (l *localInterfaceMock) PushManifest(repo string, tag string, manifest distribution.Manifest) error {
6882
args := l.Called(repo, tag, manifest)
6983
return args.Error(0)
@@ -84,7 +98,7 @@ type proxyControllerTestSuite struct {
8498
suite.Suite
8599
local *localInterfaceMock
86100
remote *testproxy.RemoteInterface
87-
ctr Controller
101+
ctr *controller
88102
proj *proModels.Project
89103
}
90104

@@ -114,12 +128,15 @@ func (p *proxyControllerTestSuite) TestUseLocalManifest_False() {
114128
ctx := context.Background()
115129
dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
116130
desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
117-
art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
131+
repo := "library/hello-world"
132+
art := lib.ArtifactInfo{Repository: repo, Digest: dig}
118133
p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, desc, nil)
119134
p.local.On("GetManifest", mock.Anything, mock.Anything).Return(nil, nil)
135+
p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(nil, "", fmt.Errorf("could not pull manifest"))
120136
result, _, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
121137
p.Assert().Nil(err)
122138
p.Assert().False(result)
139+
p.local.AssertExpectations(p.T())
123140
}
124141

125142
func (p *proxyControllerTestSuite) TestUseLocalManifest_429() {
@@ -157,6 +174,93 @@ func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_False() {
157174
p.Assert().False(result)
158175
}
159176

177+
func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_LocalRepoTrueManifest() {
178+
manifest := `{
179+
"schemaVersion": 2,
180+
"mediaType": "application/vnd.oci.image.manifest.v1+json",
181+
"config": {
182+
"mediaType": "application/vnd.example.config.v1+json",
183+
"digest": "sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
184+
"size": 123
185+
},
186+
"layers": [
187+
{
188+
"mediaType": "application/vnd.example.data.v1.tar+gzip",
189+
"digest": "sha256:e258d248fda94c63753607f7c4494ee0fcbe92f1a76bfdac795c9d84101eb317",
190+
"size": 1234
191+
}
192+
],
193+
"annotations": {
194+
"com.example.key1": "value1"
195+
}
196+
}`
197+
man, desc, err := distribution.UnmarshalManifest(v1.MediaTypeImageManifest, []byte(manifest))
198+
p.Require().NoError(err)
199+
mediaType, payload, err := man.Payload()
200+
p.Require().NoError(err)
201+
202+
ctx := context.Background()
203+
repo := "library/hello-world"
204+
art := lib.ArtifactInfo{Repository: repo, Tag: "latest"}
205+
p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
206+
p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, &desc, nil)
207+
p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(man, string(desc.Digest), nil)
208+
209+
result, manifests, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
210+
211+
p.Assert().NoError(err)
212+
p.Assert().True(result)
213+
p.Assert().NotNil(manifests)
214+
p.Assert().Equal(mediaType, manifests.ContentType())
215+
p.Assert().Equal(string(desc.Digest), manifests.Digest())
216+
p.Assert().Equal(payload, manifests.Content())
217+
218+
p.local.AssertExpectations(p.T())
219+
}
220+
221+
func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_CacheTrueManifestList() {
222+
c := cache.NewCache(p.T())
223+
p.ctr.cache = c
224+
225+
ctx := context.Background()
226+
repo := "library/hello-world"
227+
art := lib.ArtifactInfo{Repository: repo, Tag: "latest"}
228+
dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
229+
desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
230+
content := "some content"
231+
contentType := "some content type"
232+
p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
233+
p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(true, desc, nil)
234+
p.local.On("PullManifest", repo, string(desc.Digest)).Times(1).Return(nil, "", fmt.Errorf("could not pull manifest"))
235+
artInfoWithDigest := art
236+
artInfoWithDigest.Digest = dig
237+
c.On("Fetch", mock.Anything, manifestListKey(art.Repository, artInfoWithDigest), mock.Anything).
238+
Times(1).
239+
Run(func(args mock.Arguments) {
240+
ct := args.Get(2).(*[]byte)
241+
*ct = []byte(content)
242+
}).
243+
Return(nil)
244+
c.On("Fetch", mock.Anything, manifestListContentTypeKey(art.Repository, artInfoWithDigest), mock.Anything).
245+
Times(1).
246+
Run(func(args mock.Arguments) {
247+
ct := args.Get(2).(*string)
248+
*ct = contentType
249+
}).
250+
Return(nil)
251+
252+
result, manifests, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
253+
254+
p.Assert().NoError(err)
255+
p.Assert().True(result)
256+
p.Assert().NotNil(manifests)
257+
p.Assert().Equal(contentType, manifests.ContentType())
258+
p.Assert().Equal(string(desc.Digest), manifests.Digest())
259+
p.Assert().Equal([]byte(content), manifests.Content())
260+
261+
p.local.AssertExpectations(p.T())
262+
}
263+
160264
func (p *proxyControllerTestSuite) TestUseLocalBlob_True() {
161265
ctx := context.Background()
162266
dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"

‎src/controller/proxy/local.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ type localInterface interface {
4343
GetManifest(ctx context.Context, art lib.ArtifactInfo) (*artifact.Artifact, error)
4444
// PushBlob push blob to local repo
4545
PushBlob(localRepo string, desc distribution.Descriptor, bReader io.ReadCloser) error
46+
// PullManifest pulls manifest from local repo, ref can be digest or tag
47+
PullManifest(repo string, ref string) (distribution.Manifest, string, error)
4648
// PushManifest push manifest to local repo, ref can be digest or tag
4749
PushManifest(repo string, ref string, manifest distribution.Manifest) error
4850
// CheckDependencies check if the manifest's dependency is ready
@@ -109,6 +111,10 @@ func (l *localHelper) PushBlob(localRepo string, desc distribution.Descriptor, b
109111
return err
110112
}
111113

114+
func (l *localHelper) PullManifest(repo string, ref string) (distribution.Manifest, string, error) {
115+
return l.registry.PullManifest(repo, ref)
116+
}
117+
112118
func (l *localHelper) PushManifest(repo string, ref string, manifest distribution.Manifest) error {
113119
// Make sure there is only one go routing to push current artName to local repo
114120
artName := repo + ":" + ref

‎src/server/middleware/repoproxy/proxy.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,12 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
185185
}
186186
if useLocal {
187187
if man != nil {
188-
w.Header().Set(contentLength, fmt.Sprintf("%v", len(man.Content)))
189-
w.Header().Set(contentType, man.ContentType)
190-
w.Header().Set(dockerContentDigest, man.Digest)
191-
w.Header().Set(etag, man.Digest)
188+
w.Header().Set(contentLength, fmt.Sprintf("%v", len(man.Content())))
189+
w.Header().Set(contentType, man.ContentType())
190+
w.Header().Set(dockerContentDigest, man.Digest())
191+
w.Header().Set(etag, man.Digest())
192192
if r.Method == http.MethodGet {
193-
_, err = w.Write(man.Content)
193+
_, err = w.Write(man.Content())
194194
if err != nil {
195195
return err
196196
}

0 commit comments

Comments
 (0)