RUST-766 Use hyper in place of reqwest
#371
Conversation
kmahar
changed the title
hyper in place of reqwesthyper in place of reqwest
|
Hi @AsianIntel, thanks for the PR! I'm running the CI patch now and will take a look at the code once that's complete. |
|
I'm not sure where the linter is failing, it's telling me it's fine locally |
|
Try rebasing on master ( |
|
Done lmao |
There was a problem hiding this comment.
Thanks for the PR, @AsianIntel! I have one small request but otherwise this looks great. I'm also going to tag in the rest of the team for review.
Cargo.toml
Outdated
| @@ -47,11 +47,13 @@ futures-util = { version = "0.3.14", features = ["io"] } | |||
| futures-executor = "0.3.14" | |||
| hex = "0.4.0" | |||
| hmac = "0.10.1" | |||
| hyper = { version = "0.14", default-features = false, features = ["client", "http1", "tcp"], optional = true } | |||
There was a problem hiding this comment.
Would you mind updating this to be a block (i.e. [dependencies.hyper] rather than the single line here) for readability?
src/runtime/http.rs
Outdated
| .await?; | ||
| uri: Uri, | ||
| headers: &'a [(&'a str, &'a str)], | ||
| ) -> Pin<Box<dyn Future<Output = Result<Response<Body>, Box<dyn Error>>> + 'a + Send>> { |
There was a problem hiding this comment.
Can we write this type as BoxFuture<'a, Result<Response<Body>, Box<dyn Error>>> instead? It's a little shorter / easier to read. This may allow us to drop the clippy suppression too.
src/runtime/http.rs
Outdated
| Ok(value) | ||
| let mut buf = body::aggregate(res.into_body()).await?; | ||
| let mut bytes = vec![0; buf.remaining()]; | ||
| buf.copy_to_slice(&mut bytes); |
There was a problem hiding this comment.
Since we're just copying to a slice anyways, we can use body::to_bytes directly. Ditto for the other use of body::aggregate in request_and_read_string.
| .send() | ||
| .await?; | ||
| uri: Uri, | ||
| headers: &'a [(&'a str, &'a str)], |
There was a problem hiding this comment.
Is there a reason you replaced the IntoIterator logic? The Iterator::fold pattern used previously would probably work here too.
There was a problem hiding this comment.
Yeah, https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.fold takes self as an argument causing the iterator to get consumed. I think this prevents it from being used further in the redirect.
| .authority(authority.as_str()) | ||
| .path_and_query(location) | ||
| .build()?; | ||
| return self.request(method, uri, headers).await; |
There was a problem hiding this comment.
We should probably remove Authorization and other sensitive headers before following the redirect.
There was a problem hiding this comment.
I'm not sure what you mean here. This module is only used internally here (for AWS). As far as I see, none of the uses have any sensitive headers.
|
Hi @AsianIntel, upon further discussion with the team we've decided that we don't want to commit to maintaining a lower-level HTTP implementation given some of the concerns that have arisen in this PR. We do agree, though, that pulling in |
|
Going to close this out in favor of a new PR that simply moves AWS auth behind a feature flag. Thanks again for your efforts @AsianIntel! |
|
Yeah, that sounds fine. The branch is also way out of sync. |
The PR replaces
reqwestin favor ofhyper::Client. This will also allow upstream libraries and end-users to have a choice in whether to usereqwestor not.reqwest::Clientwithhyper::Clientserde_jsonas direct dependencies (They were already dependencies ofreqwest)