Download from the API rather than from archive URLs.

sdg0919 · Apr 26, 2016 · 3a930c5 · 3a930c5
1 parent 931cabb
commit 3a930c5
Show file tree

Hide file tree

Showing 7 changed files with 76 additions and 100 deletions.
diff --git a/api/client.go b/api/client.go
@@ -24,6 +24,7 @@ import (
 	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/downloader"
 	"github.com/juju/juju/network"
 	"github.com/juju/juju/tools"
 )
@@ -453,6 +454,25 @@ func openCharm(httpClient HTTPDoer, curl *charm.URL) (io.ReadCloser, error) {
 	return blob, nil
 }
 
+// NewCharmDownloader returns a new charm downloader that wraps the
+// provided API client.
+func NewCharmDownloader(client *Client) *downloader.Downloader {
+	dlr := &downloader.Downloader{
+		OpenBlob: func(url *url.URL) (io.ReadCloser, error) {
+			curl, err := charm.ParseURL(url.String())
+			if err != nil {
+				return nil, errors.Annotate(err, "did not receive a valid charm URL")
+			}
+			reader, err := client.OpenCharm(curl)
+			if err != nil {
+				return nil, errors.Trace(err)
+			}
+			return reader, nil
+		},
+	}
+	return dlr
+}
+
 // UploadTools uploads tools at the specified location to the API server over HTTPS.
 func (c *Client) UploadTools(r io.ReadSeeker, vers version.Binary, additionalSeries ...string) (tools.List, error) {
 	endpoint := fmt.Sprintf("/tools?binaryVersion=%s&series=%s", vers, strings.Join(additionalSeries, ","))

diff --git a/downloader/download.go b/downloader/download.go
@@ -106,7 +106,7 @@ func (dl *Download) run(req Request) {
 	// disableSSLHostnameVerification behavior here.
 	file, err := download(req, dl.openBlob)
 	if err != nil {
-		err = errors.Errorf("cannot download %q: %v", req.URL, err)
+		err = errors.Annotatef(err, "cannot download %q", req.URL)
 	}
 
 	if err == nil {
@@ -120,7 +120,7 @@ func (dl *Download) run(req Request) {
 			if _, err2 := file.Seek(0, os.SEEK_SET); err2 != nil {
 				logger.Errorf("failed to seek to beginning of file: %v", err)
 				if err == nil {
-					err = err2
+					err = errors.Trace(err2)
 				}
 			} else {
 				logger.Infof("download verified")

diff --git a/downloader/downloader.go b/downloader/downloader.go
@@ -19,7 +19,7 @@ var logger = loggo.GetLogger("juju.downloader")
 type Downloader struct {
 	// OpenBlob is the func used to gain access to the blob, whether
 	// through an HTTP request or some other means.
-	OpenBlob func(url *url.URL) (io.ReadCloser, error)
+	OpenBlob func(*url.URL) (io.ReadCloser, error)
 }
 
 // NewArgs holds the arguments to New().
@@ -46,6 +46,9 @@ func (dlr Downloader) Start(req Request) *Download {
 // Download starts a new download, waits for it to complete, and
 // returns the local name of the file.
 func (dlr Downloader) Download(req Request, abort <-chan struct{}) (filename string, err error) {
+	if err := os.MkdirAll(req.TargetDir, 0755); err != nil {
+		return "", errors.Trace(err)
+	}
 	dl := dlr.Start(req)
 	file, err := dl.Wait(abort)
 	if file != nil {
@@ -66,9 +69,6 @@ func (dlr Downloader) DownloadWithAlternates(requests []Request, abort <-chan st
 	}
 
 	for _, req := range requests {
-		if err := os.MkdirAll(req.TargetDir, 0755); err != nil {
-			return "", errors.Trace(err)
-		}
 		filename, err = dlr.Download(req, abort)
 		if errors.IsNotValid(err) {
 			break

diff --git a/juju/testing/conn.go b/juju/testing/conn.go
@@ -451,10 +451,11 @@ func PutCharm(st *state.State, curl *charm.URL, repo charmrepo.Interface, bumpRe
 	if sch, err := st.Charm(curl); err == nil {
 		return sch, nil
 	}
-	return addCharm(st, curl, ch)
+	return AddCharm(st, curl, ch)
 }
 
-func addCharm(st *state.State, curl *charm.URL, ch charm.Charm) (*state.Charm, error) {
+// AddCharm adds the charm to state and storage.
+func AddCharm(st *state.State, curl *charm.URL, ch charm.Charm) (*state.Charm, error) {
 	var f *os.File
 	name := charm.Quote(curl.String())
 	switch ch := ch.(type) {

diff --git a/worker/uniter/charm/bundles.go b/worker/uniter/charm/bundles.go
@@ -4,6 +4,7 @@
 package charm
 
 import (
+	"net/url"
 	"os"
 	"path"
 
@@ -16,10 +17,9 @@ import (
 
 // Download exposes the downloader.Download methods needed here.
 type Downloader interface {
-	// DownloadWithAlternates tries each of the provided requests until
-	// one succeeds. If none succeed then the error from the most recent
-	// attempt is returned. At least one request must be provided.
-	DownloadWithAlternates(requests []downloader.Request, abort <-chan struct{}) (string, error)
+	// Download starts a new charm archive download, waits for it to
+	// complete, and returns the local name of the file.
+	Download(req downloader.Request, abort <-chan struct{}) (string, error)
 }
 
 // BundlesDir is responsible for storing and retrieving charm bundles
@@ -64,23 +64,19 @@ func (d *BundlesDir) Read(info BundleInfo, abort <-chan struct{}) (Bundle, error
 // download will be stopped.
 func (d *BundlesDir) download(info BundleInfo, target string, abort <-chan struct{}) (err error) {
 	// First download...
-	archiveURLs, err := info.ArchiveURLs()
+	curl, err := url.Parse(info.URL().String())
 	if err != nil {
-		return errors.Annotatef(err, "failed to get download URLs for charm %q", info.URL())
+		return errors.Annotate(err, "could not parse charm URL")
 	}
-	targetDir := d.downloadsPath()
-	var requests []downloader.Request
-	for _, archiveURL := range archiveURLs {
-		requests = append(requests, downloader.Request{
-			URL:       archiveURL,
-			TargetDir: targetDir,
-			Verify:    downloader.NewSha256Verifier(info.ArchiveSha256),
-		})
+	req := downloader.Request{
+		URL:       curl,
+		TargetDir: d.downloadsPath(),
+		Verify:    downloader.NewSha256Verifier(info.ArchiveSha256),
 	}
-	logger.Infof("downloading %s from %d URLs", info.URL(), len(archiveURLs))
-	filename, err := d.downloader.DownloadWithAlternates(requests, abort)
+	logger.Infof("downloading %s from API server", info.URL())
+	filename, err := d.downloader.Download(req, abort)
 	if err != nil {
-		return errors.Annotatef(err, "failed to download charm %q from %q", info.URL(), archiveURLs)
+		return errors.Annotatef(err, "failed to download charm %q from API server", info.URL())
 	}
 	defer errors.DeferredAnnotatef(&err, "downloaded but failed to copy charm to %q from %q", target, filename)
 

diff --git a/worker/uniter/charm/bundles_test.go b/worker/uniter/charm/bundles_test.go
@@ -4,11 +4,6 @@
 package charm_test
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
-	"fmt"
-	"io/ioutil"
-	"net/url"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -77,72 +72,61 @@ func (s *BundlesDirSuite) TearDownTest(c *gc.C) {
 	s.HTTPSuite.TearDownTest(c)
 }
 
-func (s *BundlesDirSuite) AddCharm(c *gc.C) (charm.BundleInfo, *state.Charm, []byte) {
+func (s *BundlesDirSuite) AddCharm(c *gc.C) (charm.BundleInfo, *state.Charm) {
 	curl := corecharm.MustParseURL("cs:quantal/dummy-1")
-	storagePath := "dummy-1"
-	bunpath := testcharms.Repo.CharmArchivePath(c.MkDir(), "dummy")
-	bun, err := corecharm.ReadCharmArchive(bunpath)
-	c.Assert(err, jc.ErrorIsNil)
-	bundata, hash := readHash(c, bunpath)
-	info := state.CharmInfo{
-		Charm:       bun,
-		ID:          curl,
-		StoragePath: storagePath,
-		SHA256:      hash,
-	}
-	sch, err := s.State.AddCharm(info)
+	bun := testcharms.Repo.CharmDir("dummy")
+	sch, err := testing.AddCharm(s.State, curl, bun)
 	c.Assert(err, jc.ErrorIsNil)
+
 	apiCharm, err := s.uniter.Charm(sch.URL())
 	c.Assert(err, jc.ErrorIsNil)
 
-	surlBad, err := url.Parse(s.URL("/some/charm.bundle?bad"))
-	c.Assert(err, jc.ErrorIsNil)
-	surlGood, err := url.Parse(s.URL("/some/charm.bundle?good"))
-	c.Assert(err, jc.ErrorIsNil)
-	mock := &mockArchiveURLCharm{
-		apiCharm,
-		[]*url.URL{surlBad, surlGood},
-	}
-	return mock, sch, bundata
+	return apiCharm, sch
 }
 
-type mockArchiveURLCharm struct {
+type fakeBundleInfo struct {
 	charm.BundleInfo
-	archiveURLs []*url.URL
+	curl   *corecharm.URL
+	sha256 string
 }
 
-func (i *mockArchiveURLCharm) ArchiveURLs() ([]*url.URL, error) {
-	return i.archiveURLs, nil
+func (f fakeBundleInfo) URL() *corecharm.URL {
+	if f.curl == nil {
+		return f.BundleInfo.URL()
+	}
+	return f.curl
+}
+
+func (f fakeBundleInfo) ArchiveSha256() (string, error) {
+	if f.sha256 == "" {
+		return f.BundleInfo.ArchiveSha256()
+	}
+	return f.sha256, nil
 }
 
 func (s *BundlesDirSuite) TestGet(c *gc.C) {
 	basedir := c.MkDir()
 	bunsdir := filepath.Join(basedir, "random", "bundles")
-	d := charm.NewBundlesDir(bunsdir, nil)
+	downloader := api.NewCharmDownloader(s.st.Client())
+	d := charm.NewBundlesDir(bunsdir, downloader)
 
 	// Check it doesn't get created until it's needed.
 	_, err := os.Stat(bunsdir)
 	c.Assert(err, jc.Satisfies, os.IsNotExist)
 
 	// Add a charm to state that we can try to get.
-	apiCharm, sch, bundata := s.AddCharm(c)
+	apiCharm, sch := s.AddCharm(c)
 
 	// Try to get the charm when the content doesn't match.
-	gitjujutesting.Server.Response(200, nil, []byte("roflcopter"))
-	archiveURLs, err := apiCharm.ArchiveURLs()
-	c.Assert(err, gc.IsNil)
-	_, err = d.Read(apiCharm, nil)
-	prefix := regexp.QuoteMeta(fmt.Sprintf(`failed to download charm "cs:quantal/dummy-1" from %q: `, archiveURLs))
-	c.Assert(err, gc.ErrorMatches, prefix+fmt.Sprintf(`expected sha256 %q, got ".*"`, sch.BundleSha256()))
+	_, err = d.Read(&fakeBundleInfo{apiCharm, nil, "..."}, nil)
+	c.Assert(err, gc.ErrorMatches, regexp.QuoteMeta(`failed to download charm "cs:quantal/dummy-1" from API server: `)+`expected sha256 "...", got ".*"`)
 
 	// Try to get a charm whose bundle doesn't exist.
-	gitjujutesting.Server.Responses(2, 404, nil, nil)
-	_, err = d.Read(apiCharm, nil)
-	c.Assert(err, gc.ErrorMatches, prefix+`.* 404 Not Found`)
+	otherURL := corecharm.MustParseURL("cs:quantal/spam-1")
+	_, err = d.Read(&fakeBundleInfo{apiCharm, otherURL, ""}, nil)
+	c.Assert(err, gc.ErrorMatches, regexp.QuoteMeta(`failed to download charm "cs:quantal/spam-1" from API server: `)+`.* not found`)
 
 	// Get a charm whose bundle exists and whose content matches.
-	gitjujutesting.Server.Response(404, nil, nil)
-	gitjujutesting.Server.Response(200, nil, bundata)
 	ch, err := d.Read(apiCharm, nil)
 	c.Assert(err, jc.ErrorIsNil)
 	assertCharm(c, ch, sch)
@@ -160,7 +144,7 @@ func (s *BundlesDirSuite) TestGet(c *gc.C) {
 	go func() {
 		ch, err := d.Read(apiCharm, abort)
 		c.Assert(ch, gc.IsNil)
-		c.Assert(err, gc.ErrorMatches, prefix+"aborted")
+		c.Assert(err, gc.ErrorMatches, regexp.QuoteMeta(`failed to download charm "cs:quantal/dummy-1" from API server: aborted`))
 		close(done)
 	}()
 	close(abort)
@@ -172,14 +156,6 @@ func (s *BundlesDirSuite) TestGet(c *gc.C) {
 	}
 }
 
-func readHash(c *gc.C, path string) ([]byte, string) {
-	data, err := ioutil.ReadFile(path)
-	c.Assert(err, jc.ErrorIsNil)
-	hash := sha256.New()
-	hash.Write(data)
-	return data, hex.EncodeToString(hash.Sum(nil))
-}
-
 func assertCharm(c *gc.C, bun charm.Bundle, sch *state.Charm) {
 	actual := bun.(*corecharm.CharmArchive)
 	c.Assert(actual.Revision(), gc.Equals, sch.Revision())

diff --git a/worker/uniter/manifold.go b/worker/uniter/manifold.go
@@ -6,16 +6,14 @@ package uniter
 import (
 	"github.com/juju/errors"
 	"github.com/juju/names"
-	"github.com/juju/utils"
 	"github.com/juju/utils/clock"
 	"github.com/juju/utils/fslock"
 
 	"github.com/juju/juju/agent"
-	"github.com/juju/juju/api/base"
+	"github.com/juju/juju/api"
 	"github.com/juju/juju/api/uniter"
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/core/leadership"
-	"github.com/juju/juju/downloader"
 	"github.com/juju/juju/worker"
 	"github.com/juju/juju/worker/dependency"
 	"github.com/juju/juju/worker/fortress"
@@ -52,8 +50,8 @@ func Manifold(config ManifoldConfig) dependency.Manifold {
 			if err := context.Get(config.AgentName, &agent); err != nil {
 				return nil, err
 			}
-			var apiCaller base.APICaller
-			if err := context.Get(config.APICallerName, &apiCaller); err != nil {
+			var apiConn api.Connection
+			if err := context.Get(config.APICallerName, &apiConn); err != nil {
 				// TODO(fwereade): absence of an APICaller shouldn't be the end of
 				// the world -- we ought to return a type that can at least run the
 				// leader-deposed hook -- but that's not done yet.
@@ -77,10 +75,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold {
 				return nil, err
 			}
 
-			downloader, err := newDownloader(apiCaller)
-			if err != nil {
-				return nil, errors.Trace(err)
-			}
+			downloader := api.NewCharmDownloader(apiConn.Client())
 
 			// Configure and start the uniter.
 			config := agent.CurrentConfig()
@@ -89,7 +84,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold {
 			if !ok {
 				return nil, errors.Errorf("expected a unit tag, got %v", tag)
 			}
-			uniterFacade := uniter.NewState(apiCaller, unitTag)
+			uniterFacade := uniter.NewState(apiConn, unitTag)
 			uniter, err := NewUniter(&UniterParams{
 				UniterFacade:         uniterFacade,
 				UnitTag:              unitTag,
@@ -110,15 +105,3 @@ func Manifold(config ManifoldConfig) dependency.Manifold {
 		},
 	}
 }
-
-func newDownloader(apiCaller base.APICaller) (*downloader.Downloader, error) {
-	// Downloads always go through the API server, which at
-	// present cannot be verified due to the certificates
-	// being inadequate. We always verify the SHA-256 hash,
-	// and the data transferred is not sensitive, so this
-	// does not pose a problem.
-	dlr := downloader.New(downloader.NewArgs{
-		HostnameVerification: utils.NoVerifySSLHostnames,
-	})
-	return dlr, nil
-}