[RFC] htransport: option for customizing transport

[broady]

Allow generic tweaking of net/http:

package storage // cloud.google.com/go/storage

import raw "google.golang.org/api/storage/v1"
import htransport "google.golang.org/api/transport/http"

func NewClient(...) ... {
     _, _ = raw.NewService(ctx, htransport.WithConfigureTransport(func(t *http.Transport) {
          t.MaxIdleConns = 100
     })

     // ... or maybe:

     _, _ := raw.NewService(ctx, htransport.WithBaseTransport(&http.Transport{
          MaxIdleConns: 100,
          // but what about the defaults?!
     })
}

Ideas?

[broady]

I think either could be a candidate to live long-term in google.golang.org/api/option, likely preferred and easier to use over WithHTTPClient in most cases.

It's unclear whether the parameter to WithBaseTransport should be *http.Transport or the RoundTripper interface.

[tritone]

+1 for this. In storage, for example, adding a simple RoundTripper to a client (for example adding logging or a request header) via WithHTTPClient requires duplicating a bunch of code from the NewClient and understanding how to call into htransport. Lots of potential pitfalls.

[codyoss]

I would vote for using RoundTripper. Would provide a nice way for people to mock out our client requests should they choose to do so.

[broady]

RoundTripper seems like a better fit, I agree, but it's very awkward, for example, to set just a proxy, like in CL 55030/#434 if we don't know that we're getting an actual *http.Transport.

Functionally, accepting a RoundTripper is not too far different off from http.Client, except that it's easier to wrap with HTTP header-level things (e.g., setting user-agent), but completely fails at not disturbing TCP-level options in *http.Transport. Users would have to look up what we set as MaxIdleConns (and maybe confuse that with net/http) when all they were trying to do was add a proxy, or tweak one of those knobs, etc etc.

[codyoss]

How much benefit would adding one of these options have over just using https://pkg.go.dev/google.golang.org/api@v0.22.0/transport/http?tab=doc#NewTransport with option WithHTTPClient? Is there are particular use case you were thinking adding this option would solve or are you more worried about the sprawl of Transport level options we maintain?

[tritone]

I have run into a few issues where GCS customers want to add simple roundtrippers (e.g. add a header or logging) or change the settings for the transport, and it ends up requiring quite a bit of additional code including copying logic from NewClient in the generated client. For example here's a sample I made for adding debug headers:

func main() {
 
	ctx := context.Background()
 
	// Standard way to initialize client:
	// client, err := storage.NewClient(ctx)
	// if err != nil {
	// 	// handle error
	// }
 
	// Instead, create a custom http.Client.
	base := http.DefaultTransport
	trans, err := htransport.NewTransport(ctx, base, option.WithScopes(raw.DevstorageFullControlScope),
		option.WithUserAgent("custom-user-agent"))
	if err != nil {
		// Handle error.
	}
	c := http.Client{Transport:trans}
 
	// Add RoundTripper to the created HTTP client.
	c.Transport = withDebugHeader{c.Transport}
 
	// Supply this client to storage.NewClient
	client, err := storage.NewClient(ctx, option.WithHTTPClient(&c))
	if err != nil {
		// Handle error.
	}
}
 
type withDebugHeader struct {
	rt http.RoundTripper
}
 
func (wdh withDebugHeader) RoundTrip(r *http.Request) (*http.Response, error) {
	r.Header.Add("X-Return-Encrypted-Headers", "all")
	resp, err := wdh.rt.RoundTrip(r)
	if err == nil {
		log.Printf("Resp Header: %+v, ", resp.Header.Get("X-Encrypted-Debug-Headers"))
	} else {
		log.Printf("Error: %+v", err)
	}
	return resp, err
}

Understanding what an appropriate base client to use (including adding auth logic) is quite tricky and requires more understanding than most users have of the code in google-api-go-client. Also, this doesn't play nicely with options such as WithEndpoint and WithClientCertSource (users need to understand that they need to set these things in the http.client rather than using the options). And if we change any additional logic in NewClient at all, they are unable to pick that up.

This PR I made (https://code-review.googlesource.com/c/gocloud/+/55411 ) also shows the difficulty that this poses for making changes to the transport at the level of manual clients.

[ernestas2k]

+1 at least to provide additional RoundTripper. In our case for request/response logging. If any assistance is needed, let me know.

[broady]

If we consider the different layers of what our "transport" package does, then we can consider where we might allow people to customize.

Too many options that operate at the same level might indicate redundancy and that a generic option might be more appropriate.

HTTP	GRPC
WithTokenSource/WithAPIKey/WithQuotaProject/WithRequestReason/WithUserAgent	ditto
??? - proposed http.RoundTripper/http.Transport/http.Proxy	??? grpc.ClientConnInterface
WithHTTPClient	WithGRPCConn WithGRPCDialOption
??? - proposed WithDialer(func (context.Context, string) (netConn, error))	WithGRPCDialOption(grpc.WithDialer(...))
WithEndpoint	ditto

Options that work the same across both transports are desirable and might indicate an appropriate abstraction.

FWIW I think WithHTTPClient and one of WithGRPCConn or WithGRPCDialOption were probably mistakes.

[tritone]

Over time we've done some additional customization of the HTTP client for optimization (using http2 configs, MaxIdleConnsPerHost, etc). Just talked to another user who was attempting to add a roundtripper to do simple header manipulation but, using option.WithHTTPClient, missed out on these settings unwittingly. I think it would be good to move ahead with this when we have a chance.

[codyoss]

I will add this a point of discussion in our next meeting. I would like to formally capture use-cases we are not supporting well today.

[tkinz27]

To add a use case, I am in the process of trying to lock the GCS storage client to a specific local storage interface. My application runs on an embedded linux server that has access to LTE and WiFi modems, and we want to guarantee that our file uploads/downloads are locked to the wifi interface.

Having an equivalent to grpc.WithContextDialer would be much much appreciated. Right now I have done my best to follow how the storage client creates a http client and am using option.WithHTTPClient.

[codyoss]

As of right now we will not be moving forward with this. In our new auth module we tried to make adding auth to an existing http.Client a bit easier. Using that and the WithHTTPClient Option is how we recommend customising a transport.