objstore: Increase the response header timeout in the S3 provider to …

…2 minutes and make it configurable (#1094)

* Increase the response header timeout

* Format

* Make response header timeout configurable

* Test default values for the HTTPConfig struct

* Document new feature in the changelog

* Add explanation for new option and refactor existing documentation

* Run make docs

* Typo

* Remove redundant blank line

CHANGELOG.md

@@ -11,6 +11,10 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 
 ## Unreleased
 
+### Added
+
+- [#1094](https://github.com/improbable-eng/thanos/pull/1094) Allow configuring the response header timeout for the S3 client.
+
 ### Changed
 
 - [#1066](https://github.com/improbable-eng/thanos/pull/1066) Upgrade Thanos ui to Prometheus v2.9.1.

docs/storage.md

@@ -39,9 +39,9 @@ At that point, anyone can use your provider by spec
 
 ## AWS S3 configuration
 
-Thanos uses minio client to upload Prometheus data into AWS S3.
+Thanos uses the [minio client](https://github.com/minio/minio-go) library to upload Prometheus data into AWS S3.
 
-To configure S3 bucket as an object store you need to set these mandatory S3 variables in yaml format stored in a file:
+You can configure an S3 bucket as an object store with YAML, either by passing the configuration directly to the `--objstore.config` parameter, or (preferably) by passing the path to a configuration file to the `--objstore.config-file` option.
 
 [embedmd]:# (flags/config_s3.txt yaml)
 ```yaml
@@ -58,20 +58,28 @@ config:
   put_user_metadata: {}
   http_config:
     idle_conn_timeout: 0s
+    response_header_timeout: 0s
     insecure_skip_verify: false
   trace:
     enable: false
 ```
 
-The attribute `region` is optional. AWS region to endpoint mapping can be found in this [link](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region).
+At a minimum, you will need to provide a value for the `bucket`, `endpoint`, `access_key`, and `secret_key` keys. The rest of the keys are optional.
 
-Make sure you use a correct signature version.
-Currently AWS require signature v4, so it needs `signature-version2: false`, otherwise, you will get Access Denied error, but several other S3 compatible use `signature-version2: true`
+The AWS region to endpoint mapping can be found in this [link](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region).
+
+Make sure you use a correct signature version. Currently AWS requires signature v4, so it needs `signature-version2: false`. If you don't specify it, you will get an `Access Denied` error. On the other hand, several S3 compatible APIs use `signature-version2: true`.
+
+You can configure the timeout settings for the HTTP client by setting the `http_config.idle_conn_timeout` and `http_config.response_header_timeout` keys. As a rule of thumb, if you are seeing errors like `timeout awaiting response headers` in your logs, you may want to increase the value of `http_config.response_header_timeout`.
+
+Please refer to the documentation of [the Transport type](https://golang.org/pkg/net/http/#Transport) in the `net/http` package for detailed information on what each option does.
 
 For debug and testing purposes you can set
 
 * `insecure: true` to switch to plain insecure HTTP instead of HTTPS
+
 * `http_config.insecure_skip_verify: true` to disable TLS certificate verification (if your S3 based storage is using a self-signed certificate, for example)
+
 * `trace.enable: true` to enable the minio client's verbose logging. Each request and response will be logged into the debug logger, so debug level logging must be enabled for this functionality.
 
 ### Credentials

pkg/objstore/s3/s3.go

@@ -53,8 +53,9 @@ type TraceConfig struct {
 
 // HTTPConfig stores the http.Transport configuration for the s3 minio client.
 type HTTPConfig struct {
-	IdleConnTimeout    model.Duration `yaml:"idle_conn_timeout"`
-	InsecureSkipVerify bool           `yaml:"insecure_skip_verify"`
+	IdleConnTimeout       model.Duration `yaml:"idle_conn_timeout"`
+	ResponseHeaderTimeout model.Duration `yaml:"response_header_timeout"`
+	InsecureSkipVerify    bool           `yaml:"insecure_skip_verify"`
 }
 
 // Bucket implements the store.Bucket interface against s3-compatible APIs.
@@ -68,7 +69,10 @@ type Bucket struct {
 
 // parseConfig unmarshals a buffer into a Config with default HTTPConfig values.
 func parseConfig(conf []byte) (Config, error) {
-	defaultHTTPConfig := HTTPConfig{IdleConnTimeout: model.Duration(90 * time.Second)}
+	defaultHTTPConfig := HTTPConfig{
+		IdleConnTimeout:       model.Duration(90 * time.Second),
+		ResponseHeaderTimeout: model.Duration(2 * time.Minute),
+	}
 	config := Config{HTTPConfig: defaultHTTPConfig}
 	if err := yaml.Unmarshal(conf, &config); err != nil {
 		return Config{}, err
@@ -139,10 +143,11 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
 		IdleConnTimeout:       time.Duration(config.HTTPConfig.IdleConnTimeout),
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 1 * time.Second,
-		// The ResponseHeaderTimeout here is the only change from the
-		// default minio transport, it was introduced to cover cases
-		// where the tcp connection works but the server never answers
-		ResponseHeaderTimeout: 15 * time.Second,
+		// The ResponseHeaderTimeout here is the only change
+		// from the default minio transport, it was introduced
+		// to cover cases where the tcp connection works but
+		// the server never answers. Defaults to 2 minutes.
+		ResponseHeaderTimeout: time.Duration(config.HTTPConfig.ResponseHeaderTimeout),
 		// Set this value so that the underlying transport round-tripper
 		// doesn't try to auto decode the body of objects with
 		// content-encoding set to `gzip`.

pkg/objstore/s3/s3_test.go

@@ -7,12 +7,48 @@ import (
 	"github.com/improbable-eng/thanos/pkg/testutil"
 )
 
-func TestParseConfig_DefaultHTTPOpts(t *testing.T) {
+func TestParseConfig(t *testing.T) {
+	input := []byte(`bucket: abcd
+insecure: false`)
+	cfg, err := parseConfig(input)
+	testutil.Ok(t, err)
+
+	if cfg.Bucket != "abcd" {
+		t.Errorf("parsing of bucket failed: got %v, expected %v", cfg.Bucket, "abcd")
+	}
+	if cfg.Insecure {
+		t.Errorf("parsing of insecure failed: got %v, expected %v", cfg.Insecure, false)
+	}
+}
+
+func TestParseConfig_DefaultHTTPConfig(t *testing.T) {
+	input := []byte(`bucket: abcd
+insecure: false`)
+	cfg, err := parseConfig(input)
+	testutil.Ok(t, err)
+
+	if time.Duration(cfg.HTTPConfig.IdleConnTimeout) != time.Duration(90*time.Second) {
+		t.Errorf("parsing of idle_conn_timeout failed: got %v, expected %v",
+			time.Duration(cfg.HTTPConfig.IdleConnTimeout), time.Duration(90*time.Second))
+	}
+
+	if time.Duration(cfg.HTTPConfig.ResponseHeaderTimeout) != time.Duration(2*time.Minute) {
+		t.Errorf("parsing of response_header_timeout failed: got %v, expected %v",
+			time.Duration(cfg.HTTPConfig.IdleConnTimeout), time.Duration(2*time.Minute))
+	}
+
+	if cfg.HTTPConfig.InsecureSkipVerify {
+		t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", cfg.HTTPConfig.InsecureSkipVerify, false)
+	}
+}
+
+func TestParseConfig_CustomHTTPConfig(t *testing.T) {
 	input := []byte(`bucket: abcd
 insecure: false
 http_config:
   insecure_skip_verify: true
-  idle_conn_timeout: 50s`)
+  idle_conn_timeout: 50s
+  response_header_timeout: 1m`)
 	cfg, err := parseConfig(input)
 	testutil.Ok(t, err)
 
@@ -21,12 +57,11 @@ http_config:
 			time.Duration(cfg.HTTPConfig.IdleConnTimeout), time.Duration(50*time.Second))
 	}
 
-	if cfg.Bucket != "abcd" {
-		t.Errorf("parsing of bucket failed: got %v, expected %v", cfg.Bucket, "abcd")
-	}
-	if cfg.Insecure {
-		t.Errorf("parsing of insecure failed: got %v, expected %v", cfg.Insecure, false)
+	if time.Duration(cfg.HTTPConfig.ResponseHeaderTimeout) != time.Duration(1*time.Minute) {
+		t.Errorf("parsing of response_header_timeout failed: got %v, expected %v",
+			time.Duration(cfg.HTTPConfig.IdleConnTimeout), time.Duration(1*time.Minute))
 	}
+
 	if !cfg.HTTPConfig.InsecureSkipVerify {
 		t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", cfg.HTTPConfig.InsecureSkipVerify, false)
 	}