Refactoring: move all http.Transport configuration parameter parsing into one place

[GiedriusS]

These are all functions/structs for configuring the same thing:

https://github.com/thanos-io/thanos/blob/5bda2d48b9ef0bf138af6132641eb20102223edb/pkg/objstore/azure/helpers.go#L121 https://github.com/thanos-io/thanos/blob/46ac4d590e783368fc4ec7676f29ff97cd0bd630/pkg/objstore/cos/cos.go#L95 https://github.com/thanos-io/thanos/blob/bf963237e0b95402aeea4093dce465abde055675/pkg/objstore/s3/s3.go#L124

Another place being added: https://github.com/thanos-io/thanos/pull/4623

Move them into one struct. This probably belongs in exthttp.

[heysujal]

@GiedriusS I would like to fix this issue. Please guide me how can I get started , I am new to this project.

[GiedriusS]

All of these functions and structs can be joined into one and we can use them everywhere consistently. Please let me know if you have any specific questions.

[4molybdenum2]

So will replacing the NewTransport() function in exhttp with something like this help?

func NewTransport(config HTTPConfig) *http.Transport {
    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),
        ResponseHeaderTimeout: time.Duration(config.ResponseHeaderTimeout),
        DisableCompression: config.DisableCompression,
        TLSClientConfig:    &tls.Config{InsecureSkipVerify: config.InsecureSkipVerify},
    }
}

[Namanl2001]

So will replacing the NewTransport() function in exhttp with something like this help?

@4molybdenum2 I think replacing NewTransport() would not be helpful as it serves a different purpose, rather you can try creating a new func DefaultTransport() in exhttp, may be with the same return values as you proposed above, in the comment :)

[4molybdenum2]

Ok, will check this out, maybe make a PR...

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for the last two months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[bwplotka]

AC:

• There is only single HTTPConfig struct that can be unmarshalled from YAML in the consistent way.
• There is only single reused helper for creating Transport from this HTTPConfig
• We don't break Cortex deps

[SrushtiSapkale]

I am working on this issue🙌

[NishantBansal2003]

Hey @saswatamcode,This issue can be closed with reference to the above merged PR.