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

Drop changes to core/bundle/changes/handlers.go in cherry-pick since we
are sticking with series in transit in 3.1
  • Loading branch information
@jack-w-shaw
jack-w-shaw committed Aug 30, 2023
1 parent f1ae8f5 commit c8e01a9
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 24 deletions.
68 changes: 65 additions & 3 deletions cmd/juju/application/bundle/bundle.go
View file
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,16 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error {
_, err := devices.ParseConstraints(s)
return err
}

var errs []string
// 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 {
errs = append(errs, err.Error())
}

var verifyError error
if bundleDir == "" {
verifyError = data.Verify(verifyConstraints, verifyStorage, verifyDevices)
Expand All @@ -307,11 +318,62 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error {
}

if verr, ok := errors.Cause(verifyError).(*charm.VerificationError); ok {
errs := make([]string, len(verr.Errors))
for i, err := range verr.Errors {
errs[i] = err.Error()
for _, err := range verr.Errors {
errs = append(errs, err.Error())
}
return errors.New("the provided bundle has the following errors:\n" + strings.Join(errs, "\n"))
}
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 both 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 both 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 both supplied", name, app.Series, app.Base))
}
}
}
return nil
}
118 changes: 102 additions & 16 deletions cmd/juju/application/bundle/bundle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,21 +107,21 @@ func (s *buildModelRepSuite) TestBuildModelRepresentationApplicationsWithSubordi
Name: "default",
},
Machines: map[string]params.MachineStatus{
"0": {Base: params.Base{Name: "ubuntu", Channel: "18.04"}},
"1": {Base: params.Base{Name: "ubuntu", Channel: "18.04"}},
"0": {Base: params.Base{Name: "ubuntu", Channel: "22.04"}},
"1": {Base: params.Base{Name: "ubuntu", Channel: "22.04"}},
},
Applications: map[string]params.ApplicationStatus{
"wordpress": {
Charm: "wordpress",
Base: params.Base{Name: "ubuntu", Channel: "18.04"},
Base: params.Base{Name: "ubuntu", Channel: "22.04"},
Life: life.Alive,
Units: map[string]params.UnitStatus{
"0": {Machine: "0"},
},
},
"sub": {
Charm: "sub",
Base: params.Base{Name: "ubuntu", Channel: "18.04"},
Base: params.Base{Name: "ubuntu", Channel: "22.04"},
Life: life.Alive,
SubordinateTo: []string{"wordpress"},
},
Expand Down 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, nil)
c.Assert(err, gc.ErrorMatches, `(?s)the provided bundle has the following errors:.*application "wordpress" series "jammy" and base "ubuntu@20.04" must match if both supplied.*invalid constraints.*`)
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 @@ -294,12 +318,12 @@ func (s *buildModelRepSuite) TestBuildModelRepresentationApplicationsWithExposed
Name: "default",
},
Machines: map[string]params.MachineStatus{
"0": {Base: params.Base{Name: "ubuntu", Channel: "18.04"}},
"0": {Base: params.Base{Name: "ubuntu", Channel: "22.04"}},
},
Applications: map[string]params.ApplicationStatus{
"wordpress": {
Charm: "wordpress",
Base: params.Base{Name: "ubuntu", Channel: "18.04"},
Base: params.Base{Name: "ubuntu", Channel: "22.04"},
Life: life.Alive,
Units: map[string]params.UnitStatus{
"0": {Machine: "0"},
Expand Down Expand Up @@ -375,59 +399,121 @@ 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"
wordpress:
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"
wordpress:
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
constraints: image-id=ubuntu-bf2
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
Expand Down
7 changes: 2 additions & 5 deletions core/bundle/changes/handlers.go
View file
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 Down Expand Up @@ -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) {
Expand All @@ -1297,7 +1297,6 @@ 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) {
Expand All @@ -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)
Expand Down

0 comments on commit c8e01a9

Please sign in to comment.