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.

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 a clear error message when rate limit is hit #138

Merged

Conversation

jorgeaguileraseqera
Copy link
Contributor

Plus test I've tested using a docker command line, and the error appears as:

docker pull localhost:9090/library/hello-world
Using default tag: latest
Error response from daemon: pull access denied for localhost:9090/library/hello-world, repository does not exist or may require 'docker login': denied: 127.0.0.1 request exceeded pull rate limit

closes #133

Signed-off-by: Jorge Aguilera [email protected]

@jorgeaguileraseqera jorgeaguileraseqera self-assigned this Sep 20, 2022
@pditommaso
Copy link
Collaborator

think if it could not make more sense of changing the interface to return a boolean instead of an exception

void acquireBuild(AcquireRequest request) throws SlowDownException
void acquirePull(AcquireRequest request) throws SlowDownException

and then throw the docker exception

@jorgeaguileraseqera
Copy link
Contributor Author

think if it could not make more sense of changing the interface to return a boolean instead of an exception

void acquireBuild(AcquireRequest request) throws SlowDownException
void acquirePull(AcquireRequest request) throws SlowDownException

and then throw the docker exception

also, we can catch the specific SlowDownException in the ErrorController and return a RegistryErrorResponse

@pditommaso
Copy link
Collaborator

Yes, but I feel like an ad hoc fix. In principle, that exception could the raised somewhere else.

This is why I think the rate limiter should just tell yes/how and the inoking method use the most appropriate exception

@jorgeaguileraseqera
Copy link
Contributor Author

Ok, I just updated the PR with the ErrorController approach but will change it to this approach

Signed-off-by: Paolo Di Tommaso <[email protected]>
@pditommaso
Copy link
Collaborator

ok, just removed the docker specific handler from the global error controller.

@pditommaso pditommaso merged commit b188e15 into master Sep 21, 2022
@pditommaso pditommaso deleted the 133-return-an-clear-error-message-when-rate-limit-is-hit branch September 21, 2022 09:35
jorgeaguileraseqera added a commit that referenced this pull request Sep 26, 2022
Signed-off-by: Jorge Aguilera <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Co-authored-by: Paolo Di Tommaso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return an clear error message when rate limit is hit
2 participants