many: make snap pack --check-skeleton error logs consistent (#14547)

* many: make snap pack --check-skeleton error logs consistent This fixes inconsistencies in "snap pack --check-skeleton" error logs consistent. - shows detailed logs only if SNAPD_DEBUG is set. - returns first error caught It should be explored how to aggregate errors with consistency in mind as this is parsed by snapcraft. Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> * snap/pack/pack_test.go: adjust tested file permissions Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> * snap: add error type checks in unit tests Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> * snap/pack: revert to Noticef logger to avoid regressions Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> * cmd/snap: fix flakey TestPackPacksFailsForMissingPaths test Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> --------- Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
canonical · Oct 1, 2024 · 7446f59 · 7446f59
1 parent 0a99a8d
commit 7446f59
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 43 deletions.
diff --git a/cmd/snap/cmd_pack.go b/cmd/snap/cmd_pack.go
@@ -110,7 +110,7 @@ func (x *packCmd) Execute([]string) error {
 
  if x.CheckSkeleton {
  err := pack.CheckSkeleton(Stderr, x.Positional.SnapDir)
- if err == snap.ErrMissingPaths {
+ if errors.Is(err, snap.ErrMissingPaths) {
  return nil
  }
  return err

diff --git a/cmd/snap/cmd_pack_test.go b/cmd/snap/cmd_pack_test.go
@@ -107,7 +107,8 @@ func (s *SnapSuite) TestPackPacksFailsForMissingPaths(c *check.C) {
  snapDir := makeSnapDirForPack(c, packSnapYaml)
 
  _, err := snaprun.Parser(snaprun.Client()).ParseArgs([]string{"pack", snapDir, snapDir})
- c.Assert(err, check.ErrorMatches, ".* snap is unusable due to missing files")
+ // needed files/dirs are tracked in map so order of first error is not guaranteed
+ c.Assert(err, check.ErrorMatches, `.* snap is unusable due to missing files: path "(bin/hello|bin)" does not exist`)
 }
 
 func (s *SnapSuite) TestPackPacksASnap(c *check.C) {

diff --git a/snap/container.go b/snap/container.go
@@ -318,7 +318,7 @@ func validateContainer(c Container, needsrx, needsx, needsr, needsf, noskipd map
 
  // bad modes are logged instead of being returned because the end user
  // can do nothing with the info (and the developer can read the logs)
- hasBadModes := false
+ var firstBadModeErr error
  err := c.Walk(".", func(path string, info os.FileInfo, err error) error {
  if err != nil {
  return err
@@ -342,7 +342,9 @@ func validateContainer(c Container, needsrx, needsx, needsr, needsf, noskipd map
  symlinkInfo, err := evalAndValidateSymlink(c, path)
  if err != nil {
  logf("%s", err)
- hasBadModes = true
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  } else {
  // use target mode for checks below
  mode = symlinkInfo.targetMode
@@ -351,33 +353,48 @@ func validateContainer(c Container, needsrx, needsx, needsr, needsf, noskipd map
 
  if mode.IsDir() {
  if mode.Perm()&0555 != 0555 {
- logf("in %s %q: %q should be world-readable and executable, and isn't: %s", contType, name, path, mode)
- hasBadModes = true
+ err := fmt.Errorf("%q should be world-readable and executable, and isn't: %s", path, mode)
+ logf("in %s %q: %v", contType, name, err)
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  }
  } else {
  if needsrx[path] {
  if mode.Perm()&0555 != 0555 {
- logf("in snap %q: %q should be world-readable and executable, and isn't: %s", name, path, mode)
- hasBadModes = true
+ err := fmt.Errorf("%q should be world-readable and executable, and isn't: %s", path, mode)
+ logf("in snap %q: %v", name, err)
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  }
  }
  // XXX: do we need to match other directories?
  if needsf[path] || strings.HasPrefix(path, "meta/") {
  if mode&(os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 {
- logf("in %s %q: %q should be a regular file (or a symlink) and isn't", contType, name, path)
- hasBadModes = true
+ err := fmt.Errorf("%q should be a regular file (or a symlink) and isn't", path)
+ logf("in %s %q: ", contType, name, err)
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  }
  }
  if needsx[path] || strings.HasPrefix(path, "meta/hooks/") {
  if mode.Perm()&0111 == 0 {
- logf("in %s %q: %q should be executable, and isn't: %s", contType, name, path, mode)
- hasBadModes = true
+ err := fmt.Errorf("%q should be executable, and isn't: %s", path, mode)
+ logf("in %s %q: %v", contType, name, err)
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  }
  } else {
  // in needsr, or under meta but not a hook
  if mode.Perm()&0444 != 0444 {
- logf("in %s %q: %q should be world-readable, and isn't: %s", contType, name, path, mode)
- hasBadModes = true
+ err := fmt.Errorf("%q should be world-readable, and isn't: %s", path, mode)
+ logf("in %s %q: %v", contType, name, err)
+ if firstBadModeErr == nil {
+ firstBadModeErr = err
+ }
  }
  }
  }
@@ -387,18 +404,25 @@ func validateContainer(c Container, needsrx, needsx, needsr, needsf, noskipd map
  return err
  }
  if len(seen) != len(needsx)+len(needsrx)+len(needsr) {
+ var firstPath string
  for _, needs := range []map[string]bool{needsx, needsrx, needsr} {
  for path := range needs {
  if !seen[path] {
  logf("in %s %q: path %q does not exist", contType, name, path)
+ if firstPath == "" {
+ firstPath = path
+ }
  }
  }
  }
- return ErrMissingPaths
+ // TODO: aggregate path errors not just the first one
+ return fmt.Errorf("%w: path %q does not exist", ErrMissingPaths, firstPath)
  }
 
- if hasBadModes {
- return ErrBadModes
+ if firstBadModeErr != nil {
+ // TODO: fmt.Errorf("%w: %w", ErrBadModes, firstBadModeErr) when using go 1.20+
+ // TODO: aggregate bad mode errors not just the first one
+ return fmt.Errorf("%w: %v", ErrBadModes, firstBadModeErr)
  }
  return nil
 }

diff --git a/snap/container_test.go b/snap/container_test.go
@@ -84,10 +84,12 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(container, info, discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "meta" does not exist`)
 
  err = snap.ValidateComponentContainer(container, "empty-snap+comp.comp", discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "meta" does not exist`)
 }
 
 func (s *validateSuite) TestValidateContainerEmptyButBadPermFails(c *C) {
@@ -107,7 +109,8 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "." should be world-readable and executable, and isn't: drwx------`)
 }
 
 func (s *validateSuite) TestValidateComponentContainerEmptyButBadPermFails(c *C) {
@@ -121,29 +124,32 @@ func (s *validateSuite) TestValidateComponentContainerEmptyButBadPermFails(c *C)
  // snapdir has /meta/component.yaml, but / is 0700
 
  err = snap.ValidateComponentContainer(s.container(), "empty-snap+comp.comp", discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "." should be world-readable and executable, and isn't: drwx------`)
 }
 
 func (s *validateSuite) TestValidateContainerMissingSnapYamlFails(c *C) {
  const yaml = `name: empty-snap
 version: 1
 `
- container := s.container()
  c.Assert(os.Chmod(s.snapDirPath, 0755), IsNil)
  c.Assert(os.Mkdir(filepath.Join(s.snapDirPath, "meta"), 0755), IsNil)
+ container := s.container()
 
  // snapdir's / and /meta are 0755 (i.e. OK), but no /meta/snap.yaml
 
  info, err := snap.InfoFromSnapYaml([]byte(yaml))
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(container, info, discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "meta/snap.yaml" does not exist`)
 
  // component's / and /meta are 0755 (i.e. OK), but no /meta/component.yaml
 
  err = snap.ValidateComponentContainer(container, "empty-snap+comp.comp", discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "meta/component.yaml" does not exist`)
 }
 
 func (s *validateSuite) TestValidateContainerSnapYamlBadPermsFails(c *C) {
@@ -161,7 +167,8 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/snap.yaml" should be world-readable, and isn't: ----------`)
 }
 
 func (s *validateSuite) TestValidateComponentContainerSnapYamlBadPermsFails(c *C) {
@@ -173,7 +180,8 @@ func (s *validateSuite) TestValidateComponentContainerSnapYamlBadPermsFails(c *C
  // /meta/component.yaml exists, but isn't readable
 
  err := snap.ValidateComponentContainer(s.container(), "empty-snap+comp.comp", discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/component.yaml" should be world-readable, and isn't: ----------`)
 }
 
 func (s *validateSuite) TestValidateContainerSnapYamlNonRegularFails(c *C) {
@@ -191,7 +199,8 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/snap.yaml" should be a regular file \(or a symlink\) and isn't`)
 }
 
 // bootstrapEmptyContainer creates a minimal container directory under
@@ -233,7 +242,8 @@ apps:
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "foo" does not exist`)
 }
 
 func (s *validateSuite) TestValidateContainerBadAppPermsFails(c *C) {
@@ -252,7 +262,8 @@ apps:
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "foo" should be world-readable and executable, and isn't: -r--r--r--`)
 }
 
 func (s *validateSuite) TestValidateContainerBadAppDirPermsFails(c *C) {
@@ -272,7 +283,8 @@ apps:
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "apps" should be world-readable and executable, and isn't: drwx------`)
 }
 
 func (s *validateSuite) TestValidateContainerBadSvcPermsFails(c *C) {
@@ -293,7 +305,8 @@ apps:
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "svcs/bar" should be executable, and isn't: ----------`)
 }
 
 func (s *validateSuite) TestValidateContainerCompleterFails(c *C) {
@@ -316,7 +329,8 @@ apps:
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(s.container(), info, discard)
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "comp/foo.sh" does not exist`)
 }
 
 func (s *validateSuite) TestValidateContainerBadAppPathOK(c *C) {
@@ -404,7 +418,8 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(container, info, discard)
- c.Check(err, ErrorMatches, "snap is unusable due to bad permissions")
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/symlink" should be world-readable, and isn't: -rwx--x--x`)
 }
 
 func (s *validateSuite) TestValidateContainerSymlinksMetaBadTargetMode0000(c *C) {
@@ -429,7 +444,8 @@ version: 1
  c.Assert(err, IsNil)
 
  err = snap.ValidateSnapContainer(container, info, discard)
- c.Check(err, ErrorMatches, "snap is unusable due to bad permissions")
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/symlink" should be world-readable, and isn't: ----------`)
 }
 
 func (s *validateSuite) TestValidateContainerMetaExternalAbsSymlinksFails(c *C) {
@@ -450,7 +466,8 @@ version: 1
  }
 
  err = snap.ValidateSnapContainer(s.container(), info, mockLogf)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: external symlink found: meta/gui/icons/snap.empty-snap.png -> /etc/shadow`)
 }
 
 func (s *validateSuite) TestValidateContainerMetaExternalRelativeSymlinksFails(c *C) {
@@ -472,7 +489,8 @@ version: 1
  }
 
  err = snap.ValidateSnapContainer(s.container(), info, mockLogf)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, `snap is unusable due to bad permissions: external symlink found: meta/gui/icons/snap.empty-snap.png -> 1/../../2/../../3/4/../../../../..`)
 }
 
 func (s *validateSuite) TestValidateContainerMetaExternalRelativeSymlinksOk(c *C) {
@@ -518,7 +536,8 @@ version: 1
  c.Check(metaDirSymlinkErrFound, Equals, true)
  // the check for missing files precedes check for permission errors, so we
  // check for it instead.
- c.Check(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Check(err, ErrorMatches, `snap is unusable due to missing files: path "meta/snap.yaml" does not exist`)
 }
 
 func (s *validateSuite) TestValidateContainerAppsOK(c *C) {
@@ -599,7 +618,8 @@ version: 1
  }
 
  err = snap.ValidateSnapContainer(s.container(), info, mockLogf)
- c.Check(err, Equals, snap.ErrBadModes)
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, "snap is unusable due to bad permissions: too many levels of symbolic links")
  c.Check(loopFound, Equals, true)
 }
 

diff --git a/snap/pack/pack_test.go b/snap/pack/pack_test.go
@@ -167,7 +167,8 @@ apps:
 `)
  c.Assert(os.Remove(filepath.Join(sourceDir, "bin", "hello-world")), IsNil)
  _, err := pack.Pack(sourceDir, pack.Defaults)
- c.Assert(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Assert(err, ErrorMatches, `snap is unusable due to missing files: path "bin/hello-world" does not exist`)
 }
 
 func (s *packSuite) TestPackDefaultConfigureWithoutConfigureError(c *C) {
@@ -193,9 +194,12 @@ apps:
  c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)
  configureHooks := []string{"configure", "default-configure"}
  for _, hook := range configureHooks {
- c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0666), IsNil)
+ c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0644), IsNil)
  _, err := pack.Pack(sourceDir, pack.Defaults)
- c.Check(err, ErrorMatches, "snap is unusable due to bad permissions")
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Check(err, ErrorMatches, fmt.Sprintf("snap is unusable due to bad permissions: \"meta/hooks/%s\" should be executable, and isn't: -rw-r--r--", hook))
+ // Fix hook error to catch next hook's error
+ c.Assert(os.Chmod(filepath.Join(sourceDir, "meta", "hooks", hook), 755), IsNil)
  }
 }
 
@@ -246,7 +250,8 @@ apps:
 `
  c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "snapshots.yaml"), []byte(invalidSnapshotYaml), 0411), IsNil)
  _, err := pack.Pack(sourceDir, pack.Defaults)
- c.Assert(err, ErrorMatches, "snap is unusable due to bad permissions")
+ c.Check(err, testutil.ErrorIs, snap.ErrBadModes)
+ c.Assert(err, ErrorMatches, `snap is unusable due to bad permissions: "meta/snapshots.yaml" should be world-readable, and isn't: -r----x--x`)
 }
 
 func (s *packSuite) TestPackSnapshotYamlHappy(c *C) {
@@ -283,7 +288,8 @@ apps:
  c.Assert(os.Remove(filepath.Join(sourceDir, "bin", "hello-world")), IsNil)
 
  err = pack.CheckSkeleton(&buf, sourceDir)
- c.Assert(err, Equals, snap.ErrMissingPaths)
+ c.Check(err, testutil.ErrorIs, snap.ErrMissingPaths)
+ c.Assert(err, ErrorMatches, `snap is unusable due to missing files: path "bin/hello-world" does not exist`)
  c.Check(buf.String(), Equals, "")
 }