Skip to content

Commit

Permalink
Return error when proxy cache get too many request error(429)
Browse files Browse the repository at this point in the history
  Add 429 too many request error in http error
  Fixes #18707

Signed-off-by: stonezdj <stonezdj@gmail.com>
  • Loading branch information
stonezdj committed May 25, 2023
1 parent c399e5e commit e1bc03d
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 @@ -155,6 +155,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
}

Check warning on line 160 in src/controller/proxy/controller.go

View check run for this annotation

Codecov / codecov/patch

src/controller/proxy/controller.go#L159-L160

Added lines #L159 - L160 were not covered by tests
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

Check warning on line 682 in src/pkg/registry/client.go

View check run for this annotation

Codecov / codecov/patch

src/pkg/registry/client.go#L681-L682

Added lines #L681 - L682 were not covered by tests
}
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
}

Check warning on line 120 in src/server/middleware/repoproxy/proxy.go

View check run for this annotation

Codecov / codecov/patch

src/server/middleware/repoproxy/proxy.go#L117-L120

Added lines #L117 - L120 were not covered by tests
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) {

Check warning on line 211 in src/server/middleware/repoproxy/proxy.go

View check run for this annotation

Codecov / codecov/patch

src/server/middleware/repoproxy/proxy.go#L211

Added line #L211 was not covered by tests
return err
}
log.Warningf("Proxy to remote failed, fallback to local repo, error: %v", err)
Expand Down

0 comments on commit e1bc03d

Please sign in to comment.