From 1b19038e3979306814e99db2e5a429f30646a0d8 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Tue, 13 Aug 2024 20:17:48 +0200 Subject: [PATCH 01/10] feat(confighttp): add max_redirects configuration option --- .chloggen/add_max_redirects_confighttp.yaml | 25 ++++++ config/confighttp/confighttp.go | 35 ++++++-- config/confighttp/confighttp_test.go | 99 +++++++++++++++++++-- 3 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 .chloggen/add_max_redirects_confighttp.yaml diff --git a/.chloggen/add_max_redirects_confighttp.yaml b/.chloggen/add_max_redirects_confighttp.yaml new file mode 100644 index 00000000000..e9fa2d108cf --- /dev/null +++ b/.chloggen/add_max_redirects_confighttp.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: 'enhancement' + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confighttp + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add the max_redirects configuration option + +# One or more tracking issues or pull requests related to the change +issues: [10870] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index a62c5d7b2f5..1868db1d513 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -31,8 +31,11 @@ import ( "go.opentelemetry.io/collector/extension/auth" ) -const headerContentEncoding = "Content-Encoding" -const defaultMaxRequestBodySize = 20 * 1024 * 1024 // 20MiB +const ( + headerContentEncoding = "Content-Encoding" + defaultMaxRequestBodySize = 20 * 1024 * 1024 // 20MiB +) + var defaultCompressionAlgorithms = []string{"", "gzip", "zstd", "zlib", "snappy", "deflate"} // ClientConfig defines settings for creating an HTTP client. @@ -102,6 +105,9 @@ type ClientConfig struct { HTTP2PingTimeout time.Duration `mapstructure:"http2_ping_timeout"` // Cookies configures the cookie management of the HTTP client. Cookies *CookiesConfig `mapstructure:"cookies"` + + // Maximum number of redirections to follow, if not defined, the Client uses its default policy, which is to stop after 10 consecutive requests. + MaxRedirects *int `mapstructure:"max_redirects"` } // CookiesConfig defines the configuration of the HTTP client regarding cookies served by the server. @@ -238,12 +244,31 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett } return &http.Client{ - Transport: clientTransport, - Timeout: hcs.Timeout, - Jar: jar, + CheckRedirect: makeCheckRedirect(hcs.MaxRedirects), + Transport: clientTransport, + Timeout: hcs.Timeout, + Jar: jar, }, nil } +// makeCheckRedirect checks if max redirects are exceeded +func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { + if max == nil { + return nil + } else if *max == 0 { + return func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + } + + return func(r *http.Request, via []*http.Request) error { + if *max == len(via) { + return http.ErrUseLastResponse + } + return nil + } +} + // Custom RoundTripper that adds headers. type headerRoundTripper struct { transport http.RoundTripper diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 9c9a6fd1695..d626a8d5b1f 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -37,8 +37,7 @@ import ( "go.opentelemetry.io/collector/internal/localhostgate" ) -type customRoundTripper struct { -} +type customRoundTripper struct{} var _ http.RoundTripper = (*customRoundTripper)(nil) @@ -219,7 +218,6 @@ func TestPartialHTTPClientSettings(t *testing.T) { assert.EqualValues(t, 0, transport.MaxConnsPerHost) assert.EqualValues(t, 90*time.Second, transport.IdleConnTimeout) assert.EqualValues(t, false, transport.DisableKeepAlives) - }) } } @@ -335,6 +333,96 @@ func TestHTTPClientSettingsError(t *testing.T) { } } +func TestMaxRedirects(t *testing.T) { + toIntPtr := func(i int) *int { + return &i + } + tests := []struct { + name string + settings ClientConfig + expectedRequests int + expectedErrStr string + }{ + { + name: "No redirects config", + settings: ClientConfig{}, + expectedRequests: 10, + // default client returns an error, custom implementations should return a ErrUseLastResponse which the internal http package will skip it to let the users select the previous response without closing the body. + expectedErrStr: "stopped after 10 redirects", + }, + { + name: "Zero redirects", + settings: ClientConfig{MaxRedirects: toIntPtr(0)}, + expectedRequests: 1, + }, + { + name: "Defined max redirects", + settings: ClientConfig{MaxRedirects: toIntPtr(5)}, + expectedRequests: 5, + }, + } + + host := &mockHost{ + ext: map[component.ID]component.Component{}, + } + + countRequests := func(resp *http.Response) int { + counter := 0 + for resp != nil { + resp = resp.Request.Response + counter += 1 + } + return counter + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + client, err := test.settings.ToClient(context.Background(), host, componenttest.NewNopTelemetrySettings()) + require.NoError(t, err) + + hss := &ServerConfig{ + Endpoint: "localhost:0", + } + + ln, err := hss.ToListener(context.Background()) + require.NoError(t, err) + + url := fmt.Sprintf("http://%s", ln.Addr().String()) + + // always return a redirection to itself + s, err := hss.ToServer( + context.Background(), + componenttest.NewNopHost(), + componenttest.NewNopTelemetrySettings(), + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, url, http.StatusMovedPermanently) + })) + require.NoError(t, err) + + go func() { + _ = s.Serve(ln) + }() + + req, err := http.NewRequest(http.MethodOptions, url, nil) + require.NoError(t, err) + resp, err := client.Do(req) + + if test.expectedErrStr != "" { + assert.ErrorContains(t, err, test.expectedErrStr) + } else { + assert.NoError(t, err) + } + assert.Equal(t, http.StatusMovedPermanently, resp.StatusCode) + + defer resp.Body.Close() + + require.Equal(t, test.expectedRequests, countRequests(resp)) + + require.NoError(t, s.Close()) + }) + } +} + func TestHTTPClientSettingWithAuthConfig(t *testing.T) { tests := []struct { name string @@ -430,7 +518,8 @@ func TestHTTPClientSettingWithAuthConfig(t *testing.T) { host: &mockHost{ ext: map[component.ID]component.Component{ mockID: &authtest.MockClient{ - ResultRoundTripper: &customRoundTripper{}, MustError: true}, + ResultRoundTripper: &customRoundTripper{}, MustError: true, + }, }, }, }, @@ -560,7 +649,6 @@ func TestHTTPServerWarning(t *testing.T) { require.Len(t, observed.FilterLevelExact(zap.WarnLevel).All(), test.len) }) } - } func TestHttpReception(t *testing.T) { @@ -1307,7 +1395,6 @@ func TestServerWithDecoder(t *testing.T) { srv.Handler.ServeHTTP(response, req) // verify assert.Equal(t, response.Result().StatusCode, http.StatusOK) - } func TestServerWithDecompression(t *testing.T) { From cc94080838e7423e10e33d38be4d4dce419a4c57 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Tue, 13 Aug 2024 20:57:53 +0200 Subject: [PATCH 02/10] chore: fix linter --- config/confighttp/confighttp.go | 2 +- config/confighttp/confighttp_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 1868db1d513..ab24ea413eb 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -261,7 +261,7 @@ func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { } } - return func(r *http.Request, via []*http.Request) error { + return func(_ *http.Request, via []*http.Request) error { if *max == len(via) { return http.ErrUseLastResponse } diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index d626a8d5b1f..91e2be4960f 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -370,7 +370,7 @@ func TestMaxRedirects(t *testing.T) { counter := 0 for resp != nil { resp = resp.Request.Response - counter += 1 + counter++ } return counter } From 26bd5939717bc483f02229319cdf1158863fd524 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Wed, 14 Aug 2024 11:38:20 +0200 Subject: [PATCH 03/10] rollback auto formatting changes --- config/confighttp/confighttp.go | 7 ++----- config/confighttp/confighttp_test.go | 9 ++++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index ab24ea413eb..06b47aa499b 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -31,11 +31,8 @@ import ( "go.opentelemetry.io/collector/extension/auth" ) -const ( - headerContentEncoding = "Content-Encoding" - defaultMaxRequestBodySize = 20 * 1024 * 1024 // 20MiB -) - +const headerContentEncoding = "Content-Encoding" +const defaultMaxRequestBodySize = 20 * 1024 * 1024 // 20MiB var defaultCompressionAlgorithms = []string{"", "gzip", "zstd", "zlib", "snappy", "deflate"} // ClientConfig defines settings for creating an HTTP client. diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 91e2be4960f..bf451a7869c 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -37,7 +37,8 @@ import ( "go.opentelemetry.io/collector/internal/localhostgate" ) -type customRoundTripper struct{} +type customRoundTripper struct{ +} var _ http.RoundTripper = (*customRoundTripper)(nil) @@ -218,6 +219,7 @@ func TestPartialHTTPClientSettings(t *testing.T) { assert.EqualValues(t, 0, transport.MaxConnsPerHost) assert.EqualValues(t, 90*time.Second, transport.IdleConnTimeout) assert.EqualValues(t, false, transport.DisableKeepAlives) + }) } } @@ -518,8 +520,7 @@ func TestHTTPClientSettingWithAuthConfig(t *testing.T) { host: &mockHost{ ext: map[component.ID]component.Component{ mockID: &authtest.MockClient{ - ResultRoundTripper: &customRoundTripper{}, MustError: true, - }, + ResultRoundTripper: &customRoundTripper{}, MustError: true}, }, }, }, @@ -649,6 +650,7 @@ func TestHTTPServerWarning(t *testing.T) { require.Len(t, observed.FilterLevelExact(zap.WarnLevel).All(), test.len) }) } + } func TestHttpReception(t *testing.T) { @@ -1395,6 +1397,7 @@ func TestServerWithDecoder(t *testing.T) { srv.Handler.ServeHTTP(response, req) // verify assert.Equal(t, response.Result().StatusCode, http.StatusOK) + } func TestServerWithDecompression(t *testing.T) { From af76a716adf42cf4afbfe38c53ae6cdf5be0c57f Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Wed, 14 Aug 2024 11:44:24 +0200 Subject: [PATCH 04/10] use componenttest for nop testint host --- config/confighttp/confighttp_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index bf451a7869c..96b20392a66 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -364,10 +364,6 @@ func TestMaxRedirects(t *testing.T) { }, } - host := &mockHost{ - ext: map[component.ID]component.Component{}, - } - countRequests := func(resp *http.Response) int { counter := 0 for resp != nil { @@ -379,7 +375,7 @@ func TestMaxRedirects(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - client, err := test.settings.ToClient(context.Background(), host, componenttest.NewNopTelemetrySettings()) + client, err := test.settings.ToClient(context.Background(), componenttest.NewNopHost(), componenttest.NewNopTelemetrySettings()) require.NoError(t, err) hss := &ServerConfig{ From 87d151ef139e52f1022cf67ae0c946b9db3d8350 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Wed, 14 Aug 2024 11:50:33 +0200 Subject: [PATCH 05/10] docs: add max_redirects confighttp option --- config/confighttp/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/confighttp/README.md b/config/confighttp/README.md index d72a3805953..a302f7c9b2e 100644 --- a/config/confighttp/README.md +++ b/config/confighttp/README.md @@ -36,6 +36,7 @@ README](../configtls/README.md). - [`http2_ping_timeout`](https://pkg.go.dev/golang.org/x/net/http2#Transport) - [`cookies`](https://pkg.go.dev/net/http#CookieJar) - [`enabled`] if enabled, the client will store cookies from server responses and reuse them in subsequent requests. +- `max_redirects`: Maximum number of redirections to follow, if not defined, the [Client uses its default policy](https://pkg.go.dev/net/http#Client), which is to stop after 10 consecutive requests. Example: @@ -55,6 +56,7 @@ exporter: compression: zstd cookies: enabled: true + max_redirects: 5 ``` ## Server Configuration From ea719b5622629481ab65f769ae8d4b9b32346ff5 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Wed, 14 Aug 2024 16:24:36 +0200 Subject: [PATCH 06/10] chore: rollback removed space --- config/confighttp/confighttp_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 96b20392a66..ac01078d555 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -37,7 +37,7 @@ import ( "go.opentelemetry.io/collector/internal/localhostgate" ) -type customRoundTripper struct{ +type customRoundTripper struct { } var _ http.RoundTripper = (*customRoundTripper)(nil) From 18d431df9524e6803a8315a9a0f32b9ed7c6f55c Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Thu, 19 Sep 2024 15:25:34 +0200 Subject: [PATCH 07/10] fix: use require for error test cases --- config/confighttp/confighttp_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index f896656ac14..3dd2a6f7af3 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -409,9 +409,9 @@ func TestMaxRedirects(t *testing.T) { resp, err := client.Do(req) if test.expectedErrStr != "" { - assert.ErrorContains(t, err, test.expectedErrStr) + require.ErrorContains(t, err, test.expectedErrStr) } else { - assert.NoError(t, err) + require.NoError(t, err) } assert.Equal(t, http.StatusMovedPermanently, resp.StatusCode) From c98fe0a5fcffcc1994e58db23c5077e02cb2997c Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Sun, 22 Sep 2024 17:39:03 +0200 Subject: [PATCH 08/10] fix: match max redirects with total requests --- config/confighttp/confighttp.go | 6 +----- config/confighttp/confighttp_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 5956bd954a2..7f8c7739976 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -256,14 +256,10 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { if max == nil { return nil - } else if *max == 0 { - return func(_ *http.Request, _ []*http.Request) error { - return http.ErrUseLastResponse - } } return func(_ *http.Request, via []*http.Request) error { - if *max == len(via) { + if *max == (len(via) - 1) { return http.ErrUseLastResponse } return nil diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 3dd2a6f7af3..26f1c4058cf 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -360,10 +360,15 @@ func TestMaxRedirects(t *testing.T) { settings: ClientConfig{MaxRedirects: toIntPtr(0)}, expectedRequests: 1, }, + { + name: "One redirect", + settings: ClientConfig{MaxRedirects: toIntPtr(1)}, + expectedRequests: 2, + }, { name: "Defined max redirects", settings: ClientConfig{MaxRedirects: toIntPtr(5)}, - expectedRequests: 5, + expectedRequests: 6, }, } From 4ca761bdebb0e97b56942d0595532b72e9111351 Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Tue, 8 Oct 2024 17:57:25 +0200 Subject: [PATCH 09/10] Update config/confighttp/confighttp.go Co-authored-by: Dmitrii Anoshin --- config/confighttp/confighttp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 7f8c7739976..adea99af324 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -259,7 +259,7 @@ func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { } return func(_ *http.Request, via []*http.Request) error { - if *max == (len(via) - 1) { + if len(via) > *max { return http.ErrUseLastResponse } return nil From e6b62bb678915f56f5b8c9ee1512c3d08757808e Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Wed, 4 Dec 2024 18:25:12 +0100 Subject: [PATCH 10/10] feat: remove pointer usage --- config/confighttp/confighttp.go | 13 +++++-------- config/confighttp/confighttp_test.go | 18 +++++++++--------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index f19e664ae9a..039d6a4f5c7 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -107,7 +107,7 @@ type ClientConfig struct { Cookies *CookiesConfig `mapstructure:"cookies"` // Maximum number of redirections to follow, if not defined, the Client uses its default policy, which is to stop after 10 consecutive requests. - MaxRedirects *int `mapstructure:"max_redirects"` + MaxRedirects int `mapstructure:"max_redirects"` } // CookiesConfig defines the configuration of the HTTP client regarding cookies served by the server. @@ -132,6 +132,7 @@ func NewDefaultClientConfig() ClientConfig { MaxIdleConnsPerHost: &defaultTransport.MaxIdleConnsPerHost, MaxConnsPerHost: &defaultTransport.MaxConnsPerHost, IdleConnTimeout: &defaultTransport.IdleConnTimeout, + MaxRedirects: 10, } } @@ -253,14 +254,10 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett } // makeCheckRedirect checks if max redirects are exceeded -func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { - if max == nil { - return nil - } - +func makeCheckRedirect(max int) func(*http.Request, []*http.Request) error { return func(_ *http.Request, via []*http.Request) error { - if len(via) > *max { - return http.ErrUseLastResponse + if len(via) > max { + return fmt.Errorf("stopped after %d redirects", max) } return nil } diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 78da2847747..07ae2e5640e 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -337,9 +337,6 @@ func TestHTTPClientSettingsError(t *testing.T) { } func TestMaxRedirects(t *testing.T) { - toIntPtr := func(i int) *int { - return &i - } tests := []struct { name string settings ClientConfig @@ -347,26 +344,29 @@ func TestMaxRedirects(t *testing.T) { expectedErrStr string }{ { - name: "No redirects config", - settings: ClientConfig{}, - expectedRequests: 10, + name: "No redirects config (Default Go max redirects)", + settings: NewDefaultClientConfig(), + expectedRequests: 11, // default client returns an error, custom implementations should return a ErrUseLastResponse which the internal http package will skip it to let the users select the previous response without closing the body. expectedErrStr: "stopped after 10 redirects", }, { name: "Zero redirects", - settings: ClientConfig{MaxRedirects: toIntPtr(0)}, + settings: ClientConfig{MaxRedirects: 0}, expectedRequests: 1, + expectedErrStr: "stopped after 0 redirects", }, { name: "One redirect", - settings: ClientConfig{MaxRedirects: toIntPtr(1)}, + settings: ClientConfig{MaxRedirects: 1}, expectedRequests: 2, + expectedErrStr: "stopped after 1 redirects", }, { name: "Defined max redirects", - settings: ClientConfig{MaxRedirects: toIntPtr(5)}, + settings: ClientConfig{MaxRedirects: 5}, expectedRequests: 6, + expectedErrStr: "stopped after 5 redirects", }, }