Wire up the charm refresher

The following updates the charmhub refresher to correctly handle charmhub vs charmstore function calls. Essentially because we don't have macaroons then we don't want to end up falling into the AddCharmWithAuthorization workflow. By removing the special case we can correctly reason about what the path the charmhub refresher will take. In an ideal world we would use a better pattern for dealing with this. Maybe resort to a stragtegy, but generally just embedding the functionality into two different types also resolves this.
juju · Sep 22, 2020 · 8c23c9e · 8c23c9e
1 parent 8a0d400
commit 8c23c9e
Show file tree

Hide file tree

Showing 4 changed files with 304 additions and 23 deletions.
diff --git a/cmd/juju/application/refresher/refresher.go b/cmd/juju/application/refresher/refresher.go
@@ -13,6 +13,7 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/featureflag"
 
+	commoncharm "github.com/juju/juju/api/common/charm"
 	"github.com/juju/juju/cmd/juju/application/store"
 	"github.com/juju/juju/cmd/juju/application/utils"
 	corecharm "github.com/juju/juju/core/charm"
@@ -42,6 +43,8 @@ type RefresherConfig struct {
 	ForceSeries     bool
 }
 
+// RefresherFn defines a function alias to create a Refresher from a given
+// function.
 type RefresherFn = func(RefresherConfig) (Refresher, error)
 
 type factory struct {
@@ -58,6 +61,7 @@ func NewRefresherFactory(deps RefresherDependencies) RefresherFactory {
 	d.refreshers = []RefresherFn{
 		d.maybeReadLocal(deps.CharmAdder, defaultCharmRepo{}),
 		d.maybeCharmStore(deps.Authorizer, deps.CharmAdder, deps.CharmResolver),
+		d.maybeCharmHub(deps.CharmAdder, deps.CharmResolver),
 	}
 	return d
 }
@@ -116,15 +120,34 @@ func (d *factory) maybeReadLocal(charmAdder store.CharmAdder, charmRepo CharmRep
 func (d *factory) maybeCharmStore(authorizer store.MacaroonGetter, charmAdder store.CharmAdder, charmResolver CharmResolver) func(RefresherConfig) (Refresher, error) {
 	return func(cfg RefresherConfig) (Refresher, error) {
 		return &charmStoreRefresher{
-			authorizer:     authorizer,
-			charmAdder:     charmAdder,
-			charmResolver:  charmResolver,
-			charmURL:       cfg.CharmURL,
-			charmRef:       cfg.CharmRef,
-			channel:        cfg.Channel,
-			deployedSeries: cfg.DeployedSeries,
-			force:          cfg.Force,
-			forceSeries:    cfg.ForceSeries,
+			baseRefresher: baseRefresher{
+				charmAdder:     charmAdder,
+				charmResolver:  charmResolver,
+				charmURL:       cfg.CharmURL,
+				charmRef:       cfg.CharmRef,
+				channel:        cfg.Channel,
+				deployedSeries: cfg.DeployedSeries,
+				force:          cfg.Force,
+				forceSeries:    cfg.ForceSeries,
+			},
+			authorizer: authorizer,
+		}, nil
+	}
+}
+
+func (d *factory) maybeCharmHub(charmAdder store.CharmAdder, charmResolver CharmResolver) func(RefresherConfig) (Refresher, error) {
+	return func(cfg RefresherConfig) (Refresher, error) {
+		return &charmHubRefresher{
+			baseRefresher: baseRefresher{
+				charmAdder:     charmAdder,
+				charmResolver:  charmResolver,
+				charmURL:       cfg.CharmURL,
+				charmRef:       cfg.CharmRef,
+				channel:        cfg.Channel,
+				deployedSeries: cfg.DeployedSeries,
+				force:          cfg.Force,
+				forceSeries:    cfg.ForceSeries,
+			},
 		}, nil
 	}
 }
@@ -181,8 +204,7 @@ func (d *localCharmRefresher) String() string {
 	return fmt.Sprintf("attempting to refresh local charm %q", d.charmRef)
 }
 
-type charmStoreRefresher struct {
-	authorizer     store.MacaroonGetter
+type baseRefresher struct {
 	charmAdder     store.CharmAdder
 	charmResolver  CharmResolver
 	charmURL       *charm.URL
@@ -193,6 +215,55 @@ type charmStoreRefresher struct {
 	forceSeries    bool
 }
 
+func (r baseRefresher) ResolveCharm() (*charm.URL, commoncharm.Origin, error) {
+	refURL, err := charm.ParseURL(r.charmRef)
+	if err != nil {
+		return nil, commoncharm.Origin{}, errors.Trace(err)
+	}
+
+	origin, err := utils.DeduceOrigin(refURL, r.channel)
+	if err != nil {
+		return nil, commoncharm.Origin{}, errors.Trace(err)
+	}
+
+	// Charm has been supplied as a URL so we resolve and deploy using the store.
+	newURL, origin, supportedSeries, err := r.charmResolver.ResolveCharm(refURL, origin)
+	if err != nil {
+		return nil, commoncharm.Origin{}, errors.Trace(err)
+	}
+
+	_, seriesSupportedErr := charm.SeriesForCharm(r.deployedSeries, supportedSeries)
+	if !r.forceSeries && r.deployedSeries != "" && newURL.Series == "" && seriesSupportedErr != nil {
+		series := []string{"no series"}
+		if len(supportedSeries) > 0 {
+			series = supportedSeries
+		}
+		return nil, commoncharm.Origin{}, errors.Errorf(
+			"cannot upgrade from single series %q charm to a charm supporting %q. Use --force-series to override.",
+			r.deployedSeries, series,
+		)
+	}
+
+	// If no explicit revision was set with either SwitchURL
+	// or Revision flags, discover the latest.
+	if *newURL == *r.charmURL {
+		if refURL.Revision != -1 {
+			return nil, commoncharm.Origin{}, errors.Errorf("already running specified charm %q", newURL)
+		}
+		// No point in trying to upgrade a charm store charm when
+		// we just determined that's the latest revision
+		// available.
+		return nil, commoncharm.Origin{}, errors.Errorf("already running latest charm %q", newURL)
+	}
+
+	return newURL, origin, nil
+}
+
+type charmStoreRefresher struct {
+	baseRefresher
+	authorizer store.MacaroonGetter
+}
+
 // Allowed will attempt to check if the charm store is allowed to refresh.
 // Depending on the charm url, will then determine if that's true or not.
 func (r *charmStoreRefresher) Allowed(cfg RefresherConfig) (bool, error) {
@@ -259,7 +330,7 @@ func (r *charmStoreRefresher) Refresh() (*CharmID, error) {
 		return nil, errors.Errorf("already running latest charm %q", newURL)
 	}
 
-	curl, csMac, _, err := store.AddCharmFromURL(r.charmAdder, r.authorizer, newURL, origin, r.force)
+	curl, csMac, _, err := store.AddCharmWithAuthorizationFromURL(r.charmAdder, r.authorizer, newURL, origin, r.force)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -280,3 +351,51 @@ type defaultCharmRepo struct{}
 func (defaultCharmRepo) NewCharmAtPathForceSeries(path, series string, force bool) (charm.Charm, *charm.URL, error) {
 	return charmrepo.NewCharmAtPathForceSeries(path, series, force)
 }
+
+type charmHubRefresher struct {
+	baseRefresher
+}
+
+// Allowed will attempt to check if the charm store is allowed to refresh.
+// Depending on the charm url, will then determine if that's true or not.
+func (r *charmHubRefresher) Allowed(cfg RefresherConfig) (bool, error) {
+	if featureflag.Enabled(feature.CharmHubIntegration) {
+		path, err := charm.EnsureSchema(cfg.CharmRef)
+		if err != nil {
+			return false, errors.Trace(err)
+		}
+
+		curl, err := charm.ParseURL(path)
+		if err != nil {
+			return false, errors.Trace(err)
+		}
+
+		if charm.CharmHub.Matches(curl.Schema) {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// Refresh a given charm hub charm.
+// Bundles are not supported as there is no physical representation in Juju.
+func (r *charmHubRefresher) Refresh() (*CharmID, error) {
+	newURL, origin, err := r.ResolveCharm()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
+	curl, _, err := store.AddCharmFromURL(r.charmAdder, newURL, origin, r.force)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
+	return &CharmID{
+		URL:    curl,
+		Origin: origin.CoreCharmOrigin(),
+	}, nil
+}
+
+func (r *charmHubRefresher) String() string {
+	return fmt.Sprintf("attempting to refresh charm hub charm %q", r.charmRef)
+}
diff --git a/cmd/juju/application/refresher/refresher_test.go b/cmd/juju/application/refresher/refresher_test.go
@@ -330,3 +330,97 @@ func (s *charmStoreCharmRefresherSuite) TestRefreshWithARevision(c *gc.C) {
 	_, err = task.Refresh()
 	c.Assert(err, gc.ErrorMatches, `already running specified charm "cs:meshuggah-1"`)
 }
+
+type charmHubCharmRefresherSuite struct{}
+
+var _ = gc.Suite(&charmHubCharmRefresherSuite{})
+
+func (s *charmHubCharmRefresherSuite) TestRefresh(c *gc.C) {
+	ctrl := gomock.NewController(c)
+	defer ctrl.Finish()
+
+	ref := "ch:meshuggah"
+	curl := charm.MustParseURL(ref)
+	newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref))
+	origin := commoncharm.Origin{
+		Source: commoncharm.OriginCharmHub,
+	}
+
+	charmAdder := NewMockCharmAdder(ctrl)
+	charmAdder.EXPECT().AddCharm(newCurl, origin, false).Return(origin, nil)
+
+	charmResolver := NewMockCharmResolver(ctrl)
+	charmResolver.EXPECT().ResolveCharm(curl, origin).Return(newCurl, origin, []string{}, nil)
+
+	cfg := RefresherConfig{
+		CharmURL: curl,
+		CharmRef: ref,
+	}
+
+	refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver)
+	task, err := refresher(cfg)
+	c.Assert(err, jc.ErrorIsNil)
+
+	charmID, err := task.Refresh()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(charmID, gc.DeepEquals, &CharmID{
+		URL:    newCurl,
+		Origin: origin.CoreCharmOrigin(),
+	})
+}
+
+func (s *charmHubCharmRefresherSuite) TestRefreshWithNoUpdates(c *gc.C) {
+	ctrl := gomock.NewController(c)
+	defer ctrl.Finish()
+
+	ref := "ch:meshuggah"
+	curl := charm.MustParseURL(ref)
+	origin := commoncharm.Origin{
+		Source: commoncharm.OriginCharmHub,
+	}
+
+	charmAdder := NewMockCharmAdder(ctrl)
+
+	charmResolver := NewMockCharmResolver(ctrl)
+	charmResolver.EXPECT().ResolveCharm(curl, origin).Return(curl, origin, []string{}, nil)
+
+	cfg := RefresherConfig{
+		CharmURL: curl,
+		CharmRef: ref,
+	}
+
+	refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver)
+	task, err := refresher(cfg)
+	c.Assert(err, jc.ErrorIsNil)
+
+	_, err = task.Refresh()
+	c.Assert(err, gc.ErrorMatches, `already running latest charm "meshuggah"`)
+}
+
+func (s *charmHubCharmRefresherSuite) TestRefreshWithARevision(c *gc.C) {
+	ctrl := gomock.NewController(c)
+	defer ctrl.Finish()
+
+	ref := "ch:meshuggah-1"
+	curl := charm.MustParseURL(ref)
+	origin := commoncharm.Origin{
+		Source: commoncharm.OriginCharmHub,
+	}
+
+	charmAdder := NewMockCharmAdder(ctrl)
+
+	charmResolver := NewMockCharmResolver(ctrl)
+	charmResolver.EXPECT().ResolveCharm(curl, origin).Return(curl, origin, []string{}, nil)
+
+	cfg := RefresherConfig{
+		CharmURL: curl,
+		CharmRef: ref,
+	}
+
+	refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver)
+	task, err := refresher(cfg)
+	c.Assert(err, jc.ErrorIsNil)
+
+	_, err = task.Refresh()
+	c.Assert(err, gc.ErrorMatches, `already running specified charm "meshuggah-1"`)
+}
diff --git a/cmd/juju/application/store/store.go b/cmd/juju/application/store/store.go
@@ -1,9 +1,6 @@
 // Copyright 2012, 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-// TODO(natefinch): change the code in this file to use the
-// github.com/juju/juju/charmstore package to interact with the charmstore.
-
 package store
 
 import (
@@ -22,10 +19,23 @@ import (
 )
 
 // AddCharmFromURL calls the appropriate client API calls to add the
-// given charm URL to state. For non-public charm URLs, this function also
-// handles the macaroon authorization process using the given csClient.
+// given charm URL to state.
+func AddCharmFromURL(client CharmAdder, curl *charm.URL, origin commoncharm.Origin, force bool) (*charm.URL, commoncharm.Origin, error) {
+	resultOrigin, err := client.AddCharm(curl, origin, force)
+	if err != nil {
+		if params.IsCodeUnauthorized(err) {
+			return nil, commoncharm.Origin{}, errors.Forbiddenf(err.Error())
+		}
+		return nil, commoncharm.Origin{}, errors.Trace(err)
+	}
+	return curl, resultOrigin, nil
+}
+
+// AddCharmWithAuthorizationFromURL calls the appropriate client API calls to
+// add the given charm URL to state. For non-public charm URLs, this function
+// also handles the macaroon authorization process using the given csClient.
 // The resulting charm URL of the added charm is displayed on stdout.
-func AddCharmFromURL(client CharmAdder, cs MacaroonGetter, curl *charm.URL, origin commoncharm.Origin, force bool) (*charm.URL, *macaroon.Macaroon, commoncharm.Origin, error) {
+func AddCharmWithAuthorizationFromURL(client CharmAdder, cs MacaroonGetter, curl *charm.URL, origin commoncharm.Origin, force bool) (*charm.URL, *macaroon.Macaroon, commoncharm.Origin, error) {
 	var csMac *macaroon.Macaroon
 	resultOrigin, err := client.AddCharm(curl, origin, force)
 	if err != nil {