[mod] start putting timeout in certificate code #231
Conversation
| try: | ||
| intermediate_certificate = requests.get(INTERMEDIATE_CERTIFICATE_URL, timeout=30).text | ||
| except Timeout: | ||
| # XXX what should we do here? retry? |
There was a problem hiding this comment.
If we didn't get the intermediate certificate, the certificate won't work, so I'd say we just do a raise MoulinetteError(errno.EINVAL, m18n.n('certmanager_couldnt_fetch_intermediate_cert'))
(Btw, I never know if we do care about which errno to use ?)
There was a problem hiding this comment.
trusting @alexAubin on this
| requests.head("http://" + ip, headers={"Host": domain}) | ||
| requests.head("http://" + ip, headers={"Host": domain}, timeout=30) | ||
| except Timeout as e: | ||
| # XXX what should we do here? retry? |
There was a problem hiding this comment.
If we really have a 30 sec timeout (which to me is crazy for what we are trying to do here), then we should return False.
But maybe we could have something like :
- have a 10-15 second timeout first
- if it doesn't work, have something like "Warning : having trouble contacting domain. Retrying..."
- retry with a 30 second timeout
- return False if still timeouting
But I'm having a hard time imagining a situation where the domain was up and everything running properly, but it takes more than 30 seconds for a HEAD request to complete :/... (even on a 'slow' connection)
There was a problem hiding this comment.
30 sec also seems a lot to me. 5 to 10 should be more than enough. Here we are trying to contact ourself, and we just want to detect the case where it does not work at all (due to hairpinning or other weird firewall setups).
There was a problem hiding this comment.
I may not see the big picture on this, but I would go for something way more simple : if timeout after 5 to 10 seconds, then just abort with a good error message, like "timeout reached when your server tried to contact itself via its public IP address. you may very well be experiencing hairpinning, or the firewall/router ahead of your server is misconfigured"
There was a problem hiding this comment.
@julienmalik Well yep, but @Psycojoker was pushing to have a 30 sec timeout 😛 . But I'd be in favor of this too.
|
Haven't tested the real timeout, but I tested the lines inside the exceptions 😕 |
|
We need one more approval for this one. |
| intermediate_certificate = requests.get(INTERMEDIATE_CERTIFICATE_URL).text | ||
| try: | ||
| intermediate_certificate = requests.get(INTERMEDIATE_CERTIFICATE_URL, timeout=30).text | ||
| except Timeout: |
There was a problem hiding this comment.
you should add as e to be conpliant with yunohost coding standards (tm)
| requests.head("http://" + ip, headers={"Host": domain}) | ||
| requests.head("http://" + ip, headers={"Host": domain}, timeout=10) | ||
| except Timeout as e: | ||
| logger.warning(m18n.n('certmanager_http_check_timeout')) |
There was a problem hiding this comment.
error message should include domain and ip information to be more relevant
|
Thanks for the feedback ! |
As discussed with @julienmalik and @alexAubin, I'm starting this PR so we can discuss about how we should handle timeouting requests in the cert management.