Skip to content
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

http: Add the ability to not follow redirects #278

Merged
merged 4 commits into from
Feb 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion config/http_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wheter → whether

And I'd prefer "follow HTTP 3xx redirects" over "follow the HTTP 3xx rediracts".

// 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.
Expand Down Expand Up @@ -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{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overrides anything that might be pre-populated in *c. Not sure if we ever do that, but wouldn't it be safer (and easier) to do c.FollowRedirects = true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have switched to the exact same pattern as prometheus (expose default struct)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FTR: If c is nil, then L179, as above, panics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmmmh yes indeed. but still, this is the pattern we use everywhere, let's stick to it?

FollowRedirects: true,
}
if err := unmarshal((*plain)(c)); err != nil {
return err
}
Expand All @@ -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
Expand Down
52 changes: 51 additions & 1 deletion config/http_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,46 @@ 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:
w.Header().Set("Location", "/redirected")
w.WriteHeader(http.StatusFound)
fmt.Fprintf(w, "It should follow the redirect.")
}
},
}, {
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().Set("Location", "/redirected")
w.WriteHeader(http.StatusFound)
fmt.Fprintf(w, ExpectedMessage)
}
},
},
}

Expand All @@ -317,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", validConfig.clientConfig)
t.Errorf("Can't connect to the test server using this config: %+v %v", validConfig.clientConfig, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of hard to see that there is an error following after the config. How about:

"Can't connect to the test server using config %+v: %v"

continue
}

Expand Down Expand Up @@ -932,6 +972,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 {
Expand Down