Skip to content

Commit

Permalink
Allow series and base in bundles if they match
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jack-w-shaw committed Aug 21, 2023
1 parent d338beb commit a3fce20
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 15 deletions.
58 changes: 58 additions & 0 deletions cmd/juju/application/bundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package bundle

import (
"fmt"
"reflect"
"sort"
"strings"
Expand Down Expand Up @@ -299,6 +300,11 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error {
_, err := devices.ParseConstraints(s)
return err
}

if err := verifyMixedSeriesBasesMatch(data); err != nil {
return errors.Trace(err)
}

var verifyError error
if bundleDir == "" {
verifyError = data.Verify(verifyConstraints, verifyStorage, verifyDevices)
Expand All @@ -315,3 +321,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
}
99 changes: 94 additions & 5 deletions cmd/juju/application/bundle/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, []string{s.overlayFile})
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")
Expand Down Expand Up @@ -375,29 +399,29 @@ func (m stringSliceMatcher) String() string {
}

const wordpressBundle = `
series: bionic
default-base: ubuntu@18.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: xenial
base: ubuntu@16.04
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: xenial
base: ubuntu@16.04
num_units: 1
to:
- "1"
machines:
"0":
series: xenial
base: ubuntu@16.04
"1":
series: xenial
base: ubuntu@16.04
relations:
- - wordpress:db
- mysql:db
Expand Down Expand Up @@ -432,3 +456,68 @@ relations:
- - wordpress:db
- mysql:db
`

const mixedSeriesBaseBundle = `
series: bionic
default-base: ubuntu@18.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: xenial
base: ubuntu@16.04
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: jammy
num_units: 1
to:
- "1"
machines:
"0":
series: xenial
base: ubuntu@16.04
"1":
series: jammy
relations:
- - wordpress:db
- mysql:db
`

const mixedSeriesBaseBundleMismatch = `
series: bionic
default-base: ubuntu@18.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: xenial
base: ubuntu@16.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: xenial
base: ubuntu@16.04
"1":
series: jammy
relations:
- - wordpress:db
- mysql:db
`
11 changes: 11 additions & 0 deletions core/bundle/changes/changes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
21 changes: 11 additions & 10 deletions core/bundle/changes/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand All @@ -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))
Expand All @@ -62,7 +62,7 @@ func (r *resolver) handleApplications() (map[string]string, error) {
application.Series = corebase.LegacyKubernetesSeries()
}
existingApp := existing.GetApplication(name)
computedSeries, err := getSeries(application, defaultSeries)
series, err := getSeries(application, defaultSeries)

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (darwin, amd64)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (localhost)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, amd64)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, ppc64le)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, arm64)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (microk8s)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, s390x)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (windows, amd64)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (localhost)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (ubuntu-latest)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (macOS-latest)

series declared and not used

Check failure on line 65 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (microk8s)

series declared and not used
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -418,6 +418,10 @@ func (r *resolver) handleMachines() (map[string]*AddMachineChange, error) {
add := r.changes.add
machines := r.bundle.Machines
existing := r.model
defaultBase, err := computeBase(r.bundle.DefaultBase, r.bundle.Series, corebase.Base{})

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (darwin, amd64)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (localhost)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, amd64)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, ppc64le)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, arm64)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (microk8s)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, s390x)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (windows, amd64)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (localhost)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (ubuntu-latest)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (macOS-latest)

undefined: computeBase

Check failure on line 421 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (microk8s)

undefined: computeBase
if err != nil {
return nil, errors.Trace(err)
}

var defaultSeries string
if r.bundle.Series != "" {
Expand Down Expand Up @@ -1190,7 +1194,7 @@ func (r *resolver) handleUnits(addedApplications map[string]string, addedMachine
}

processor.addAllNeededUnits()
err := processor.processUnitPlacement()
err = processor.processUnitPlacement()

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (darwin, amd64)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (localhost)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, amd64)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, ppc64le)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, arm64)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (microk8s)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, s390x)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (windows, amd64)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (localhost)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (ubuntu-latest)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (macOS-latest)

undefined: err

Check failure on line 1197 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (microk8s)

undefined: err
if err != nil {

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (darwin, amd64)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (localhost)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, amd64)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, ppc64le)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, arm64)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (microk8s)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, s390x)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (windows, amd64)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (localhost)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (ubuntu-latest)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (macOS-latest)

undefined: err

Check failure on line 1198 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (microk8s)

undefined: err
return errors.Trace(err)

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (darwin, amd64)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (localhost)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, amd64)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, ppc64le)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, arm64)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Smoke (microk8s)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (linux, s390x)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Build (windows, amd64)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (localhost)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (ubuntu-latest)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Client Tests (macOS-latest)

undefined: err

Check failure on line 1199 in core/bundle/changes/handlers.go

View workflow job for this annotation

GitHub Actions / Upgrade (microk8s)

undefined: err
}
Expand Down Expand Up @@ -1286,7 +1290,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) {
Expand All @@ -1297,26 +1301,23 @@ 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)
}
// Return the default series from the local charm.
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)
Expand All @@ -1328,7 +1329,7 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string
}
return charmURL.Series, nil
}
return defaultSeries, nil
return "", nil
}

func baseToSeries(b string) (string, error) {
Expand Down

0 comments on commit a3fce20

Please sign in to comment.