From b2d760b8b82323cae437ef8ebca33ce1854a3b6e Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Wed, 24 Feb 2021 22:57:37 +0100 Subject: [PATCH 1/4] http: Add the ability to not follow redirects Currently, when we unmarshal, we get the default value, which is to follow redirects. Signed-off-by: Julien Pivotto --- config/http_config.go | 15 +++++++++++- config/http_config_test.go | 50 +++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/config/http_config.go b/config/http_config.go index 07b389eb..b7feab74 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -111,6 +111,10 @@ type HTTPClientConfig struct { ProxyURL URL `yaml:"proxy_url,omitempty"` // TLSConfig to use to connect to the targets. TLSConfig TLSConfig `yaml:"tls_config,omitempty"` + // FollowRedirects specifies wheter the client should follow the HTTP 3xx redirects. + // The omitempty flag is not set, because it would be hidden from the + // marshalled configuration when set to false. + FollowRedirects bool `yaml:"follow_redirects"` } // SetDirectory joins any relative file paths with dir. @@ -172,6 +176,9 @@ func (c *HTTPClientConfig) Validate() error { // UnmarshalYAML implements the yaml.Unmarshaler interface func (c *HTTPClientConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { type plain HTTPClientConfig + *c = HTTPClientConfig{ + FollowRedirects: true, + } if err := unmarshal((*plain)(c)); err != nil { return err } @@ -196,7 +203,13 @@ func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, e if err != nil { return nil, err } - return newClient(rt), nil + client := newClient(rt) + if !cfg.FollowRedirects { + client.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + } + return client, nil } // NewRoundTripperFromConfig returns a new HTTP RoundTripper configured for the diff --git a/config/http_config_test.go b/config/http_config_test.go index a35ae895..9762d60a 100644 --- a/config/http_config_test.go +++ b/config/http_config_test.go @@ -296,6 +296,44 @@ func TestNewClientFromConfig(t *testing.T) { fmt.Fprint(w, ExpectedMessage) } }, + }, { + clientConfig: HTTPClientConfig{ + FollowRedirects: true, + TLSConfig: TLSConfig{ + CAFile: TLSCAChainPath, + CertFile: ClientCertificatePath, + KeyFile: ClientKeyNoPassPath, + ServerName: "", + InsecureSkipVerify: false}, + }, + handler: func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/redirected": + fmt.Fprintf(w, ExpectedMessage) + default: + http.Redirect(w, r, "/redirected", http.StatusFound) + } + }, + }, { + clientConfig: HTTPClientConfig{ + FollowRedirects: false, + TLSConfig: TLSConfig{ + CAFile: TLSCAChainPath, + CertFile: ClientCertificatePath, + KeyFile: ClientKeyNoPassPath, + ServerName: "", + InsecureSkipVerify: false}, + }, + handler: func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/redirected": + fmt.Fprint(w, "The redirection was followed.") + default: + w.Header()["Content-Type"] = nil + http.Redirect(w, r, "/redirected", http.StatusFound) + fmt.Fprint(w, ExpectedMessage) + } + }, }, } @@ -317,7 +355,7 @@ func TestNewClientFromConfig(t *testing.T) { } response, err := client.Get(testServer.URL) if err != nil { - t.Errorf("Can't connect to the test server using this config: %+v", validConfig.clientConfig) + t.Errorf("Can't connect to the test server using this config: %+v %v", validConfig.clientConfig, err) continue } @@ -932,6 +970,16 @@ func TestHideHTTPClientConfigSecrets(t *testing.T) { } } +func TestDefaultFollowRedirect(t *testing.T) { + cfg, _, err := LoadHTTPConfigFile("testdata/http.conf.good.yml") + if err != nil { + t.Errorf("Error loading HTTP client config: %v", err) + } + if !cfg.FollowRedirects { + t.Errorf("follow_redirects should be true") + } +} + func TestValidateHTTPConfig(t *testing.T) { cfg, _, err := LoadHTTPConfigFile("testdata/http.conf.good.yml") if err != nil { From 062d0d0083ae05bc049898085881dd8626bb7716 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Wed, 24 Feb 2021 23:22:21 +0100 Subject: [PATCH 2/4] Make test compatible with old go versions. Signed-off-by: Julien Pivotto --- config/http_config_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/config/http_config_test.go b/config/http_config_test.go index 9762d60a..3bd0b283 100644 --- a/config/http_config_test.go +++ b/config/http_config_test.go @@ -311,7 +311,9 @@ func TestNewClientFromConfig(t *testing.T) { case "/redirected": fmt.Fprintf(w, ExpectedMessage) default: - http.Redirect(w, r, "/redirected", http.StatusFound) + w.Header().Set("Location", "/redirected") + w.WriteHeader(http.StatusFound) + fmt.Fprintf(w, "It should follow the redirect.") } }, }, { @@ -329,9 +331,9 @@ func TestNewClientFromConfig(t *testing.T) { case "/redirected": fmt.Fprint(w, "The redirection was followed.") default: - w.Header()["Content-Type"] = nil - http.Redirect(w, r, "/redirected", http.StatusFound) - fmt.Fprint(w, ExpectedMessage) + w.Header().Set("Location", "/redirected") + w.WriteHeader(http.StatusFound) + fmt.Fprintf(w, ExpectedMessage) } }, }, From f9eb2716978544e31b52731b812e24c6aaeab6ab Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Thu, 25 Feb 2021 23:47:53 +0100 Subject: [PATCH 3/4] Reuse Prometheus/prometheus pattern Signed-off-by: Julien Pivotto --- config/http_config.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/config/http_config.go b/config/http_config.go index b7feab74..061d626f 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -33,6 +33,11 @@ import ( "gopkg.in/yaml.v2" ) +// DefaultHTTPClientConfig is the default HTTP client configuration. +var DefaultHTTPClientConfig = HTTPClientConfig{ + FollowRedirects: true, +} + type closeIdler interface { CloseIdleConnections() } @@ -111,7 +116,7 @@ type HTTPClientConfig struct { ProxyURL URL `yaml:"proxy_url,omitempty"` // TLSConfig to use to connect to the targets. TLSConfig TLSConfig `yaml:"tls_config,omitempty"` - // FollowRedirects specifies wheter the client should follow the HTTP 3xx redirects. + // FollowRedirects specifies wheter the client should follow HTTP 3xx redirects. // The omitempty flag is not set, because it would be hidden from the // marshalled configuration when set to false. FollowRedirects bool `yaml:"follow_redirects"` @@ -176,9 +181,7 @@ func (c *HTTPClientConfig) Validate() error { // UnmarshalYAML implements the yaml.Unmarshaler interface func (c *HTTPClientConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { type plain HTTPClientConfig - *c = HTTPClientConfig{ - FollowRedirects: true, - } + *c = DefaultHTTPClientConfig if err := unmarshal((*plain)(c)); err != nil { return err } From bdce8c00a6004958c6a1256329bd77a8c89a4318 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Fri, 26 Feb 2021 18:33:55 +0100 Subject: [PATCH 4/4] Address review comments Signed-off-by: Julien Pivotto --- config/http_config.go | 2 +- config/http_config_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/http_config.go b/config/http_config.go index 061d626f..af9e463b 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -116,7 +116,7 @@ type HTTPClientConfig struct { ProxyURL URL `yaml:"proxy_url,omitempty"` // TLSConfig to use to connect to the targets. TLSConfig TLSConfig `yaml:"tls_config,omitempty"` - // FollowRedirects specifies wheter the client should follow HTTP 3xx redirects. + // FollowRedirects specifies whether the client should follow HTTP 3xx redirects. // The omitempty flag is not set, because it would be hidden from the // marshalled configuration when set to false. FollowRedirects bool `yaml:"follow_redirects"` diff --git a/config/http_config_test.go b/config/http_config_test.go index 3bd0b283..c96560e4 100644 --- a/config/http_config_test.go +++ b/config/http_config_test.go @@ -357,7 +357,7 @@ func TestNewClientFromConfig(t *testing.T) { } response, err := client.Get(testServer.URL) if err != nil { - t.Errorf("Can't connect to the test server using this config: %+v %v", validConfig.clientConfig, err) + t.Errorf("Can't connect to the test server using this config: %+v: %v", validConfig.clientConfig, err) continue }