Merge lp:~frankban/juju-core/get-charm-api into lp:~go-bot/juju-core/trunk

Proposed by Francesco Banconi on 2014-02-24
Status: Merged
Approved by: Francesco Banconi on 2014-02-26
Approved revision: 2363
Merged at revision: 2355
Proposed branch: lp:~frankban/juju-core/get-charm-api
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 555 lines (+356/-30)
4 files modified
state/api/params/internal.go (+4/-3)
state/apiserver/apiserver.go (+1/-1)
state/apiserver/charms.go (+168/-3)
state/apiserver/charms_test.go (+183/-23)
To merge this branch: bzr merge lp:~frankban/juju-core/get-charm-api
Reviewer Review Type Date Requested Status
Juju Engineering 2014-02-24 Pending
Review via email: mp+207994@code.launchpad.net

Commit message

Implement the get charm file API.

Add to the HTTPS API server the ability
to serve local charm files.

As discussed with Rick and Dimiter if the query includes the file,
then the file contents are served. If file points to a directory
an error is returned. If the query does not include a file, then
a JSON response is sent including a list of charm files.

Description of the change

Implement the get charm file API.

Add to the HTTPS API server the ability
to serve local charm files.

As discussed with Rick and Dimiter if the query includes the file,
then the file contents are served. If file points to a directory
an error is returned. If the query does not include a file, then
a JSON response is sent including a list of charm files.

QA:
- Download the Ghost charm from https://github.com/hatched/ghost-charm
  (the zip archive can be downloaded from the sidebar on the right).
- Bootstrap a local environment using this branch.
- Upload the ghost local charm, e.g.:
  `curl -ikL --data-binary @/path/to/ghost-charm-master.zip -H "Content-Type: application/zip" https://user-admin:MYPASSWD@10.0.3.1:17070/charms?series=precise`.
  The response should be something like: {"CharmURL":"local:precise/ghost-4"}.
- Download ghost charm files, e.g.:
  `curl -ikLG -d "url=local:precise/ghost-4&file=icon.svg" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`
  or
  `curl -ikLG -d "url=local:precise/ghost-4&file=hooks/install" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`:
  you should see the file contents.
- Ensure directories are not allowed, e.g.:
  `curl -ikLG -d "url=local:precise/ghost-4&file=hooks" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`: you should see a 403 forbidden.
- Retrieve a list of the charm files:
  `curl -ikLG -d "url=local:precise/ghost-4" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`.
- Done, destroy the environment, thank you!

https://codereview.appspot.com/67750045/

To post a comment you must log in.
Francesco Banconi (frankban) wrote :

Reviewers: mp+207994_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the get charm file API.

Add to the HTTPS API server the ability
to serve local charm files.

As discussed with Rick and Dimiter if the query includes the file,
then the file contents are served. If file points to a directory
an error is returned. If the query does not include a file, then
a JSON response is sent including a list of charm files.

QA:
- Download the Ghost charm from https://github.com/hatched/ghost-charm
   (the zip archive can be downloaded from the sidebar on the right).
- Bootstrap a local environment using this branch.
- Upload the ghost local charm, e.g.:
   `curl -ikL --data-binary @/path/to/ghost-charm-master.zip -H
"Content-Type: application/zip"
https://user-admin:MYPASSWD@10.0.3.1:17070/charms?series=precise`.
   The response should be something like:
{"CharmURL":"local:precise/ghost-4"}.
- Download ghost charm files, e.g.:
   `curl -ikLG -d "url=local:precise/ghost-4&file=icon.svg"
https://user-admin:MYPASSWD@10.0.3.1:17070/charms`
   or
   `curl -ikLG -d "url=local:precise/ghost-4&file=hooks/install"
https://user-admin:MYPASSWD@10.0.3.1:17070/charms`:
   you should see the file contents.
- Ensure directories are not allowed, e.g.:
   `curl -ikLG -d "url=local:precise/ghost-4&file=hooks"
https://user-admin:MYPASSWD@10.0.3.1:17070/charms`: you should see a 403
forbidden.
- Retrieve a list of the charm files:
   `curl -ikLG -d "url=local:precise/ghost-4"
https://user-admin:MYPASSWD@10.0.3.1:17070/charms`.
- Done, destroy the environment, thank you!

https://code.launchpad.net/~frankban/juju-core/get-charm-api/+merge/207994

(do not edit description out of merge proposal)

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

Affected files (+352, -30 lines):
   A [revision details]
   M state/api/params/internal.go
   M state/apiserver/apiserver.go
   M state/apiserver/charms.go
   M state/apiserver/charms_test.go

Dimiter Naydenov (dimitern) wrote :

LGTM, thanks for doing this!

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go#newcode509
state/api/params/internal.go:509: // CharmsResponse is the server
response to charm upload or get requests.
s/get/GET/

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms.go#newcode540
state/apiserver/charms.go:540: if !strings.HasPrefix(filePath,
bundlePath+"/") {
Please, use filepath.Dir(filePath) != bundlePath here instead.

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms_test.go#newcode249
state/apiserver/charms_test.go:249: "expected url=CharmURL query
argument")
I'd prefer if you formatted this and other places where
assertErrorResponse is too long, like this:

s.assertErrorResponse(
     c, resp, http.StatusBadRequest,
     expected url=CharmURL query argument",
)

https://codereview.appspot.com/67750045/

lp:~frankban/juju-core/get-charm-api updated on 2014-02-25
2355. By Francesco Banconi on 2014-02-25

Changes as per review.

2356. By Francesco Banconi on 2014-02-25

Merge trunk.

Dimiter Naydenov (dimitern) wrote :

On second though and after discussion with the team on today's standup,
some changes are needed before this can land. Please ping me to take a
look again when ready.

Instead of downloading AND extracting the charm, we got a general
agreement you should cache the charm archive, and then use something
like charm.zipOpen to read a single file from it, giving a path. You can
also simplify file path related operations, because the zip reader
allows you to range over all files, including their paths and do simple
matching urlFileArg == zipFile.Name(). Look into findArchiveRootDir and
fixPath inside the charms handler in apiserver.

https://codereview.appspot.com/67750045/

lp:~frankban/juju-core/get-charm-api updated on 2014-02-25
2357. By Francesco Banconi on 2014-02-25

Checkpoint.

2358. By Francesco Banconi on 2014-02-25

Checkpoint.

2359. By Francesco Banconi on 2014-02-25

All done.

2360. By Francesco Banconi on 2014-02-25

Merge trunk.

2361. By Francesco Banconi on 2014-02-25

Handle content type and length.

Francesco Banconi (frankban) wrote :

Please take a look.

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go#newcode509
state/api/params/internal.go:509: // CharmsResponse is the server
response to charm upload or get requests.
On 2014/02/25 10:07:09, dimitern wrote:
> s/get/GET/

Done.

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms_test.go#newcode249
state/apiserver/charms_test.go:249: "expected url=CharmURL query
argument")
On 2014/02/25 10:07:09, dimitern wrote:
> I'd prefer if you formatted this and other places where
assertErrorResponse is
> too long, like this:

> s.assertErrorResponse(
> c, resp, http.StatusBadRequest,
> expected url=CharmURL query argument",
> )

Done.

https://codereview.appspot.com/67750045/

Dimiter Naydenov (dimitern) wrote :
Download full text (3.6 KiB)

Much better, thanks! LGTM with just a few formatting suggestions below.
Make sure you've run gofmt on the file, because indentation looks
inconsistent in places.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode125
state/apiserver/charms.go:125: for _, file := range reader.File {
gofmt again?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode138
state/apiserver/charms.go:138: } else {
Instead of else, you could just return in the if block above.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode511
state/apiserver/charms.go:511: // processGet handles a charm file
download GET request after authentication.
s/download//

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode546
state/apiserver/charms.go:546: // save the corresponding zip archive to
the given charmArchivePath.
s/save/saves/

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode580
state/apiserver/charms.go:580: if err =
os.Rename(tempCharmArchive.Name(), charmArchivePath); err != nil {
In this case I think you should defer os.Remove(tempCharmArchive.Name())
here.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode265
state/apiserver/charms_test.go:265: func (s *charmsSuite)
TestGetReturnsFileNotFound(c *gc.C) {
s/TestGetReturnsFileNotFound/TestGetReturnsNotFoundWhenMissing/ ?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode271
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files
not included in the charm.
s/fo/for/

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode271
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files
not included in the charm.
Please, put a blank line before this one, for readability (I'll use "b"
in all similar cases below).

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode283
state/apiserver/charms_test.go:283: func (s *charmsSuite)
TestGetReturnsDirectoryForbidden(c *gc.C) {
Similarly,
s/TestGetReturnsDirectoryForbidden/TestGetReturnsForbiddenWithDirectory/
?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode289
state/apiserver/charms_test.go:289: // Ensure a 403 is returned if the
requested file is a directory.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode302
state/apiserver/charms_test.go:302: // Ensure the file contents are
properly returned.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode335
state/apiserver/charms_test.go:335: // Ensure charm files are properly
listed.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode353
state/apiserver/charms_test.go:353: // Create and save t...

Read more...

lp:~frankban/juju-core/get-charm-api updated on 2014-02-26
2362. By Francesco Banconi on 2014-02-26

Changes as per review.

2363. By Francesco Banconi on 2014-02-26

Merge trunk.

Francesco Banconi (frankban) wrote :

Please take a look.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode125
state/apiserver/charms.go:125: for _, file := range reader.File {
On 2014/02/25 17:40:03, dimitern wrote:
> gofmt again?

Done, and it seems already ok.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode138
state/apiserver/charms.go:138: } else {
On 2014/02/25 17:40:03, dimitern wrote:
> Instead of else, you could just return in the if block above.

We need contents in the scope, that's why I used else.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode511
state/apiserver/charms.go:511: // processGet handles a charm file
download GET request after authentication.
On 2014/02/25 17:40:03, dimitern wrote:
> s/download//

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode546
state/apiserver/charms.go:546: // save the corresponding zip archive to
the given charmArchivePath.
On 2014/02/25 17:40:03, dimitern wrote:
> s/save/saves/

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode580
state/apiserver/charms.go:580: if err =
os.Rename(tempCharmArchive.Name(), charmArchivePath); err != nil {
On 2014/02/25 17:40:03, dimitern wrote:
> In this case I think you should defer
os.Remove(tempCharmArchive.Name()) here.

Good catch, fixed.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode265
state/apiserver/charms_test.go:265: func (s *charmsSuite)
TestGetReturnsFileNotFound(c *gc.C) {
On 2014/02/25 17:40:03, dimitern wrote:
> s/TestGetReturnsFileNotFound/TestGetReturnsNotFoundWhenMissing/ ?

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode271
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files
not included in the charm.
On 2014/02/25 17:40:03, dimitern wrote:
> s/fo/for/

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode283
state/apiserver/charms_test.go:283: func (s *charmsSuite)
TestGetReturnsDirectoryForbidden(c *gc.C) {
On 2014/02/25 17:40:03, dimitern wrote:
> Similarly,

s/TestGetReturnsDirectoryForbidden/TestGetReturnsForbiddenWithDirectory/
?

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode441
state/apiserver/charms_test.go:441: ctype :=
resp.Header.Get("content-type")
On 2014/02/25 17:40:03, dimitern wrote:
> s/content-type/Content-Type/

Done.

https://codereview.appspot.com/67750045/

Dimiter Naydenov (dimitern) wrote :

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode138
state/apiserver/charms.go:138: } else {
On 2014/02/26 09:34:28, frankban wrote:
> On 2014/02/25 17:40:03, dimitern wrote:
> > Instead of else, you could just return in the if block above.

> We need contents in the scope, that's why I used else.

Then you can just move the assignment out of the if.

https://codereview.appspot.com/67750045/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/api/params/internal.go'
2--- state/api/params/internal.go 2014-02-03 11:45:15 +0000
3+++ state/api/params/internal.go 2014-02-26 09:13:18 +0000
4@@ -506,10 +506,11 @@
5 Results []RelationUnitsWatchResult
6 }
7
8-// CharmsResponse is the server response to a charm upload request.
9+// CharmsResponse is the server response to charm upload or GET requests.
10 type CharmsResponse struct {
11- Error string `json:",omitempty"`
12- CharmURL string `json:",omitempty"`
13+ Error string `json:",omitempty"`
14+ CharmURL string `json:",omitempty"`
15+ Files []string `json:",omitempty"`
16 }
17
18 // RunParams is used to provide the parameters to the Run method.
19
20=== modified file 'state/apiserver/apiserver.go'
21--- state/apiserver/apiserver.go 2014-02-04 10:18:04 +0000
22+++ state/apiserver/apiserver.go 2014-02-26 09:13:18 +0000
23@@ -152,7 +152,7 @@
24 }()
25 mux := http.NewServeMux()
26 mux.HandleFunc("/", srv.apiHandler)
27- mux.Handle("/charms", &charmsHandler{state: srv.state})
28+ mux.Handle("/charms", &charmsHandler{state: srv.state, dataDir: srv.dataDir})
29 // The error from http.Serve is not interesting.
30 http.Serve(lis, mux)
31 }
32
33=== modified file 'state/apiserver/charms.go'
34--- state/apiserver/charms.go 2014-02-03 14:31:54 +0000
35+++ state/apiserver/charms.go 2014-02-26 09:13:18 +0000
36@@ -12,10 +12,12 @@
37 "fmt"
38 "io"
39 "io/ioutil"
40+ "mime"
41 "net/http"
42 "net/url"
43 "os"
44 "path/filepath"
45+ "strconv"
46 "strings"
47
48 "github.com/errgo/errgo"
49@@ -30,9 +32,14 @@
50
51 // charmsHandler handles charm upload through HTTPS in the API server.
52 type charmsHandler struct {
53- state *state.State
54+ state *state.State
55+ dataDir string
56 }
57
58+// zipContentsSenderFunc functions are responsible of sending a zip archive
59+// related response. The zip archive can be accessed through the given reader.
60+type zipContentsSenderFunc func(w http.ResponseWriter, r *http.Request, reader *zip.ReadCloser)
61+
62 func (h *charmsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
63 if err := h.authenticate(r); err != nil {
64 h.authError(w)
65@@ -41,13 +48,28 @@
66
67 switch r.Method {
68 case "POST":
69+ // Add a local charm to the store provider.
70+ // Requires a "series" query specifying the series to use for the charm.
71 charmURL, err := h.processPost(r)
72 if err != nil {
73 h.sendError(w, http.StatusBadRequest, err.Error())
74 return
75 }
76 h.sendJSON(w, http.StatusOK, &params.CharmsResponse{CharmURL: charmURL.String()})
77- // Possible future extensions, like GET.
78+ case "GET":
79+ // Retrieve or list charm files.
80+ // Requires "url" (charm URL) and an optional "file" (the path to the
81+ // charm file) to be included in the query.
82+ if charmArchivePath, filePath, err := h.processGet(r); err != nil {
83+ // An error occurred retrieving the charm bundle.
84+ h.sendError(w, http.StatusBadRequest, err.Error())
85+ } else if filePath == "" {
86+ // The client requested the list of charm files.
87+ sendZipContents(w, r, charmArchivePath, h.fileListSender)
88+ } else {
89+ // The client requested a specific file.
90+ sendZipContents(w, r, charmArchivePath, h.fileSender(filePath))
91+ }
92 default:
93 h.sendError(w, http.StatusMethodNotAllowed, fmt.Sprintf("unsupported method: %q", r.Method))
94 }
95@@ -55,6 +77,7 @@
96
97 // sendJSON sends a JSON-encoded response to the client.
98 func (h *charmsHandler) sendJSON(w http.ResponseWriter, statusCode int, response *params.CharmsResponse) error {
99+ w.Header().Set("Content-Type", "application/json")
100 w.WriteHeader(statusCode)
101 body, err := json.Marshal(response)
102 if err != nil {
103@@ -64,6 +87,72 @@
104 return nil
105 }
106
107+// sendZipContents uses the given zipContentsSenderFunc to send a response
108+// related to the zip archive located in the given archivePath.
109+func sendZipContents(w http.ResponseWriter, r *http.Request, archivePath string, zipContentsSender zipContentsSenderFunc) {
110+ reader, err := zip.OpenReader(archivePath)
111+ if err != nil {
112+ http.Error(
113+ w, fmt.Sprintf("unable to read archive in %q: %v", archivePath, err),
114+ http.StatusInternalServerError)
115+ return
116+ }
117+ defer reader.Close()
118+ // The zipContentsSenderFunc will handle the zip contents, set up and send
119+ // an appropriate response.
120+ zipContentsSender(w, r, reader)
121+}
122+
123+// fileListSender sends a JSON-encoded response to the client including the
124+// list of files contained in the zip archive.
125+func (h *charmsHandler) fileListSender(w http.ResponseWriter, r *http.Request, reader *zip.ReadCloser) {
126+ var files []string
127+ for _, file := range reader.File {
128+ fileInfo := file.FileInfo()
129+ if !fileInfo.IsDir() {
130+ files = append(files, file.Name)
131+ }
132+ }
133+ h.sendJSON(w, http.StatusOK, &params.CharmsResponse{Files: files})
134+}
135+
136+// fileSender returns a zipContentsSenderFunc which is responsible of sending
137+// the contents of filePath included in the given zip.
138+// A 404 page not found is returned if path does not exist in the zip.
139+// A 403 forbidden error is returned if path points to a directory.
140+func (h *charmsHandler) fileSender(filePath string) zipContentsSenderFunc {
141+ return func(w http.ResponseWriter, r *http.Request, reader *zip.ReadCloser) {
142+ for _, file := range reader.File {
143+ if h.fixPath(file.Name) != filePath {
144+ continue
145+ }
146+ fileInfo := file.FileInfo()
147+ if fileInfo.IsDir() {
148+ http.Error(w, "directory listing not allowed", http.StatusForbidden)
149+ return
150+ }
151+ if contents, err := file.Open(); err != nil {
152+ http.Error(
153+ w, fmt.Sprintf("unable to read file %q: %v", filePath, err),
154+ http.StatusInternalServerError)
155+ return
156+ } else {
157+ defer contents.Close()
158+ ctype := mime.TypeByExtension(filepath.Ext(filePath))
159+ if ctype != "" {
160+ w.Header().Set("Content-Type", ctype)
161+ }
162+ w.Header().Set("Content-Length", strconv.FormatInt(fileInfo.Size(), 10))
163+ w.WriteHeader(http.StatusOK)
164+ io.Copy(w, contents)
165+ }
166+ return
167+ }
168+ http.NotFound(w, r)
169+ return
170+ }
171+}
172+
173 // sendError sends a JSON-encoded error response.
174 func (h *charmsHandler) sendError(w http.ResponseWriter, statusCode int, message string) error {
175 return h.sendJSON(w, statusCode, &params.CharmsResponse{Error: message})
176@@ -113,7 +202,7 @@
177 query := r.URL.Query()
178 series := query.Get("series")
179 if series == "" {
180- return nil, fmt.Errorf("expected series= URL argument")
181+ return nil, fmt.Errorf("expected series=URL argument")
182 }
183 // Make sure the content type is zip.
184 contentType := r.Header.Get("Content-Type")
185@@ -419,3 +508,79 @@
186 }
187 return nil
188 }
189+
190+// processGet handles a charm file GET request after authentication.
191+// It returns the bundle path, the requested file path (if any) and an error.
192+func (h *charmsHandler) processGet(r *http.Request) (string, string, error) {
193+ query := r.URL.Query()
194+
195+ // Retrieve and validate query parameters.
196+ curl := query.Get("url")
197+ if curl == "" {
198+ return "", "", fmt.Errorf("expected url=CharmURL query argument")
199+ }
200+ var filePath string
201+ file := query.Get("file")
202+ if file == "" {
203+ filePath = ""
204+ } else {
205+ filePath = h.fixPath(file)
206+ }
207+
208+ // Prepare the bundle directories.
209+ name := charm.Quote(curl)
210+ charmArchivePath := filepath.Join(h.dataDir, "charm-get-cache", name+".zip")
211+
212+ // Check if the charm archive is already in the cache.
213+ if _, err := os.Stat(charmArchivePath); os.IsNotExist(err) {
214+ // Download the charm archive and save it to the cache.
215+ if err = h.downloadCharm(name, charmArchivePath); err != nil {
216+ return "", "", fmt.Errorf("unable to retrieve and save the charm: %v", err)
217+ }
218+ } else if err != nil {
219+ return "", "", fmt.Errorf("cannot access the charms cache: %v", err)
220+ }
221+ return charmArchivePath, filePath, nil
222+}
223+
224+// downloadCharm downloads the given charm name from the provider storage and
225+// saves the corresponding zip archive to the given charmArchivePath.
226+func (h *charmsHandler) downloadCharm(name, charmArchivePath string) error {
227+ // Get the provider storage.
228+ storage, err := envtesting.GetEnvironStorage(h.state)
229+ if err != nil {
230+ return errgo.Annotate(err, "cannot access provider storage")
231+ }
232+
233+ // Use the storage to retrieve and save the charm archive.
234+ reader, err := storage.Get(name)
235+ if err != nil {
236+ return errgo.Annotate(err, "charm not found in the provider storage")
237+ }
238+ defer reader.Close()
239+ data, err := ioutil.ReadAll(reader)
240+ if err != nil {
241+ return errgo.Annotate(err, "cannot read charm data")
242+ }
243+ // In order to avoid races, the archive is saved in a temporary file which
244+ // is then atomically renamed. The temporary file is created in the
245+ // charm cache directory so that we can safely assume the rename source and
246+ // target live in the same file system.
247+ cacheDir := filepath.Dir(charmArchivePath)
248+ if err = os.MkdirAll(cacheDir, 0755); err != nil {
249+ return errgo.Annotate(err, "cannot create the charms cache")
250+ }
251+ tempCharmArchive, err := ioutil.TempFile(cacheDir, "charm")
252+ if err != nil {
253+ return errgo.Annotate(err, "cannot create charm archive temp file")
254+ }
255+ defer tempCharmArchive.Close()
256+ if err = ioutil.WriteFile(tempCharmArchive.Name(), data, 0644); err != nil {
257+ return errgo.Annotate(err, "error processing charm archive download")
258+ }
259+ if err = os.Rename(tempCharmArchive.Name(), charmArchivePath); err != nil {
260+ defer os.Remove(tempCharmArchive.Name())
261+ return errgo.Annotate(err, "error renaming the charm archive")
262+ }
263+ return nil
264+}
265
266=== modified file 'state/apiserver/charms_test.go'
267--- state/apiserver/charms_test.go 2014-01-30 16:55:00 +0000
268+++ state/apiserver/charms_test.go 2014-02-26 09:13:18 +0000
269@@ -4,6 +4,8 @@
270 package apiserver_test
271
272 import (
273+ "archive/zip"
274+ "bytes"
275 "encoding/json"
276 "fmt"
277 "io"
278@@ -55,13 +57,13 @@
279 func (s *charmsSuite) TestRequiresAuth(c *gc.C) {
280 resp, err := s.sendRequest(c, "", "", "GET", s.charmsURI(c, ""), "", nil)
281 c.Assert(err, gc.IsNil)
282- s.assertResponse(c, resp, http.StatusUnauthorized, "unauthorized", "")
283+ s.assertErrorResponse(c, resp, http.StatusUnauthorized, "unauthorized")
284 }
285
286-func (s *charmsSuite) TestUploadRequiresPOST(c *gc.C) {
287- resp, err := s.authRequest(c, "GET", s.charmsURI(c, ""), "", nil)
288+func (s *charmsSuite) TestRequiresPOSTorGET(c *gc.C) {
289+ resp, err := s.authRequest(c, "PUT", s.charmsURI(c, ""), "", nil)
290 c.Assert(err, gc.IsNil)
291- s.assertResponse(c, resp, http.StatusMethodNotAllowed, `unsupported method: "GET"`, "")
292+ s.assertErrorResponse(c, resp, http.StatusMethodNotAllowed, `unsupported method: "PUT"`)
293 }
294
295 func (s *charmsSuite) TestAuthRequiresUser(c *gc.C) {
296@@ -77,18 +79,18 @@
297
298 resp, err := s.sendRequest(c, machine.Tag(), password, "GET", s.charmsURI(c, ""), "", nil)
299 c.Assert(err, gc.IsNil)
300- s.assertResponse(c, resp, http.StatusUnauthorized, "unauthorized", "")
301+ s.assertErrorResponse(c, resp, http.StatusUnauthorized, "unauthorized")
302
303 // Now try a user login.
304 resp, err = s.authRequest(c, "GET", s.charmsURI(c, ""), "", nil)
305 c.Assert(err, gc.IsNil)
306- s.assertResponse(c, resp, http.StatusMethodNotAllowed, `unsupported method: "GET"`, "")
307+ s.assertErrorResponse(c, resp, http.StatusBadRequest, "expected url=CharmURL query argument")
308 }
309
310 func (s *charmsSuite) TestUploadRequiresSeries(c *gc.C) {
311 resp, err := s.authRequest(c, "POST", s.charmsURI(c, ""), "", nil)
312 c.Assert(err, gc.IsNil)
313- s.assertResponse(c, resp, http.StatusBadRequest, "expected series= URL argument", "")
314+ s.assertErrorResponse(c, resp, http.StatusBadRequest, "expected series=URL argument")
315 }
316
317 func (s *charmsSuite) TestUploadFailsWithInvalidZip(c *gc.C) {
318@@ -100,12 +102,12 @@
319 // check the error at extraction time later.
320 resp, err := s.uploadRequest(c, s.charmsURI(c, "?series=quantal"), true, tempFile.Name())
321 c.Assert(err, gc.IsNil)
322- s.assertResponse(c, resp, http.StatusBadRequest, "cannot open charm archive: zip: not a valid zip file", "")
323+ s.assertErrorResponse(c, resp, http.StatusBadRequest, "cannot open charm archive: zip: not a valid zip file")
324
325 // Now try with the default Content-Type.
326 resp, err = s.uploadRequest(c, s.charmsURI(c, "?series=quantal"), false, tempFile.Name())
327 c.Assert(err, gc.IsNil)
328- s.assertResponse(c, resp, http.StatusBadRequest, "expected Content-Type: application/zip, got: application/octet-stream", "")
329+ s.assertErrorResponse(c, resp, http.StatusBadRequest, "expected Content-Type: application/zip, got: application/octet-stream")
330 }
331
332 func (s *charmsSuite) TestUploadBumpsRevision(c *gc.C) {
333@@ -124,7 +126,7 @@
334 resp, err := s.uploadRequest(c, s.charmsURI(c, "?series=quantal"), true, ch.Path)
335 c.Assert(err, gc.IsNil)
336 expectedURL := charm.MustParseURL("local:quantal/dummy-2")
337- s.assertResponse(c, resp, http.StatusOK, "", expectedURL.String())
338+ s.assertUploadResponse(c, resp, expectedURL.String())
339 sch, err := s.State.Charm(expectedURL)
340 c.Assert(err, gc.IsNil)
341 c.Assert(sch.URL(), gc.DeepEquals, expectedURL)
342@@ -152,7 +154,7 @@
343 resp, err := s.uploadRequest(c, s.charmsURI(c, "?series=quantal"), true, tempFile.Name())
344 c.Assert(err, gc.IsNil)
345 expectedURL := charm.MustParseURL("local:quantal/dummy-123")
346- s.assertResponse(c, resp, http.StatusOK, "", expectedURL.String())
347+ s.assertUploadResponse(c, resp, expectedURL.String())
348 sch, err := s.State.Charm(expectedURL)
349 c.Assert(err, gc.IsNil)
350 c.Assert(sch.URL(), gc.DeepEquals, expectedURL)
351@@ -207,7 +209,7 @@
352 resp, err := s.uploadRequest(c, s.charmsURI(c, "?series=quantal"), true, tempFile.Name())
353 c.Assert(err, gc.IsNil)
354 expectedURL := charm.MustParseURL("local:quantal/dummy-1")
355- s.assertResponse(c, resp, http.StatusOK, "", expectedURL.String())
356+ s.assertUploadResponse(c, resp, expectedURL.String())
357 sch, err := s.State.Charm(expectedURL)
358 c.Assert(err, gc.IsNil)
359 c.Assert(sch.URL(), gc.DeepEquals, expectedURL)
360@@ -240,6 +242,141 @@
361 c.Assert(bundle.Config(), jc.DeepEquals, sch.Config())
362 }
363
364+func (s *charmsSuite) TestGetRequiresCharmURL(c *gc.C) {
365+ uri := s.charmsURI(c, "?file=hooks/install")
366+ resp, err := s.authRequest(c, "GET", uri, "", nil)
367+ c.Assert(err, gc.IsNil)
368+ s.assertErrorResponse(
369+ c, resp, http.StatusBadRequest,
370+ "expected url=CharmURL query argument",
371+ )
372+}
373+
374+func (s *charmsSuite) TestGetFailsWithInvalidCharmURL(c *gc.C) {
375+ uri := s.charmsURI(c, "?url=local:precise/no-such")
376+ resp, err := s.authRequest(c, "GET", uri, "", nil)
377+ c.Assert(err, gc.IsNil)
378+ s.assertErrorResponse(
379+ c, resp, http.StatusBadRequest,
380+ "unable to retrieve and save the charm: charm not found in the provider storage: .*",
381+ )
382+}
383+
384+func (s *charmsSuite) TestGetReturnsNotFoundWhenMissing(c *gc.C) {
385+ // Add the dummy charm.
386+ ch := coretesting.Charms.Bundle(c.MkDir(), "dummy")
387+ _, err := s.uploadRequest(
388+ c, s.charmsURI(c, "?series=quantal"), true, ch.Path)
389+ c.Assert(err, gc.IsNil)
390+
391+ // Ensure a 404 is returned for files not included in the charm.
392+ for i, file := range []string{
393+ "no-such-file", "..", "../../../etc/passwd", "hooks/delete",
394+ } {
395+ c.Logf("test %d: %s", i, file)
396+ uri := s.charmsURI(c, "?url=local:quantal/dummy-1&file="+file)
397+ resp, err := s.authRequest(c, "GET", uri, "", nil)
398+ c.Assert(err, gc.IsNil)
399+ c.Assert(resp.StatusCode, gc.Equals, http.StatusNotFound)
400+ }
401+}
402+
403+func (s *charmsSuite) TestGetReturnsForbiddenWithDirectory(c *gc.C) {
404+ // Add the dummy charm.
405+ ch := coretesting.Charms.Bundle(c.MkDir(), "dummy")
406+ _, err := s.uploadRequest(
407+ c, s.charmsURI(c, "?series=quantal"), true, ch.Path)
408+ c.Assert(err, gc.IsNil)
409+
410+ // Ensure a 403 is returned if the requested file is a directory.
411+ uri := s.charmsURI(c, "?url=local:quantal/dummy-1&file=hooks")
412+ resp, err := s.authRequest(c, "GET", uri, "", nil)
413+ c.Assert(err, gc.IsNil)
414+ c.Assert(resp.StatusCode, gc.Equals, http.StatusForbidden)
415+}
416+
417+func (s *charmsSuite) TestGetReturnsFileContents(c *gc.C) {
418+ // Add the dummy charm.
419+ ch := coretesting.Charms.Bundle(c.MkDir(), "dummy")
420+ _, err := s.uploadRequest(
421+ c, s.charmsURI(c, "?series=quantal"), true, ch.Path)
422+ c.Assert(err, gc.IsNil)
423+
424+ // Ensure the file contents are properly returned.
425+ for i, t := range []struct {
426+ summary string
427+ file string
428+ response string
429+ }{{
430+ summary: "relative path",
431+ file: "revision",
432+ response: "1",
433+ }, {
434+ summary: "exotic path",
435+ file: "./hooks/../revision",
436+ response: "1",
437+ }, {
438+ summary: "sub-directory path",
439+ file: "hooks/install",
440+ response: "#!/bin/bash\necho \"Done!\"\n",
441+ },
442+ } {
443+ c.Logf("test %d: %s", i, t.summary)
444+ uri := s.charmsURI(c, "?url=local:quantal/dummy-1&file="+t.file)
445+ resp, err := s.authRequest(c, "GET", uri, "", nil)
446+ c.Assert(err, gc.IsNil)
447+ s.assertGetFileResponse(c, resp, t.response, "text/plain; charset=utf-8")
448+ }
449+}
450+
451+func (s *charmsSuite) TestGetListsFiles(c *gc.C) {
452+ // Add the dummy charm.
453+ ch := coretesting.Charms.Bundle(c.MkDir(), "dummy")
454+ _, err := s.uploadRequest(
455+ c, s.charmsURI(c, "?series=quantal"), true, ch.Path)
456+ c.Assert(err, gc.IsNil)
457+
458+ // Ensure charm files are properly listed.
459+ uri := s.charmsURI(c, "?url=local:quantal/dummy-1")
460+ resp, err := s.authRequest(c, "GET", uri, "", nil)
461+ c.Assert(err, gc.IsNil)
462+ expectedFiles := []string{
463+ "revision", "config.yaml", "hooks/install", "metadata.yaml",
464+ "src/hello.c",
465+ }
466+ s.assertGetFileListResponse(c, resp, expectedFiles)
467+ ctype := resp.Header.Get("content-type")
468+ c.Assert(ctype, gc.Equals, "application/json")
469+}
470+
471+func (s *charmsSuite) TestGetUsesCache(c *gc.C) {
472+ // Add a fake charm archive in the cache directory.
473+ cacheDir := filepath.Join(s.DataDir(), "charm-get-cache")
474+ err := os.MkdirAll(cacheDir, 0755)
475+ c.Assert(err, gc.IsNil)
476+
477+ // Create and save the zip archive.
478+ buffer := new(bytes.Buffer)
479+ writer := zip.NewWriter(buffer)
480+ file, err := writer.Create("utils.js")
481+ c.Assert(err, gc.IsNil)
482+ contents := "// these are the voyages"
483+ _, err = file.Write([]byte(contents))
484+ c.Assert(err, gc.IsNil)
485+ err = writer.Close()
486+ c.Assert(err, gc.IsNil)
487+ charmArchivePath := filepath.Join(
488+ cacheDir, charm.Quote("local:trusty/django-42")+".zip")
489+ err = ioutil.WriteFile(charmArchivePath, buffer.Bytes(), 0644)
490+ c.Assert(err, gc.IsNil)
491+
492+ // Ensure the cached contents are properly retrieved.
493+ uri := s.charmsURI(c, "?url=local:trusty/django-42&file=utils.js")
494+ resp, err := s.authRequest(c, "GET", uri, "", nil)
495+ c.Assert(err, gc.IsNil)
496+ s.assertGetFileResponse(c, resp, contents, "application/javascript")
497+}
498+
499 func (s *charmsSuite) charmsURI(c *gc.C, query string) string {
500 _, info, err := s.APIConn.Environ.StateInfo()
501 c.Assert(err, gc.IsNil)
502@@ -278,19 +415,42 @@
503 return s.authRequest(c, "POST", uri, contentType, file)
504 }
505
506-func (s *charmsSuite) assertResponse(c *gc.C, resp *http.Response, expCode int, expError, expCharmURL string) {
507+func (s *charmsSuite) assertUploadResponse(c *gc.C, resp *http.Response, expCharmURL string) {
508+ body := assertResponse(c, resp, http.StatusOK, "application/json")
509+ charmResponse := jsonResponse(c, body)
510+ c.Check(charmResponse.Error, gc.Equals, "")
511+ c.Check(charmResponse.CharmURL, gc.Equals, expCharmURL)
512+}
513+
514+func (s *charmsSuite) assertGetFileResponse(c *gc.C, resp *http.Response, expBody, expContentType string) {
515+ body := assertResponse(c, resp, http.StatusOK, expContentType)
516+ c.Check(string(body), gc.Equals, expBody)
517+}
518+
519+func (s *charmsSuite) assertGetFileListResponse(c *gc.C, resp *http.Response, expFiles []string) {
520+ body := assertResponse(c, resp, http.StatusOK, "application/json")
521+ charmResponse := jsonResponse(c, body)
522+ c.Check(charmResponse.Error, gc.Equals, "")
523+ c.Check(charmResponse.Files, gc.DeepEquals, expFiles)
524+}
525+
526+func (s *charmsSuite) assertErrorResponse(c *gc.C, resp *http.Response, expCode int, expError string) {
527+ body := assertResponse(c, resp, expCode, "application/json")
528+ c.Check(jsonResponse(c, body).Error, gc.Matches, expError)
529+}
530+
531+func assertResponse(c *gc.C, resp *http.Response, expCode int, expContentType string) []byte {
532+ c.Check(resp.StatusCode, gc.Equals, expCode)
533 body, err := ioutil.ReadAll(resp.Body)
534 defer resp.Body.Close()
535 c.Assert(err, gc.IsNil)
536- var jsonResponse params.CharmsResponse
537- err = json.Unmarshal(body, &jsonResponse)
538+ ctype := resp.Header.Get("Content-Type")
539+ c.Assert(ctype, gc.Equals, expContentType)
540+ return body
541+}
542+
543+func jsonResponse(c *gc.C, body []byte) (jsonResponse params.CharmsResponse) {
544+ err := json.Unmarshal(body, &jsonResponse)
545 c.Assert(err, gc.IsNil)
546- if expError != "" {
547- c.Check(jsonResponse.Error, gc.Matches, expError)
548- c.Check(jsonResponse.CharmURL, gc.Equals, "")
549- } else {
550- c.Check(jsonResponse.Error, gc.Equals, "")
551- c.Check(jsonResponse.CharmURL, gc.Equals, expCharmURL)
552- }
553- c.Check(resp.StatusCode, gc.Equals, expCode)
554+ return
555 }

Subscribers

People subscribed via source and target branches

to status/vote changes: