-
Notifications
You must be signed in to change notification settings - Fork 80
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
GCS: Adds HTTP Config similar to S3 #86
Changes from all commits
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 |
---|---|---|
|
@@ -8,9 +8,11 @@ import ( | |
"context" | ||
"fmt" | ||
"io" | ||
"net/http" | ||
"runtime" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
"cloud.google.com/go/storage" | ||
"github.com/go-kit/log" | ||
|
@@ -19,17 +21,23 @@ import ( | |
"golang.org/x/oauth2/google" | ||
"google.golang.org/api/iterator" | ||
"google.golang.org/api/option" | ||
htransport "google.golang.org/api/transport/http" | ||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/codes" | ||
"google.golang.org/grpc/status" | ||
"gopkg.in/yaml.v2" | ||
|
||
"github.com/thanos-io/objstore" | ||
"github.com/thanos-io/objstore/exthttp" | ||
) | ||
|
||
// DirDelim is the delimiter used to model a directory structure in an object store bucket. | ||
const DirDelim = "/" | ||
|
||
var DefaultConfig = Config{ | ||
HTTPConfig: exthttp.DefaultHTTPConfig, | ||
} | ||
|
||
// Config stores the configuration for gcs bucket. | ||
type Config struct { | ||
Bucket string `yaml:"bucket"` | ||
|
@@ -39,7 +47,8 @@ type Config struct { | |
// when direct path is not enabled. | ||
// See https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API for more details | ||
// on how to enable direct path. | ||
GRPCConnPoolSize int `yaml:"grpc_conn_pool_size"` | ||
GRPCConnPoolSize int `yaml:"grpc_conn_pool_size"` | ||
HTTPConfig exthttp.HTTPConfig `yaml:"http_config"` | ||
} | ||
|
||
// Bucket implements the store.Bucket and shipper.Bucket interfaces against GCS. | ||
|
@@ -51,14 +60,23 @@ type Bucket struct { | |
closer io.Closer | ||
} | ||
|
||
// parseConfig unmarshals a buffer into a Config with default values. | ||
func parseConfig(conf []byte) (Config, error) { | ||
config := DefaultConfig | ||
if err := yaml.UnmarshalStrict(conf, &config); err != nil { | ||
return Config{}, err | ||
} | ||
|
||
return config, nil | ||
} | ||
|
||
// NewBucket returns a new Bucket against the given bucket handle. | ||
func NewBucket(ctx context.Context, logger log.Logger, conf []byte, component string) (*Bucket, error) { | ||
var gc Config | ||
if err := yaml.Unmarshal(conf, &gc); err != nil { | ||
config, err := parseConfig(conf) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return NewBucketWithConfig(ctx, logger, gc, component) | ||
return NewBucketWithConfig(ctx, logger, config, component) | ||
} | ||
|
||
// NewBucketWithConfig returns a new Bucket with gcs Config struct. | ||
|
@@ -76,12 +94,38 @@ func NewBucketWithConfig(ctx context.Context, logger log.Logger, gc Config, comp | |
return nil, errors.Wrap(err, "failed to create credentials from JSON") | ||
} | ||
opts = append(opts, option.WithCredentials(credentials)) | ||
} else { | ||
opts = append(opts, option.WithoutAuthentication()) | ||
Comment on lines
+97
to
+98
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. This change is described as "Fix tests"; it stops the GCS client from using config from environment variables. 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. +1, From what I understand, this would break GCP workload identity from working Context: https://cloud-native.slack.com/archives/CK5RSSC10/p1633961188039700 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. Literally came to this repo to report this bug; this just hit me in a pre-prod environment and came to report it 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. @JoaoBraveCoding would you have time to look into this? 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. I think the test fails as we call 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. Had plans to fix it this morning but @saswatamcode beat me to it (thank you!) Apologies for having broken GCS authentication with env vars! |
||
} | ||
|
||
opts = append(opts, | ||
option.WithUserAgent(fmt.Sprintf("thanos-%s/%s (%s)", component, version.Version, runtime.Version())), | ||
) | ||
|
||
// Check if a roundtripper has been set in the config | ||
// otherwise build the default transport. | ||
var rt http.RoundTripper | ||
if gc.HTTPConfig.Transport != nil { | ||
rt = gc.HTTPConfig.Transport | ||
} else { | ||
var err error | ||
rt, err = exthttp.DefaultTransport(gc.HTTPConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
gRT, err := htransport.NewTransport(context.Background(), rt, opts...) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
httpCli := &http.Client{ | ||
Transport: gRT, | ||
Timeout: time.Duration(gc.HTTPConfig.IdleConnTimeout), | ||
} | ||
opts = append(opts, option.WithHTTPClient(httpCli)) | ||
|
||
return newBucket(ctx, logger, gc, opts) | ||
} | ||
|
||
|
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 this might lead to an unexpected breaking change with every other provider except S3. Do we know what the defaults for GCS currently are?
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.
IIUC nowadays how objstorage works then it will eventually call
defaultBaseTransport
on the google-api-go-client in transport/http/dial.go which actually useshttp.DefaultTransport
src/net/http/transport.go, they only overwriteMaxIdleConnsPerHost
to 100 (same value we use).So looking at the differences if we were to move forward with my change we would be setting:
MaxConnsPerHost
Since this is not set on the DefaultTransport struct I believe it will just use defaultint
value of0
which would match our valueResponseHeaderTimeout
Again IIUC here the default here would be0
which would differ from our "default" of 2 minutes.DialContext.DualStack
This apparently has been deprecated so we can even remove it from our "default" since it's enabled by defaultAnd we would not be setting
ForceAttemptHTTP2
which http.DefaultTransport sets. This is where we could break some things.With all this being said I don't have much experience with all this so I'm happy to play it safe and change the PR to make GCS use
http.DefaultTransport
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.
Thanks for the detailed explanation 👍 Let's try this out and see if it has any significant impact.