From fa4a30e981f5543254596cd9ac0038bc0cdc0ff3 Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Fri, 11 Aug 2023 11:14:27 +0100 Subject: [PATCH] Allow series and base in bundles if they match Add a verification step into the client to ensure that series and base match. As such, we can treat series and bases as equivalent when they show up precedence-wise. Immidately combine into a base in core. As a helpful side effect, this means we can minimise our dealing with series in this part of the code, which will make it very easy to remove completely --- cmd/juju/application/bundle/bundle.go | 62 ++++++++++++ cmd/juju/application/bundle/bundle_test.go | 105 +++++++++++++++++++-- core/bundle/changes/changes_test.go | 11 +++ core/bundle/changes/handlers.go | 13 +-- 4 files changed, 173 insertions(+), 18 deletions(-) diff --git a/cmd/juju/application/bundle/bundle.go b/cmd/juju/application/bundle/bundle.go index deb3a258ebd1..26e1d082f78d 100644 --- a/cmd/juju/application/bundle/bundle.go +++ b/cmd/juju/application/bundle/bundle.go @@ -4,6 +4,7 @@ package bundle import ( + "fmt" "reflect" "sort" "strings" @@ -299,6 +300,15 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error { _, err := devices.ParseConstraints(s) return err } + + // This method cannot be included within data.Verify because + // to verify corresponding series and base match we need to be + // able to compare them. The charm package, however, treats bases + // and series generically and is unable to do this. + if err := verifyMixedSeriesBasesMatch(data); err != nil { + return errors.Trace(err) + } + var verifyError error if bundleDir == "" { verifyError = data.Verify(verifyConstraints, verifyStorage, verifyDevices) @@ -315,3 +325,55 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error { } return errors.Trace(verifyError) } + +func verifyMixedSeriesBasesMatch(data *charm.BundleData) error { + if data == nil { + return nil + } + if data.Series != "" && data.DefaultBase != "" { + b, err := corebase.ParseBaseFromString(data.DefaultBase) + if err != nil { + return errors.Trace(err) + } + s, err := corebase.GetSeriesFromBase(b) + if err != nil { + return errors.Trace(err) + } + if s != data.Series { + return errors.NewNotValid(nil, fmt.Sprintf("bundle series %q and base %q must match if supplied", data.Series, data.DefaultBase)) + } + } + + for name, m := range data.Machines { + if m != nil && m.Series != "" && m.Base != "" { + b, err := corebase.ParseBaseFromString(m.Base) + if err != nil { + return errors.Trace(err) + } + s, err := corebase.GetSeriesFromBase(b) + if err != nil { + return errors.Trace(err) + } + if s != m.Series { + return errors.NewNotValid(nil, fmt.Sprintf("machine %q series %q and base %q must match if supplied", name, m.Series, m.Base)) + } + } + } + + for name, app := range data.Applications { + if app != nil && app.Series != "" && app.Base != "" { + b, err := corebase.ParseBaseFromString(app.Base) + if err != nil { + return errors.Trace(err) + } + s, err := corebase.GetSeriesFromBase(b) + if err != nil { + return errors.Trace(err) + } + if s != app.Series { + return errors.NewNotValid(nil, fmt.Sprintf("application %q series %q and base %q must match if supplied", name, app.Series, app.Base)) + } + } + } + return nil +} diff --git a/cmd/juju/application/bundle/bundle_test.go b/cmd/juju/application/bundle/bundle_test.go index a93355529307..ed402419b6ac 100644 --- a/cmd/juju/application/bundle/bundle_test.go +++ b/cmd/juju/application/bundle/bundle_test.go @@ -263,6 +263,30 @@ func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleOverlayUnmarshallEr c.Assert(unmarshallErrors[0], gc.Equals, expectedError) } +func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleMixingBaseAndSeries(c *gc.C) { + defer s.setupMocks(c).Finish() + bundleData, err := charm.ReadBundleData(strings.NewReader(mixedSeriesBaseBundle)) + c.Assert(err, jc.ErrorIsNil) + s.expectParts(&charm.BundleDataPart{Data: bundleData}) + s.expectBasePath() + + obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, nil) + c.Assert(err, jc.ErrorIsNil) + c.Assert(obtained, gc.DeepEquals, bundleData) +} + +func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleMixingBaseAndSeriesMisMatch(c *gc.C) { + defer s.setupMocks(c).Finish() + bundleData, err := charm.ReadBundleData(strings.NewReader(mixedSeriesBaseBundleMismatch)) + c.Assert(err, jc.ErrorIsNil) + s.expectParts(&charm.BundleDataPart{Data: bundleData}) + s.expectBasePath() + + obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, nil) + c.Assert(err, gc.ErrorMatches, `application "wordpress" series "jammy" and base "ubuntu@20.04" must match if supplied`) + c.Assert(obtained, gc.IsNil) +} + func (s *composeAndVerifyRepSuite) setupOverlayFile(c *gc.C) { s.overlayDir = c.MkDir() s.overlayFile = filepath.Join(s.overlayDir, "config.yaml") @@ -375,13 +399,13 @@ func (m stringSliceMatcher) String() string { } const wordpressBundle = ` -series: bionic +default-base: ubuntu@22.04 applications: mysql: charm: ch:mysql revision: 42 channel: stable - series: xenial + base: ubuntu@20.04 num_units: 1 to: - "0" @@ -389,28 +413,27 @@ applications: charm: ch:wordpress channel: stable revision: 47 - series: xenial + base: ubuntu@20.04 num_units: 1 to: - "1" machines: "0": - series: xenial + base: ubuntu@20.04 "1": - series: xenial + base: ubuntu@20.04 relations: - - wordpress:db - mysql:db ` const typoBundle = ` -sries: bionic +sries: jammy applications: mysql: charm: ch:mysql revision: 42 channel: stable - series: xenial num_units: 1 to: - "0" @@ -418,16 +441,78 @@ applications: charm: ch:wordpress channel: stable revision: 47 - series: xenial num_units: 1 to: - "1" machines: "0": - series: xenial constrai: arch=arm64 "1": - series: xenial +relations: +- - wordpress:db + - mysql:db +` + +const mixedSeriesBaseBundle = ` +series: jammy +default-base: ubuntu@22.04 +applications: + mysql: + charm: ch:mysql + revision: 42 + channel: stable + series: focal + base: ubuntu@20.04 + num_units: 1 + to: + - "0" + wordpress: + charm: ch:wordpress + channel: stable + revision: 47 + series: jammy + num_units: 1 + to: + - "1" +machines: + "0": + series: focal + base: ubuntu@20.04 + "1": + series: jammy +relations: +- - wordpress:db + - mysql:db +` + +const mixedSeriesBaseBundleMismatch = ` +series: jammy +default-base: ubuntu@22.04 +applications: + mysql: + charm: ch:mysql + revision: 42 + channel: stable + series: focal + base: ubuntu@20.04 + num_units: 1 + to: + - "0" + wordpress: + charm: ch:wordpress + channel: stable + revision: 47 + series: jammy + base: ubuntu@20.04 + num_units: 1 + to: + - "1" +machines: + "0": + series: focal + base: ubuntu@20.04 + "1": + series: jammy relations: - - wordpress:db - mysql:db diff --git a/core/bundle/changes/changes_test.go b/core/bundle/changes/changes_test.go index 70e30200a304..da37ec4ab8c7 100644 --- a/core/bundle/changes/changes_test.go +++ b/core/bundle/changes/changes_test.go @@ -3756,6 +3756,17 @@ func (s *changesSuite) TestLocalCharmWithExplicitSeries(c *gc.C) { charm: %s series: xenial `, charmDir) + charmMeta := ` +name: multi-series +summary: That's a dummy charm with multi-series. +description: A dummy charm. +series: + - jammy + - focal + - bionic +`[1:] + err := os.WriteFile(filepath.Join(charmDir, "metadata.yaml"), []byte(charmMeta), 0644) + c.Assert(err, jc.ErrorIsNil) s.assertLocalBundleChanges(c, charmDir, bundleContent, "xenial") s.assertLocalBundleChangesWithDevices(c, charmDir, bundleContent, "xenial") } diff --git a/core/bundle/changes/handlers.go b/core/bundle/changes/handlers.go index 4db0f0b131a3..d126214a242d 100644 --- a/core/bundle/changes/handlers.go +++ b/core/bundle/changes/handlers.go @@ -33,6 +33,7 @@ type resolver struct { func (r *resolver) handleApplications() (map[string]string, error) { add := r.changes.add applications := r.bundle.Applications + existing := r.model var defaultSeries string if r.bundle.Series != "" { @@ -43,7 +44,6 @@ func (r *resolver) handleApplications() (map[string]string, error) { return nil, errors.Trace(err) } } - existing := r.model charms := make(map[string]string, len(applications)) addedApplications := make(map[string]string, len(applications)) @@ -1286,7 +1286,7 @@ func applicationKey(charm, arch, series, channel string, revision int) string { } // getSeries retrieves the series of a application from the ApplicationSpec or from the -// charm path or URL if provided, otherwise falling back on a default series. +// charm path or URL if provided. // // DEPRECATED: This should be all about bases. func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string, error) { @@ -1297,17 +1297,16 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string return baseToSeries(application.Base) } - // Handle local charm paths. if charm.IsValidLocalCharmOrBundlePath(application.Charm) { _, charmURL, err := corecharm.NewCharmAtPath(application.Charm, defaultSeries) if corecharm.IsMissingSeriesError(err) { // local charm path is valid but the charm doesn't declare a default series. - return defaultSeries, nil + return "", nil } else if corecharm.IsUnsupportedSeriesError(err) { // The bundle's default series is not supported by the charm, but we'll // use it anyway. This is no different to the case above where application.Series // is used without checking for potential charm incompatibility. - return defaultSeries, nil + return "", nil } else if err != nil { return "", errors.Trace(err) } @@ -1315,8 +1314,6 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string return charmURL.Series, nil } - // The following is safe because the bundle data is assumed to be already - // verified, and therefore this must be a valid charm URL. charmURL, err := charm.ParseURL(application.Charm) if err != nil { return "", errors.Trace(err) @@ -1328,7 +1325,7 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string } return charmURL.Series, nil } - return defaultSeries, nil + return "", nil } func baseToSeries(b string) (string, error) {