From 9072acbfa611367e9e88ce1c5ea1bd6b4ca9ea8d Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 27 Jun 2023 01:06:50 +0000 Subject: [PATCH] Tidy formatting of `Reqwest` errors (#4336) ## Issue Addressed NA ## Proposed Changes Implements the `PrettyReqwestError` to wrap a `reqwest::Error` and give nicer `Debug` formatting. It also wraps the `Url` component in a `SensitiveUrl` to avoid leaking sensitive info in logs. ### Before ``` Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(9999), path: "/eth/v1/node/version", query: None, fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 61, kind: ConnectionRefused, message: "Connection refused" })) }) ``` ### After ``` HttpClient(url: http://localhost:9999/, kind: request, detail: error trying to connect: tcp connect error: Connection refused (os error 61)) ``` ## Additional Info I've also renamed the `Reqwest` error enum variants to `HttpClient`, to give people a better chance at knowing what's going on. Reqwest is pretty odd and looks like a typo. I've implemented it in the `eth2` and `execution_layer` crates. This should affect most logs in the VC and EE-related ones in the BN. I think the last crate that could benefit from the is the `beacon_node/eth1` crate. I haven't updated it in this PR since its error type is not so amenable to it (everything goes into a `String`). I don't have a whole lot of time to jig around with that at the moment and I feel that this PR as it stands is a significant enough improvement to merge on its own. Leaving it as-is is fine for the time being and we can always come back for it later (or implement in-protocol deposits!). --- Cargo.lock | 12 ++++ Cargo.toml | 1 + beacon_node/builder_client/src/lib.rs | 6 +- beacon_node/execution_layer/Cargo.toml | 1 + beacon_node/execution_layer/src/engine_api.rs | 5 +- common/eth2/Cargo.toml | 5 ++ common/eth2/src/lib.rs | 13 ++-- common/eth2/src/lighthouse.rs | 4 +- common/eth2/src/lighthouse_vc/http_client.rs | 12 ++-- common/pretty_reqwest_error/Cargo.toml | 10 +++ common/pretty_reqwest_error/src/lib.rs | 62 +++++++++++++++++++ common/sensitive_url/src/lib.rs | 2 +- 12 files changed, 113 insertions(+), 20 deletions(-) create mode 100644 common/pretty_reqwest_error/Cargo.toml create mode 100644 common/pretty_reqwest_error/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 700aecb83..02922b2d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2292,6 +2292,8 @@ dependencies = [ "libsecp256k1", "lighthouse_network", "mediatype", + "mime", + "pretty_reqwest_error", "procinfo", "proto_array", "psutil", @@ -2302,6 +2304,7 @@ dependencies = [ "serde_json", "slashing_protection", "store", + "tokio", "types", ] @@ -2742,6 +2745,7 @@ dependencies = [ "lru 0.7.8", "mev-rs", "parking_lot 0.12.1", + "pretty_reqwest_error", "rand 0.8.5", "reqwest", "sensitive_url", @@ -6204,6 +6208,14 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "pretty_reqwest_error" +version = "0.1.0" +dependencies = [ + "reqwest", + "sensitive_url", +] + [[package]] name = "prettyplease" version = "0.1.25" diff --git a/Cargo.toml b/Cargo.toml index bbe77d209..5c39e01ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,7 @@ members = [ "common/lru_cache", "common/malloc_utils", "common/oneshot_broadcast", + "common/pretty_reqwest_error", "common/sensitive_url", "common/slot_clock", "common/system_health", diff --git a/beacon_node/builder_client/src/lib.rs b/beacon_node/builder_client/src/lib.rs index 255c2fdd1..c78f686d0 100644 --- a/beacon_node/builder_client/src/lib.rs +++ b/beacon_node/builder_client/src/lib.rs @@ -72,7 +72,7 @@ impl BuilderHttpClient { .await? .json() .await - .map_err(Error::Reqwest) + .map_err(Into::into) } /// Perform a HTTP GET request, returning the `Response` for further processing. @@ -85,7 +85,7 @@ impl BuilderHttpClient { if let Some(timeout) = timeout { builder = builder.timeout(timeout); } - let response = builder.send().await.map_err(Error::Reqwest)?; + let response = builder.send().await.map_err(Error::from)?; ok_or_error(response).await } @@ -114,7 +114,7 @@ impl BuilderHttpClient { if let Some(timeout) = timeout { builder = builder.timeout(timeout); } - let response = builder.json(body).send().await.map_err(Error::Reqwest)?; + let response = builder.json(body).send().await.map_err(Error::from)?; ok_or_error(response).await } diff --git a/beacon_node/execution_layer/Cargo.toml b/beacon_node/execution_layer/Cargo.toml index a96cfb6ca..2cb28346f 100644 --- a/beacon_node/execution_layer/Cargo.toml +++ b/beacon_node/execution_layer/Cargo.toml @@ -50,3 +50,4 @@ keccak-hash = "0.10.0" hash256-std-hasher = "0.15.2" triehash = "0.8.4" hash-db = "0.15.2" +pretty_reqwest_error = { path = "../../common/pretty_reqwest_error" } \ No newline at end of file diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 4d2eb565e..826294d5f 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -10,6 +10,7 @@ pub use ethers_core::types::Transaction; use ethers_core::utils::rlp::{self, Decodable, Rlp}; use http::deposit_methods::RpcError; pub use json_structures::{JsonWithdrawal, TransitionConfigurationV1}; +use pretty_reqwest_error::PrettyReqwestError; use reqwest::StatusCode; use serde::{Deserialize, Serialize}; use std::convert::TryFrom; @@ -32,7 +33,7 @@ pub type PayloadId = [u8; 8]; #[derive(Debug)] pub enum Error { - Reqwest(reqwest::Error), + HttpClient(PrettyReqwestError), Auth(auth::Error), BadResponse(String), RequestFailed(String), @@ -67,7 +68,7 @@ impl From for Error { ) { Error::Auth(auth::Error::InvalidToken) } else { - Error::Reqwest(e) + Error::HttpClient(e.into()) } } } diff --git a/common/eth2/Cargo.toml b/common/eth2/Cargo.toml index 4eabd3ff8..d8e1a375f 100644 --- a/common/eth2/Cargo.toml +++ b/common/eth2/Cargo.toml @@ -27,6 +27,11 @@ futures = "0.3.8" store = { path = "../../beacon_node/store", optional = true } slashing_protection = { path = "../../validator_client/slashing_protection", optional = true } mediatype = "0.19.13" +mime = "0.3.16" +pretty_reqwest_error = { path = "../../common/pretty_reqwest_error" } + +[dev-dependencies] +tokio = { version = "1.14.0", features = ["full"] } [target.'cfg(target_os = "linux")'.dependencies] psutil = { version = "3.2.2", optional = true } diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index e871efbc2..217d35696 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -19,6 +19,7 @@ use self::types::{Error as ResponseError, *}; use futures::Stream; use futures_util::StreamExt; use lighthouse_network::PeerId; +use pretty_reqwest_error::PrettyReqwestError; pub use reqwest; use reqwest::{IntoUrl, RequestBuilder, Response}; pub use reqwest::{StatusCode, Url}; @@ -39,7 +40,7 @@ pub const CONSENSUS_VERSION_HEADER: &str = "Eth-Consensus-Version"; #[derive(Debug)] pub enum Error { /// The `reqwest` client raised an error. - Reqwest(reqwest::Error), + HttpClient(PrettyReqwestError), /// The server returned an error message where the body was able to be parsed. ServerMessage(ErrorMessage), /// The server returned an error message with an array of errors. @@ -70,7 +71,7 @@ pub enum Error { impl From for Error { fn from(error: reqwest::Error) -> Self { - Error::Reqwest(error) + Error::HttpClient(error.into()) } } @@ -78,7 +79,7 @@ impl Error { /// If the error has a HTTP status code, return it. pub fn status(&self) -> Option { match self { - Error::Reqwest(error) => error.status(), + Error::HttpClient(error) => error.inner().status(), Error::ServerMessage(msg) => StatusCode::try_from(msg.code).ok(), Error::ServerIndexedMessage(msg) => StatusCode::try_from(msg.code).ok(), Error::StatusCode(status) => Some(*status), @@ -278,7 +279,7 @@ impl BeaconNodeHttpClient { .await? .json() .await - .map_err(Error::Reqwest) + .map_err(Into::into) } /// Perform a HTTP POST request with a custom timeout. @@ -303,7 +304,7 @@ impl BeaconNodeHttpClient { .await? .json() .await - .map_err(Error::Reqwest) + .map_err(Error::from) } /// Generic POST function supporting arbitrary responses and timeouts. @@ -1645,7 +1646,7 @@ impl BeaconNodeHttpClient { .bytes_stream() .map(|next| match next { Ok(bytes) => EventKind::from_sse_bytes(bytes.as_ref()), - Err(e) => Err(Error::Reqwest(e)), + Err(e) => Err(Error::HttpClient(e.into())), })) } diff --git a/common/eth2/src/lighthouse.rs b/common/eth2/src/lighthouse.rs index bb933dbe1..1b4bcc0e3 100644 --- a/common/eth2/src/lighthouse.rs +++ b/common/eth2/src/lighthouse.rs @@ -364,12 +364,12 @@ pub struct DatabaseInfo { impl BeaconNodeHttpClient { /// Perform a HTTP GET request, returning `None` on a 404 error. async fn get_bytes_opt(&self, url: U) -> Result>, Error> { - let response = self.client.get(url).send().await.map_err(Error::Reqwest)?; + let response = self.client.get(url).send().await.map_err(Error::from)?; match ok_or_error(response).await { Ok(resp) => Ok(Some( resp.bytes() .await - .map_err(Error::Reqwest)? + .map_err(Error::from)? .into_iter() .collect::>(), )), diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 720d8c779..cd7873c9b 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -170,7 +170,7 @@ impl ValidatorClientHttpClient { .map_err(|_| Error::InvalidSignatureHeader)? .to_string(); - let body = response.bytes().await.map_err(Error::Reqwest)?; + let body = response.bytes().await.map_err(Error::from)?; let message = Message::parse_slice(digest(&SHA256, &body).as_ref()).expect("sha256 is 32 bytes"); @@ -222,7 +222,7 @@ impl ValidatorClientHttpClient { .headers(self.headers()?) .send() .await - .map_err(Error::Reqwest)?; + .map_err(Error::from)?; ok_or_error(response).await } @@ -236,7 +236,7 @@ impl ValidatorClientHttpClient { .await? .json() .await - .map_err(Error::Reqwest) + .map_err(Error::from) } /// Perform a HTTP GET request, returning `None` on a 404 error. @@ -266,7 +266,7 @@ impl ValidatorClientHttpClient { .json(body) .send() .await - .map_err(Error::Reqwest)?; + .map_err(Error::from)?; ok_or_error(response).await } @@ -297,7 +297,7 @@ impl ValidatorClientHttpClient { .json(body) .send() .await - .map_err(Error::Reqwest)?; + .map_err(Error::from)?; let response = ok_or_error(response).await?; self.signed_body(response).await?; Ok(()) @@ -316,7 +316,7 @@ impl ValidatorClientHttpClient { .json(body) .send() .await - .map_err(Error::Reqwest)?; + .map_err(Error::from)?; ok_or_error(response).await } diff --git a/common/pretty_reqwest_error/Cargo.toml b/common/pretty_reqwest_error/Cargo.toml new file mode 100644 index 000000000..ca9f4812b --- /dev/null +++ b/common/pretty_reqwest_error/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "pretty_reqwest_error" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +reqwest = { version = "0.11.0", features = ["json","stream"] } +sensitive_url = { path = "../sensitive_url" } diff --git a/common/pretty_reqwest_error/src/lib.rs b/common/pretty_reqwest_error/src/lib.rs new file mode 100644 index 000000000..4c605f38a --- /dev/null +++ b/common/pretty_reqwest_error/src/lib.rs @@ -0,0 +1,62 @@ +use sensitive_url::SensitiveUrl; +use std::error::Error as StdError; +use std::fmt; + +pub struct PrettyReqwestError(reqwest::Error); + +impl PrettyReqwestError { + pub fn inner(&self) -> &reqwest::Error { + &self.0 + } +} + +impl fmt::Debug for PrettyReqwestError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + if let Some(url) = self.0.url() { + if let Ok(url) = SensitiveUrl::new(url.clone()) { + write!(f, "url: {}", url)?; + } else { + write!(f, "url: unable_to_parse")?; + }; + } + + let kind = if self.0.is_builder() { + "builder" + } else if self.0.is_redirect() { + "redirect" + } else if self.0.is_status() { + "status" + } else if self.0.is_timeout() { + "timeout" + } else if self.0.is_request() { + "request" + } else if self.0.is_connect() { + "connect" + } else if self.0.is_body() { + "body" + } else if self.0.is_decode() { + "decode" + } else { + "unknown" + }; + write!(f, ", kind: {}", kind)?; + + if let Some(status) = self.0.status() { + write!(f, ", status_code: {}", status)?; + } + + if let Some(ref source) = self.0.source() { + write!(f, ", detail: {}", source)?; + } else { + write!(f, ", source: unknown")?; + } + + Ok(()) + } +} + +impl From for PrettyReqwestError { + fn from(inner: reqwest::Error) -> Self { + Self(inner) + } +} diff --git a/common/sensitive_url/src/lib.rs b/common/sensitive_url/src/lib.rs index b6705eb60..b6068a2dc 100644 --- a/common/sensitive_url/src/lib.rs +++ b/common/sensitive_url/src/lib.rs @@ -75,7 +75,7 @@ impl SensitiveUrl { SensitiveUrl::new(surl) } - fn new(full: Url) -> Result { + pub fn new(full: Url) -> Result { let mut redacted = full.clone(); redacted .path_segments_mut()