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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/snap/cmd_pack.go
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
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
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works.
It made me think about error aggregation and identification of first instance as a generally useful concept.

Could create error type for aggregation, containing list of Errors []error that defaults to error[0] for message, but can be iterated to concert errors to e.g. json output or detail log entry etc as required.
This could the become e.g. accErrorBadModes.add(...)

Anyways, more applicable to longer term solution...

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use %w twice? I think we can but I'm not sure if this goes all the way back to go 1.18.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can't, I believe we need go 1.20 for that https://tip.golang.org/doc/go1.20#errors

}
return nil
}
Expand Down
62 changes: 41 additions & 21 deletions snap/container_test.go
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------`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add something that checks with the equivalent of errors.Is, for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, thanks! I added those to all relevant tests

}

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
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
Loading