fix #2, return full host:port info from getHost

[santsai]

I ran into #2, i think its safe to simply return r.Host because r.URL.IsAbs() case is already doing that and is covered by existing tests cases.

[elithrar]

See comment.

I would also expect an update of the .Host() method to indicate that matches will include/require the port in the non-80 case.

[elithrar]

Can you point me to the relevant section of the RFC that covers this requirement for the Host header? It is mentioned in the context of a URL, but seems more ambiguous in the context of a Host header. It is not immediately clear in 7230 either: https://tools.ietf.org/html/rfc7230#section-5.4

3.2.2 http URL
The "http" scheme is used to locate network resources via the HTTP
protocol. This section defines the scheme-specific syntax and
semantics for http URLs.
http_URL = "http:" "//" host [ ":" port ] [ abs_path [ "?" query ]]
If the port is empty or not given, port 80 is assumed.

I'm cautious about making changes here as it may change the matching behaviour (read: breaking change) of existing users.

[santsai]

The section covering Host header is here https://tools.ietf.org/html/rfc2616#section-14.23
I understood your concern about breaking changes, however it's strange for getHost() to have different behaviour for host from abs url (requires "host:port" in non-80 port case for .Host() to match) and host from header (requires "host" and no port for non-80 port). In the later case, if one build a url from such route, the resulting url would also be wrong since the port number would be missing.

the abs url behaviour is covered by existing test case:

mux/mux_test.go

Lines 80 to 88 in e0b5aba

	{
	title: "Host route with port, match",
	route: new(Route).Host("aaa.bbb.ccc:1234"),
	request: newRequest("GET", "http://aaa.bbb.ccc:1234/111/222/333"),
	vars: map[string]string{},
	host: "aaa.bbb.ccc:1234",
	path: "",
	shouldMatch: true,
	},

the host from header behaviour is marked as bug in a test case:

mux/mux_test.go

Line 116 in e0b5aba

// BUG {new(Route).Host("aaa.bbb.ccc:1234"), newRequestHost("GET", "/111/222/333", "aaa.bbb.ccc:1234"), map[string]string{}, "aaa.bbb.ccc:1234", "", true},

[kisielk]

It's a bit of a scary change, but it makes sense in theory and I guess we can merge it if all the tests (plus additional ones) pass.

[elithrar]

I'm OK to merge this. It will require:

• A minor version bump (tagged as such)
• A commit message that clarifies this -

summary: getHost() now returns full host & port information.
full commit message: Previously, getHost only returned the host. As it now returns the port as well, any .Host matches on a route will need to be updated to also support matching on the port for cases where the port is not 80 (scheme == "http").

[cognusion]

Tracking down why my apps suddenly stopped doing host matching properly, and stumbled onto this. Seems like a big backward-breaking change for a minor release. At the very least, when the Route.Host() contains just a host, and not a port, then hostname:* should be assumed, I'd think.

[elithrar]

I'm open to a PR for that.

…

On Tue, Feb 12, 2019 at 10:29 AM M@ ***@***.***> wrote: Tracking down why my apps suddenly stopped doing host matching properly, and stumbled onto this. Seems like a big backward-breaking change for a minor release. At the very least, when the .Host() contains just a host, and not a port, then hostname:* should be assumed, I'd think. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcBssUbxHGPjewAPXr-5-Vk1l1Eclks5vMwgagaJpZM4UQJkz> .

[cognusion]

PR #447 does the job efficiently.

I'm open to a PR for that.
…
On Tue, Feb 12, 2019 at 10:29 AM M@ @.**> wrote: Tracking down why my apps suddenly stopped doing host matching properly, and stumbled onto this. Seems like a big backward-breaking change for a minor release. At the very least, when the .Host() contains just a host, and not a port, then hostname: should be assumed, I'd think. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#383 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AABIcBssUbxHGPjewAPXr-5-Vk1l1Eclks5vMwgagaJpZM4UQJkz .

[harshavardhana]

This broke API compatibility with 1.6.x we have a regression in MinIO because of this.