Merge pull request juju#12888 from SimonRichardson/validate-model-target

juju#12888 Instead of using the charm URL, which will be wrong for bases, instead use only the metadata. The metadata will always be right! So the following code changes the purpose of this validate code, from catching a non-container on a container workload. We drop the series check because we validate that check in other places. ## QA steps ```sh $ juju bootstrap lxd test $ juju deploy hello-juju ```
sdg0919 · Apr 19, 2021 · f2b4ec0 · f2b4ec0
2 parents 2976859 + 16a006d
commit f2b4ec0
Show file tree

Hide file tree

Showing 39 changed files with 359 additions and 123 deletions.
diff --git a/apiserver/common/charms/common_test.go b/apiserver/common/charms/common_test.go
@@ -4,6 +4,9 @@
 package charms_test
 
 import (
+	"fmt"
+
+	"github.com/juju/charm/v8"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -12,6 +15,9 @@ import (
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/apiserver/testing"
 	jujutesting "github.com/juju/juju/juju/testing"
+	"github.com/juju/juju/testcharms"
+	jtesting "github.com/juju/juju/testing"
+	"github.com/juju/juju/testing/factory"
 )
 
 type charmsSuite struct {
@@ -36,6 +42,118 @@ func (s *charmsSuite) SetUpTest(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *charmsSuite) TestClientCharmInfoCAAS(c *gc.C) {
+	var clientCharmInfoTests = []struct {
+		about    string
+		series   string
+		charm    string
+		url      string
+		expected params.Charm
+		err      string
+	}{
+		{
+			about:  "charm info for meta format v2 with containers on a CAAS model",
+			series: "focal",
+			// Use cockroach for tests so that we can compare Provides and Requires.
+			charm: "cockroach",
+			url:   "local:focal/cockroachdb-0",
+			expected: params.Charm{
+				Revision: 0,
+				URL:      "local:focal/cockroachdb-0",
+				Config:   map[string]params.CharmOption{},
+				Manifest: &params.CharmManifest{
+					Bases: []params.CharmBase{
+						{
+							Name:          "ubuntu",
+							Channel:       "20.04/stable",
+							Architectures: []string{"amd64"},
+						},
+					},
+				},
+				Meta: &params.CharmMeta{
+					Name:        "cockroachdb",
+					Summary:     "cockroachdb",
+					Description: "cockroachdb",
+					Storage: map[string]params.CharmStorage{
+						"database": {
+							Name:     "database",
+							Type:     "filesystem",
+							CountMin: 1,
+							CountMax: 1,
+						},
+					},
+					Containers: map[string]params.CharmContainer{
+						"cockroachdb": {
+							Resource: "cockroachdb-image",
+							Mounts: []params.CharmMount{
+								{
+									Storage:  "database",
+									Location: "/cockroach/cockroach-data",
+								},
+							},
+						},
+					},
+					Provides: map[string]params.CharmRelation{
+						"db": {
+							Name:      "db",
+							Role:      "provider",
+							Interface: "roach",
+							Scope:     "global",
+						},
+					},
+					Resources: map[string]params.CharmResourceMeta{
+						"cockroachdb-image": {
+							Name:        "cockroachdb-image",
+							Type:        "oci-image",
+							Description: "OCI image used for cockroachdb",
+						},
+					},
+					MinJujuVersion: "0.0.0",
+				},
+				Actions: &params.CharmActions{},
+			},
+		},
+	}
+
+	for i, t := range clientCharmInfoTests {
+		c.Logf("test %d. %s", i, t.about)
+
+		otherModelOwner := s.Factory.MakeModelUser(c, nil)
+		otherSt := s.Factory.MakeCAASModel(c, &factory.ModelParams{
+			Owner: otherModelOwner.UserTag,
+			ConfigAttrs: jtesting.Attrs{
+				"controller": false,
+			},
+		})
+		defer otherSt.Close()
+
+		otherModel, err := otherSt.Model()
+		c.Assert(err, jc.ErrorIsNil)
+
+		repo := testcharms.RepoForSeries(t.series)
+		ch := repo.CharmDir(t.charm)
+		ident := fmt.Sprintf("%s-%d", ch.Meta().Name, ch.Revision())
+		curl := charm.MustParseURL(fmt.Sprintf("local:%s/%s", t.series, ident))
+
+		_, err = jujutesting.AddCharm(otherModel.State(), curl, ch, false)
+
+		c.Assert(err, jc.ErrorIsNil)
+
+		s.api, err = charms.NewCharmsAPI(otherModel.State(), s.auth)
+		c.Assert(err, jc.ErrorIsNil)
+
+		info, err := s.api.CharmInfo(params.CharmURL{URL: t.url})
+		if t.err != "" {
+			c.Check(err, gc.ErrorMatches, t.err)
+			continue
+		}
+		if c.Check(err, jc.ErrorIsNil) == false {
+			continue
+		}
+		c.Check(info, jc.DeepEquals, t.expected)
+	}
+}
+
 func (s *charmsSuite) TestClientCharmInfo(c *gc.C) {
 	var clientCharmInfoTests = []struct {
 		about    string
@@ -243,9 +361,9 @@ func (s *charmsSuite) TestClientCharmInfo(c *gc.C) {
 			},
 		},
 		{
-			about: "retrieves new format 2 charm info",
+			about: "charm info for meta format v2 without containers on a IAAS model",
 			// Use cockroach for tests so that we can compare Provides and Requires.
-			charm:  "cockroach",
+			charm:  "cockroach-container-less",
 			series: "focal",
 			url:    "local:focal/cockroachdb-0",
 			expected: params.Charm{
@@ -265,17 +383,6 @@ func (s *charmsSuite) TestClientCharmInfo(c *gc.C) {
 					Name:        "cockroachdb",
 					Summary:     "cockroachdb",
 					Description: "cockroachdb",
-					Containers: map[string]params.CharmContainer{
-						"cockroachdb": {
-							Resource: "cockroachdb-image",
-							Mounts: []params.CharmMount{
-								{
-									Storage:  "database",
-									Location: "/cockroach/cockroach-data",
-								},
-							},
-						},
-					},
 					Storage: map[string]params.CharmStorage{
 						"database": {
 							Name:     "database",

diff --git a/charmhub/find.go b/charmhub/find.go
@@ -196,9 +196,6 @@ var defaultFindResultFilter = []string{
 }
 
 var defaultRevisionFilter = []string{
-	"revision.platforms.architecture",
-	"revision.platforms.os",
-	"revision.platforms.series",
 	"revision.bases.architecture",
 	"revision.bases.name",
 	"revision.bases.channel",

diff --git a/cmd/juju/application/deploy_test.go b/cmd/juju/application/deploy_test.go
@@ -1291,7 +1291,7 @@ func (s *DeploySuite) TestInvalidSeriesForModel(c *gc.C) {
 	withCharmDeployable(s.fakeAPI, curl, "bionic", charmDir.Meta(), charmDir.Metrics(), false, false, 1, nil, nil)
 
 	err := s.runDeployForState(c, charmDir.Path, "portlandia", "--series", "kubernetes")
-	c.Assert(err, gc.ErrorMatches, `cannot add application "portlandia": series "kubernetes" in a non container model not valid`)
+	c.Assert(err, gc.ErrorMatches, `cannot add application "portlandia": container-based charm for non container based model type not valid`)
 }
 
 func (s *DeploySuite) TestForceMachineExistingContainer(c *gc.C) {

diff --git a/cmd/juju/application/deployer/deployer.go b/cmd/juju/application/deployer/deployer.go
@@ -299,26 +299,26 @@ func (d *factory) newDeployBundle(ds charm.BundleDataSource) deployBundle {
 
 func (d *factory) maybeReadLocalCharm(getter ModelConfigGetter) (Deployer, error) {
 	// NOTE: Here we select the series using the algorithm defined by
-	// `seriesSelector.charmSeries`. This serves to override the algorithm found in
-	// `charmrepo.NewCharmAtPath` which is outdated (but must still be
+	// `seriesSelector.charmSeries`. This serves to override the algorithm found
+	// in `charmrepo.NewCharmAtPath` which is outdated (but must still be
 	// called since the code is coupled with path interpretation logic which
 	// cannot easily be factored out).
 
-	// NOTE: Reading the charm here is only meant to aid in inferring the correct
-	// series, if this fails we fall back to the argument series. If reading
-	// the charm fails here it will also fail below (the charm is read again
-	// below) where it is handled properly. This is just an expedient to get
-	// the correct series. A proper refactoring of the charmrepo package is
+	// NOTE: Reading the charm here is only meant to aid in inferring the
+	// correct series, if this fails we fall back to the argument series. If
+	// reading the charm fails here it will also fail below (the charm is read
+	// again below) where it is handled properly. This is just an expedient to
+	// get the correct series. A proper refactoring of the charmrepo package is
 	// needed for a more elegant fix.
 	charmOrBundle := d.charmOrBundle
 	if isLocalSchema(charmOrBundle) {
 		charmOrBundle = charmOrBundle[6:]
 	}
 
+	var imageStream string
 	seriesName := d.series
-	ch, err := charm.ReadCharm(charmOrBundle)
 
-	var imageStream string
+	ch, err := charm.ReadCharm(charmOrBundle)
 	if err == nil {
 		modelCfg, err := getModelConfig(getter)
 		if err != nil {
@@ -352,6 +352,20 @@ func (d *factory) maybeReadLocalCharm(getter ModelConfigGetter) (Deployer, error
 		if err = charmValidationError(seriesName, ch.Meta().Name, errors.Trace(err)); err != nil {
 			return nil, errors.Trace(err)
 		}
+
+		// Prevent deploying a charm that isn't valid for the model target (CAAS or
+		// IAAS models)
+		modelType, err := d.model.ModelType()
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		var containers map[string]charm.Container
+		if ch != nil && ch.Meta() != nil {
+			containers = ch.Meta().Containers
+		}
+		if err := model.ValidateModelTarget(modelType, []string{seriesName}, containers); err != nil {
+			return nil, errors.Annotatef(err, "cannot add application %q", d.applicationName)
+		}
 	}
 
 	// Charm may have been supplied via a path reference.
@@ -377,7 +391,8 @@ func (d *factory) maybeReadLocalCharm(getter ModelConfigGetter) (Deployer, error
 		return nil, nil
 	}
 
-	// Avoid deploying charm if it's not valid for the model.
+	// Avoid deploying charm if the charm series is not correct for the
+	// avaliable image streams.
 	if err := d.validateCharmSeriesWithName(seriesName, curl.Name, imageStream); err != nil {
 		return nil, errors.Trace(err)
 	}

diff --git a/cmd/juju/application/deployer/deployer_test.go b/cmd/juju/application/deployer/deployer_test.go
@@ -57,6 +57,7 @@ func (s *deployerSuite) SetUpTest(_ *gc.C) {
 func (s *deployerSuite) TestGetDeployerPredeployedLocalCharm(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 	s.expectFilesystem()
+	s.expectModelType()
 
 	cfg := s.basicDeployerConfig()
 	ch := charm.MustParseURL("local:test-charm")
@@ -105,6 +106,7 @@ func (s *deployerSuite) TestGetDeployerLocalCharmError(c *gc.C) {
 func (s *deployerSuite) TestGetDeployerCharmStoreCharm(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 	s.expectFilesystem()
+	s.expectModelType()
 	// NotValid ensures that maybeReadRepositoryBundle won't find
 	// charmOrBundle is a bundle.
 	s.expectResolveBundleURL(errors.NotValidf("not a bundle"), 1)
@@ -123,6 +125,7 @@ func (s *deployerSuite) TestGetDeployerCharmStoreCharm(c *gc.C) {
 func (s *deployerSuite) TestCharmStoreSeriesOverride(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 	s.expectFilesystem()
+	s.expectModelType()
 	s.expectResolveBundleURL(errors.NotValidf("not a bundle"), 1)
 
 	cfg := s.basicDeployerConfig()

diff --git a/core/model/model.go b/core/model/model.go
@@ -7,9 +7,7 @@ import (
 	"github.com/juju/charm/v8"
 	"github.com/juju/collections/set"
 	"github.com/juju/errors"
-	"github.com/juju/systems"
 
-	"github.com/juju/juju/core/os"
 	"github.com/juju/juju/core/series"
 )
 
@@ -41,32 +39,25 @@ type Model struct {
 	ModelType ModelType
 }
 
-var caasOS = set.NewStrings(os.Kubernetes.String())
-
-// TODO: hml 2021-04-15
-// This should be re-written to remove the systems pkg.
-// ValidateSeries ensures the charm series is valid for the model type.
-func ValidateSeries(modelType ModelType, charmSeries string, charmFormat charm.Format) error {
-	if charmFormat >= charm.FormatV2 {
-		_, err := systems.ParseBaseFromSeries(charmSeries)
-		if err != nil {
-			return errors.Trace(err)
-		}
-	} else {
-		os, err := series.GetOSFromSeries(charmSeries)
-		if err != nil {
-			return errors.Trace(err)
+// ValidateModelTarget ensures the charm is valid for the model target type.
+// This works for both v1 and v2 of the charm metadata. By looking if the
+// series for v1 charm contains kubernetes or by checking the existence of
+// containers within the v2 metadata as a way to see if kubernetes is supported.
+func ValidateModelTarget(modelType ModelType, metaSeries []string, metaContainers map[string]charm.Container) error {
+	isSeriesK8s := set.NewStrings(metaSeries...).Contains(series.Kubernetes.String())
+	isIAAS := !(isSeriesK8s || len(metaContainers) > 0)
+
+	switch modelType {
+	case CAAS:
+		if isIAAS {
+			return errors.NotValidf("non container-based charm for container based model type")
 		}
-		switch modelType {
-		case CAAS:
-			if !caasOS.Contains(os.String()) {
-				return errors.NotValidf("series %q in a kubernetes model", charmSeries)
-			}
-		case IAAS:
-			if caasOS.Contains(os.String()) {
-				return errors.NotValidf("series %q in a non container model", charmSeries)
-			}
+	case IAAS:
+		if !isIAAS {
+			return errors.NotValidf("container-based charm for non container based model type")
 		}
+	default:
+		return errors.Errorf("invalid model type")
 	}
 	return nil
 }
diff --git a/core/model/model_test.go b/core/model/model_test.go
@@ -20,37 +20,23 @@ type ModelSuite struct {
 var _ = gc.Suite(&ModelSuite{})
 
 func (*ModelSuite) TestValidateSeries(c *gc.C) {
-	for _, t := range []struct {
-		modelType model.ModelType
-		series    string
-		valid     bool
-	}{
-		{model.IAAS, "bionic", true},
-		{model.IAAS, "kubernetes", false},
-		{model.CAAS, "bionic", false},
-		{model.CAAS, "kubernetes", true},
-	} {
-		err := model.ValidateSeries(t.modelType, t.series, charm.FormatV1)
-		if t.valid {
-			c.Check(err, jc.ErrorIsNil)
-		} else {
-			c.Check(err, jc.Satisfies, errors.IsNotValid)
-		}
+	type meta struct {
+		Series     []string
+		Containers map[string]charm.Container
 	}
-}
-
-func (*ModelSuite) TestValidateSeriesNewCharm(c *gc.C) {
 	for _, t := range []struct {
 		modelType model.ModelType
-		series    string
+		meta      meta
 		valid     bool
 	}{
-		{model.IAAS, "bionic", true},
-		{model.IAAS, "bionic", true},
-		{model.CAAS, "bionic", true},
-		{model.CAAS, "bionic", true},
+		{model.IAAS, meta{Series: []string{"bionic"}}, true},
+		{model.IAAS, meta{Series: []string{"kubernetes"}}, false},
+		{model.IAAS, meta{Containers: map[string]charm.Container{"focal": {}}}, false},
+		{model.CAAS, meta{Series: []string{"bionic"}}, false},
+		{model.CAAS, meta{Series: []string{"kubernetes"}}, true},
+		{model.CAAS, meta{Containers: map[string]charm.Container{"focal": {}}}, true},
 	} {
-		err := model.ValidateSeries(t.modelType, t.series, charm.FormatV2)
+		err := model.ValidateModelTarget(t.modelType, t.meta.Series, t.meta.Containers)
 		if t.valid {
 			c.Check(err, jc.ErrorIsNil)
 		} else {

diff --git a/juju/testing/conn.go b/juju/testing/conn.go
@@ -742,7 +742,7 @@ func AddCharm(st *state.State, curl *charm.URL, ch charm.Charm, force bool) (*st
 	}
 	sch, err := st.AddCharm(info)
 	if err != nil {
-		return nil, fmt.Errorf("cannot add charm: %v", err)
+		return nil, errors.Annotatef(err, "cannot add charm")
 	}
 	return sch, nil
 }