Ensure we validate the model target

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",

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",

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) {

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)
 	}

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()

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
 }

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 {

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
 }