Using upstream hostname for request (#294)

[mitchelldavis]

No description provided.

[magiconair]

This looks reasonable. A couple of comments;

1. Please roll the two tests into one. You can define a routing table with two entries and then use t.Run subtests to test the two cases.

2. For boolean flags the =true or =false is redundant since the existence or non-existence of the option is sufficient. In route.go you can use _, t.UseUpstreamHostame = r.Opts["..."] to achieve this.

3. I think the name UseUpstreamHostname is too long. Can you think of something shorter and less verbose, e.g. fwdhost, usehost, upstreamhost, ... I don't have better ideas either.

[mitchelldavis]

• So if the two tests are redundant, should I roll them into one or just get rid of the second?
• I set UseUpstreamHostname on line 73 in route.go. Is that not correct?
• usehost works for me and the only one I can come up with otherwise is resetHost.

[magiconair]

You can replace the tests with this:

func TestProxyUsehost(t *testing.T) {
	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprint(w, r.Host)
	}))

	proxy := httptest.NewServer(&HTTPProxy{
		Transport: &http.Transport{
			Dial: func(network, addr string) (net.Conn, error) {
				addr = server.URL[len("http://"):]
				return net.Dial(network, addr)
			},
		},
		Lookup: func(r *http.Request) *route.Target {
			routes := "route add mock /usehost http://a.com/ opts \"usehost\"\n"
			routes += "route add mock / http://a.com/"
			tbl, _ := route.NewTable(routes)
			return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"])
		},
	})
	defer proxy.Close()

	check := func(uri, host string) {
		resp, body := mustGet(proxy.URL + uri)
		if got, want := resp.StatusCode, http.StatusOK; got != want {
			t.Fatalf("got status %d want %d", got, want)
		}
		if got, want := string(body), host; got != want {
			t.Fatalf("got body %q want %q", got, want)
		}
	}

	proxyHost := proxy.URL[len("http://"):]
	t.Run("use host", func(t *testing.T) { check("/usehost", "a.com") })
	t.Run("no host", func(t *testing.T) { check("/", proxyHost) })
}

2. I mean to use usehost instead of usehost=(true|false). This changes on how you set the t.UseHost flag since checking for an empty string is not sufficient.

3. usehost it is then. Pls also use t.UseHost internally.

Thx a lot

[magiconair]

Left some small comments. The rest looks good.

[magiconair]

This is redundant. Just use what I wrote initially

_, t.UseHost = r.Opts["usehost"]

[magiconair]

UseHost determines whether the the origin or the target hostname
is used for the upstream request

[magiconair]

Can you drop the comment, too please? Don't want to be overly pick but it really is redundant.

[mitchelldavis]

No Worries. It looks like I need to rerun the build anyways. :-D

[magiconair]

I'll let this sit for another day or two before I merge it. usehost is concise but also ambiguous. I'll see whether I can come up with something better. Thx for your patience.

[mitchelldavis]

@magiconair, how about usedesthost?

[magiconair]

How about host=dst?

[mitchelldavis]

ooooo.... that's way better. Default will be src?

[magiconair]

Yes. I think that makes more sense and provides some flexibility for the future. boolean flags are always somewhat limiting unless you can be absolutely sure that there will ever only be two choices.

[magiconair]

Can you make that change?

[mitchelldavis]

I'm on it!

[magiconair]

Two small changes and then we can merge it.

[mitchelldavis]

Thats a bummer. It looks like an unstable test. :-(