Dont retry connection on SSL certificate error #16245
Conversation
assadyousuf
changed the title
| } | ||
| } | ||
|
|
||
| // Use the default strategy and check for additional transient error cases. |
There was a problem hiding this comment.
Unsure on if its possible/how to simulate an SSL certificate failure with the mock server so we can get a test going. Open to ideas on how to test the new code path
There was a problem hiding this comment.
I don't have a great idea either unfortunately, I'm using https://badssl.com/ for manual testing but we don't want to make CI depend on a third-party website. I tried building something with https://github.com/rustls/rcgen but it was clashing with the rustls features we were enabling, it would be great if we could fix that on the rcgen side and then use a self-signed cert in the test.
There was a problem hiding this comment.
I would think setting SSL_CERT_FILE to a self signed cert chain in the test context may help, e.g. maybe with a self-signed cert for pypi.org and running typical uv project commands. I'd expect it to fail on hostname validation.
If we really need some other kind of testing requiring our own mocked server, we could use a similar approach like we did here, but would require some additional minor setup to get some dummy certs working.
assadyousuf
changed the title
| } | ||
| } | ||
|
|
||
| // Use the default strategy and check for additional transient error cases. |
There was a problem hiding this comment.
I don't have a great idea either unfortunately, I'm using https://badssl.com/ for manual testing but we don't want to make CI depend on a third-party website. I tried building something with https://github.com/rustls/rcgen but it was clashing with the rustls features we were enabling, it would be great if we could fix that on the rcgen side and then use a self-signed cert in the test.
|
|
||
| impl RetryableStrategy for UvRetryableStrategy { | ||
| fn handle(&self, res: &Result<Response, reqwest_middleware::Error>) -> Option<Retryable> { | ||
| if let Err(err) = res { |
There was a problem hiding this comment.
Why do we have this check both here and in is_transient_network_error?
| if let reqwest_middleware::Error::Reqwest(reqwest_err) = &**reqwest_err { | ||
| if default_on_request_error(reqwest_err) == Some(Retryable::Transient) { | ||
| if is_ssl_request_error(reqwest_err) { | ||
| trace!("Cannot retry nested reqwest middleware SSL error"); |
3 participants
Summary
SSL errors should not be retryable from the cached on non cached client: #16235
Test Plan
Unsure on if its possible/how to simulate an SSL certificate failure with the mock server so we can get a test going. Open to ideas on how to test this. See comment below