Add a SafeHttpClient method

[biilmann]

This adds a tracing method to a client that will make sure
any request to a reserved IP is canceled at DNSDone time

[rybit]

Generally I like the approach but I think that you can do a few small things to make it more clear:

• override only the transport
• use anonymous functions
• use an optional logger

Like this (mostly untested)

type noLocalTransport struct {
	inner  http.RoundTripper
	errlog logrus.FieldLogger
}

func (no noLocalTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	ctx, cancel := context.WithCancel(req.Context())

	ctx = httptrace.WithClientTrace(ctx, &httptrace.ClientTrace{
		DNSDone: func(info httptrace.DNSDoneInfo) {
			if endpoint := isLocal(info); endpoint != "" {
				cancel()
				if no.errlog != nil {
					no.errlog.WithFields(logrus.Fields{
						"original_url":     req.URL.String(),
						"blocked_endpoint": endpoint,
					})
				}
			}
		},
	})

	req = req.WithContext(ctx)
	return no.inner.RoundTrip(req)
}

func isLocal(info httptrace.DNSDoneInfo) string {
	return "what was blocked"
}

func newLocalBlocker(trans http.RoundTripper, log logrus.FieldLogger) *noLocalTransport {
	if trans == nil {
		trans = http.DefaultTransport
	}

	ret := &noLocalTransport{
		inner:  trans,
		errlog: log.WithField("transport", "local_blocker"),
	}

	return ret
}

I think that it does less setting on the transport object. I am mostly afraid of some kind of concurrency nightmare in the transport.

[rybit]

I'd use the httptest lib instead. https://golang.org/pkg/net/http/httptest/#Server

[rybit]

s/Http/HTTP

[biilmann]

I like that suggestion - plopped that implementation in there

[mraerino]

This only needs minor adjustments.
The tests have room for improvement in general.

Also, outer users should be able to allow localhost for testing:
https://github.com/netlify/gotrue/blob/22a973955d0644e0a9163dda4dd8667fce7fcfe7/api/hook_test.go#L48-L49

[mraerino]

This should be removed because it pollutes logs

[mraerino]

would be great to log these and previous errors on the errlog field of the noLocalTransport

[vbrown608]

I'm going to update the tests a bit

[mraerino]

This looks really good.

There is still one case it does not cover:
We want to be able to allow certain ips/ranges inside of tests in other packages.

I propose adding a variadic arg to SafeRountripper and SafeHTTPClient for blocks you want to allow.
I'm thinking of this signature:

func SafeHTTPClient(client *http.Client, log logrus.FieldLogger, allowedBlocks ...*net.IPNet) *http.Client {
...

Another way could be to expose a method on the roundtripper to allow blocks temporarily.

[mraerino]

There also is a typo in SafeRountripper you know? 😛

[vbrown608]

Ah, I thought you intended for that to be a follow-up (along with un-inlining this functionality in gotrue). I can put it in here.

[mraerino]

🚢