-
Notifications
You must be signed in to change notification settings - Fork 90
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
acme modules: also support 503 for retries #513
Conversation
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.
Did you also mean to update lines 56-57?
if info['status'] not in [429, 503]:
return False
Regardless of their explicit support for it I would probably add 408
in there as well.
Hmm, I'm pretty sure I did do that change. I'm curious what happened to it... maybe it was just too early in the morning :) |
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
@Ajpantuso thanks for reviewing this! |
try: | ||
retry_after = min(max(1, int(info.get('retry-after'))), 60) | ||
except (TypeError, ValueError) as dummy: | ||
retry_after = 10 | ||
module.log('Retrieved a 429 Too Many Requests on %s, retrying in %s seconds' % (info['url'], retry_after)) | ||
module.log('Retrieved a %d HTTP status on %s, retrying in %s seconds' % (info['status'], info['url'], retry_after)) |
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.
@felixfontein FYI it's possible to retrieve the phrase using:
import http
http.HTTPStatus(info['status']).phrase
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.
Does this also work for Python 2.6 and 2.7?
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.
@felixfontein since Python 2.5, there was something similar in httplib
: https://docs.python.org/2/library/httplib.html#httplib.responses + the table above it. You'd get the same via httplib.responses[info['status']]
.
And six
has from six.moves import http_client
that maps to httplib
under Python 2 and http.client
under Python 3: https://six.readthedocs.io/#module-six.moves.
So yes, with a bit of compatibility glue, it can be made usable.
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.
Note to self: Python 3 has https://docs.python.org/3/library/http.client.html#http.client.responses, so that should work. I'd probably use .get()
instead of []
to avoid failures when some unknown HTTP code shows up (there are unofficial ones, not sure whether they are also covered there, see https://en.wikipedia.org/wiki/HTTP_status_code#Unofficial_codes).
@@ -52,18 +52,22 @@ | |||
IPADDRESS_IMPORT_ERROR = None | |||
|
|||
|
|||
RETRY_STATUS_CODES = (408, 429, 503) |
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.
This could be more readable if it used names
from http import HTTPStatus
RETRY_STATUS_CODES = {
HTTPStatus.BAD_REQUEST,
HTTPStatus.REQUEST_TIMEOUT,
HTTPStatus.SERVICE_UNAVAILABLE,
}
These enums are comparable with regular numbers too:
In [36]: from http import HTTPStatus
In [37]: 400 in {HTTPStatus.BAD_REQUEST, HTTPStatus.REQUEST_TIMEOUT, HTTPStatus.SERVICE_UNAVAILABLE}
Out[37]: True
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.
That's Python 3.5+, so it won't work with Python 2.6 or Python 2.7 (both which are still supported by this collection).
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 didn't realize. But here's your chance to add a six-based compat helper: #513 (comment).
SUMMARY
A couple of hours after releasing a new version of the collection with support for HTTP status code 529 for Let's Encrypt, Let's Encrypt decided to switch to HTTP status code 503 (https://community.letsencrypt.org/t/new-service-busy-responses-beginning-during-high-load/184174/2) 🙄.
This also supports that status code...
ISSUE TYPE
COMPONENT NAME
acme module utils