Skip to content

Allow to customize DialContext in HTTPClientConfig #290

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow to customize DialContext in HTTPClientConfig #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 19 additions & 4 deletions config/http_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ package config

import (
"bytes"
"context"
"crypto/sha256"
"crypto/tls"
"crypto/x509"
"fmt"
"io/ioutil"
"net"
"net/http"
"net/url"
"strings"
Expand Down Expand Up @@ -120,6 +122,9 @@ type HTTPClientConfig struct {
// The omitempty flag is not set, because it would be hidden from the
// marshalled configuration when set to false.
FollowRedirects bool `yaml:"follow_redirects"`
// DialContext allows downstream projects to customize the dialer used by the
// HTTP client to open the network connection to the server.
DialContext func(ctx context.Context, network, addr string) (net.Conn, error) `yaml:"-"`
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want a config field here that doesn't parse from (or into) Yaml.

Perhaps it's better to add another parameter to NewRoundTripperFromConfig below?

I don't have a lot of context (no pun intended) on how this is used throughout the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give you a bit more context. We're trying to customise the dialer used by HTTP client used by Alertmanager notifiers (eg. webhook). If we move it away from the config we would have to propagate back the changes across notifiers using it so that we can customize their creation in Cortex. Having the DialContext in the config would make it transparent and reduce the change surface, but if you prefer that way I can work on it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's why we need the context how the HTTPClientConfig is used across the project. My gut feeling is that it is essentially used as a definition for how the config should look like in Yaml config files. Having a field in there that doesn't render as or parse from Yaml would go against that semantics. But my gut feeling might be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Let's wait for @roidelapluie confirmation and then I can start working on refactoring the common HTTP client and alertmanager receiver integrations to be able to inject it everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative approach:
#291

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering what you need extra from the dialer, are we missing options in our http client? I also think we should indeed not adding this to this struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're trying to do this cortexproject/cortex#4085. Anyway, I proposed a different approach in #291.

}

// SetDirectory joins any relative file paths with dir.
Expand Down Expand Up @@ -219,6 +224,19 @@ func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, e
// given config.HTTPClientConfig. The name is used as go-conntrack metric label.
func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool) (http.RoundTripper, error) {
newRT := func(tlsConfig *tls.Config) (http.RoundTripper, error) {
var dialContext func(ctx context.Context, network, addr string) (net.Conn, error)

if cfg.DialContext != nil {
dialContext = conntrack.NewDialContextFunc(
conntrack.DialWithDialContextFunc(cfg.DialContext),
conntrack.DialWithTracing(),
conntrack.DialWithName(name))
} else {
dialContext = conntrack.NewDialContextFunc(
conntrack.DialWithTracing(),
conntrack.DialWithName(name))
}

// The only timeout we care about is the configured scrape timeout.
// It is applied on request. So we leave out any timings here.
var rt http.RoundTripper = &http.Transport{
Expand All @@ -233,10 +251,7 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli
IdleConnTimeout: 5 * time.Minute,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
DialContext: conntrack.NewDialContextFunc(
conntrack.DialWithTracing(),
conntrack.DialWithName(name),
),
DialContext: dialContext,
}
if enableHTTP2 {
// HTTP/2 support is golang has many problematic cornercases where
Expand Down
27 changes: 27 additions & 0 deletions config/http_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@
package config

import (
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"io/ioutil"
"net"
"net/http"
"net/http/httptest"
"os"
Expand Down Expand Up @@ -50,6 +53,7 @@ const (
MissingKey = "missing/secret.key"

ExpectedMessage = "I'm here to serve you!!!"
ExpectedError = "expected error"
AuthorizationCredentials = "theanswertothegreatquestionoflifetheuniverseandeverythingisfortytwo"
AuthorizationCredentialsFile = "testdata/bearer.token"
AuthorizationType = "APIKEY"
Expand Down Expand Up @@ -413,6 +417,29 @@ func TestNewClientFromInvalidConfig(t *testing.T) {
}
}

func TestCustomDialContext(t *testing.T) {
cfg := HTTPClientConfig{
DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
return nil, errors.New(ExpectedError)
},
}

err := cfg.Validate()
if err != nil {
t.Fatal(err.Error())
}

client, err := NewClientFromConfig(cfg, "test", false, true)
if err != nil {
t.Fatalf("Can't create a client from this config: %+v", cfg)
}

_, err = client.Get("http://localhost")
if err == nil || !strings.Contains(err.Error(), ExpectedError) {
t.Errorf("Expected error %q but got %q", ExpectedError, err)
}
}

func TestMissingBearerAuthFile(t *testing.T) {
cfg := HTTPClientConfig{
BearerTokenFile: MissingBearerTokenFile,
Expand Down