state/apiserver: Charm upload through HTTP

state/apiserver: Charm upload through HTTP

WIP: for discussion only

https://code.launchpad.net/~dimitern/juju-core/220-post-charm-upload/+merge/198076

(do not edit description out of merge proposal)

[rog, 2013-12-06 15:30:40]

Looks fantastic in general so far.
Some minor comments below, but nothing substantive.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:150: mux.Handle("/charms",
http.HandlerFunc(srv.charmsHandler))
you could call mux.HandleFunc directly.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:155: func (srv *Server) httpAuthenticate(w
http.ResponseWriter, r *http.Request) error {
I think we should probably return the authenticated tag
from this function, and check in the caller that it's a
user tag (we don't want agents to be able to post charms).

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:180: func (srv *Server) requireHttpAuth(w
http.ResponseWriter, r *http.Request) {
I think I'd call this authError or something that
implies it's generating an authentication error.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:182: w.WriteHeader(http.StatusUnauthorized)
http.Error(w, "401 Unauthorized", http.StatusUnauthorized)

would be equivalent and slightly more idiomatic.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:192: http.Error(w, fmt.Sprintf("unsupported method:
%s", r.Method), http.StatusMethodNotAllowed)
s/: %s/%q/ ?

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:213: }
Should we check that the MIME type is correct before proceeding?

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:219: defer tempFile.Close()
also:

defer os.Remove(tempfile.Name())

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:220: buffer := make([]byte, 100000)
I think you can just do:

err := io.Copy(tempFile, part)
instead of the loop below.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:243: }
Presumably this is where we'll actually put the charm into state.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go
File state/apiserver/server_test.go (right):

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go...
state/apiserver/server_test.go:260: func (s *charmsSuite) uploadRequest(c *gc.C,
uri string, paths ...string) (*http.Response, error) {
Rather than passing file paths here, it might work
out more nicely to pass io.Readers - then most of the
tests below won't need to create temporary directories
or files.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go...
state/apiserver/server_test.go:261: body := &bytes.Buffer{}
or
var body bytes.Buffer

[dimitern, 2013-12-10 14:48:36]

Replying to comments, then closing this CL, because I realized I have to split
it in two and the prerequisite is proposed as
https://codereview.appspot.com/40160043, so I'll delete this branch and
repropose it.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:150: mux.Handle("/charms",
http.HandlerFunc(srv.charmsHandler))
On 2013/12/06 15:30:40, rog wrote:
> you could call mux.HandleFunc directly.

Done differently, by extracting a charmsHandler as http.Handler.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:155: func (srv *Server) httpAuthenticate(w
http.ResponseWriter, r *http.Request) error {
On 2013/12/06 15:30:40, rog wrote:
> I think we should probably return the authenticated tag
> from this function, and check in the caller that it's a
> user tag (we don't want agents to be able to post charms).

Even better, now after a successful authentication I check the tag to make sure
it's a user and returning ErrBadCreds if it's not.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:180: func (srv *Server) requireHttpAuth(w
http.ResponseWriter, r *http.Request) {
On 2013/12/06 15:30:40, rog wrote:
> I think I'd call this authError or something that
> implies it's generating an authentication error.

Done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:182: w.WriteHeader(http.StatusUnauthorized)
On 2013/12/06 15:30:40, rog wrote:
> http.Error(w, "401 Unauthorized", http.StatusUnauthorized)
> 
> would be equivalent and slightly more idiomatic.

Since I changed the response to be JSON encoded, this is done slightly
differently.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:192: http.Error(w, fmt.Sprintf("unsupported method:
%s", r.Method), http.StatusMethodNotAllowed)
On 2013/12/06 15:30:40, rog wrote:
> s/: %s/%q/ ?

Done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:213: }
On 2013/12/06 15:30:40, rog wrote:
> Should we check that the MIME type is correct before proceeding?

Wasn't sure if I can, but I as discussed, now this is done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:219: defer tempFile.Close()
On 2013/12/06 15:30:40, rog wrote:
> also:
> 
> defer os.Remove(tempfile.Name())

Done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:220: buffer := make([]byte, 100000)
On 2013/12/06 15:30:40, rog wrote:
> I think you can just do:
> 
> err := io.Copy(tempFile, part)
> instead of the loop below.

Great idea! I was sure I'm overcomplicating things :) Done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/apiserver.go#n...
state/apiserver/apiserver.go:243: }
On 2013/12/06 15:30:40, rog wrote:
> Presumably this is where we'll actually put the charm into state.

Yes, exactly.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go
File state/apiserver/server_test.go (right):

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go...
state/apiserver/server_test.go:260: func (s *charmsSuite) uploadRequest(c *gc.C,
uri string, paths ...string) (*http.Response, error) {
On 2013/12/06 15:30:40, rog wrote:
> Rather than passing file paths here, it might work
> out more nicely to pass io.Readers - then most of the
> tests below won't need to create temporary directories
> or files.

I'm not sure, I'll revisit this after all tests are done.

https://codereview.appspot.com/38370043/diff/1/state/apiserver/server_test.go...
state/apiserver/server_test.go:261: body := &bytes.Buffer{}
On 2013/12/06 15:30:40, rog wrote:
> or
> var body bytes.Buffer

Done.