Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devicestate/firstboot: check for missing bases early #7219

Merged
merged 12 commits into from Aug 24, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 28 additions & 21 deletions image/image.go
Expand Up @@ -789,6 +789,32 @@ func copyLocalSnapFile(snapPath, targetDir string, info *snap.Info) (dstPath str
return dst, osutil.CopyFile(snapPath, dst, 0)
}

// CheckBasesAndProviders checks that all bases/default-providers are part of the seed
func CheckBasesAndProviders(snapInfos map[string]*snap.Info) []error {
var errs []error
for _, info := range snapInfos {
// ensure base is available
if info.Base != "" && info.Base != "none" {
if _, ok := snapInfos[info.Base]; !ok {
errs = append(errs, fmt.Errorf("cannot use snap %q: base %q is missing", info.InstanceName(), info.Base))
}
}
// ensure core is available
if info.Base == "" && info.SnapType == snap.TypeApp && info.InstanceName() != "snapd" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this out into info.EffectiveBase()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as we find at least a couple other places that can use it

if _, ok := snapInfos["core"]; !ok {
errs = append(errs, fmt.Errorf(`cannot use snap %q: required snap "core" missing`, info.InstanceName()))
}
}
// ensure default-providers are available
for _, dp := range neededDefaultProviders(info) {
if _, ok := snapInfos[dp]; !ok {
errs = append(errs, fmt.Errorf("cannot use snap %q: default provider %q is missing", info.InstanceName(), dp))
}
}
}
return errs
}

func ValidateSeed(seedFile string) error {
seed, err := snap.ReadSeedYaml(seedFile)
if err != nil {
Expand Down Expand Up @@ -820,28 +846,9 @@ func ValidateSeed(seedFile string) error {
errs = append(errs, fmt.Errorf("the core or snapd snap must be part of the seed"))
}

// check that all bases/default-providers are part of the seed
for _, info := range snapInfos {
// ensure base is available
if info.Base != "" && info.Base != "none" {
if _, ok := snapInfos[info.Base]; !ok {
errs = append(errs, fmt.Errorf("cannot use snap %q: base %q is missing", info.InstanceName(), info.Base))
}
}
// ensure core is available
if info.Base == "" && info.SnapType == snap.TypeApp && info.InstanceName() != "snapd" {
if _, ok := snapInfos["core"]; !ok {
errs = append(errs, fmt.Errorf(`cannot use snap %q: required snap "core" missing`, info.InstanceName()))
}
}
// ensure default-providers are available
for _, dp := range neededDefaultProviders(info) {
if _, ok := snapInfos[dp]; !ok {
errs = append(errs, fmt.Errorf("cannot use snap %q: default provider %q is missing", info.InstanceName(), dp))
}
}
if errs2 := CheckBasesAndProviders(snapInfos); errs2 != nil {
errs = append(errs, errs2...)
}

if errs != nil {
var buf bytes.Buffer
for _, err := range errs {
Expand Down
13 changes: 13 additions & 0 deletions overlord/devicestate/firstboot.go
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/snapcore/snapd/asserts/snapasserts"
"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/i18n"
"github.com/snapcore/snapd/image"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to import in this direction though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved CheckBasesAndProviders to snap package (and renamed).

"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/overlord/assertstate"
"github.com/snapcore/snapd/overlord/devicestate/internal"
Expand Down Expand Up @@ -127,6 +128,9 @@ func populateStateFromSeedImpl(st *state.State, tm timings.Measurer) ([]*state.T
}
alreadySeeded := make(map[string]bool, 3)

// allSnapInfos are collected for cross-check validation of bases
allSnapInfos := make(map[string]*snap.Info, len(seed.Snaps))

tsAll := []*state.TaskSet{}
configTss := []*state.TaskSet{}

Expand All @@ -149,6 +153,7 @@ func populateStateFromSeedImpl(st *state.State, tm timings.Measurer) ([]*state.T
}
tsAll = append(tsAll, ts)
alreadySeeded[snapName] = true
allSnapInfos[snapName] = info
return info, nil
}

Expand Down Expand Up @@ -238,6 +243,14 @@ func populateStateFromSeedImpl(st *state.State, tm timings.Measurer) ([]*state.T
}
infos = append(infos, info)
infoToTs[info] = ts
allSnapInfos[info.InstanceName()] = info
}

// validate that all snaps have bases
errs := image.CheckBasesAndProviders(allSnapInfos)
if errs != nil {
// only report the first error encountered
return nil, errs[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do something similar to https://github.com/snapcore/snapd/pull/7219/files#diff-da07f373046a0a24e67b18699dc15a70L845 here, i.e. build an error that contains all the individual issues? Maybe we can make the result from "CheckBaseAndProviders" something like:

type MultiErr struct {
  Header string
  errs []errors
}
func (e MultiErr) Add(errs []error) { ... }
func (e MultiErr) Error() string {
  var buf bytes.Buffer
  for _, err := range e.errs {
	fmt.Fprintf(&buf, "\n- %s", err)
   }
   return fmt.Sprintf("%s:%s", e.Header, buf.Bytes())
}

but looking at it it (after I typed it out) it seems like this should probably a separate PR and get a 👍 from @pedrons first :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was considering this and I'm happy to reconsider. The reason I did it this way is prepare-image validates all aspects of the image and reports all errors at once and this is useful summary for the user when assembling an image. However, when the image is already booted I thought it's fine to report just one error which is in-line with error reporting we normally do in snapd.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I don't think it's worth the complexity in seeding code itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread the issue, I'm fine either way given this will end up in snap/ code.

}

// now add/chain the tasksets in the right order, note that we
Expand Down
57 changes: 57 additions & 0 deletions overlord/devicestate/firstboot_test.go
Expand Up @@ -1657,3 +1657,60 @@ snaps:
c.Check(conn.(map[string]interface{})["auto"], Equals, true)
c.Check(conn.(map[string]interface{})["interface"], Equals, "content")
}

func (s *FirstBootTestSuite) TestPopulateFromSeedMissingBase(c *C) {
devAcct := assertstest.NewAccount(s.storeSigning, "developer", map[string]interface{}{
"account-id": "developerid",
}, "")

devAcctFn := filepath.Join(dirs.SnapSeedDir, "assertions", "developer.account")
c.Assert(ioutil.WriteFile(devAcctFn, asserts.Encode(devAcct), 0644), IsNil)

// add a model assertion and its chain
assertsChain := s.makeModelAssertionChain(c, "my-model", nil, "bar")
for i, as := range assertsChain {
fn := filepath.Join(dirs.SnapSeedDir, "assertions", strconv.Itoa(i))
err := ioutil.WriteFile(fn, asserts.Encode(as), 0644)
c.Assert(err, IsNil)
}

coreFname, kernelFname, gadgetFname := s.makeCoreSnaps(c, "")

// local snap with unknown base
snapYaml = `name: local
base: foo
unasserted: true
version: 1.0`
mockSnapFile := snaptest.MakeTestSnapWithFiles(c, snapYaml, nil)
targetSnapFile2 := filepath.Join(dirs.SnapSeedDir, "snaps", filepath.Base(mockSnapFile))
fooFname, fooDecl, fooRev := s.makeAssertedSnap(c, snapYaml, nil, snap.R(128), "developerid")
c.Assert(os.Rename(mockSnapFile, targetSnapFile2), IsNil)

declFn := filepath.Join(dirs.SnapSeedDir, "assertions", "foo.snap-declaration")
c.Assert(ioutil.WriteFile(declFn, asserts.Encode(fooDecl), 0644), IsNil)

revFn := filepath.Join(dirs.SnapSeedDir, "assertions", "foo.snap-revision")
c.Assert(ioutil.WriteFile(revFn, asserts.Encode(fooRev), 0644), IsNil)

// create a seed.yaml
content := []byte(fmt.Sprintf(`
snaps:
- name: core
file: %s
- name: pc-kernel
file: %s
- name: pc
file: %s
- name: local
file: %s
`, coreFname, kernelFname, gadgetFname, fooFname))

c.Assert(ioutil.WriteFile(filepath.Join(dirs.SnapSeedDir, "seed.yaml"), content, 0644), IsNil)

// run the firstboot stuff
st := s.overlord.State()
st.Lock()
defer st.Unlock()
_, err := devicestate.PopulateStateFromSeedImpl(st, s.perfTimings)
c.Assert(err, ErrorMatches, `cannot use snap "local": base "foo" is missing`)
}