juju-core

Merge lp:~axwalk/juju-core/lp1235102-httpstorage-head into lp:~go-bot/juju-core/trunk

  1. lp1235102-httpstorage-head
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1950
Proposed branch: lp:~axwalk/juju-core/lp1235102-httpstorage-head
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 77 lines (+19/-8)
2 files modified
environs/httpstorage/backend.go (+17/-6)
environs/httpstorage/backend_test.go (+2/-2)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1235102-httpstorage-head
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+189225@code.launchpad.net

Commit message

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://codereview.appspot.com/14388043/

Description of the change

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://codereview.appspot.com/14388043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+189225_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/14388043/

Affected files (+19, -6 lines):
   A [revision details]
   M environs/httpstorage/backend.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131003170932-4tkr0tu40wb0nfgl
+New revision: <email address hidden>

Index: environs/httpstorage/backend.go
=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go 2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend.go 2013-10-04 07:38:49 +0000
@@ -22,9 +22,9 @@
  type storageBackend struct {
   backend storage.Storage

- // httpsBaseURL is the base URL to send to clients
+ // httpsPort is the port to send to clients
   // if they perform a HEAD request.
- httpsBaseURL string
+ httpsPort int

   // authkey is non-empty if modifying requests
   // require an auth key.
@@ -37,7 +37,7 @@
   case "PUT", "DELETE":
    // Don't allow modifying operations if there's an HTTPS backend
    // to handle that, and ensure the user is authorised/authenticated.
- if s.httpsBaseURL != "" || !s.authorised(req) {
+ if s.httpsPort != 0 || !s.authorised(req) {
     http.Error(w, "unauthorised access", http.StatusUnauthorized)
     return
    }
@@ -72,8 +72,19 @@
  // handleHead returns the HTTPS URL for the specified
  // path in the Location header.
  func (s *storageBackend) handleHead(w http.ResponseWriter, req
*http.Request) {
- if s.httpsBaseURL != "" {
- w.Header().Set("Location", s.httpsBaseURL+req.URL.Path)
+ if s.httpsPort != 0 {
+ hostonly := req.Host
+ if i := strings.LastIndex(hostonly, "]"); i != -1 {
+ // [ipv6]:port
+ hostonly = hostonly[:i+1]
+ } else {
+ // host:port
+ if i := strings.LastIndex(hostonly, ":"); i != -1 {
+ hostonly = hostonly[:i]
+ }
+ }
+ url := fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort,
req.URL.Path)
+ w.Header().Set("Location", url)
   } else {
    http.Error(w, "method HEAD is not supported",
http.StatusMethodNotAllowed)
    return
@@ -192,7 +203,7 @@
     listener.Close()
     return nil, fmt.Errorf("cannot start TLS listener: %v", err)
    }
- backend.httpsBaseURL = fmt.Sprintf("https://%s", tlsListener.Addr())
+ backend.httpsPort = tlsListener.Addr().(*net.TCPAddr).Port
    goServe(tlsListener, tlsBackend)
   }
   goServe(listener, backend)

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/14388043/

Revision history for this message
William Reade (fwereade) wrote :

LGTM, thanks

https://codereview.appspot.com/14388043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with some minor grumblings.

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

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode65
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/backend.go#newcode76
environs/httpstorage/backend.go:76: hostonly := req.Host
Can't you use net.SplitHostPort here?

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode86
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/backend_test.go
File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go#newcode152
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.

https://codereview.appspot.com/14388043/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/backend.go
> File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode65
> 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/backend.go#newcode76
> 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/backend.go#newcode86
> 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/backend_test.go
> File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go#newcode152
> 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.

https://codereview.appspot.com/14388043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go 2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend.go 2013-10-04 08:07:30 +0000
@@ -22,9 +22,9 @@
22type storageBackend struct {22type storageBackend struct {
23 backend storage.Storage23 backend storage.Storage
2424
25 // httpsBaseURL is the base URL to send to clients25 // httpsPort is the port to send to clients
26 // if they perform a HEAD request.26 // if they perform a HEAD request.
27 httpsBaseURL string27 httpsPort int
2828
29 // authkey is non-empty if modifying requests29 // authkey is non-empty if modifying requests
30 // require an auth key.30 // require an auth key.
@@ -37,7 +37,7 @@
37 case "PUT", "DELETE":37 case "PUT", "DELETE":
38 // Don't allow modifying operations if there's an HTTPS backend38 // Don't allow modifying operations if there's an HTTPS backend
39 // to handle that, and ensure the user is authorised/authenticated.39 // to handle that, and ensure the user is authorised/authenticated.
40 if s.httpsBaseURL != "" || !s.authorised(req) {40 if s.httpsPort != 0 || !s.authorised(req) {
41 http.Error(w, "unauthorised access", http.StatusUnauthorized)41 http.Error(w, "unauthorised access", http.StatusUnauthorized)
42 return42 return
43 }43 }
@@ -72,8 +72,19 @@
72// handleHead returns the HTTPS URL for the specified72// handleHead returns the HTTPS URL for the specified
73// path in the Location header.73// path in the Location header.
74func (s *storageBackend) handleHead(w http.ResponseWriter, req *http.Request) {74func (s *storageBackend) handleHead(w http.ResponseWriter, req *http.Request) {
75 if s.httpsBaseURL != "" {75 if s.httpsPort != 0 {
76 w.Header().Set("Location", s.httpsBaseURL+req.URL.Path)76 hostonly := req.Host
77 if i := strings.LastIndex(hostonly, "]"); i != -1 {
78 // [ipv6]:port
79 hostonly = hostonly[:i+1]
80 } else {
81 // host:port
82 if i := strings.LastIndex(hostonly, ":"); i != -1 {
83 hostonly = hostonly[:i]
84 }
85 }
86 url := fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort, req.URL.Path)
87 w.Header().Set("Location", url)
77 } else {88 } else {
78 http.Error(w, "method HEAD is not supported", http.StatusMethodNotAllowed)89 http.Error(w, "method HEAD is not supported", http.StatusMethodNotAllowed)
79 return90 return
@@ -192,7 +203,7 @@
192 listener.Close()203 listener.Close()
193 return nil, fmt.Errorf("cannot start TLS listener: %v", err)204 return nil, fmt.Errorf("cannot start TLS listener: %v", err)
194 }205 }
195 backend.httpsBaseURL = fmt.Sprintf("https://%s", tlsListener.Addr())206 backend.httpsPort = tlsListener.Addr().(*net.TCPAddr).Port
196 goServe(tlsListener, tlsBackend)207 goServe(tlsListener, tlsBackend)
197 }208 }
198 goServe(listener, backend)209 goServe(listener, backend)
199210
=== modified file 'environs/httpstorage/backend_test.go'
--- environs/httpstorage/backend_test.go 2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend_test.go 2013-10-04 08:07:30 +0000
@@ -61,7 +61,7 @@
61 caKeyPEM := []byte(coretesting.CAKey)61 caKeyPEM := []byte(coretesting.CAKey)
62 listener, err = httpstorage.ServeTLS("127.0.0.1:0", embedded, caCertPEM, caKeyPEM, hostnames, testAuthkey)62 listener, err = httpstorage.ServeTLS("127.0.0.1:0", embedded, caCertPEM, caKeyPEM, hostnames, testAuthkey)
63 c.Assert(err, gc.IsNil)63 c.Assert(err, gc.IsNil)
64 return listener, fmt.Sprintf("http://%s/", listener.Addr()), dataDir64 return listener, fmt.Sprintf("http://localhost:%d/", listener.Addr().(*net.TCPAddr).Port), dataDir
65}65}
6666
67type testCase struct {67type testCase struct {
@@ -149,7 +149,7 @@
149 c.Assert(resp.StatusCode, gc.Equals, http.StatusOK)149 c.Assert(resp.StatusCode, gc.Equals, http.StatusOK)
150 location, err := resp.Location()150 location, err := resp.Location()
151 c.Assert(err, gc.IsNil)151 c.Assert(err, gc.IsNil)
152 c.Assert(location.String(), gc.Matches, "https://127.0.0.1:[0-9]{5}/arbitrary")152 c.Assert(location.String(), gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
153}153}
154154
155func (s *backendSuite) TestGet(c *gc.C) {155func (s *backendSuite) TestGet(c *gc.C) {

Subscribers

People subscribed via source and target branches

to status/vote changes: