Merge lp:~chipaca/snappy/no-sideload-overwrite into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

Don't let people sideload over things that are installed; it breaks stuff.

[John Lenton (chipaca)]

This breaks sideloading an OEM snap. I'm not sure whether that is good or bad, please advise.

[lp:~chipaca/snappy/no-sideload-overwrite]

762. By John Lenton on 2015-10-10

fixes lp:1504754

763. By John Lenton on 2015-10-10

oops, leftover println

[John Lenton (chipaca)]

<sergiusens> Chipaca, sideloading of oem was contemplated by some as a development feature, from my PoV, with our current system it should not be allowed. Only during image creation
<ogra_> Chipaca, it is needed for porting ... but only works in --developer-mode
<ogra_> Chipaca, i agree with sergiusens, it shouldnt be allowed on a running system, but the CI people might disagree (i think they want to test oem snaps like this)
<sergiusens> ogra_, half of what the oem snap does is only done on first boot though
<ogra_> right

for CI sideloading the right approach is the staging store (or airgap, when/if that happens).

[Michael Vogt (mvo)]

Thanks for this branch. I think the change is ok, it make the whole system more predictable.

However this also makes me wonder if we can go back to have the "origin" in the framework path again. I.e. instead of /apps/hello-dbus-fwk we would use /apps/hello-dbus-fwk.ubuntu to match what every other snap has. IIRC we did not do that initially because we wanted to ensure that apps have a stable path regardless of origin. However we also said at some point that frameworks can not be forked, so unless I miss something we could reconsider the path and simply some internal code. But obviously not in this branch :)

review: Approve

[Sergio Schvezov (sergiusens)]

On Mon, Oct 12, 2015 at 3:31 AM, Michael Vogt <email address hidden>
wrote:

> Review: Approve
>
> Thanks for this branch. I think the change is ok, it make the whole system
> more predictable.
>
> However this also makes me wonder if we can go back to have the "origin"
> in the framework path again. I.e. instead of /apps/hello-dbus-fwk we would
> use /apps/hello-dbus-fwk.ubuntu to match what every other snap has. IIRC we
> did not do that initially because we wanted to ensure that apps have a
> stable path regardless of origin. However we also said at some point that
> frameworks can not be forked, so unless I miss something we could
> reconsider the path and simply some internal code. But obviously not in
> this branch :)
>

Doing this would break docker, it has the same problems we saw when doing
that camera framework at the sprint, you need to specify the path to the
binary, as in /apps/docker/bin/docker and not /apps/bin/docker

[John Lenton (chipaca)]

On 13 October 2015 at 12:24, Sergio Schvezov
<email address hidden> wrote:
> Doing this would break docker, it has the same problems we saw when doing
> that camera framework at the sprint, you need to specify the path to the
> binary, as in /apps/docker/bin/docker and not /apps/bin/docker

but that could be /apps/docker.docker/current/bin/docker just fine, as
long as we tracked sideloaded things separately from the filesystem
path.

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-10-07 15:47:01 +0000
+++ snappy/click_test.go	2015-10-10 13:38:04 +0000
@@ -453,12 +453,15 @@
 	defer func() { policy.SecBase = secbase }()
 	policy.SecBase = c.MkDir()
 
-	snapName := s.buildFramework(c)
+	s.buildFramework(c)
+
+	// ...?
+
 	// rename the policy
 	//poldir := filepath.Join(tmpdir, "meta", "framework-policy", "apparmor", "policygroups")
 
-	_, err := installClick(snapName, 0, nil, testOrigin)
-	c.Assert(err, IsNil)
+	// _, err := installClick(snapName, 0, nil, testOrigin)
+	// c.Assert(err, IsNil)
 	// appdir := filepath.Join(s.tempdir, "apps", "hello.testspacethename", "1.0.1")
 	// c.Assert(removeClick(appdir, nil), IsNil)
 }
@@ -520,6 +523,7 @@
 vendor: Foo Bar <[e-mail]>`)
 	_, err := installClick(snapFile, AllowOEM, nil, testOrigin)
 	c.Assert(err, IsNil)
+	c.Assert(storeMinimalRemoteManifest("foo", "foo", testOrigin, "1.0", ""), IsNil)
 
 	contentFile := filepath.Join(s.tempdir, "oem", "foo", "1.0", "bin", "foo")
 	_, err = os.Stat(contentFile)
@@ -527,7 +531,7 @@
 	_, err = os.Stat(filepath.Join(s.tempdir, "oem", "foo", "1.0", ".click", "info", "foo.manifest"))
 	c.Assert(err, IsNil)
 
-	// an package update
+	// a package update
 	snapFile = makeTestSnapPackage(c, `name: foo
 version: 2.0
 type: oem
@@ -535,11 +539,17 @@
 vendor: Foo Bar <[e-mail]>`)
 	_, err = installClick(snapFile, 0, nil, testOrigin)
 	c.Check(err, IsNil)
+	c.Assert(storeMinimalRemoteManifest("foo", "foo", testOrigin, "2.0", ""), IsNil)
 
-	// different origin, this shows we have no origin support at this
-	// level, but sideloading also works.
-	_, err = installClick(snapFile, 0, nil, SideloadedOrigin)
-	c.Check(err, IsNil)
+	// XXX: I think this next test now tests something we actually don't
+	// want. At least for fwks and apps, sideloading something installed
+	// is a no-no. Are OEMs different in this regard?
+	//
+	// // different origin, this shows we have no origin support at this
+	// // level, but sideloading also works.
+	// _, err = installClick(snapFile, 0, nil, SideloadedOrigin)
+	// c.Check(err, IsNil)
+	// c.Assert(storeMinimalRemoteManifest("foo", "foo", SideloadedOrigin, "1.0", ""), IsNil)
 
 	// a package name fork, IOW, a different OEM package.
 	snapFile = makeTestSnapPackage(c, `name: foo-fork

=== modified file 'snappy/common_test.go'
--- snappy/common_test.go	2015-10-08 10:33:37 +0000
+++ snappy/common_test.go	2015-10-10 13:38:04 +0000
@@ -26,11 +26,13 @@
 	"path/filepath"
 	"strings"
 
+	. "gopkg.in/check.v1"
+	"gopkg.in/yaml.v2"
+
+	"launchpad.net/snappy/dirs"
 	"launchpad.net/snappy/helpers"
 	"launchpad.net/snappy/pkg"
-
-	. "gopkg.in/check.v1"
-	"gopkg.in/yaml.v2"
+	"launchpad.net/snappy/pkg/remote"
 )
 
 const (
@@ -90,9 +92,34 @@
 		return "", err
 	}
 
+	if err := storeMinimalRemoteManifest(dirName, m.Name, testOrigin, m.Version, "Hello"); err != nil {
+		return "", err
+	}
+
 	return yamlFile, nil
 }
 
+func storeMinimalRemoteManifest(qn, name, origin, version, desc string) error {
+	if origin == SideloadedOrigin {
+		panic("store remote manifest for sideloaded package")
+	}
+	content, err := yaml.Marshal(remote.Snap{
+		Name:        name,
+		Origin:      origin,
+		Version:     version,
+		Description: desc,
+	})
+	if err != nil {
+		return err
+	}
+
+	if err := ioutil.WriteFile(filepath.Join(dirs.SnapMetaDir, fmt.Sprintf("%s_%s.manifest", qn, version)), content, 0644); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func addDefaultApparmorJSON(tempdir, apparmorJSONPath string) error {
 	appArmorDir := filepath.Join(tempdir, "var", "lib", "apparmor", "clicks")
 	if err := os.MkdirAll(appArmorDir, 0775); err != nil {
@@ -167,19 +194,23 @@
 		packageYaml += strings.Join(extra, "\n") + "\n"
 	}
 
+	qn := "foo." + testOrigin
 	if snapType != pkg.TypeApp {
 		packageYaml += fmt.Sprintf("type: %s\n", snapType)
+		qn = "foo"
 	}
 
 	snapFile := makeTestSnapPackage(c, packageYaml+"version: 1.0")
 	n, err := installClick(snapFile, AllowUnauthenticated|AllowOEM, inter, testOrigin)
 	c.Assert(err, IsNil)
 	c.Assert(n, Equals, "foo")
+	c.Assert(storeMinimalRemoteManifest(qn, "foo", testOrigin, "1.0", ""), IsNil)
 
 	snapFile = makeTestSnapPackage(c, packageYaml+"version: 2.0")
 	n, err = installClick(snapFile, AllowUnauthenticated|AllowOEM, inter, testOrigin)
 	c.Assert(err, IsNil)
 	c.Assert(n, Equals, "foo")
+	c.Assert(storeMinimalRemoteManifest(qn, "foo", testOrigin, "2.0", ""), IsNil)
 
 	m := NewMetaRepository()
 	installed, err := m.Installed()

=== modified file 'snappy/parts_test.go'
--- snappy/parts_test.go	2015-10-01 19:28:27 +0000
+++ snappy/parts_test.go	2015-10-10 13:38:04 +0000
@@ -84,7 +84,7 @@
 		{QualifiedName, pkg.TypeApp, "app." + testOrigin},
 		{QualifiedName, pkg.TypeFramework, "fwk"},
 		{FullName, pkg.TypeApp, "app." + testOrigin},
-		{FullName, pkg.TypeFramework, "fwk.sideload"}, // .sideload here is an artifact of where we're storing the origin for frameworks :-/
+		{FullName, pkg.TypeFramework, "fwk." + testOrigin},
 	} {
 		names, err := ActiveSnapIterByType(t.f, t.t)
 		c.Check(err, IsNil)
@@ -212,7 +212,7 @@
 	c.Assert(err, IsNil)
 
 	parts := FindSnapsByNameAndVersion("fmk."+testOrigin, "1", installed)
-	c.Check(parts, HasLen, 0)
+	c.Check(parts, HasLen, 1)
 	parts = FindSnapsByNameAndVersion("fmk.badOrigin", "1", installed)
 	c.Check(parts, HasLen, 0)
 

=== modified file 'snappy/purge_test.go'
--- snappy/purge_test.go	2015-09-29 07:07:13 +0000
+++ snappy/purge_test.go	2015-10-10 13:38:04 +0000
@@ -41,6 +41,7 @@
 func (s *purgeSuite) SetUpTest(c *C) {
 	s.tempdir = c.MkDir()
 	dirs.SetRootDir(s.tempdir)
+	os.MkdirAll(dirs.SnapMetaDir, 0755)
 	os.MkdirAll(filepath.Join(dirs.SnapServicesDir, "multi-user.target.wants"), 0755)
 	systemd.SystemctlCmd = func(cmd ...string) ([]byte, error) {
 		return []byte("ActiveState=inactive\n"), nil

=== modified file 'snappy/service_test.go'
--- snappy/service_test.go	2015-10-10 13:38:04 +0000
+++ snappy/service_test.go	2015-10-10 13:38:04 +0000
@@ -76,6 +76,7 @@
 	os.Setenv("TZ", "")
 
 	dirs.SetRootDir(c.MkDir())
+	c.Assert(os.MkdirAll(dirs.SnapMetaDir, 0755), IsNil)
 	// TODO: this mkdir hack is so enable doesn't fail; remove when enable is the same as the rest
 	c.Assert(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "/etc/systemd/system/multi-user.target.wants"), 0755), IsNil)
 	systemd.SystemctlCmd = s.myRun

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-10-08 17:42:22 +0000
+++ snappy/snapp.go	2015-10-10 13:38:04 +0000
@@ -374,10 +374,8 @@
 		return nil
 	}
 
-	if m.Type != pkg.TypeFramework && m.Type != pkg.TypeOem {
-		if part.Origin() != origin {
-			return ErrPackageNameAlreadyInstalled
-		}
+	if part.Origin() != origin {
+		return ErrPackageNameAlreadyInstalled
 	}
 
 	return nil

=== modified file 'snappy/snapp_test.go'
--- snappy/snapp_test.go	2015-10-07 15:47:01 +0000
+++ snappy/snapp_test.go	2015-10-10 13:38:04 +0000
@@ -63,6 +63,7 @@
 	policy.SecBase = filepath.Join(s.tempdir, "security")
 	os.MkdirAll(dirs.SnapServicesDir, 0755)
 	os.MkdirAll(dirs.SnapSeccompDir, 0755)
+	os.MkdirAll(dirs.SnapMetaDir, 0755)
 
 	release.Override(release.Release{Flavor: "core", Series: "15.04"})
 
@@ -1090,8 +1091,7 @@
 }
 
 func (s *SnapTestSuite) TestIgnoresAlreadyInstalledSameOrigin(c *C) {
-	// XXX: should this be allowed? right now it is (=> you can re-sideload the same version of your apps)
-	//      (remote snaps are stopped before clickInstall gets to run)
+	// NOTE remote snaps are stopped before clickInstall gets to run
 
 	data := "name: afoo\nversion: 1\nvendor: foo"
 	yamlPath, err := makeInstalledMockSnap(s.tempdir, data)
@@ -1103,15 +1103,26 @@
 	c.Check(yaml.checkForPackageInstalled(testOrigin), IsNil)
 }
 
-func (s *SnapTestSuite) TestIgnoresAlreadyInstalledFrameworks(c *C) {
-	data := "name: afoo\nversion: 1\nvendor: foo\ntype: framework"
-	yamlPath, err := makeInstalledMockSnap(s.tempdir, data)
-	c.Assert(err, IsNil)
-	c.Assert(makeSnapActive(yamlPath), IsNil)
-
-	yaml, err := parsePackageYamlData([]byte(data), false)
-	c.Assert(err, IsNil)
-	c.Check(yaml.checkForPackageInstalled("otherns"), IsNil)
+func (s *SnapTestSuite) TestIgnoresAlreadyInstalledFrameworkSameOrigin(c *C) {
+	data := "name: afoo\nversion: 1\nvendor: foo\ntype: framework"
+	yamlPath, err := makeInstalledMockSnap(s.tempdir, data)
+	c.Assert(err, IsNil)
+	c.Assert(makeSnapActive(yamlPath), IsNil)
+
+	yaml, err := parsePackageYamlData([]byte(data), false)
+	c.Assert(err, IsNil)
+	c.Check(yaml.checkForPackageInstalled(testOrigin), IsNil)
+}
+
+func (s *SnapTestSuite) TestDetectsAlreadyInstalledFramework(c *C) {
+	data := "name: afoo\nversion: 1\nvendor: foo\ntype: framework"
+	yamlPath, err := makeInstalledMockSnap(s.tempdir, data)
+	c.Assert(err, IsNil)
+	c.Assert(makeSnapActive(yamlPath), IsNil)
+
+	yaml, err := parsePackageYamlData([]byte(data), false)
+	c.Assert(err, IsNil)
+	c.Check(yaml.checkForPackageInstalled("otherns"), Equals, ErrPackageNameAlreadyInstalled)
 }
 
 func (s *SnapTestSuite) TestUsesStoreMetaData(c *C) {