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

Refactor DefaultTransport() from objstore to package exthttp #5447

Merged
merged 22 commits into from
Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ab05957
Refactoring the DefaultTransport func in package exthttp
SrushtiSapkale Jun 24, 2022
5e6cb84
Refactoring the DefaultTransport func from s3 in package exthttp
SrushtiSapkale Jun 24, 2022
55a6464
Merge branch 'branch_refactoring_issue' of https://github.com/Srushti…
SrushtiSapkale Jun 24, 2022
b98372c
Updated helpers.go
SrushtiSapkale Jun 27, 2022
9765391
Changed the argument type in getContainerURL
SrushtiSapkale Jun 28, 2022
55fd8bc
Update pkg/exthttp/transport.go
SrushtiSapkale Jun 28, 2022
7f5d8be
Update pkg/exthttp/transport.go
SrushtiSapkale Jun 28, 2022
b81eeac
Removed the use of NewTransport() in cos.go
SrushtiSapkale Jun 28, 2022
141f32e
Moved TLSConfig struct and functions that need it from objstore to ex…
SrushtiSapkale Jun 28, 2022
8b9377f
Changed s3.go
SrushtiSapkale Jun 29, 2022
df1c015
Kept s3.go and helpers.go unchanged to not break the cortex deps
SrushtiSapkale Jun 29, 2022
324a272
Merge branch 'thanos-io:main' into branch_refactoring_issue
SrushtiSapkale Jun 29, 2022
22169f7
Consistency changed made while pair++ programming.
bwplotka Jun 30, 2022
cd81174
Created a new tlsconfig in exthttp and minor changes in cos.go
SrushtiSapkale Jun 30, 2022
79460cb
Commented in s3.go
SrushtiSapkale Jun 30, 2022
8789947
Minor changes in transport.go
SrushtiSapkale Jun 30, 2022
7e949eb
Merge branch 'thanos-io:main' into branch_refactoring_issue
SrushtiSapkale Jun 30, 2022
195925f
Changed transport.go
SrushtiSapkale Jul 1, 2022
be8f345
Merge branch 'branch_refactoring_issue' of https://github.com/Srushti…
SrushtiSapkale Jul 1, 2022
d66d816
Changed transport.go and tlsconfig.go
SrushtiSapkale Jul 4, 2022
04cb13f
Removed changes from prometheus.mod and prometheus.sum
SrushtiSapkale Jul 4, 2022
a95172b
Minor updation in cos.go
SrushtiSapkale Jul 4, 2022
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
62 changes: 62 additions & 0 deletions pkg/exthttp/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,28 @@ import (
"net"
"net/http"
"time"

"github.com/prometheus/common/model"
"github.com/thanos-io/thanos/pkg/objstore"
)

type HTTPConfig struct {
IdleConnTimeout model.Duration `yaml:"idle_conn_timeout"`
ResponseHeaderTimeout model.Duration `yaml:"response_header_timeout"`
InsecureSkipVerify bool `yaml:"insecure_skip_verify"`

TLSHandshakeTimeout model.Duration `yaml:"tls_handshake_timeout"`
ExpectContinueTimeout model.Duration `yaml:"expect_continue_timeout"`
MaxIdleConns int `yaml:"max_idle_conns"`
MaxIdleConnsPerHost int `yaml:"max_idle_conns_per_host"`
MaxConnsPerHost int `yaml:"max_conns_per_host"`

// Allow upstream callers to inject a round tripper
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Allow upstream callers to inject a round tripper
// Transport field allows upstream callers to inject a custom round tripper.

See https://thanos.io/tip/contributing/coding-style-guide.md/#comments-should-be-full-sentences

Transport http.RoundTripper `yaml:"-"`

TLSConfig objstore.TLSConfig `yaml:"tls_config"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we port this TLSConfig type to exthttp as well? It is better to keep packages as independent as possible. This allows us to avoid circular dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to move TLSConfig struct from objstore to exthttp, but turns out there are functions depending on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in tlsconfig.go

}

// NewTransport creates a new http.Transport with default settings.
func NewTransport() *http.Transport {
return &http.Transport{
Expand All @@ -25,3 +45,45 @@ func NewTransport() *http.Transport {
ExpectContinueTimeout: 1 * time.Second,
}
}

// DefaultTransport - this default transport is based on the Minio
// DefaultTransport up until the following commit:
// https://github.com/minio/minio-go/commit/008c7aa71fc17e11bf980c209a4f8c4d687fc884
// The values have since diverged.
func DefaultTransport(config HTTPConfig) (*http.Transport, error) {
bwplotka marked this conversation as resolved.
Show resolved Hide resolved
tlsConfig, err := objstore.NewTLSConfig(&config.TLSConfig)
if err != nil {
return nil, err
}

if config.InsecureSkipVerify {
tlsConfig.InsecureSkipVerify = true
}
SrushtiSapkale marked this conversation as resolved.
Show resolved Hide resolved

return &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
DualStack: true,
}).DialContext,

MaxIdleConns: config.MaxIdleConns,
MaxIdleConnsPerHost: config.MaxIdleConnsPerHost,
IdleConnTimeout: time.Duration(config.IdleConnTimeout),
MaxConnsPerHost: config.MaxConnsPerHost,
TLSHandshakeTimeout: time.Duration(config.TLSHandshakeTimeout),
ExpectContinueTimeout: time.Duration(config.ExpectContinueTimeout),
// A custom ResponseHeaderTimeout was introduced
// to cover cases where the tcp connection works but
// the server never answers. Defaults to 2 minutes.
ResponseHeaderTimeout: time.Duration(config.ResponseHeaderTimeout),
// Set this value so that the underlying transport round-tripper
// doesn't try to auto decode the body of objects with
// content-encoding set to `gzip`.
//
// Refer: https://golang.org/src/net/http/transport.go?h=roundTrip#L1843.
DisableCompression: true,
TLSClientConfig: tlsConfig,
}, nil
}
3 changes: 2 additions & 1 deletion pkg/objstore/azure/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"
"time"

"github.com/thanos-io/thanos/pkg/exthttp"
"github.com/thanos-io/thanos/pkg/testutil"
)

Expand Down Expand Up @@ -235,7 +236,7 @@ http_config:
`)
cfg, err := parseConfig(input)
testutil.Ok(t, err)
transport, err := DefaultTransport(cfg)
transport, err := exthttp.DefaultTransport(cfg)
SrushtiSapkale marked this conversation as resolved.
Show resolved Hide resolved
testutil.Ok(t, err)
testutil.Equals(t, true, transport.TLSClientConfig.InsecureSkipVerify)
}
38 changes: 5 additions & 33 deletions pkg/objstore/azure/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ package azure
import (
"context"
"fmt"
"net"

//"net"
"net/http"
"net/url"
"regexp"
Expand All @@ -18,7 +19,8 @@ import (
"github.com/Azure/go-autorest/autorest/azure/auth"
"github.com/go-kit/log"
"github.com/go-kit/log/level"
"github.com/thanos-io/thanos/pkg/objstore"
"github.com/thanos-io/thanos/pkg/exthttp"
//"github.com/thanos-io/thanos/pkg/objstore"
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid commenting code in a PR (: It's easy to merge such and create a bit of mess 🙈

)

// DirDelim is the delimiter used to model a directory structure in an object store bucket.
Expand Down Expand Up @@ -104,7 +106,7 @@ func getContainerURL(ctx context.Context, logger log.Logger, conf Config) (blob.
retryOptions.TryTimeout = time.Until(deadline)
}

dt, err := DefaultTransport(conf)
dt, err := exthttp.DefaultTransport(conf.HTTPConfig)
SrushtiSapkale marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return blob.ContainerURL{}, err
}
Expand Down Expand Up @@ -140,36 +142,6 @@ func getContainerURL(ctx context.Context, logger log.Logger, conf Config) (blob.
return service.NewContainerURL(conf.ContainerName), nil
}

func DefaultTransport(config Config) (*http.Transport, error) {
tlsConfig, err := objstore.NewTLSConfig(&config.HTTPConfig.TLSConfig)
if err != nil {
return nil, err
}

if config.HTTPConfig.InsecureSkipVerify {
tlsConfig.InsecureSkipVerify = true
}
return &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
DualStack: true,
}).DialContext,

MaxIdleConns: config.HTTPConfig.MaxIdleConns,
MaxIdleConnsPerHost: config.HTTPConfig.MaxIdleConnsPerHost,
IdleConnTimeout: time.Duration(config.HTTPConfig.IdleConnTimeout),
MaxConnsPerHost: config.HTTPConfig.MaxConnsPerHost,
TLSHandshakeTimeout: time.Duration(config.HTTPConfig.TLSHandshakeTimeout),
ExpectContinueTimeout: time.Duration(config.HTTPConfig.ExpectContinueTimeout),

ResponseHeaderTimeout: time.Duration(config.HTTPConfig.ResponseHeaderTimeout),
DisableCompression: config.HTTPConfig.DisableCompression,
TLSClientConfig: tlsConfig,
}, nil
}

func getContainer(ctx context.Context, logger log.Logger, conf Config) (blob.ContainerURL, error) {
c, err := getContainerURL(ctx, logger, conf)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/objstore/cos/cos.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ type HTTPConfig struct {
MaxConnsPerHost int `yaml:"max_conns_per_host"`
}

// DefaultTransport build http.Transport from config.
func DefaultTransport(c HTTPConfig) *http.Transport {
transport := exthttp.NewTransport()
transport.IdleConnTimeout = time.Duration(c.IdleConnTimeout)
Expand Down Expand Up @@ -149,6 +148,7 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
bucketURL = cos.NewBucketURL(fmt.Sprintf("%s-%s", config.Bucket, config.AppId), config.Region, true)
}
b := &cos.BaseURL{BucketURL: bucketURL}

client := cos.NewClient(b, &http.Client{
Transport: &cos.AuthorizationTransport{
SecretID: config.SecretId,
Expand Down
91 changes: 16 additions & 75 deletions pkg/objstore/s3/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"fmt"
"io"
"io/ioutil"
"net"
"net/http"
"os"
"runtime"
Expand All @@ -28,7 +27,9 @@ import (
"github.com/prometheus/common/version"
"gopkg.in/yaml.v2"

"github.com/thanos-io/thanos/pkg/exthttp"
"github.com/thanos-io/thanos/pkg/objstore"

"github.com/thanos-io/thanos/pkg/runutil"
)

Expand Down Expand Up @@ -57,7 +58,7 @@ const (

var DefaultConfig = Config{
PutUserMetadata: map[string]string{},
HTTPConfig: HTTPConfig{
HTTPConfig: exthttp.HTTPConfig{
IdleConnTimeout: model.Duration(90 * time.Second),
ResponseHeaderTimeout: model.Duration(2 * time.Minute),
TLSHandshakeTimeout: model.Duration(10 * time.Second),
Expand All @@ -71,18 +72,18 @@ var DefaultConfig = Config{

// Config stores the configuration for s3 bucket.
type Config struct {
Bucket string `yaml:"bucket"`
Endpoint string `yaml:"endpoint"`
Region string `yaml:"region"`
AWSSDKAuth bool `yaml:"aws_sdk_auth"`
AccessKey string `yaml:"access_key"`
Insecure bool `yaml:"insecure"`
SignatureV2 bool `yaml:"signature_version2"`
SecretKey string `yaml:"secret_key"`
PutUserMetadata map[string]string `yaml:"put_user_metadata"`
HTTPConfig HTTPConfig `yaml:"http_config"`
TraceConfig TraceConfig `yaml:"trace"`
ListObjectsVersion string `yaml:"list_objects_version"`
Bucket string `yaml:"bucket"`
Endpoint string `yaml:"endpoint"`
Region string `yaml:"region"`
AWSSDKAuth bool `yaml:"aws_sdk_auth"`
AccessKey string `yaml:"access_key"`
Insecure bool `yaml:"insecure"`
SignatureV2 bool `yaml:"signature_version2"`
SecretKey string `yaml:"secret_key"`
PutUserMetadata map[string]string `yaml:"put_user_metadata"`
HTTPConfig exthttp.HTTPConfig `yaml:"http_config"`
TraceConfig TraceConfig `yaml:"trace"`
ListObjectsVersion string `yaml:"list_objects_version"`
// PartSize used for multipart upload. Only used if uploaded object size is known and larger than configured PartSize.
// NOTE we need to make sure this number does not produce more parts than 10 000.
PartSize uint64 `yaml:"part_size"`
Expand All @@ -103,66 +104,6 @@ type TraceConfig struct {
Enable bool `yaml:"enable"`
}

// HTTPConfig stores the http.Transport configuration for the s3 minio client.
type HTTPConfig struct {
IdleConnTimeout model.Duration `yaml:"idle_conn_timeout"`
ResponseHeaderTimeout model.Duration `yaml:"response_header_timeout"`
InsecureSkipVerify bool `yaml:"insecure_skip_verify"`

TLSHandshakeTimeout model.Duration `yaml:"tls_handshake_timeout"`
ExpectContinueTimeout model.Duration `yaml:"expect_continue_timeout"`
MaxIdleConns int `yaml:"max_idle_conns"`
MaxIdleConnsPerHost int `yaml:"max_idle_conns_per_host"`
MaxConnsPerHost int `yaml:"max_conns_per_host"`

// Allow upstream callers to inject a round tripper
Transport http.RoundTripper `yaml:"-"`

TLSConfig objstore.TLSConfig `yaml:"tls_config"`
}

// DefaultTransport - this default transport is based on the Minio
// DefaultTransport up until the following commit:
// https://github.com/minio/minio-go/commit/008c7aa71fc17e11bf980c209a4f8c4d687fc884
// The values have since diverged.
func DefaultTransport(config Config) (*http.Transport, error) {
tlsConfig, err := objstore.NewTLSConfig(&config.HTTPConfig.TLSConfig)
if err != nil {
return nil, err
}

if config.HTTPConfig.InsecureSkipVerify {
tlsConfig.InsecureSkipVerify = true
}

return &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
DualStack: true,
}).DialContext,

MaxIdleConns: config.HTTPConfig.MaxIdleConns,
MaxIdleConnsPerHost: config.HTTPConfig.MaxIdleConnsPerHost,
IdleConnTimeout: time.Duration(config.HTTPConfig.IdleConnTimeout),
MaxConnsPerHost: config.HTTPConfig.MaxConnsPerHost,
TLSHandshakeTimeout: time.Duration(config.HTTPConfig.TLSHandshakeTimeout),
ExpectContinueTimeout: time.Duration(config.HTTPConfig.ExpectContinueTimeout),
// A custom ResponseHeaderTimeout was introduced
// to cover cases where the tcp connection works but
// the server never answers. Defaults to 2 minutes.
ResponseHeaderTimeout: time.Duration(config.HTTPConfig.ResponseHeaderTimeout),
// Set this value so that the underlying transport round-tripper
// doesn't try to auto decode the body of objects with
// content-encoding set to `gzip`.
//
// Refer: https://golang.org/src/net/http/transport.go?h=roundTrip#L1843.
DisableCompression: true,
TLSClientConfig: tlsConfig,
}, nil
}

// Bucket implements the store.Bucket interface against s3-compatible APIs.
type Bucket struct {
logger log.Logger
Expand Down Expand Up @@ -258,7 +199,7 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
rt = config.HTTPConfig.Transport
} else {
var err error
rt, err = DefaultTransport(config)
rt, err = exthttp.DefaultTransport(config.HTTPConfig)
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/objstore/s3/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/go-kit/log"
"github.com/minio/minio-go/v7/pkg/encrypt"

"github.com/thanos-io/thanos/pkg/exthttp"
"github.com/thanos-io/thanos/pkg/testutil"
)

Expand Down Expand Up @@ -198,7 +199,7 @@ http_config:
`)
cfg, err := parseConfig(input)
testutil.Ok(t, err)
transport, err := DefaultTransport(cfg)
transport, err := exthttp.DefaultTransport(cfg.HTTPConfig)
testutil.Ok(t, err)
testutil.Equals(t, true, transport.TLSClientConfig.InsecureSkipVerify)
}
Expand Down
4 changes: 3 additions & 1 deletion test/e2e/e2ethanos/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ import (
"gopkg.in/yaml.v2"

"github.com/thanos-io/thanos/pkg/alert"
"github.com/thanos-io/thanos/pkg/exthttp"
"github.com/thanos-io/thanos/pkg/httpconfig"
"github.com/thanos-io/thanos/pkg/objstore"
"github.com/thanos-io/thanos/pkg/objstore/client"
"github.com/thanos-io/thanos/pkg/objstore/s3"

"github.com/thanos-io/thanos/pkg/queryfrontend"
"github.com/thanos-io/thanos/pkg/receive"
)
Expand Down Expand Up @@ -952,7 +954,7 @@ func NewS3Config(bucket, endpoint, basePath string) s3.Config {
SecretKey: e2edb.MinioSecretKey,
Endpoint: endpoint,
Insecure: false,
HTTPConfig: s3.HTTPConfig{
HTTPConfig: exthttp.HTTPConfig{
TLSConfig: objstore.TLSConfig{
CAFile: filepath.Join(basePath, "certs", "CAs", "ca.crt"),
CertFile: filepath.Join(basePath, "certs", "public.crt"),
Expand Down