-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(confighttp): add max_redirects configuration option #10877
base: main
Are you sure you want to change the base?
Changes from 18 commits
1b19038
cc94080
26bd593
af76a71
87d151e
4ca354e
ea719b5
c502bb3
ed51bde
5001cda
a3bf503
4aefcbb
b96ecc1
e434e4b
b985772
4033e56
77008ab
18d431d
aef83ac
c98fe0a
a038f96
4ca761b
67c1704
76807a3
e6b62bb
98a73c7
c59449f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,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. | ||
|
@@ -242,12 +245,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this case needed here? if max is set to 0, will the check at line 266 return on the first call the same as is this block here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great catch! Actually, this branch is not needed and resulted in the same output when If Fixed in c98fe0a Thanks! |
||
return func(_ *http.Request, _ []*http.Request) error { | ||
return http.ErrUseLastResponse | ||
} | ||
} | ||
|
||
return func(_ *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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can even keep it
int
with10
by default. That should be cleaner. The only problem is it'll be a significant breaking change (no redirects) for a component that doesn't useNewDefaultClientConfig
, but I don't think we have any of those in contrib. Maybe we can check... All components should use that anyway. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am totally open to adding this, but I am concert about receivers initiating the config without using the default method (max_redirects will be set to 0). Also, the only difference is that maybe we should align with the Go library error message after 10 redirects:
There is this ongoing effort to use the
NewDefaultClientConfig
: open-telemetry/opentelemetry-collector-contrib#35457There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different message isn't a problem I thought. I would like it to be merged without a pointer after all exporters use the default constructor. This is the pattern we use in other configs I believe. Let me know if that's ok with you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, let's keep it pointer free :) e6b62bb
Should we wait until open-telemetry/opentelemetry-collector-contrib#35457 is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contrib tests failing due open-telemetry/opentelemetry-collector-contrib#36681