From 31bc84c91e7d6c509443f6e73bda0df32a0a5cba Mon Sep 17 00:00:00 2001
From: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
Date: Mon, 16 Oct 2023 10:56:25 +0100
Subject: [PATCH] Default connection and request timeouts of 5 seconds (#4928)

* Default connection and request timeouts of 5 seconds

* Clippy

* Allow disabling timeouts
---
 object_store/src/aws/mod.rs    |  3 +-
 object_store/src/azure/mod.rs  |  2 +-
 object_store/src/client/mod.rs | 66 ++++++++++++++++++++++++++++++++--
 object_store/src/gcp/mod.rs    |  2 +-
 4 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs
index 70170a3cf48a..3ddce08002c4 100644
--- a/object_store/src/aws/mod.rs
+++ b/object_store/src/aws/mod.rs
@@ -1130,8 +1130,7 @@ impl AmazonS3Builder {
 
             Arc::new(TokenCredentialProvider::new(
                 token,
-                // The instance metadata endpoint is access over HTTP
-                self.client_options.clone().with_allow_http(true).client()?,
+                self.client_options.metadata_client()?,
                 self.retry_config.clone(),
             )) as _
         };
diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs
index 9017634c42da..190b73bf9490 100644
--- a/object_store/src/azure/mod.rs
+++ b/object_store/src/azure/mod.rs
@@ -1070,7 +1070,7 @@ impl MicrosoftAzureBuilder {
                 );
                 Arc::new(TokenCredentialProvider::new(
                     msi_credential,
-                    self.client_options.clone().with_allow_http(true).client()?,
+                    self.client_options.metadata_client()?,
                     self.retry_config.clone(),
                 )) as _
             };
diff --git a/object_store/src/client/mod.rs b/object_store/src/client/mod.rs
index ee9d62a44f0c..137da2b37594 100644
--- a/object_store/src/client/mod.rs
+++ b/object_store/src/client/mod.rs
@@ -166,7 +166,7 @@ impl FromStr for ClientConfigKey {
 }
 
 /// HTTP client configuration for remote object stores
-#[derive(Debug, Clone, Default)]
+#[derive(Debug, Clone)]
 pub struct ClientOptions {
     user_agent: Option<ConfigValue<HeaderValue>>,
     content_type_map: HashMap<String, String>,
@@ -188,6 +188,35 @@ pub struct ClientOptions {
     http2_only: ConfigValue<bool>,
 }
 
+impl Default for ClientOptions {
+    fn default() -> Self {
+        // Defaults based on
+        // <https://docs.aws.amazon.com/sdkref/latest/guide/feature-smart-config-defaults.html>
+        // <https://docs.aws.amazon.com/whitepapers/latest/s3-optimizing-performance-best-practices/timeouts-and-retries-for-latency-sensitive-applications.html>
+        // Which recommend a connection timeout of 3.1s and a request timeout of 2s
+        Self {
+            user_agent: None,
+            content_type_map: Default::default(),
+            default_content_type: None,
+            default_headers: None,
+            proxy_url: None,
+            proxy_ca_certificate: None,
+            proxy_excludes: None,
+            allow_http: Default::default(),
+            allow_insecure: Default::default(),
+            timeout: Some(Duration::from_secs(5).into()),
+            connect_timeout: Some(Duration::from_secs(5).into()),
+            pool_idle_timeout: None,
+            pool_max_idle_per_host: None,
+            http2_keep_alive_interval: None,
+            http2_keep_alive_timeout: None,
+            http2_keep_alive_while_idle: Default::default(),
+            http1_only: Default::default(),
+            http2_only: Default::default(),
+        }
+    }
+}
+
 impl ClientOptions {
     /// Create a new [`ClientOptions`] with default values
     pub fn new() -> Self {
@@ -367,17 +396,37 @@ impl ClientOptions {
     ///
     /// The timeout is applied from when the request starts connecting until the
     /// response body has finished
+    ///
+    /// Default is 5 seconds
     pub fn with_timeout(mut self, timeout: Duration) -> Self {
         self.timeout = Some(ConfigValue::Parsed(timeout));
         self
     }
 
+    /// Disables the request timeout
+    ///
+    /// See [`Self::with_timeout`]
+    pub fn with_timeout_disabled(mut self) -> Self {
+        self.timeout = None;
+        self
+    }
+
     /// Set a timeout for only the connect phase of a Client
+    ///
+    /// Default is 5 seconds
     pub fn with_connect_timeout(mut self, timeout: Duration) -> Self {
         self.connect_timeout = Some(ConfigValue::Parsed(timeout));
         self
     }
 
+    /// Disables the connection timeout
+    ///
+    /// See [`Self::with_connect_timeout`]
+    pub fn with_connect_timeout_disabled(mut self) -> Self {
+        self.timeout = None;
+        self
+    }
+
     /// Set the pool max idle timeout
     ///
     /// This is the length of time an idle connection will be kept alive
@@ -444,7 +493,20 @@ impl ClientOptions {
         }
     }
 
-    pub(crate) fn client(&self) -> super::Result<Client> {
+    /// Create a [`Client`] with overrides optimised for metadata endpoint access
+    ///
+    /// In particular:
+    /// * Allows HTTP as metadata endpoints do not use TLS
+    /// * Configures a low connection timeout to provide quick feedback if not present
+    #[cfg(any(feature = "aws", feature = "gcp", feature = "azure"))]
+    pub(crate) fn metadata_client(&self) -> Result<Client> {
+        self.clone()
+            .with_allow_http(true)
+            .with_connect_timeout(Duration::from_secs(1))
+            .client()
+    }
+
+    pub(crate) fn client(&self) -> Result<Client> {
         let mut builder = ClientBuilder::new();
 
         match &self.user_agent {
diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs
index f80704b91765..f8a16310dd1e 100644
--- a/object_store/src/gcp/mod.rs
+++ b/object_store/src/gcp/mod.rs
@@ -1071,7 +1071,7 @@ impl GoogleCloudStorageBuilder {
         } else {
             Arc::new(TokenCredentialProvider::new(
                 InstanceCredentialProvider::new(audience),
-                self.client_options.clone().with_allow_http(true).client()?,
+                self.client_options.metadata_client()?,
                 self.retry_config.clone(),
             )) as _
         };