Skip to content

Commit

Permalink
many: make snap pack --check-skeleton error logs consistent (#14547)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* snap/pack/pack_test.go: adjust tested file permissions

Signed-off-by: Zeyad Gouda <[email protected]>

* snap: add error type checks in unit tests

Signed-off-by: Zeyad Gouda <[email protected]>

* snap/pack: revert to Noticef logger to avoid regressions

Signed-off-by: Zeyad Gouda <[email protected]>

* cmd/snap: fix flakey TestPackPacksFailsForMissingPaths test

Signed-off-by: Zeyad Gouda <[email protected]>

---------

Signed-off-by: Zeyad Gouda <[email protected]>
  • Loading branch information
@ZeyadYasser
ZeyadYasser authored Oct 1, 2024
1 parent 0a99a8d commit 7446f59
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 43 deletions.
2 changes: 1 addition & 1 deletion cmd/snap/cmd_pack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion cmd/snap/cmd_pack_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
54 changes: 39 additions & 15 deletions snap/container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}
}
}
}
Expand All @@ -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
}
Expand Down
62 changes: 41 additions & 21 deletions snap/container_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}

Expand Down
16 changes: 11 additions & 5 deletions snap/pack/pack_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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, "")
}

Expand Down

0 comments on commit 7446f59

Please sign in to comment.