Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/v5: Support symlink for files in archive. #817

Open
wants to merge 2 commits into
base: v5
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions internal/charmstore/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ package charmstore // import "gopkg.in/juju/charmstore.v5/internal/charmstore"
import (
"archive/zip"
"bytes"
"fmt"
"io"
"io/ioutil"
"os"
"path"
"strings"
Expand Down Expand Up @@ -113,6 +115,9 @@ func (r *multiReadSeekCloser) Close() error {
// If no such file was found, it returns an error
// with a params.ErrNotFound cause.
//
// If the file is a symlink, it returns an error ErrRedirect that holds the path
// that the symbolic link refers to.
//
// If the file is actually a directory in the blob, it returns
// an error with a params.ErrForbidden cause.
func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, error) {
Expand All @@ -135,11 +140,30 @@ func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64,
if err != nil {
return nil, 0, errgo.Notef(err, "unable to read file %q", filePath)
}
if file.Mode()&os.ModeSymlink != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this behaviour in the doc comment for OpenBlobFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

defer content.Close()
url, err := ioutil.ReadAll(content)
if err != nil {
return nil, 0, errgo.Notef(err, "cannot read archive data for symlink %s", file.Name)
}
return nil, 0, &ErrRedirect{Path: string(url)}
}
return content, fileInfo.Size(), nil
}
return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath)
}

// ErrRedirect holds the error returned by OpenBlobFile when a symbolic link
// is opened. The Path field holds the name of the file that the symbolic link
// refers to.
type ErrRedirect struct {
Path string
}

func (e ErrRedirect) Error() string {
return fmt.Sprintf("file is a symbolic link to %q", e.Path)
}

// OpenCachedBlobFile opens a file from the given entity's archive blob.
// The file is identified by the provided fileId. If the file has not
// previously been opened on this entity, the isFile function will be
Expand Down

This file was deleted.

12 changes: 12 additions & 0 deletions internal/v5/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"gopkg.in/juju/charmstore.v5/internal/charmstore"
"gopkg.in/juju/charmstore.v5/internal/mongodoc"
"gopkg.in/juju/charmstore.v5/internal/router"
"path"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems in the wrong place

)

// GET id/archive
Expand Down Expand Up @@ -333,6 +334,17 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri
// with the given id.
func (h *ReqHandler) ServeBlobFile(w http.ResponseWriter, req *http.Request, id *router.ResolvedURL, blob *charmstore.Blob) error {
r, size, err := h.Store.OpenBlobFile(blob, req.URL.Path)
if redirect, ok := err.(*charmstore.ErrRedirect); ok {
p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path))
if err != nil {
return errgo.Notef(err, "cannot make relative URL from %q and %q", req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path))
}
// We can't use the http.redirect function because it tries to build an absolute url but the path in the
// request URL has been changed.
w.Header().Set("Location", p)
w.WriteHeader(http.StatusMovedPermanently)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think StatusFound would be better. We cannot say it is moved perminently only until a new zip file is uploaded.

return nil
}
if err != nil {
return errgo.Mask(err, errgo.Is(params.ErrNotFound), errgo.Is(params.ErrForbidden))
}
Expand Down
49 changes: 34 additions & 15 deletions internal/v5/archive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"net/http/httptest"
"net/url"
"os"
"path"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -1178,25 +1179,24 @@ func (s *ArchiveSuite) TestArchiveFileGet(c *gc.C) {
s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/install")
}

func (s *ArchiveSuite) TestSymLinkArchiveFileGet(c *gc.C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a few more tests of this behaviour. For example, does it work when the symlink is absolute, contains multiple slashes, trailing slashes? Does it work when the URL path contains a trailing slash or isn't clean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say if a zipfile contains an absolute symlink then it is broken.

ch := storetesting.Charms.CharmArchive(c.MkDir(), "all-hooks")
id := newResolvedURL("cs:~charmers/utopic/all-hooks-0", 0)
s.addPublicCharm(c, ch, id)
zipFile, err := zip.OpenReader(ch.Path)
c.Assert(err, gc.Equals, nil)
defer zipFile.Close()

// Check a file in a subdirectory.
s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/foo-relation-departed")
}

// assertArchiveFileContents checks that the response returned by the
// serveArchiveFile endpoint is correct for the given archive and URL path.
func (s *ArchiveSuite) assertArchiveFileContents(c *gc.C, zipFile *zip.ReadCloser, path string) {
// For example: trusty/django/archive/hooks/install -> hooks/install.
filePath := strings.SplitN(path, "/archive/", 2)[1]

// Retrieve the expected bytes.
var expectBytes []byte
for _, file := range zipFile.File {
if file.Name == filePath {
r, err := file.Open()
c.Assert(err, gc.Equals, nil)
defer r.Close()
expectBytes, err = ioutil.ReadAll(r)
c.Assert(err, gc.Equals, nil)
break
}
}
c.Assert(expectBytes, gc.Not(gc.HasLen), 0)
expectBytes := retrieveExpectedBytes(c, zipFile, filePath)

// Make the request.
url := storeURL(path)
Expand All @@ -1207,14 +1207,33 @@ func (s *ArchiveSuite) assertArchiveFileContents(c *gc.C, zipFile *zip.ReadClose

// Ensure the response is what we expect.
c.Assert(rec.Code, gc.Equals, http.StatusOK)
c.Assert(rec.Body.Bytes(), gc.DeepEquals, expectBytes)
c.Assert(string(rec.Body.Bytes()), gc.DeepEquals, string(expectBytes))
headers := rec.Header()
c.Assert(headers.Get("Content-Length"), gc.Equals, strconv.Itoa(len(expectBytes)))
// We only have text files in the charm repository used for tests.
c.Assert(headers.Get("Content-Type"), gc.Equals, "text/plain; charset=utf-8")
assertCacheControl(c, rec.Header(), true)
}

func retrieveExpectedBytes(c *gc.C, zipFile *zip.ReadCloser, filePath string) (expectBytes []byte) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better named readZipFilePath, but honestly I think we could just leave things as they were and just test the symlink behaviour directly in the symlink test rather than calling assertArchiveFileContents.

for _, file := range zipFile.File {
if file.Name == filePath {
r, err := file.Open()
c.Assert(err, gc.Equals, nil)
defer r.Close()
expectBytes, err = ioutil.ReadAll(r)
c.Assert(err, gc.Equals, nil)
if file.Mode()&os.ModeSymlink != 0 {
newPath := path.Join(path.Dir(filePath), string(expectBytes))
return retrieveExpectedBytes(c, zipFile, newPath)
}
break
}
}
c.Assert(expectBytes, gc.Not(gc.HasLen), 0)
return expectBytes
}

func (s *ArchiveSuite) TestDelete(c *gc.C) {
// Add a charm to the database (including the archive).
id, _ := s.addPublicCharm(c, storetesting.NewCharm(nil), newResolvedURL("~charmers/utopic/mysql-42", -1))
Expand Down