/snapd

many: fix test cases to work with different DistroLibExecDir #3222

Merged
merged 23 commits into from Jun 19, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+304 −209
Split
View
2 cmd/cmd.go
@@ -69,7 +69,7 @@ func distroSupportsReExec() bool {
// Ensure we do not use older version of snapd, look for info file and ignore
// version of core that do not yet have it.
func coreSupportsReExec(corePath string) bool {
- fullInfo := filepath.Join(corePath, "/usr/lib/snapd/info")
+ fullInfo := filepath.Join(corePath, filepath.Join(dirs.CoreLibExecDir, "info"))
if !osutil.FileExists(fullInfo) {
return false
}
View
18 cmd/snap/cmd_run.go
@@ -365,6 +365,15 @@ func migrateXauthority(info *snap.Info) (string, error) {
func runSnapConfine(info *snap.Info, securityTag, snapApp, command, hook string, args []string) error {
snapConfine := filepath.Join(dirs.DistroLibExecDir, "snap-confine")
+ // if we re-exec, we must run the snap-confine from the core snap
+ // as well, if they get out of sync, havoc will happen
+ if isReexeced() {
@niemeyer

niemeyer Apr 26, 2017

Contributor

This should be done only once in this function. Right now it's being done twice, and it's also bogus because code right below will test for a path that does not exist. Proper way is likely to bring the code currently at the bottom here, and make sure snapConfine points to the proper thing upfront.

@morphis

morphis Apr 27, 2017

Contributor

Reworked the logic a bit.
+ // run snap-confine from the core snap. that will work because
+ // snap-confine on the core snap is mostly statically linked
+ // (except libudev and libc)
+ snapConfine = filepath.Join(dirs.SnapMountDir, "core/current", dirs.CoreLibExecDir, "snap-confine")
+ }
+
if !osutil.FileExists(snapConfine) {
if hook != "" {
logger.Noticef("WARNING: skipping running hook %q of snap %q: missing snap-confine", hook, info.Name())
@@ -401,15 +410,6 @@ func runSnapConfine(info *snap.Info, securityTag, snapApp, command, hook string,
cmd = append(cmd, snapApp)
cmd = append(cmd, args...)
- // if we re-exec, we must run the snap-confine from the core snap
- // as well, if they get out of sync, havoc will happen
- if isReexeced() {
- // run snap-confine from the core snap. that will work because
- // snap-confine on the core snap is mostly statically linked
- // (except libudev and libc)
- cmd[0] = filepath.Join(dirs.SnapMountDir, "core/current", cmd[0])
- }
-
extraEnv := make(map[string]string)
if len(xauthPath) > 0 {
extraEnv["XAUTHORITY"] = xauthPath
View
46 cmd/snap/cmd_run_test.go
@@ -98,7 +98,7 @@ func (s *SnapSuite) TestSnapRunAppIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
@mvo5

mvo5 Jun 7, 2017

Collaborator

Pardon my ignorance, but it looks like the argument for mockSnapConfine() is always dirs.DistroLibExecDir - do we really need this new argument in this case?

@morphis

morphis Jun 7, 2017

Contributor

There is exactly one case where it isn't. See https://github.com/snapcore/snapd/pull/3222/files/f78d7918f26f71239000599fd11db988cd605cc0#diff-78f744cbc3eb83652f8ed773dceac016R466

@mvo5

mvo5 Jun 7, 2017

Collaborator

Aha, silly me. I even found this in subsequent reading but then forgot to delete the comment. Sorry for the noise.
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R("x2"),
@@ -126,7 +126,7 @@ func (s *SnapSuite) TestSnapRunAppIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.app",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"snapname.app", "--arg1", "arg2"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=x2")
}
@@ -135,7 +135,7 @@ func (s *SnapSuite) TestSnapRunClassicAppIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
si := snaptest.MockSnap(c, string(mockYaml)+"confinement: classic\n", string(mockContents), &snap.SideInfo{
Revision: snap.R("x2"),
@@ -163,7 +163,7 @@ func (s *SnapSuite) TestSnapRunClassicAppIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"), "--classic",
"snap.snapname.app",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"snapname.app", "--arg1", "arg2"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=x2")
}
@@ -172,7 +172,7 @@ func (s *SnapSuite) TestSnapRunAppWithCommandIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R(42),
@@ -199,7 +199,7 @@ func (s *SnapSuite) TestSnapRunAppWithCommandIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.app",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"--command=my-command", "snapname.app", "arg1", "arg2"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=42")
}
@@ -225,7 +225,7 @@ func (s *SnapSuite) TestSnapRunHookIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R(42),
@@ -252,7 +252,7 @@ func (s *SnapSuite) TestSnapRunHookIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.hook.configure",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"--hook=configure", "snapname"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=42")
}
@@ -261,7 +261,7 @@ func (s *SnapSuite) TestSnapRunHookUnsetRevisionIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R(42),
@@ -288,7 +288,7 @@ func (s *SnapSuite) TestSnapRunHookUnsetRevisionIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.hook.configure",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"--hook=configure", "snapname"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=42")
}
@@ -297,7 +297,7 @@ func (s *SnapSuite) TestSnapRunHookSpecificRevisionIntegration(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
// Create both revisions 41 and 42
snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
@@ -326,7 +326,7 @@ func (s *SnapSuite) TestSnapRunHookSpecificRevisionIntegration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.hook.configure",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"--hook=configure", "snapname"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=41")
}
@@ -398,14 +398,14 @@ func (s *SnapSuite) TestSnapRunErorsForMissingApp(c *check.C) {
func (s *SnapSuite) TestSnapRunErorrForUnavailableApp(c *check.C) {
_, err := snaprun.Parser().ParseArgs([]string{"run", "not-there"})
- c.Assert(err, check.ErrorMatches, "cannot find current revision for snap not-there: readlink /snap/not-there/current: no such file or directory")
+ c.Assert(err, check.ErrorMatches, fmt.Sprintf("cannot find current revision for snap not-there: readlink %s/not-there/current: no such file or directory", dirs.SnapMountDir))
}
func (s *SnapSuite) TestSnapRunSaneEnvironmentHandling(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R(42),
@@ -451,8 +451,8 @@ func (s *SnapSuite) TestSnapRunIsReexeced(c *check.C) {
readlink string
expected bool
}{
- {filepath.Join(dirs.SnapMountDir, "/usr/lib/snapd/snapd"), true},
- {"/usr/lib/snapd/snapd", false},
+ {filepath.Join(dirs.SnapMountDir, dirs.CoreLibExecDir, "snapd"), true},
+ {fmt.Sprintf("%s/snapd", dirs.DistroLibExecDir), false},
@mvo5

mvo5 Jun 7, 2017

Collaborator

filepath.Join() is probably more natural here

@morphis

morphis Jun 7, 2017

Contributor

Fixed.
} {
osReadlinkResult = t.readlink
c.Check(snaprun.IsReexeced(), check.Equals, t.expected)
@@ -463,7 +463,7 @@ func (s *SnapSuite) TestSnapRunAppIntegrationFromCore(c *check.C) {
// mock installed snap
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(filepath.Join(dirs.SnapMountDir, "core", "current", dirs.CoreLibExecDir))()
si := snaptest.MockSnap(c, string(mockYaml), string(mockContents), &snap.SideInfo{
Revision: snap.R("x2"),
@@ -473,7 +473,7 @@ func (s *SnapSuite) TestSnapRunAppIntegrationFromCore(c *check.C) {
// pretend to be running from core
restorer := snaprun.MockOsReadlink(func(string) (string, error) {
- return filepath.Join(dirs.SnapMountDir, "/core/111//usr/bin/snap"), nil
+ return filepath.Join(dirs.SnapMountDir, "core/111/usr/bin/snap"), nil
})
defer restorer()
@@ -493,11 +493,11 @@ func (s *SnapSuite) TestSnapRunAppIntegrationFromCore(c *check.C) {
rest, err := snaprun.Parser().ParseArgs([]string{"run", "snapname.app", "--arg1", "arg2"})
c.Assert(err, check.IsNil)
c.Assert(rest, check.DeepEquals, []string{"snapname.app", "--arg1", "arg2"})
- c.Check(execArg0, check.Equals, filepath.Join(dirs.SnapMountDir, "/core/current", dirs.DistroLibExecDir, "snap-confine"))
+ c.Check(execArg0, check.Equals, filepath.Join(dirs.SnapMountDir, "/core/current", dirs.CoreLibExecDir, "snap-confine"))
c.Check(execArgs, check.DeepEquals, []string{
- filepath.Join(dirs.SnapMountDir, "/core/current", dirs.DistroLibExecDir, "snap-confine"),
+ filepath.Join(dirs.SnapMountDir, "/core/current", dirs.CoreLibExecDir, "snap-confine"),
"snap.snapname.app",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"snapname.app", "--arg1", "arg2"})
c.Check(execEnv, testutil.Contains, "SNAP_REVISION=x2")
}
@@ -507,7 +507,7 @@ func (s *SnapSuite) TestSnapRunXauthorityMigration(c *check.C) {
// below /tmp which the Xauthority migration expects.
dirs.SetRootDir(c.MkDir())
defer func() { dirs.SetRootDir("/") }()
- defer mockSnapConfine()()
+ defer mockSnapConfine(dirs.DistroLibExecDir)()
u, err := user.Current()
c.Assert(err, check.IsNil)
@@ -553,7 +553,7 @@ func (s *SnapSuite) TestSnapRunXauthorityMigration(c *check.C) {
c.Check(execArgs, check.DeepEquals, []string{
filepath.Join(dirs.DistroLibExecDir, "snap-confine"),
"snap.snapname.app",
- filepath.Join(dirs.DistroLibExecDir, "snap-exec"),
+ filepath.Join(dirs.CoreLibExecDir, "snap-exec"),
"snapname.app"})
expectedXauthPath := filepath.Join(dirs.XdgRuntimeDirBase, u.Uid, ".Xauthority")
View
6 cmd/snap/main_test.go
@@ -152,9 +152,9 @@ func mockVersion(v string) (restore func()) {
return func() { cmd.Version = old }
}
-func mockSnapConfine() func() {
- snapConfine := filepath.Join(dirs.DistroLibExecDir, "snap-confine")
- if err := os.MkdirAll(dirs.DistroLibExecDir, 0755); err != nil {
+func mockSnapConfine(libExecDir string) func() {
+ snapConfine := filepath.Join(libExecDir, "snap-confine")
+ if err := os.MkdirAll(libExecDir, 0755); err != nil {
panic(err)
}
if err := ioutil.WriteFile(snapConfine, nil, 0644); err != nil {
View
22 dirs/dirs.go
@@ -32,7 +32,18 @@ import (
var (
GlobalRootDir string
- SnapMountDir string
+ SnapMountDir string
+
+ // CoreSnapMountDir is the path of the snap mount dir inside
+ // the snap confinement environment. Give this nature it will
@zyga

zyga May 19, 2017

Contributor

Given its nature maybe?
+ // not include the GlobalRootDir as prefix.
+ CoreSnapMountDir string
+
+ // CoreLibExecDir is similar to CoreSnapMountDir but for the
+ // LibExecDir used by snapd.
+ CoreLibExecDir string
+ DistroLibExecDir string
+
SnapBlobDir string
@pedronis

pedronis May 11, 2017

Contributor

we probably want a newline to separate things here

@morphis

morphis May 11, 2017

Contributor

Will add one.
SnapDataDir string
SnapDataHomeGlob string
@@ -72,9 +83,6 @@ var (
ClassicDir string
- DistroLibExecDir string
- CoreLibExecDir string
-
XdgRuntimeDirBase string
XdgRuntimeDirGlob string
@@ -181,7 +189,11 @@ func SetRootDir(rootdir string) {
DistroLibExecDir = filepath.Join(rootdir, "/usr/lib/snapd")
}
- CoreLibExecDir = filepath.Join(rootdir, "/usr/lib/snapd")
+ // These are directories which are static inside the core snap and
+ // can never be prefixed as they will be always absolute once we
+ // are in the snap confinement environment.
+ CoreLibExecDir = "/usr/lib/snapd"
@zyga

zyga May 19, 2017

Contributor

Why don't we const those?
+ CoreSnapMountDir = "/snap"
@niemeyer

niemeyer May 10, 2017

Contributor

What's the line for deciding what has a prefix or not? Why do we have BaseSnapMountDir with rootdir, but the ones above not? Hard to guess on call sites, which makes it quite error prone.

@morphis

morphis May 11, 2017 •
Edited 1 time

Contributor

We need a variable which has just the clean mount dir without the root prefix as otherwise we end up with something $prefix/$prefix/$snapmountdir in the unit tests. See https://github.com/snapcore/snapd/pull/3222/files#diff-76f691a17e89c1330742e51bb468ef76R203 as an example. The only place where it is ever being used are unit tests. I can move these variables into test code and keep it away from dirs/dirs.go but actually it would be nice to keep these things in a central place.

Also CoreLibExecDir/CoreSnapMountDir are needed as these describe where in the core snap those directories are located. These are different on Fedora than the outside SnapMountDir/LibExecDir so we have to keep them distinct.

@pedronis

pedronis May 11, 2017

Contributor

if a unit test tries to write to those Core* dirs missing the rootdir will creates errors

@morphis

morphis May 11, 2017

Contributor

The unit test doesn't try to write into those dirs. They are used for example in the content interface where we check if the mount entries generated are ok. @zyga told me that those should never use SnapMountDir (which might be prefixed) but the mount dir without any prefix so /snap on Ubuntu and /var/lib/snapd/snap on Fedora.

@niemeyer

niemeyer May 12, 2017

Contributor

I suggest keeping the rootdir as originally, and stripping GlobalRootDir out of it when really needed. That makes it consistent.

@morphis

morphis May 15, 2017

Contributor

@niemeyer Doing that now. Please have another look.
XdgRuntimeDirBase = filepath.Join(rootdir, "/run/user")
XdgRuntimeDirGlob = filepath.Join(rootdir, XdgRuntimeDirBase, "*/")
View
3 dirs/dirs_test.go
@@ -50,6 +50,9 @@ func (s *DirsTestSuite) TestStripRootDir(c *C) {
}
func (s *DirsTestSuite) TestClassicConfinementSupport(c *C) {
+ // Ensure that we have a distribution as base which supports classic confinement
+ reset := release.MockReleaseInfo(&release.OS{ID: "ubuntu"})
+ defer reset()
dirs.SetRootDir("/")
c.Assert(dirs.SupportsClassicConfinement(), Equals, true)
dirs.SetRootDir("/alt")
View
15 interfaces/builtin/content.go
@@ -25,6 +25,7 @@ import (
"path/filepath"
"strings"
+ "github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/mount"
@@ -116,13 +117,23 @@ func (iface *contentInterface) path(slot *interfaces.Slot, name string) []string
return out
}
+const (
@zyga

zyga May 19, 2017

Contributor

This would feel better with a dedicated type.
+ SnapMountDir = iota
+ SnapDataDir
+ SnapCommonDir
@zyga

zyga May 19, 2017

Contributor

Wait, are those even used?

@morphis

morphis May 19, 2017

Contributor

Hm, they aren't. Seems to be a leftover. Good spot!

@morphis

morphis May 19, 2017

Contributor

Drop these now.
+)
+
// resolveSpecialVariable resolves one of the three $SNAP* variables at the
// beginning of a given path. The variables are $SNAP, $SNAP_DATA and
// $SNAP_COMMON. If there are no variables then $SNAP is implicitly assumed
// (this is the behavior that was used before the variables were supporter).
func resolveSpecialVariable(path string, snapInfo *snap.Info) string {
if strings.HasPrefix(path, "$SNAP/") || path == "$SNAP" {
- return strings.Replace(path, "$SNAP", snapInfo.MountDir(), 1)
+ // NOTE: We use dirs.CoreSnapMountDir here as the path used will be always
+ // inside the mount namespace snap-confine creates and there we will
+ // always have a /snap directory available regardless if the system
+ // we're running on supports this or not.
+ return strings.Replace(path, "$SNAP", snap.MountDirWithBasePath(dirs.CoreSnapMountDir, snapInfo.Name(), snapInfo.Revision), 1)
}
if strings.HasPrefix(path, "$SNAP_DATA/") || path == "$SNAP_DATA" {
return strings.Replace(path, "$SNAP_DATA", snapInfo.DataDir(), 1)
@@ -131,7 +142,7 @@ func resolveSpecialVariable(path string, snapInfo *snap.Info) string {
return strings.Replace(path, "$SNAP_COMMON", snapInfo.CommonDataDir(), 1)
}
// NOTE: assume $SNAP by default if nothing else is provided, for compatibility
- return filepath.Join(snapInfo.MountDir(), path)
+ return filepath.Join(snap.MountDirWithBasePath(dirs.CoreSnapMountDir, snapInfo.Name(), snapInfo.Revision), path)
}
func mountEntry(plug *interfaces.Plug, slot *interfaces.Slot, relSrc string, extraOptions ...string) mount.Entry {
Oops, something went wrong.