Merge pull request #12038 from SimonRichardson/charmhub-refresher

#12038 ## Description of change 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. ## QA steps Unsure here?
juju · Sep 23, 2020 · 6631b84 · 6631b84
2 parents 2991b49 + d9afa31
commit 6631b84
Show file tree

Hide file tree

Showing 6 changed files with 312 additions and 59 deletions.
diff --git a/cmd/juju/application/deployer/bundlehandler.go b/cmd/juju/application/deployer/bundlehandler.go
@@ -543,7 +543,7 @@ func (h *bundleHandler) addCharm(change *bundlechanges.AddCharmChange) error {
 
 	var macaroon *macaroon.Macaroon
 	var charmOrigin commoncharm.Origin
-	url, macaroon, charmOrigin, err = store.AddCharmFromURL(h.deployAPI, h.authorizer, url, origin, h.force, series)
+	url, macaroon, charmOrigin, err = store.AddCharmWithAuthorizationFromURL(h.deployAPI, h.authorizer, url, origin, h.force, series)
 	if err != nil {
 		return errors.Annotatef(err, "cannot add charm %q", chParms.Charm)
 	}

diff --git a/cmd/juju/application/deployer/charm.go b/cmd/juju/application/deployer/charm.go
@@ -440,7 +440,7 @@ func (c *charmStoreCharm) PrepareAndDeploy(ctx *cmd.Context, deployAPI DeployerA
 	}
 
 	// Store the charm in the controller
-	curl, csMac, csOrigin, err := store.AddCharmFromURL(deployAPI, macaroonGetter, storeCharmOrBundleURL, c.origin, c.force, series)
+	curl, csMac, csOrigin, err := store.AddCharmWithAuthorizationFromURL(deployAPI, macaroonGetter, storeCharmOrBundleURL, c.origin, c.force, series)
 	if err != nil {
 		if termErr, ok := errors.Cause(err).(*common.TermsRequiredError); ok {
 			return errors.Trace(termErr.UserErr())

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,46 +215,21 @@ type charmStoreRefresher struct {
 	forceSeries    bool
 }
 
-// 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) {
-	// If we're a charm hub charm reference, then skip the charm store and
-	// move onto the next
-	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 false, nil
-		}
-	}
-	return true, nil
-}
-
-// Refresh a given charm store charm.
-// Bundles are not supported as there is no physical representation in Juju.
-func (r *charmStoreRefresher) Refresh() (*CharmID, error) {
+func (r baseRefresher) ResolveCharm() (*charm.URL, commoncharm.Origin, error) {
 	refURL, err := charm.ParseURL(r.charmRef)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, commoncharm.Origin{}, errors.Trace(err)
 	}
 
 	origin, err := utils.DeduceOrigin(refURL, r.channel)
 	if err != nil {
-		return nil, errors.Trace(err)
+		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, errors.Trace(err)
+		return nil, commoncharm.Origin{}, errors.Trace(err)
 	}
 
 	_, seriesSupportedErr := charm.SeriesForCharm(r.deployedSeries, supportedSeries)
@@ -241,7 +238,7 @@ func (r *charmStoreRefresher) Refresh() (*CharmID, error) {
 		if len(supportedSeries) > 0 {
 			series = supportedSeries
 		}
-		return nil, errors.Errorf(
+		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,
 		)
@@ -251,15 +248,54 @@ func (r *charmStoreRefresher) Refresh() (*CharmID, error) {
 	// or Revision flags, discover the latest.
 	if *newURL == *r.charmURL {
 		if refURL.Revision != -1 {
-			return nil, errors.Errorf("already running specified charm %q", newURL)
+			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, errors.Errorf("already running latest charm %q", newURL)
+		return nil, commoncharm.Origin{}, errors.Errorf("already running latest charm %q", newURL)
 	}
 
-	curl, csMac, _, err := store.AddCharmFromURL(r.charmAdder, r.authorizer, newURL, origin, r.force, r.deployedSeries)
+	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) {
+	// If we're a charm hub charm reference, then skip the charm store and
+	// move onto the next
+	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 false, nil
+		}
+	}
+	return true, nil
+}
+
+// Refresh a given charm store charm.
+// Bundles are not supported as there is no physical representation in Juju.
+func (r *charmStoreRefresher) Refresh() (*CharmID, error) {
+	newURL, origin, err := r.ResolveCharm()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
+	curl, csMac, _, err := store.AddCharmWithAuthorizationFromURL(r.charmAdder, r.authorizer, newURL, origin, r.force, r.deployedSeries)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -280,3 +316,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, r.deployedSeries)
+	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,98 @@ 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, "bionic").Return(origin, nil)
+
+	charmResolver := NewMockCharmResolver(ctrl)
+	charmResolver.EXPECT().ResolveCharm(curl, origin).Return(newCurl, origin, []string{}, nil)
+
+	cfg := RefresherConfig{
+		CharmURL:       curl,
+		CharmRef:       ref,
+		DeployedSeries: "bionic",
+	}
+
+	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
@@ -19,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, series string) (*charm.URL, commoncharm.Origin, error) {
+	resultOrigin, err := client.AddCharm(curl, origin, force, series)
+	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, series string) (*charm.URL, *macaroon.Macaroon, commoncharm.Origin, error) {
+func AddCharmWithAuthorizationFromURL(client CharmAdder, cs MacaroonGetter, curl *charm.URL, origin commoncharm.Origin, force bool, series string) (*charm.URL, *macaroon.Macaroon, commoncharm.Origin, error) {
 	var csMac *macaroon.Macaroon
 	resultOrigin, err := client.AddCharm(curl, origin, force, series)
 	if err != nil {