From aec443b2057e7f075edf53128936bedad2ec57b4 Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Thu, 19 Dec 2024 10:23:31 +0100 Subject: [PATCH] Improve error messages: * Add response headers to bad discovery response. Fix #2369 * Improve connect error in ServiceClient --- crates/service-client/src/http.rs | 62 ++++++++++++++++-------- crates/service-client/src/lib.rs | 4 +- crates/service-protocol/src/discovery.rs | 12 ++--- 3 files changed, 51 insertions(+), 27 deletions(-) diff --git a/crates/service-client/src/http.rs b/crates/service-client/src/http.rs index 05ee781f5..7a901a0a9 100644 --- a/crates/service-client/src/http.rs +++ b/crates/service-client/src/http.rs @@ -28,9 +28,9 @@ use restate_types::config::HttpOptions; use rustls::ClientConfig; use std::error::Error; use std::fmt::Debug; -use std::future; use std::future::Future; use std::sync::LazyLock; +use std::{fmt, future}; type ProxiedHttpsConnector = ProxyConnector>; type ProxiedHttpConnector = ProxyConnector; @@ -182,34 +182,22 @@ impl HttpClient { Either::Left(async move { match fut.await { Ok(res) => Ok(res), - Err(err) if is_possible_h11_only_error(&err) => { - Err(HttpError::PossibleHTTP11Only(err)) - } - Err(err) => Err(HttpError::Hyper(err)), + Err(err) => Err(err.into()), } }) } } -fn is_possible_h11_only_error(err: &hyper_util::client::legacy::Error) -> bool { - // this is the error we see from the h2 lib when the server sends back an http1.1 response - // to an http2 request. http2 is designed to start requests with what looks like an invalid - // HTTP1.1 method, so typically 1.1 servers respond with a 40x, and the h2 client sees - // this as an invalid frame. - err.source() - .and_then(|err| err.downcast_ref::()) - .and_then(|err| err.reason()) - == Some(h2::Reason::FRAME_SIZE_ERROR) -} - #[derive(Debug, thiserror::Error)] pub enum HttpError { - #[error(transparent)] - Hyper(#[from] hyper_util::client::legacy::Error), #[error(transparent)] Http(#[from] http::Error), - #[error("server possibly only supports HTTP1.1, consider discovery with --use-http1.1: {0}")] + #[error("server possibly only supports HTTP1.1, consider discovery with --use-http1.1.\nReason: {}", FormatHyperError(.0))] PossibleHTTP11Only(#[source] hyper_util::client::legacy::Error), + #[error("unable to reach the remote endpoint.\nReason: {}", FormatHyperError(.0))] + Connect(#[source] hyper_util::client::legacy::Error), + #[error("{}", FormatHyperError(.0))] + Hyper(#[source] hyper_util::client::legacy::Error), } impl HttpError { @@ -220,6 +208,42 @@ impl HttpError { HttpError::Hyper(err) => err.is_retryable(), HttpError::Http(err) => err.is_retryable(), HttpError::PossibleHTTP11Only(_) => false, + HttpError::Connect(_) => true, + } + } + + fn is_possible_h11_only_error(err: &hyper_util::client::legacy::Error) -> bool { + // this is the error we see from the h2 lib when the server sends back an http1.1 response + // to an http2 request. http2 is designed to start requests with what looks like an invalid + // HTTP1.1 method, so typically 1.1 servers respond with a 40x, and the h2 client sees + // this as an invalid frame. + err.source() + .and_then(|err| err.downcast_ref::()) + .and_then(|err| err.reason()) + == Some(h2::Reason::FRAME_SIZE_ERROR) + } +} + +impl From for HttpError { + fn from(err: hyper_util::client::legacy::Error) -> Self { + if Self::is_possible_h11_only_error(&err) { + Self::PossibleHTTP11Only(err) + } else if err.is_connect() { + Self::Connect(err) + } else { + Self::Hyper(err) + } + } +} + +struct FormatHyperError<'a>(&'a hyper_util::client::legacy::Error); + +impl<'a> fmt::Display for FormatHyperError<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(source) = self.0.source() { + write!(f, "{}, {}", self.0, source) + } else { + write!(f, "{}", self.0) } } } diff --git a/crates/service-client/src/lib.rs b/crates/service-client/src/lib.rs index 993721de2..0af3c436b 100644 --- a/crates/service-client/src/lib.rs +++ b/crates/service-client/src/lib.rs @@ -152,9 +152,9 @@ impl ServiceClient { #[derive(Debug, thiserror::Error)] pub enum ServiceClientError { - #[error(transparent)] + #[error("HTTP client error: {0}")] Http(#[from] http::HttpError), - #[error(transparent)] + #[error("Lambda client error: {0}")] Lambda(#[from] lambda::LambdaError), #[error(transparent)] IdentityV1(#[from] as SignRequest>::Error), diff --git a/crates/service-protocol/src/discovery.rs b/crates/service-protocol/src/discovery.rs index 27ac5dd8b..a514381ac 100644 --- a/crates/service-protocol/src/discovery.rs +++ b/crates/service-protocol/src/discovery.rs @@ -145,9 +145,9 @@ pub enum DiscoveryError { Decode(#[source] serde_json::Error, Bytes), // Network related retryable errors - #[error("bad status code: {0}")] - BadStatusCode(u16), - #[error("client error: {0}")] + #[error("bad status code '{}'. Response headers: {:?}", .0.status, .0.headers)] + BadStatusCode(http::response::Parts), + #[error(transparent)] Client(#[from] ServiceClientError), #[error("cannot read body: {0}")] BodyError(GenericError), @@ -180,8 +180,8 @@ impl DiscoveryError { /// retrying can succeed. pub fn is_retryable(&self) -> bool { match self { - DiscoveryError::BadStatusCode(status) => matches!( - StatusCode::from_u16(*status).expect("should be valid status code"), + DiscoveryError::BadStatusCode(parts) => matches!( + parts.status, StatusCode::REQUEST_TIMEOUT | StatusCode::TOO_MANY_REQUESTS | StatusCode::INTERNAL_SERVER_ERROR @@ -410,7 +410,7 @@ impl ServiceDiscovery { .into_parts(); if !parts.status.is_success() { - return Err(DiscoveryError::BadStatusCode(parts.status.as_u16())); + return Err(DiscoveryError::BadStatusCode(parts)); } Ok((