GCS: Adds HTTP Config similar to S3 #86
Conversation
JoaoBraveCoding
force-pushed
the
gcs-add-http-config
branch
from
c0afd4b to
0c4a592
Compare
Signed-off-by: Joao Marcal <[email protected]>
JoaoBraveCoding
force-pushed
the
gcs-add-http-config
branch
from
0c4a592 to
26d27e2
Compare
Signed-off-by: Joao Marcal <[email protected]>
There was a problem hiding this comment.
Thanks, this looks good to me overall!
Maybe @fpetkovski can take a look as well, as he's familiar with GCS.
| @@ -11,6 +11,16 @@ import ( | |||
| "github.com/prometheus/common/model" | |||
| ) | |||
|
|
|||
| var DefaultHTTPConfig = HTTPConfig{ | |||
There was a problem hiding this comment.
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.
IIUC nowadays how objstorage works then it will eventually call defaultBaseTransport on the google-api-go-client in transport/http/dial.go which actually uses http.DefaultTransport src/net/http/transport.go, they only overwrite MaxIdleConnsPerHost to 100 (same value we use).
So looking at the differences if we were to move forward with my change we would be setting:
MaxConnsPerHostSince this is not set on the DefaultTransport struct I believe it will just use defaultintvalue of0which would match our valueResponseHeaderTimeoutAgain IIUC here the default here would be0which would differ from our "default" of 2 minutes.DialContext.DualStackThis apparently has been deprecated so we can even remove it from our "default" since it's enabled by default
And 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.
Thanks for the detailed explanation 👍 Let's try this out and see if it has any significant impact.
|
@saswatamcode @fpetkovski anything else I could do to move this forward? |
|
@saswatamcode @fpetkovski I've updated the PR is there anything else we can do to move this forward? Soon this might become a blocker for Loki adoption of thanos.io/objstore |
| } else { | ||
| opts = append(opts, option.WithoutAuthentication()) |
There was a problem hiding this comment.
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.
+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.
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.
@JoaoBraveCoding would you have time to look into this?
There was a problem hiding this comment.
I think the test fails as we call htransport.NewTransport later which errors out. Trying to fix here: #106
There was a problem hiding this comment.
Had plans to fix it this morning but @saswatamcode beat me to it (thank you!) Apologies for having broken GCS authentication with env vars!
Issue was reported in thanos-io#106 (comment)
Issue was reported in thanos-io#106 (comment) Signed-off-by: Joao Marcal <[email protected]>
GCS: adds scope to fix bug introduced in #86 Issue was reported in #106 (comment) Signed-off-by: Joao Marcal <[email protected]>
Changes
Adds HTTP Config to GCS. This would be required to allow us to support hedging while using this project as a storage backend. HTTP Config allows us to configure a Transport which would enable the utilization of https://github.com/cristalhq/hedgedhttp when using the GCS provider
Related with: grafana/loki#11132
Verification