environs/httpstorage: use req.Host in HEAD

environs/httpstorage: use req.Host in HEAD

The HEAD response was returning the listening IP
address, rather than the host presented in the
original HTTP Request. This is changed to now use
the request's Host field.

Fixes #1235102

https://code.launchpad.net/~axwalk/juju-core/lp1235102-httpstorage-head/+merge/189225

(do not edit description out of merge proposal)

[axw, 2013-10-04 07:57:45]

Please take a look.

[axw, 2013-10-04 08:09:05]

Please take a look.

[fwereade, 2013-10-04 09:25:15]

LGTM, thanks

[rog, 2013-10-04 12:08:36]

LGTM with some minor grumblings.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
environs/httpstorage/backend.go:65: func (s *storageBackend) authorised(req
*http.Request) bool {
s/authorised/authorized/

We standardise on US spelling in this code base,
to match Go's conventions.
(I just spent a while grepping for "authorized")

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
environs/httpstorage/backend.go:76: hostonly := req.Host
Can't you use net.SplitHostPort here?

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
environs/httpstorage/backend.go:86: url := fmt.Sprintf("https://%s:%d%s",
hostonly, s.httpsPort, req.URL.Path)
tbh this feel like abuse of the HEAD method.
It should at least check that req.URL.Path is /. 
But if it's conventional, I *guess* it's ok.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
environs/httpstorage/backend_test.go:152: c.Assert(location.String(),
gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
We should continue with the test and check that we can
actually connect with the given URL.

[axw, 2013-10-07 01:47:57]

On 2013/10/04 12:08:36, rog wrote:
> LGTM with some minor grumblings.

Thanks, I'll address these in another CL since I've already landed this.

>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> File environs/httpstorage/backend.go (right):
> 
>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> environs/httpstorage/backend.go:65: func (s *storageBackend) authorised(req
> *http.Request) bool {
> s/authorised/authorized/
> 
> We standardise on US spelling in this code base,
> to match Go's conventions.
> (I just spent a while grepping for "authorized")

Thanks, will update.

>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> environs/httpstorage/backend.go:76: hostonly := req.Host
> Can't you use net.SplitHostPort here?

Yes. I thought there was a function, just didn't find it in my admittedly short
search.

>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> environs/httpstorage/backend.go:86: url := fmt.Sprintf("https://%s:%d%s",
> hostonly, s.httpsPort, req.URL.Path)
> tbh this feel like abuse of the HEAD method.

Indeed :)
I considered doing redirects, but not all client methods follow them, and it'd
cause two requests for each Put/Remove.

> It should at least check that req.URL.Path is /. 
> But if it's conventional, I *guess* it's ok.

So, the only way Path might be empty is if the request line had URI with no
path. Right?
Never going to happen for this, so I'd rather not complicate it.

>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> File environs/httpstorage/backend_test.go (right):
> 
>
https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backen...
> environs/httpstorage/backend_test.go:152: c.Assert(location.String(),
> gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
> We should continue with the test and check that we can
> actually connect with the given URL.

Okay.