net/http: How to copy settings from http.DefaultTransport?

[Mateusz Czapliński]

We have a function like below in our codebase:

package tls

func NewTransport(config *tls.Config) http.RoundTripper {
    // default values as of Go 1.9.2 + custom TLSClientConfig
    // NOTE: don't try to simplify it like below:
    //    defaultCopy := *http.DefaultTransport.(*http.Transport)
    //    defaultCopy.TLSClientConfig = cConfig
    //    return &defaultCopy, nil
    // as this would result in data races on private fields of http.Transport
    // TODO: somehow make this code not need reviewing on each Go upgrade
    return &http.Transport{
        Proxy: http.ProxyFromEnvironment,
        DialContext: (&net.Dialer{
            Timeout:   30 * time.Second,
            KeepAlive: 30 * time.Second,
            DualStack: true,
        }).DialContext,
        MaxIdleConns:          100,
        IdleConnTimeout:       90 * time.Second,
        TLSHandshakeTimeout:   10 * time.Second,
        ExpectContinueTimeout: 1 * time.Second,

        TLSClientConfig: config,
    }
}

The problem here is, that we need to create new http.Clients with some custom TLS settings at various places. Unfortunately, it is not easy to reuse the default "sane" values from http.DefaultTransport, because:

  - it's not possible to just copy *http.DefaultTransport by value, as this leads to data races (because private fields are copied too);
  - it's not OK to use "default 0" values here, as they are often defined to mean "no limit" (e.g. MaxIdleConns, IdleConnTimeout), not "reuse value from http.DefaultTransport"
  - new fields are added with new releases, and they get assigned new default values in http.DefaultTransport (this has already bitten us!), so this code needs constant reviewing on each Go release.

Do you know of any approach we could use to not have to manually review this code on each Go upgrade?

I'm tempted to submit a proposal on golang.org/issue for a new function, say "http.NewDefaultTransport()", which would build a structure like above, except the TLSClientConfig. But that would be a somewhat ugly addition to the net/http API, so I'm interested if anyone is aware of, or has any ideas, how we could resolve this in some reasonable way?

TIA
/M.

[Manlio Perillo]

Il giorno mercoledì 20 giugno 2018 17:29:57 UTC+2, Mateusz Czapliński ha scritto:

We have a function like below in our codebase:

package tls

func NewTransport(config *tls.Config) http.RoundTripper {
    // default values as of Go 1.9.2 + custom TLSClientConfig
    // NOTE: don't try to simplify it like below:
    //    defaultCopy := *http.DefaultTransport.(*http.Transport)
    //    defaultCopy.TLSClientConfig = cConfig
    //    return &defaultCopy, nil
    // as this would result in data races on private fields of http.Transport
    // TODO: somehow make this code not need reviewing on each Go upgrade
    return &http.Transport{
        Proxy: http.ProxyFromEnvironment,
        DialContext: (&net.Dialer{
            Timeout:   30 * time.Second,
            KeepAlive: 30 * time.Second,
            DualStack: true,
        }).DialContext,
        MaxIdleConns:          100,
        IdleConnTimeout:       90 * time.Second,
        TLSHandshakeTimeout:   10 * time.Second,
        ExpectContinueTimeout: 1 * time.Second,

        TLSClientConfig: config,
    }
}

The problem here is, that we need to create new http.Clients with some custom TLS settings at various places. Unfortunately, it is not easy to reuse the default "sane" values from http.DefaultTransport, because:

Not tested:

    return &http.Transport{
        Proxy: http.DefaultTransport.ProxyFromEnvironment,
        DialContext: http.DefaultTransport.DialContext,

        MaxIdleConns:  http.DefaultTransport.MaxIdleConns,
        IdleConnTimeout:       http.DefaultTransport.IdleConnTimeout,
        TLSHandshakeTimeout:   http.DefaultTransport.TLSHandshakeTimeout,
        ExpectContinueTimeout: http.DefaultTransport.ExpectContinueTimeout,

        TLSClientConfig: config,
    }

 

If you want to generalize it, you can use reflect to copy all exported fields (again, not tested).

Note that a custom transport does not support HTTP2 by default, and must be enabled explicitly.

> [...]

Manlio

[Manlio Perillo]

Il giorno mercoledì 20 giugno 2018 18:20:22 UTC+2, Manlio Perillo ha scritto:

Il giorno mercoledì 20 giugno 2018 17:29:57 UTC+2, Mateusz Czapliński ha scritto:

> [...] 

The problem here is, that we need to create new http.Clients with some custom TLS settings at various places. Unfortunately, it is not easy to reuse the default "sane" values from http.DefaultTransport, because:

> [...] 

 

If you want to generalize it, you can use reflect to copy all exported fields (again, not tested).

Note that a custom transport does not support HTTP2 by default, and must be enabled explicitly.

Here is a simple implementation of a function to copy a struct:

https://play.golang.org/p/cotZaihUxdi

Manlio

[Mateusz Czaplinski]

As to the first idea, copying fields by hand does not solve the need for review during upgrade, as new fields may be added in new Go releases. So it does not help me, unfortunately.

As to the second one, copying with reflect... eh; right, that's also something I pondered; what bothers me here, is first of all, that it's *suuuper* overcomplicated for the situation. And secondly, the DialContext field is somewhat risky in this situation. In this particular case, reviewing it seems to show there's no possibility for storing internal state; but it feels much too fragile, more like crossing fingers than solid & robust engineering... Again, in some next revision, something could be added which would invalidate this assumption, and introduce some subtle unexpected behavior.

So, thanks for first ideas! That said, I still see this more as unresolved, than resolved...

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/JmpHoAd76aU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Manlio Perillo]

Il giorno giovedì 21 giugno 2018 10:42:17 UTC+2, Mateusz Czapliński ha scritto:

As to the first idea, copying fields by hand does not solve the need for review during upgrade, as new fields may be added in new Go releases. So it does not help me, unfortunately.

As to the second one, copying with reflect... eh; right, that's also something I pondered; what bothers me here, is first of all, that it's *suuuper* overcomplicated for the situation. And secondly, the DialContext field is somewhat risky in this situation. In this particular case, reviewing it seems to show there's no possibility for storing internal state; but it feels much too fragile, more like crossing fingers than solid & robust engineering... Again, in some next revision, something could be added which would invalidate this assumption, and introduce some subtle unexpected behavior.

So, thanks for first ideas! That said, I still see this more as unresolved, than resolved...

I opened an issue:

https://github.com/golang/go/issues/26013

Manlio 

[Dave Cheney]

// NOTE: don't try to simplify it like below:
// defaultCopy := *http.DefaultTransport.(*http.Transport)
// defaultCopy.TLSClientConfig = cConfig
// return &defaultCopy, nil
// as this would result in data races on private fields of http.Transport

I have tried this once in the past and believe that it worked.

[Mateusz Czapliński]

@Manlio: thanks!

@Dave: The NOTE is because we also tried once. There's a notable comment now from Brad Fitz in the issue discussion, explaining how we both can actually be right ;)

Thanks,
/M.