-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Return error when proxy cache get too many request error(429) #18728
Return error when proxy cache get too many request error(429) #18728
Conversation
Codecov Report
@@ Coverage Diff @@
## main #18728 +/- ##
==========================================
- Coverage 67.38% 67.37% -0.02%
==========================================
Files 980 980
Lines 106749 106759 +10
Branches 2665 2665
==========================================
- Hits 71937 71932 -5
- Misses 30948 30965 +17
+ Partials 3864 3862 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7a765eb
to
cc4c08b
Compare
@@ -46,6 +46,8 @@ const ( | |||
ensureTagMaxRetry = 60 | |||
) | |||
|
|||
var tooManyRequestsError = errors.New("too many requests to upstream registry").WithCode(errors.RateLimitCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have exceeded the pull rate limit for the proxy cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -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 | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should handle 429 as a special case, we handle any error the same way, i.e. when error happens refreshing the cache and the cache is not expired, we log the error and serve the data in the cache.
IMHO, what needs to be refined is in the path when an error happens and the cache is expired or the data is not cached.
In this case, we better return the error more explicitly instead of a 404. I think a better choice here is to return 500 which wraps the root cause (in this case 429), if 500 can't be rendered in a friendly way for mainstream clients, we find a proper error so most clients can handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid leaking any server information. if return 500 error to the client, it just sends the "internal server error" to the client, no other useful information.
see
https://github.com/goharbor/harbor/blob/cc4c08b0a1cef086fdb47d90be8f6b32efe170cf/src/lib/http/error.go#L55-L54
cc4c08b
to
e1bc03d
Compare
I'm not quite convinced why we fix 429 but leave other error-handling situations as-is. That said it's up to @stonezdj to make decision. |
For any other error: For the least impact to the current system, keep the current implementation. |
e1bc03d
to
8224aaf
Compare
Add 429 too many request error in http error Fixes goharbor#18707 Signed-off-by: stonezdj <[email protected]>
8224aaf
to
0bbe5d9
Compare
…or#18728) Add 429 too many request error in http error Fixes goharbor#18707 Signed-off-by: stonezdj <[email protected]> Signed-off-by: Wilfred Almeida <[email protected]>
…or#18728) Add 429 too many request error in http error Fixes goharbor#18707 Signed-off-by: stonezdj <[email protected]>
Add 429 too many request error in http error
Fixes #18707
Thank you for contributing to Harbor!
Comprehensive Summary of your change
Issue being fixed
Fixes #(issue)
Please indicate you've done the following: