Skip to content

Commit

Permalink
Return error when proxy cache get too many request error(429) (goharb…
Browse files Browse the repository at this point in the history
…or#18728)

Add 429 too many request error in http error
  Fixes goharbor#18707

Signed-off-by: stonezdj <stonezdj@gmail.com>
  • Loading branch information
stonezdj authored and WilfredAlmeida committed Jul 8, 2023
1 parent 9af90b1 commit aee82e9
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 1 deletion.
3 changes: 3 additions & 0 deletions src/controller/proxy/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ func (c *controller) UseLocalManifest(ctx context.Context, art lib.ArtifactInfo,
remoteRepo := getRemoteRepo(art)
exist, desc, err := remote.ManifestExist(remoteRepo, getReference(art)) // HEAD
if err != nil {
if errors.IsRateLimitError(err) && a != nil { // if rate limit, use local if it exists, otherwise return error
return true, nil, nil
}
return false, nil, err
}
if !exist || desc == nil {
Expand Down
24 changes: 24 additions & 0 deletions src/controller/proxy/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,30 @@ func (p *proxyControllerTestSuite) TestUseLocalManifest_False() {
p.Assert().False(result)
}

func (p *proxyControllerTestSuite) TestUseLocalManifest_429() {
ctx := context.Background()
dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(false, desc, errors.New("too many requests").WithCode(errors.RateLimitCode))
p.local.On("GetManifest", mock.Anything, mock.Anything).Return(nil, nil)
_, _, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
p.Assert().NotNil(err)
errors.IsRateLimitError(err)
}

func (p *proxyControllerTestSuite) TestUseLocalManifest_429ToLocal() {
ctx := context.Background()
dig := "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b"
desc := &distribution.Descriptor{Digest: digest.Digest(dig)}
art := lib.ArtifactInfo{Repository: "library/hello-world", Digest: dig}
p.remote.On("ManifestExist", mock.Anything, mock.Anything).Return(false, desc, errors.New("too many requests").WithCode(errors.RateLimitCode))
p.local.On("GetManifest", mock.Anything, mock.Anything).Return(&artifact.Artifact{}, nil)
result, _, err := p.ctr.UseLocalManifest(ctx, art, p.remote)
p.Assert().Nil(err)
p.Assert().True(result)
}

func (p *proxyControllerTestSuite) TestUseLocalManifestWithTag_False() {
ctx := context.Background()
art := lib.ArtifactInfo{Repository: "library/hello-world", Tag: "latest"}
Expand Down
7 changes: 7 additions & 0 deletions src/lib/errors/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const (
ForbiddenCode = "FORBIDDEN"
// MethodNotAllowedCode ...
MethodNotAllowedCode = "METHOD_NOT_ALLOWED"
// RateLimitCode
RateLimitCode = "TOO_MANY_REQUEST"
// PreconditionCode ...
PreconditionCode = "PRECONDITION"
// GeneralCode ...
Expand Down Expand Up @@ -105,3 +107,8 @@ func IsConflictErr(err error) bool {
func IsChallengesUnsupportedErr(err error) bool {
return IsErr(err, ChallengesUnsupportedCode)
}

// IsRateLimitError checks whether the err chains contains rate limit error
func IsRateLimitError(err error) bool {
return IsErr(err, RateLimitCode)
}
1 change: 1 addition & 0 deletions src/lib/http/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var (
errors.MethodNotAllowedCode: http.StatusMethodNotAllowed,
errors.DENIED: http.StatusForbidden,
errors.NotFoundCode: http.StatusNotFound,
errors.RateLimitCode: http.StatusTooManyRequests,
errors.ConflictCode: http.StatusConflict,
errors.PreconditionCode: http.StatusPreconditionFailed,
errors.ViolateForeignKeyConstraintCode: http.StatusPreconditionFailed,
Expand Down
2 changes: 2 additions & 0 deletions src/pkg/registry/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,8 @@ func (c *client) do(req *http.Request) (*http.Response, error) {
code = errors.ForbiddenCode
case http.StatusNotFound:
code = errors.NotFoundCode
case http.StatusTooManyRequests:
code = errors.RateLimitCode
}
return nil, errors.New(nil).WithCode(code).
WithMessage(message)
Expand Down
8 changes: 7 additions & 1 deletion src/server/middleware/repoproxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const (
ensureTagMaxRetry = 60
)

var tooManyRequestsError = errors.New("too many requests to upstream registry").WithCode(errors.RateLimitCode)

// BlobGetMiddleware handle get blob request
func BlobGetMiddleware() func(http.Handler) http.Handler {
return middleware.New(func(w http.ResponseWriter, r *http.Request, next http.Handler) {
Expand Down Expand Up @@ -112,6 +114,10 @@ func ManifestMiddleware() func(http.Handler) http.Handler {
httpLib.SendError(w, err)
return
}
if errors.IsRateLimitError(err) {
httpLib.SendError(w, tooManyRequestsError)
return
}
log.Errorf("failed to proxy manifest, fallback to local, request uri: %v, error: %v", r.RequestURI, err)
next.ServeHTTP(w, r)
}
Expand Down Expand Up @@ -202,7 +208,7 @@ func handleManifest(w http.ResponseWriter, r *http.Request, next http.Handler) e
err = proxyManifestGet(ctx, w, proxyCtl, p, art, remote)
}
if err != nil {
if errors.IsNotFoundErr(err) {
if errors.IsNotFoundErr(err) || errors.IsRateLimitError(err) {
return err
}
log.Warningf("Proxy to remote failed, fallback to local repo, error: %v", err)
Expand Down

0 comments on commit aee82e9

Please sign in to comment.