Issue 40290044: state/apiserver: Charms upload support via HTTP (Closed)
Descriptionstate/apiserver: Charms upload support via HTTP
This implements https://<apiserver>/charms handler,
which supports uploading local charms through the
API server into the state and provider storage.
In brief, HTTPS POST requests with URI /charms?series=<series>
and a binary body, containing a zip file upload
(Content-Type: application/zip header is required)
is processed and added to state database and uploaded
to the provider storage, before returning the response.
HTTP basic authentiction is used for authorization.
Full specification:
https://docs.google.com/a/canonical.com/document/d/1TxnOCLPDqG6y3kCzmUGIkDr0tywXk1XQnHx7G6gO5tI/edit
https://code.launchpad.net/~dimitern/juju-core/220-post-charm-upload/+merge/198438
Requires: https://code.launchpad.net/~dimitern/juju-core/219-state-charm-upload/+merge/198405
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 44
Patch Set 2 : state/apiserver: Charms upload support via HTTP #
Total comments: 9
Patch Set 3 : state/apiserver: Charms upload support via HTTP #
Total comments: 2
Patch Set 4 : state/apiserver: Charms upload support via HTTP #
MessagesTotal messages: 7
Please take a look.
Sign in to reply to this message.
Really good direction, thanks. Quite a few comments but nothing too substantial. https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go#n... state/apiserver/apiserver.go:32: charms *charmsHandler Is this field actually used anywhere? https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:37: Code int `json:"code,omitempty"` For consistency with the rest of the API, this should be named "ErrorCode" and all fields should use the Go capitalisation (i.e. no need to rename the field). The omitempty should stay though. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:61: } else if code == http.StatusOK { This seems odd (why would we call sendError with StatusOK?), and I don't think it can ever happen. I'm not sure whether it's a good idea to conflate the http error status with the error code. How about we only ever return two possible http statuses: 200 and 400, but then use the usual juju API status codes inside the error json? That way we conform to HTTP standards but can also blend in to the other juju API error handling. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:65: err := h.sendJSON(w, &CharmsResponse{Code: code, Error: message}) I'm not sure about this. I think that if we send an error back, we should also set the http response code so we behave correctly from an http perspective (the body can still contain the json error representation). https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:75: if r == nil { This can't happen - not worth testing for IMO. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:78: parts := strings.SplitN(r.Header.Get("Authorization"), " ", 2) I think we should probably use strings.Fields here - it's allowable to have multiple spaces, I believe, and we probably want to fail if there are more than two fields. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:83: // Challenge is a base64-encoded "tag:pass" string. // See RFC 2617, Section 2. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:163: archiveUrl := charm.MustParseURL(preparedUrl) Why not just create the URL directly rather than making a string then parsing it? archiveURL := &charm.URL{ Schema: "local", Series: series, Name: archive.Meta().Name, Revision: archive.Revision(), } https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:188: tempDir := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%d", archive.Meta().Name, rand.Int())) This seems a bit dubious to me - it loses most of the usual temp file logic. Why not just use ioutil.TempDir? We could put both the temporary download file and the bundle directory inside it with no need to use unseeded random number generation. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:211: bundleSha256, size, err := getSha256(tempFile) We don't have to read the file again to get the hash - you can make the BundleTo write to the hasher as the same time (see io.MultiWriter), then use the return from tempfile.Seek(0, 2) to find out the final size. Also, I suspect this isn't actually working currently - won't tempFile be positioned at the end after it's been bundled to? I bet the sum is currently always e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:223: return fmt.Errorf("cannot upload charm: %v", err) s/charm/charm to provider storage/ ? otherwise the user might find the error message surprising. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:272: func (h *charmsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { Personally I prefer top level methods to be at the beginning of the file so we can read the code in a natural order, but YMMV. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:280: h.processPost(w, r) It might be somewhat nicer for readability if processPost looked like: func (h *charmsHandler) processPost(w http.ResponseWriter, r *http.Request) (*charm.URL, error) and the code here was responsible for sending any error or the json response. Then it's obvious at the top level what processPost is doing, and it can be written in more usual style, without so many calls to h.sendError, and guaranteed consistency (it currently is not consistent). We'd probably want to define one or two new error types so we can send back the right http response codes. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:237: // The following is copied from mime/multipart Please mention that it's copied from Writer.CreateFormFile so that it's easy to refer back to the original. I wonder if we should just make things easy for ourselves and use application/octet-stream - that way we won't need to copy this code, and JS clients are likely to have an easier time of it because they can use the default file uploading code. The only benefit of using a correct content type is a slightly better error message for erroneous clients AFAICS. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:241: escapeQuotes := func(s string) string { escapeQuotes := quoteEscaper.Replace https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:247: fmt.Sprintf(`form-data; name="%s"; filename="%s"`, Can't we just use %q here?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go#n... state/apiserver/apiserver.go:32: charms *charmsHandler On 2013/12/10 18:36:43, rog wrote: > Is this field actually used anywhere? Yes it is - in run(), but you're suggesting to create the type there directly? https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:37: Code int `json:"code,omitempty"` On 2013/12/10 18:36:43, rog wrote: > For consistency with the rest of the API, this should be named "ErrorCode" and > all fields should use the Go capitalisation (i.e. no need to rename the field). > The omitempty should stay though. I'm thinking of removing Code altogether. I quite like the lower-case versions myself, it's more javascript-like. And this is not part of the API, it's a separate "channel" of communication. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:61: } else if code == http.StatusOK { On 2013/12/10 18:36:43, rog wrote: > This seems odd (why would we call sendError with StatusOK?), and I don't think > it can ever happen. > > I'm not sure whether it's a good idea to conflate the http error status with the > error code. > > How about we only ever return two possible http statuses: 200 and 400, but then > use the usual juju API status codes inside the error json? > That way we conform to HTTP standards but can also blend in to the other juju > API error handling. Code doesn't really help much, the important info is the error message. I agree we can use 200/400 as status codes depending on whether or not error is empty. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:65: err := h.sendJSON(w, &CharmsResponse{Code: code, Error: message}) On 2013/12/10 18:36:43, rog wrote: > I'm not sure about this. > I think that if we send an error back, we should also set the http response code > so we behave correctly from an http perspective (the body can still contain the > json error representation). Not sure about what? sendJSON sets the http status code as well as the one in the body, both of which are taken from charmresponse's code field. But as I said, i'm removing the code field. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:75: if r == nil { On 2013/12/10 18:36:43, rog wrote: > This can't happen - not worth testing for IMO. Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:78: parts := strings.SplitN(r.Header.Get("Authorization"), " ", 2) On 2013/12/10 18:36:43, rog wrote: > I think we should probably use strings.Fields here - it's allowable to have > multiple spaces, I believe, and we probably want to fail if there are more than > two fields. We don't really care if there are more than 2 fields, because it that case parts[0] will contain "Digest" instead of "Basic". And we're already checking to see if the client specified Basic auth below. If the client sends a valid Authorization header with Basic+base64 part and some extra garbage, we also don't care, as long as we can decode and authenticate the user. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:83: // Challenge is a base64-encoded "tag:pass" string. On 2013/12/10 18:36:43, rog wrote: > // See RFC 2617, Section 2. Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:163: archiveUrl := charm.MustParseURL(preparedUrl) On 2013/12/10 18:36:43, rog wrote: > Why not just create the URL directly rather than making a string > then parsing it? > > archiveURL := &charm.URL{ > Schema: "local", > Series: series, > Name: archive.Meta().Name, > Revision: archive.Revision(), > } Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:188: tempDir := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%d", archive.Meta().Name, rand.Int())) On 2013/12/10 18:36:43, rog wrote: > This seems a bit dubious to me - it loses most of the usual temp file logic. > > Why not just use ioutil.TempDir? We could put both the temporary download file > and the bundle directory inside it with no need to use unseeded random number > generation. Oh, you're right - it was late and I was baffled how to do it, so I copied some stuff from gocheck. ioutil.TempDir is obviously better, thanks. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:211: bundleSha256, size, err := getSha256(tempFile) On 2013/12/10 18:36:43, rog wrote: > We don't have to read the file again to get the hash - you can make the BundleTo > write to the hasher as the same time (see io.MultiWriter), then use the return > from tempfile.Seek(0, 2) to find out the final size. > > Also, I suspect this isn't actually working currently - won't tempFile be > positioned at the end after it's been bundled to? I bet the sum is currently > always e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855. Good idea about the MultiWriter, will do. Incidentially I got rid of the getSha256 function and moved it to the test package only. You're right, because the file was already written to in BundleTo (both in tests and in the actual code), the hash was always reading past EOF so that the digest was always the same. This is fixed now. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:223: return fmt.Errorf("cannot upload charm: %v", err) On 2013/12/10 18:36:43, rog wrote: > s/charm/charm to provider storage/ ? > otherwise the user might find the error message surprising. Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:272: func (h *charmsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { On 2013/12/10 18:36:43, rog wrote: > Personally I prefer top level methods to be at the beginning of the file so we > can read the code in a natural order, but YMMV. Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:280: h.processPost(w, r) On 2013/12/10 18:36:43, rog wrote: > It might be somewhat nicer for readability if processPost > looked like: > > func (h *charmsHandler) processPost(w http.ResponseWriter, r *http.Request) > (*charm.URL, error) > > and the code here was responsible for sending any error or the json response. > Then it's obvious at the top level what processPost is doing, and it can be > written in more usual style, without so many calls to h.sendError, and > guaranteed consistency (it currently is not consistent). > > We'd probably want to define one or two new error types so we can send back the > right http response codes. Will change the return types of processPost, and will send the response here, good idea. What do you mean by it not being consistent? Also, I was thinking earlier about adding specific error types that know how to serialize themselves, but decided it's not worth it. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:237: // The following is copied from mime/multipart On 2013/12/10 18:36:43, rog wrote: > Please mention that it's copied from Writer.CreateFormFile > so that it's easy to refer back to the original. > > I wonder if we should just make things easy for ourselves > and use application/octet-stream - that way we won't > need to copy this code, and JS clients are likely to > have an easier time of it because they can use the default > file uploading code. The only benefit of using a correct > content type is a slightly better error message for erroneous > clients AFAICS. I will include what you suggested to the comment. However, removing this code will make it impossible to test the upload code with the content-type check in place, since CreateFromFile always sends application/octet-stream (a blatant omission from the go standard library in my opinion!). I like the better error message, even at the expense of a bit more complicated implementation. We should treat the users nicely. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:241: escapeQuotes := func(s string) string { On 2013/12/10 18:36:43, rog wrote: > escapeQuotes := quoteEscaper.Replace Done. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:247: fmt.Sprintf(`form-data; name="%s"; filename="%s"`, On 2013/12/10 18:36:43, rog wrote: > Can't we just use %q here? And you're the one complaining that when coping a code from somewhere everything should be left exactly as is :) But ok, fair point will use %q.
Sign in to reply to this message.
A few more comments and suggestions. Not far off now - just the multipart logic to strip out. :-) https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go#n... state/apiserver/apiserver.go:32: charms *charmsHandler On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > Is this field actually used anywhere? > > Yes it is - in run(), but you're suggesting to create the type there directly? Yeah - it doesn't seem to add much value here. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:37: Code int `json:"code,omitempty"` On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > For consistency with the rest of the API, this should be named "ErrorCode" and > > all fields should use the Go capitalisation (i.e. no need to rename the > field). > > The omitempty should stay though. > > I'm thinking of removing Code altogether. I quite like the lower-case versions > myself, it's more javascript-like. And this is not part of the API, it's a > separate "channel" of communication. As John said in the standup, it really is part of the API, even though it's not websockets based. Consistency is a virtue - if we're consistent here, it's less of a surprise to clients and they may even be able to use some of the same code for parsing the response. In every other part of the API we use capitalised field names in our JSON. Also, from the Go client point of view, if we use the same error code scheme, we can trivially make calls that happen to use the non-websocket parts of the API return error messages that fit within the same ErrCode scheme that we use in other places - the detail of how the call is implemented can be hidden. I'm OK if you want to leave ErrorCode out for now (it's future-compatible if we want to add it later) but let's please be consistent in the field capitalisation. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:61: } else if code == http.StatusOK { On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > This seems odd (why would we call sendError with StatusOK?), and I don't think > > it can ever happen. > > > > I'm not sure whether it's a good idea to conflate the http error status with > the > > error code. > > > > How about we only ever return two possible http statuses: 200 and 400, but > then > > use the usual juju API status codes inside the error json? > > That way we conform to HTTP standards but can also blend in to the other juju > > API error handling. > > Code doesn't really help much, the important info is the error message. I agree > we can use 200/400 as status codes depending on whether or not error is empty. The code will be useful in the future. For example we probably want to return a params.CodeNotFound code when a client tries to download a charm that does not exist. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:65: err := h.sendJSON(w, &CharmsResponse{Code: code, Error: message}) On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > I'm not sure about this. > > I think that if we send an error back, we should also set the http response > code > > so we behave correctly from an http perspective (the body can still contain > the > > json error representation). > > Not sure about what? sendJSON sets the http status code as well as the one in > the body, both of which are taken from charmresponse's code field. But as I > said, i'm removing the code field. Sorry, that was a stale comment - I only realised later that the code was also used to set the HTTP response code. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:78: parts := strings.SplitN(r.Header.Get("Authorization"), " ", 2) On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > I think we should probably use strings.Fields here - it's allowable to have > > multiple spaces, I believe, and we probably want to fail if there are more > than > > two fields. > > We don't really care if there are more than 2 fields, because it that case > parts[0] will contain "Digest" instead of "Basic". And we're already checking to > see if the client specified Basic auth below. > If the client sends a valid Authorization header with Basic+base64 part and some > extra garbage, we also don't care, as long as we can decode and authenticate the > user. If the user sends an Authorisation of "Basic dXNlcjpwYXNz" which is perfectly valid according to rfc 2617, this code will fail. It's trivial to make it work, so let's do that, please. We could check len(fields) >= 2 if you'd like users to be able to send garbage (that's not actually a bad idea for future compatibility reasons) https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:247: fmt.Sprintf(`form-data; name="%s"; filename="%s"`, On 2013/12/11 10:55:01, dimitern wrote: > On 2013/12/10 18:36:43, rog wrote: > > Can't we just use %q here? > > And you're the one complaining that when coping a code from somewhere everything > should be left exactly as is :) But ok, fair point will use %q. Um, if we're gonna use %q it's really wrong to escapeQuotes... We should use either one or the other. Sorry that I didn't make that clear. https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:52: h.sendJSON(w, http.StatusOK, &CharmsResponse{CharmURL: charmUrl.String()}) This looks much nicer, thanks. https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:179: tempDir, err := ioutil.TempDir("", archive.Meta().Name) One other possible simplification: How about something like this? tempDir, err := ioutil.TempDir("", "charm-download") if err ... defer os.RemoveAll(tempDir) zipPath := filepath.Join(tempDir, "bundle.zip") charmPath := filepath.Join(tempDir, "charm") then everything's under one directory and we only need to defer one remove (and things are slightly more obvious if something goes wrong and the temporary files are left around) https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:203: multiWriter := io.MultiWriter(hash, tempFile) or inline https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... state/apiserver/charms_test.go:293: if _, err := source.Seek(0, 0); err != nil { ISTM that the rewind logic should be in the same place that wrote the file. Then you could have getSHA256(r io.Reader) which seems more usual. Oh yes, in Go it's conventional to capitalise the whole acronym if the first letter is capitalised (hence for instance we have ServeHTTP not ServeHttp) Could we do that throughout, please? (it applies to "URL" too)
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/apiserver.go#n... state/apiserver/apiserver.go:32: charms *charmsHandler On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > Is this field actually used anywhere? > > > > Yes it is - in run(), but you're suggesting to create the type there directly? > > Yeah - it doesn't seem to add much value here. Removed and instantiated directly below in run(). https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:37: Code int `json:"code,omitempty"` On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > For consistency with the rest of the API, this should be named "ErrorCode" > and > > > all fields should use the Go capitalisation (i.e. no need to rename the > > field). > > > The omitempty should stay though. > > > > I'm thinking of removing Code altogether. I quite like the lower-case versions > > myself, it's more javascript-like. And this is not part of the API, it's a > > separate "channel" of communication. > > As John said in the standup, it really is part of the API, even though it's not > websockets based. > > Consistency is a virtue - if we're consistent here, it's less of a surprise to > clients and they may even be able to use some of the same code for parsing the > response. > > In every other part of the API we use capitalised field names in our JSON. > > Also, from the Go client point of view, if we use the same error code scheme, > we can trivially make calls that happen to use the non-websocket parts of the > API return error messages that fit within the same ErrCode scheme that we use in > other places - the detail of how the call is implemented can be hidden. > > I'm OK if you want to leave ErrorCode out for now (it's future-compatible if we > want to add it later) but let's please be consistent in the field > capitalisation. Ok, let's be consistent then :) Removed json renaming, left omittempty. Let's add ErrorCode as needed in the future. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:61: } else if code == http.StatusOK { On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > This seems odd (why would we call sendError with StatusOK?), and I don't > think > > > it can ever happen. > > > > > > I'm not sure whether it's a good idea to conflate the http error status with > > the > > > error code. > > > > > > How about we only ever return two possible http statuses: 200 and 400, but > > then > > > use the usual juju API status codes inside the error json? > > > That way we conform to HTTP standards but can also blend in to the other > juju > > > API error handling. > > > > Code doesn't really help much, the important info is the error message. I > agree > > we can use 200/400 as status codes depending on whether or not error is empty. > > The code will be useful in the future. > For example we probably want to return a params.CodeNotFound code when a client > tries to download a charm that does not exist. Agreed, for now though we don't need it. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:65: err := h.sendJSON(w, &CharmsResponse{Code: code, Error: message}) On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > I'm not sure about this. > > > I think that if we send an error back, we should also set the http response > > code > > > so we behave correctly from an http perspective (the body can still contain > > the > > > json error representation). > > > > Not sure about what? sendJSON sets the http status code as well as the one in > > the body, both of which are taken from charmresponse's code field. But as I > > said, i'm removing the code field. > > Sorry, that was a stale comment - I only realised later that the code was also > used to set the HTTP response code. No problem :) https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms.go#newc... state/apiserver/charms.go:78: parts := strings.SplitN(r.Header.Get("Authorization"), " ", 2) On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > I think we should probably use strings.Fields here - it's allowable to have > > > multiple spaces, I believe, and we probably want to fail if there are more > > than > > > two fields. > > > > We don't really care if there are more than 2 fields, because it that case > > parts[0] will contain "Digest" instead of "Basic". And we're already checking > to > > see if the client specified Basic auth below. > > If the client sends a valid Authorization header with Basic+base64 part and > some > > extra garbage, we also don't care, as long as we can decode and authenticate > the > > user. > > If the user sends an Authorisation of "Basic dXNlcjpwYXNz" which is perfectly > valid according to rfc 2617, this code will fail. It's trivial to make it work, > so let's do that, please. We could check len(fields) >= 2 if you'd like users to > be able to send garbage (that's not actually a bad idea for future compatibility > reasons) Actually, I ripped of most of this parsing code from some example I've seen online, I admit. Changed to use strings.Fields, as suggested. https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/1/state/apiserver/charms_test.go... state/apiserver/charms_test.go:247: fmt.Sprintf(`form-data; name="%s"; filename="%s"`, On 2013/12/11 13:25:41, rog wrote: > On 2013/12/11 10:55:01, dimitern wrote: > > On 2013/12/10 18:36:43, rog wrote: > > > Can't we just use %q here? > > > > And you're the one complaining that when coping a code from somewhere > everything > > should be left exactly as is :) But ok, fair point will use %q. > > Um, if we're gonna use %q it's really wrong to escapeQuotes... > We should use either one or the other. > Sorry that I didn't make that clear. Ah, good to know %q works like escapeQuotes. But anyway, this code is gone now, as it's not needed without multipart support. https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:52: h.sendJSON(w, http.StatusOK, &CharmsResponse{CharmURL: charmUrl.String()}) On 2013/12/11 13:25:41, rog wrote: > This looks much nicer, thanks. \o/ https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:179: tempDir, err := ioutil.TempDir("", archive.Meta().Name) On 2013/12/11 13:25:41, rog wrote: > One other possible simplification: > How about something like this? > > tempDir, err := ioutil.TempDir("", "charm-download") > if err ... > defer os.RemoveAll(tempDir) > > zipPath := filepath.Join(tempDir, "bundle.zip") > charmPath := filepath.Join(tempDir, "charm") > > then everything's under one directory and we only > need to defer one remove (and things are slightly > more obvious if something goes wrong and the temporary > files are left around) Done similarly. https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms.go#... state/apiserver/charms.go:203: multiWriter := io.MultiWriter(hash, tempFile) On 2013/12/11 13:25:41, rog wrote: > or inline Done. https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... state/apiserver/charms_test.go:293: if _, err := source.Seek(0, 0); err != nil { On 2013/12/11 13:25:41, rog wrote: > ISTM that the rewind logic should be in the same place that wrote the file. > Then you could have getSHA256(r io.Reader) which seems more usual. > > Oh yes, in Go it's conventional to capitalise the whole acronym if the first > letter is capitalised (hence for instance we have ServeHTTP not ServeHttp) > Could we do that throughout, please? (it applies to "URL" too) Done. Although where were you when charm.Bundle.BundleSha256() landed? :)
Sign in to reply to this message.
LGTM, thanks! https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... File state/apiserver/charms_test.go (right): https://codereview.appspot.com/40290044/diff/20001/state/apiserver/charms_tes... state/apiserver/charms_test.go:293: if _, err := source.Seek(0, 0); err != nil { On 2013/12/11 15:23:41, dimitern wrote: > On 2013/12/11 13:25:41, rog wrote: > > ISTM that the rewind logic should be in the same place that wrote the file. > > Then you could have getSHA256(r io.Reader) which seems more usual. > > > > Oh yes, in Go it's conventional to capitalise the whole acronym if the first > > letter is capitalised (hence for instance we have ServeHTTP not ServeHttp) > > Could we do that throughout, please? (it applies to "URL" too) > > Done. Although where were you when charm.Bundle.BundleSha256() landed? :) Not on the ball, evidently :-) Thanks. https://codereview.appspot.com/40290044/diff/40001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/40001/state/apiserver/charms.go#... state/apiserver/charms.go:136: defer r.Body.Close() No need for this, I think.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/40290044/diff/40001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/40290044/diff/40001/state/apiserver/charms.go#... state/apiserver/charms.go:136: defer r.Body.Close() On 2013/12/11 16:41:02, rog wrote: > No need for this, I think. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
