From 4df9f2c783c16c93111756ed6719813ced016efc Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Fri, 22 Sep 2017 11:31:03 +0200 Subject: [PATCH 01/13] Skeleton. --- cmd/snap-exec/main_test.go | 7 ++++- cmd/snap/main_test.go | 3 +++ interfaces/repo.go | 54 ++++++++++++++++++++++++++++++++++++++ snap/info.go | 15 +++++++++++ snap/info_test.go | 9 ++++++- 5 files changed, 86 insertions(+), 2 deletions(-) diff --git a/cmd/snap-exec/main_test.go b/cmd/snap-exec/main_test.go index 0bf1fbd343..faf8c74ccd 100644 --- a/cmd/snap-exec/main_test.go +++ b/cmd/snap-exec/main_test.go @@ -39,7 +39,9 @@ import ( // Hook up check.v1 into the "go test" runner func Test(t *testing.T) { TestingT(t) } -type snapExecSuite struct{} +type snapExecSuite struct { + restore func() +} var _ = Suite(&snapExecSuite{}) @@ -47,11 +49,14 @@ func (s *snapExecSuite) SetUpTest(c *C) { // clean previous parse runs opts.Command = "" opts.Hook = "" + + s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) } func (s *snapExecSuite) TearDown(c *C) { syscallExec = syscall.Exec dirs.SetRootDir("/") + s.restore() } var mockYaml = []byte(`name: snapname diff --git a/cmd/snap/main_test.go b/cmd/snap/main_test.go index 100a78ec46..a0a1b96e6e 100644 --- a/cmd/snap/main_test.go +++ b/cmd/snap/main_test.go @@ -37,6 +37,7 @@ import ( "github.com/snapcore/snapd/cmd" "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/osutil" + snapdsnap "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/testutil" snap "github.com/snapcore/snapd/cmd/snap" @@ -74,6 +75,8 @@ func (s *BaseSnapSuite) SetUpTest(c *C) { snap.ReadPassword = s.readPassword s.AuthFile = filepath.Join(c.MkDir(), "json") os.Setenv(TestAuthFileEnvKey, s.AuthFile) + + snapdsnap.MockSanitizePlugsSlots(func(snapInfo *snapdsnap.Info) error { return nil }) } func (s *BaseSnapSuite) TearDownTest(c *C) { diff --git a/interfaces/repo.go b/interfaces/repo.go index da05ff603f..d1ecddaab6 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -44,6 +44,10 @@ type Repository struct { backends map[SecuritySystem]SecurityBackend } +func init() { + snap.SanitizePlugsSlots = sanitizePlugsSlots +} + // NewRepository creates an empty plug repository. func NewRepository() *Repository { return &Repository{ @@ -848,6 +852,56 @@ func (e *BadInterfacesError) Error() string { return strings.TrimSuffix(buf.String(), "; ") } +func sanitizePlugsSlots(snapInfo *snap.Info) error { + snapName := snapInfo.Name() + + bad := BadInterfacesError{ + snap: snapName, + issues: make(map[string]string), + } + + for plugName, plugInfo := range snapInfo.Plugs { + iface, ok := r.ifaces[plugInfo.Interface] + if !ok { + bad.issues[plugName] = "unknown interface" + continue + } + // Reject plug with invalid name + if err := ValidateName(plugName); err != nil { + bad.issues[plugName] = err.Error() + continue + } + plug := &Plug{PlugInfo: plugInfo} + if err := plug.Sanitize(iface); err != nil { + bad.issues[plugName] = err.Error() + continue + } + } + + for slotName, slotInfo := range snapInfo.Slots { + iface, ok := r.ifaces[slotInfo.Interface] + if !ok { + bad.issues[slotName] = "unknown interface" + continue + } + // Reject slot with invalid name + if err := ValidateName(slotName); err != nil { + bad.issues[slotName] = err.Error() + continue + } + slot := &Slot{SlotInfo: slotInfo} + if err := slot.Sanitize(iface); err != nil { + bad.issues[slotName] = err.Error() + continue + } + } + + if len(bad.issues) > 0 { + return &bad + } + return nil +} + // AddSnap adds plugs and slots declared by the given snap to the repository. // // This function can be used to implement snap install or, when used along with diff --git a/snap/info.go b/snap/info.go index 6c6801f8c6..7e2353d5c7 100644 --- a/snap/info.go +++ b/snap/info.go @@ -590,6 +590,16 @@ func (e NotFoundError) Error() string { return fmt.Sprintf("cannot find installed snap %q at revision %s", e.Snap, e.Revision) } +func MockSanitizePlugsSlots(f func(snapInfo *Info) error) (restore func()) { + old := SanitizePlugsSlots + SanitizePlugsSlots = f + return func() { SanitizePlugsSlots = old } +} + +var SanitizePlugsSlots = func(snapInfo *Info) error { + panic("SanitizePlugsSlots function not set") +} + // ReadInfo reads the snap information for the installed snap with the given name and given side-info. func ReadInfo(name string, si *SideInfo) (*Info, error) { snapYamlFn := filepath.Join(MountDir(name, si.Revision), "meta", "snap.yaml") @@ -617,6 +627,11 @@ func ReadInfo(name string, si *SideInfo) (*Info, error) { return nil, err } + err = SanitizePlugsSlots(info) + if err != nil { + return nil, err + } + return info, nil } diff --git a/snap/info_test.go b/snap/info_test.go index 0bdd7047f6..aa40756f8a 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -44,7 +44,14 @@ var _ = Suite(&infoSuite{}) func (s *infoSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) hookType := snap.NewHookType(regexp.MustCompile(".*")) - s.restore = snap.MockSupportedHookTypes([]*snap.HookType{hookType}) + restore1 := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { + return nil + }) + restore2 := snap.MockSupportedHookTypes([]*snap.HookType{hookType}) + s.restore = func() { + restore1() + restore2() + } } func (s *infoSuite) TearDownTest(c *C) { From c700354b2f30f943acc752b7ce34690988227c25 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Mon, 25 Sep 2017 14:47:31 -0400 Subject: [PATCH 02/13] ifacestate tests. --- interfaces/repo.go | 12 ++-- overlord/configstate/handler_test.go | 7 +++ overlord/devicestate/devicestate_test.go | 4 ++ overlord/hookstate/hookstate_test.go | 2 + overlord/ifacestate/ifacestate_test.go | 80 ++++++++++++++---------- 5 files changed, 66 insertions(+), 39 deletions(-) diff --git a/interfaces/repo.go b/interfaces/repo.go index d1ecddaab6..fe9c020cab 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -44,13 +44,9 @@ type Repository struct { backends map[SecuritySystem]SecurityBackend } -func init() { - snap.SanitizePlugsSlots = sanitizePlugsSlots -} - // NewRepository creates an empty plug repository. func NewRepository() *Repository { - return &Repository{ + repo := &Repository{ ifaces: make(map[string]Interface), plugs: make(map[string]map[string]*Plug), slots: make(map[string]map[string]*Slot), @@ -58,6 +54,10 @@ func NewRepository() *Repository { plugSlots: make(map[*Plug]map[*Slot]*Connection), backends: make(map[SecuritySystem]SecurityBackend), } + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { + return repo.sanitizePlugsSlots(snapInfo) + } + return repo } // Interface returns an interface with a given name. @@ -852,7 +852,7 @@ func (e *BadInterfacesError) Error() string { return strings.TrimSuffix(buf.String(), "; ") } -func sanitizePlugsSlots(snapInfo *snap.Info) error { +func (r *Repository) sanitizePlugsSlots(snapInfo *snap.Info) error { snapName := snapInfo.Name() bad := BadInterfacesError{ diff --git a/overlord/configstate/handler_test.go b/overlord/configstate/handler_test.go index de624870b9..8f1ef13789 100644 --- a/overlord/configstate/handler_test.go +++ b/overlord/configstate/handler_test.go @@ -43,6 +43,7 @@ type configureHandlerSuite struct { state *state.State context *hookstate.Context handler hookstate.Handler + restore func() } var _ = Suite(&configureHandlerSuite{}) @@ -52,6 +53,8 @@ func (s *configureHandlerSuite) SetUpTest(c *C) { s.state.Lock() defer s.state.Unlock() + s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + task := s.state.NewTask("test-task", "my test task") setup := &hookstate.HookSetup{Snap: "test-snap", Revision: snap.R(1), Hook: "test-hook"} @@ -62,6 +65,10 @@ func (s *configureHandlerSuite) SetUpTest(c *C) { s.handler = configstate.NewConfigureHandler(s.context) } +func (s *configureHandlerSuite) TearDownTest(c *C) { + s.restore() +} + func (s *configureHandlerSuite) TestBeforeInitializesTransaction(c *C) { // Initialize context s.context.Lock() diff --git a/overlord/devicestate/devicestate_test.go b/overlord/devicestate/devicestate_test.go index 6aa86585f8..2e226bc034 100644 --- a/overlord/devicestate/devicestate_test.go +++ b/overlord/devicestate/devicestate_test.go @@ -77,6 +77,7 @@ type deviceMgrSuite struct { restoreOnClassic func() restoreGenericClassicMod func() + restoreSanitize func() } var _ = Suite(&deviceMgrSuite{}) @@ -106,6 +107,8 @@ func (s *deviceMgrSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) os.MkdirAll(dirs.SnapRunDir, 0755) + s.restoreSanitize = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.bootloader = boottest.NewMockBootloader("mock", c.MkDir()) partition.ForceBootloader(s.bootloader) @@ -161,6 +164,7 @@ func (s *deviceMgrSuite) TearDownTest(c *C) { dirs.SetRootDir("") s.restoreGenericClassicMod() s.restoreOnClassic() + s.restoreSanitize() } var settleTimeout = 15 * time.Second diff --git a/overlord/hookstate/hookstate_test.go b/overlord/hookstate/hookstate_test.go index e38f4372c7..2833c98d63 100644 --- a/overlord/hookstate/hookstate_test.go +++ b/overlord/hookstate/hookstate_test.go @@ -94,6 +94,8 @@ func (s *hookManagerSuite) SetUpTest(c *C) { s.manager = manager s.o.AddManager(s.manager) + s.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil })) + hooksup := &hookstate.HookSetup{ Snap: "test-snap", Hook: "configure", diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go index 5068dbc159..27076b1f37 100644 --- a/overlord/ifacestate/ifacestate_test.go +++ b/overlord/ifacestate/ifacestate_test.go @@ -46,17 +46,17 @@ import ( func TestInterfaceManager(t *testing.T) { TestingT(t) } type interfaceManagerSuite struct { - o *overlord.Overlord - state *state.State - db *asserts.Database - privateMgr *ifacestate.InterfaceManager - privateHookMgr *hookstate.HookManager - extraIfaces []interfaces.Interface - extraBackends []interfaces.SecurityBackend - secBackend *ifacetest.TestSecurityBackend - restoreBackends func() - mockSnapCmd *testutil.MockCmd - storeSigning *assertstest.StoreStack + o *overlord.Overlord + state *state.State + db *asserts.Database + privateMgr *ifacestate.InterfaceManager + privateHookMgr *hookstate.HookManager + extraIfaces []interfaces.Interface + extraBackends []interfaces.SecurityBackend + secBackend *ifacetest.TestSecurityBackend + restore func() + mockSnapCmd *testutil.MockCmd + storeSigning *assertstest.StoreStack } var _ = Suite(&interfaceManagerSuite{}) @@ -78,6 +78,8 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { err = db.Add(s.storeSigning.StoreAccountKey("")) c.Assert(err, IsNil) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.state.Lock() assertstate.ReplaceDB(s.state, s.db) s.state.Unlock() @@ -90,7 +92,11 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { // TODO: transition this so that we don't load real backends and instead // just load the test backend here and this is nicely integrated with // extraBackends above. - s.restoreBackends = ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend}) + restoreBackends := ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend}) + s.restore = func() { + restoreBackends() + restoreSanitize() + } } func (s *interfaceManagerSuite) TearDownTest(c *C) { @@ -100,7 +106,7 @@ func (s *interfaceManagerSuite) TearDownTest(c *C) { s.privateMgr.Stop() } dirs.SetRootDir("") - s.restoreBackends() + s.restore() } func (s *interfaceManagerSuite) manager(c *C) *ifacestate.InterfaceManager { @@ -136,7 +142,7 @@ func (s *interfaceManagerSuite) TestSmoke(c *C) { } func (s *interfaceManagerSuite) TestConnectTask(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) _ = s.manager(c) @@ -262,7 +268,7 @@ func (s *interfaceManagerSuite) TestConnectDoesntConflict(c *C) { } func (s *interfaceManagerSuite) TestEnsureProcessesConnectTask(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) _ = s.manager(c) @@ -312,8 +318,7 @@ func (s *interfaceManagerSuite) TestEnsureProcessesConnectTask(c *C) { } func (s *interfaceManagerSuite) TestConnectTaskCheckInterfaceMismatch(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test2"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) _ = s.manager(c) @@ -346,7 +351,7 @@ func (s *interfaceManagerSuite) TestConnectTaskCheckInterfaceMismatch(c *C) { } func (s *interfaceManagerSuite) TestConnectTaskNoSuchSlot(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) _ = s.manager(c) @@ -358,7 +363,7 @@ func (s *interfaceManagerSuite) TestConnectTaskNoSuchSlot(c *C) { } func (s *interfaceManagerSuite) TestConnectTaskNoSuchPlug(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) _ = s.manager(c) @@ -435,7 +440,7 @@ slots: - $SLOT_PUBLISHER_ID `)) defer restore() - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) setup() _ = s.manager(c) @@ -491,7 +496,7 @@ func (s *interfaceManagerSuite) TestDisconnectFull(c *C) { func (s *interfaceManagerSuite) testDisconnect(c *C, plugSnap, plugName, slotSnap, slotName string) { // Put two snaps in place They consumer has an plug that can be connected // to slot on the producer. - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) @@ -560,6 +565,10 @@ func (s *interfaceManagerSuite) mockIface(c *C, iface interfaces.Interface) { s.extraIfaces = append(s.extraIfaces, iface) } +func (s *interfaceManagerSuite) mockIfaces(c *C, ifaces ...interfaces.Interface) { + s.extraIfaces = append(s.extraIfaces, ifaces...) +} + func (s *interfaceManagerSuite) mockSnapDecl(c *C, name, publisher string, extraHeaders map[string]interface{}) { _, err := s.db.Find(asserts.AccountType, map[string]string{ "account-id": publisher, @@ -843,7 +852,7 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsPlugs(c *C) { // The setup-profiles task will auto-connect slots with viable candidates. func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsSlots(c *C) { // Mock the interface that will be used by the test - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) // Add an OS snap. s.mockSnap(c, ubuntuCoreSnapYaml) // Add a consumer snap with unconnect plug (interface "test") @@ -892,7 +901,7 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsSlots(c *C) { // The setup-profiles task will auto-connect slots with viable multiple candidates. func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsSlotsMultiplePlugs(c *C) { // Mock the interface that will be used by the test - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) // Add an OS snap. s.mockSnap(c, ubuntuCoreSnapYaml) // Add a consumer snap with unconnect plug (interface "test") @@ -1019,7 +1028,7 @@ slots: `)) defer restore() // Add the producer snap - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnapDecl(c, "producer", "one-publisher", nil) s.mockSnap(c, producerYaml) @@ -1150,20 +1159,20 @@ func (s *interfaceManagerSuite) TestDoSetupProfilesAddsImplicitSlots(c *C) { } func (s *interfaceManagerSuite) TestDoSetupSnapSecuirtyReloadsConnectionsWhenInvokedOnPlugSide(c *C) { + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) snapInfo := s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) s.testDoSetupSnapSecuirtyReloadsConnectionsWhenInvokedOn(c, snapInfo.Name(), snapInfo.Revision) } func (s *interfaceManagerSuite) TestDoSetupSnapSecuirtyReloadsConnectionsWhenInvokedOnSlotSide(c *C) { + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) snapInfo := s.mockSnap(c, producerYaml) s.testDoSetupSnapSecuirtyReloadsConnectionsWhenInvokedOn(c, snapInfo.Name(), snapInfo.Revision) } func (s *interfaceManagerSuite) testDoSetupSnapSecuirtyReloadsConnectionsWhenInvokedOn(c *C, snapName string, revision snap.Revision) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) - s.state.Lock() s.state.Set("conns", map[string]interface{}{ "consumer:plug producer:slot": map[string]interface{}{"interface": "test"}, @@ -1437,7 +1446,7 @@ func (s *interfaceManagerSuite) testUndoDicardConns(c *C, snapName string) { } func (s *interfaceManagerSuite) TestDoRemove(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) @@ -1482,7 +1491,7 @@ func (s *interfaceManagerSuite) TestDoRemove(c *C) { } func (s *interfaceManagerSuite) TestConnectTracksConnectionsInState(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) @@ -1521,10 +1530,10 @@ func (s *interfaceManagerSuite) TestConnectTracksConnectionsInState(c *C) { } func (s *interfaceManagerSuite) TestConnectSetsUpSecurity(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) + s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) - _ = s.manager(c) s.state.Lock() @@ -1558,7 +1567,7 @@ func (s *interfaceManagerSuite) TestConnectSetsUpSecurity(c *C) { } func (s *interfaceManagerSuite) TestDisconnectSetsUpSecurity(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) @@ -1603,7 +1612,7 @@ func (s *interfaceManagerSuite) TestDisconnectSetsUpSecurity(c *C) { } func (s *interfaceManagerSuite) TestDisconnectTracksConnectionsInState(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) s.state.Lock() @@ -1643,7 +1652,7 @@ func (s *interfaceManagerSuite) TestDisconnectTracksConnectionsInState(c *C) { } func (s *interfaceManagerSuite) TestManagerReloadsConnections(c *C) { - s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"}) + s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"}) s.mockSnap(c, consumerYaml) s.mockSnap(c, producerYaml) @@ -1675,6 +1684,11 @@ func (s *interfaceManagerSuite) TestSetupProfilesDevModeMultiple(c *C) { InterfaceName: "test", }) c.Assert(err, IsNil) + err = repo.AddInterface(&ifacetest.TestInterface{ + InterfaceName: "test2", + }) + c.Assert(err, IsNil) + err = repo.AddSlot(&interfaces.Slot{ SlotInfo: &snap.SlotInfo{ Snap: siC, From c2526260c3cae21cd30ecd3450e890123ba3d70a Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Mon, 25 Sep 2017 16:09:03 -0400 Subject: [PATCH 03/13] More tests, all green for now. --- overlord/snapstate/booted_test.go | 7 +++++++ overlord/snapstate/check_snap_test.go | 8 +++++++- overlord/snapstate/snapstate_test.go | 12 +++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/overlord/snapstate/booted_test.go b/overlord/snapstate/booted_test.go index 407883a62d..0ed7479e2d 100644 --- a/overlord/snapstate/booted_test.go +++ b/overlord/snapstate/booted_test.go @@ -46,6 +46,7 @@ type bootedSuite struct { state *state.State snapmgr *snapstate.SnapManager fakeBackend *fakeSnappyBackend + restore func() } var _ = Suite(&bootedSuite{}) @@ -55,6 +56,11 @@ func (bs *bootedSuite) SetUpTest(c *C) { err := os.MkdirAll(filepath.Dir(dirs.SnapStateFile), 0755) c.Assert(err, IsNil) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + bs.restore = func() { + restoreSanitize() + } + // booted is not running on classic release.MockOnClassic(false) @@ -83,6 +89,7 @@ func (bs *bootedSuite) TearDownTest(c *C) { release.MockOnClassic(true) dirs.SetRootDir("") partition.ForceBootloader(nil) + bs.restore() } var osSI1 = &snap.SideInfo{RealName: "core", Revision: snap.R(1)} diff --git a/overlord/snapstate/check_snap_test.go b/overlord/snapstate/check_snap_test.go index c5e655c889..00e2126cca 100644 --- a/overlord/snapstate/check_snap_test.go +++ b/overlord/snapstate/check_snap_test.go @@ -37,7 +37,8 @@ import ( ) type checkSnapSuite struct { - st *state.State + st *state.State + restore func() } var _ = Suite(&checkSnapSuite{}) @@ -45,10 +46,15 @@ var _ = Suite(&checkSnapSuite{}) func (s *checkSnapSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) s.st = state.New(nil) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.restore = func() { + restoreSanitize() + } } func (s *checkSnapSuite) TearDownTest(c *C) { dirs.SetRootDir("") + s.restore() } func (s *checkSnapSuite) TestCheckSnapErrorOnUnsupportedArchitecture(c *C) { diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index 2ab44c434a..290a3c9699 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -83,6 +83,8 @@ func (s *snapmgrTestSuite) SetUpTest(c *C) { s.o = overlord.Mock() s.state = s.o.State() + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.fakeBackend = &fakeSnappyBackend{} s.fakeStore = &fakeStore{ fakeCurrentProgress: 75, @@ -117,6 +119,7 @@ func (s *snapmgrTestSuite) SetUpTest(c *C) { restore2() restore1() + restoreSanitize() dirs.SetRootDir("/") } @@ -4945,7 +4948,8 @@ func (s *snapmgrTestSuite) TestDefaultRefreshScheduleParsing(c *C) { } type snapmgrQuerySuite struct { - st *state.State + st *state.State + restore func() } var _ = Suite(&snapmgrQuerySuite{}) @@ -4955,6 +4959,11 @@ func (s *snapmgrQuerySuite) SetUpTest(c *C) { st.Lock() defer st.Unlock() + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.restore = func() { + restoreSanitize() + } + s.st = st dirs.SetRootDir(c.MkDir()) @@ -4989,6 +4998,7 @@ description: | func (s *snapmgrQuerySuite) TearDownTest(c *C) { dirs.SetRootDir("") + s.restore() } func (s *snapmgrQuerySuite) TestInfo(c *C) { From eabdbb0ff3acf755f78a83364288639a2aa75235 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 26 Sep 2017 11:16:26 -0400 Subject: [PATCH 04/13] Test updates. Use no-op SanitizePlugAndSlots for snap and snap-exec commands. --- cmd/snap-exec/main.go | 2 ++ cmd/snap/main.go | 2 ++ interfaces/repo.go | 36 ++++-------------------------------- interfaces/repo_test.go | 32 ++++++++++++++++++-------------- snap/info_test.go | 11 +++++++++++ 5 files changed, 37 insertions(+), 46 deletions(-) diff --git a/cmd/snap-exec/main.go b/cmd/snap-exec/main.go index bcb3a0682e..90e7a804ed 100644 --- a/cmd/snap-exec/main.go +++ b/cmd/snap-exec/main.go @@ -43,6 +43,8 @@ var opts struct { } func main() { + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { return nil } + if err := run(); err != nil { fmt.Fprintf(os.Stderr, "cannot snap-exec: %s\n", err) os.Exit(1) diff --git a/cmd/snap/main.go b/cmd/snap/main.go index 1d58ce3b32..5b1cbb4bbb 100644 --- a/cmd/snap/main.go +++ b/cmd/snap/main.go @@ -38,11 +38,13 @@ import ( "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/logger" "github.com/snapcore/snapd/osutil" + "github.com/snapcore/snapd/snap" ) func init() { // set User-Agent for when 'snap' talks to the store directly (snap download etc...) httputil.SetUserAgentFromVersion(cmd.Version, "snap") + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { return nil } } // Standard streams, redirected for testing. diff --git a/interfaces/repo.go b/interfaces/repo.go index fe9c020cab..82e0c645dd 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -55,7 +55,7 @@ func NewRepository() *Repository { backends: make(map[SecuritySystem]SecurityBackend), } snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { - return repo.sanitizePlugsSlots(snapInfo) + return repo.SanitizePlugsSlots(snapInfo) } return repo } @@ -852,7 +852,7 @@ func (e *BadInterfacesError) Error() string { return strings.TrimSuffix(buf.String(), "; ") } -func (r *Repository) sanitizePlugsSlots(snapInfo *snap.Info) error { +func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { snapName := snapInfo.Name() bad := BadInterfacesError{ @@ -936,46 +936,18 @@ func (r *Repository) AddSnap(snapInfo *snap.Info) error { } for plugName, plugInfo := range snapInfo.Plugs { - iface, ok := r.ifaces[plugInfo.Interface] - if !ok { - bad.issues[plugName] = "unknown interface" - continue - } - // Reject plug with invalid name - if err := ValidateName(plugName); err != nil { - bad.issues[plugName] = err.Error() - continue - } - plug := &Plug{PlugInfo: plugInfo} - if err := plug.Sanitize(iface); err != nil { - bad.issues[plugName] = err.Error() - continue - } if r.plugs[snapName] == nil { r.plugs[snapName] = make(map[string]*Plug) } + plug := &Plug{PlugInfo: plugInfo} r.plugs[snapName][plugName] = plug } for slotName, slotInfo := range snapInfo.Slots { - iface, ok := r.ifaces[slotInfo.Interface] - if !ok { - bad.issues[slotName] = "unknown interface" - continue - } - // Reject slot with invalid name - if err := ValidateName(slotName); err != nil { - bad.issues[slotName] = err.Error() - continue - } - slot := &Slot{SlotInfo: slotInfo} - if err := slot.Sanitize(iface); err != nil { - bad.issues[slotName] = err.Error() - continue - } if r.slots[snapName] == nil { r.slots[snapName] = make(map[string]*Slot) } + slot := &Slot{SlotInfo: slotInfo} r.slots[snapName][slotName] = slot } diff --git a/interfaces/repo_test.go b/interfaces/repo_test.go index 1901155d12..144cce4431 100644 --- a/interfaces/repo_test.go +++ b/interfaces/repo_test.go @@ -1539,7 +1539,7 @@ func (s *AddRemoveSuite) SetUpTest(c *C) { c.Assert(err, IsNil) } -func (s *AddRemoveSuite) TestAddSnapComplexErrorHandling(c *C) { +/*func (s *AddRemoveSuite) TestAddSnapComplexErrorHandling(c *C) { err := s.repo.AddInterface(&ifacetest.TestInterface{ InterfaceName: "invalid-plug-iface", SanitizePlugCallback: func(plug *Plug) error { return fmt.Errorf("plug is invalid") }, @@ -1569,7 +1569,7 @@ slots: c.Check(s.repo.Plug("complex", "unknown-plug-iface"), IsNil) c.Check(s.repo.Slot("complex", "invalid-slot-iface"), IsNil) c.Check(s.repo.Slot("complex", "unknown-slot-iface"), IsNil) -} +}*/ const testConsumerYaml = ` name: consumer @@ -1616,18 +1616,6 @@ func (s *AddRemoveSuite) TestAddSnapAddsPlugs(c *C) { c.Assert(s.repo.Plug("consumer", "iface"), Not(IsNil)) } -func (s *AddRemoveSuite) TestAddSnapErrorsOnInvalidSlotNames(c *C) { - _, err := s.addSnap(c, testConsumerInvalidSlotNameYaml) - c.Assert(err, NotNil) - c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) -} - -func (s *AddRemoveSuite) TestAddSnapErrorsOnInvalidPlugNames(c *C) { - _, err := s.addSnap(c, testConsumerInvalidPlugNameYaml) - c.Assert(err, NotNil) - c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) -} - func (s *AddRemoveSuite) TestAddSnapErrorsOnExistingSnapPlugs(c *C) { _, err := s.addSnap(c, testConsumerYaml) c.Assert(err, IsNil) @@ -1925,3 +1913,19 @@ slots: {Name: "i2", Summary: "i2 summary"}, }) } + +func (s *RepositorySuite) TestSanitizeErrorsOnInvalidSlotNames(c *C) { + c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) + snapInfo := snaptest.MockInfo(c, testConsumerInvalidSlotNameYaml, nil) + err := s.testRepo.SanitizePlugsSlots(snapInfo) + c.Assert(err, NotNil) + c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) +} + +func (s *RepositorySuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { + c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) + snapInfo := snaptest.MockInfo(c, testConsumerInvalidPlugNameYaml, nil) + err := s.testRepo.SanitizePlugsSlots(snapInfo) + c.Assert(err, NotNil) + c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) +} diff --git a/snap/info_test.go b/snap/info_test.go index aa40756f8a..1d6155b8d3 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -504,6 +504,17 @@ func (s *infoSuite) checkInstalledSnapAndSnapFile(c *C, yaml string, contents st checker(c, info) } +func (s *infoSuite) TestReadInfoErrorsOnPlugSlotValidation(c *C) { + snap.SanitizePlugsSlots = func(info *snap.Info) error { + return fmt.Errorf("ERROR") + } + si := &snap.SideInfo{Revision: snap.R(1)} + snaptest.MockSnap(c, sampleYaml, sampleContents, si) + _, err := snap.ReadInfo("sample", si) + c.Assert(err, NotNil) + c.Assert(err, ErrorMatches, "ERROR") +} + func (s *infoSuite) TestReadInfoNoHooks(c *C) { yaml := `name: foo version: 1.0` From 6231016e28538e7bc231b2944b1a73cdaa6cd080 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 26 Sep 2017 15:26:30 -0400 Subject: [PATCH 05/13] Ignore BadInterfaceError. --- interfaces/repo.go | 28 +++++++++++++--------------- snap/info.go | 3 +-- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/interfaces/repo.go b/interfaces/repo.go index 82e0c645dd..67975ae5a8 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -55,7 +55,14 @@ func NewRepository() *Repository { backends: make(map[SecuritySystem]SecurityBackend), } snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { - return repo.SanitizePlugsSlots(snapInfo) + if err := repo.SanitizePlugsSlots(snapInfo); err != nil { + if _, ok := err.(*BadInterfacesError); ok { + //task.Logf("%s", err) FIXME + } else { + return err + } + } + return nil } return repo } @@ -853,6 +860,11 @@ func (e *BadInterfacesError) Error() string { } func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { + err := snap.Validate(snapInfo) + if err != nil { + return err + } + snapName := snapInfo.Name() bad := BadInterfacesError{ @@ -916,11 +928,6 @@ func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { // Unknown interfaces and plugs/slots that don't validate are not added. // Information about those failures are returned to the caller. func (r *Repository) AddSnap(snapInfo *snap.Info) error { - err := snap.Validate(snapInfo) - if err != nil { - return err - } - r.m.Lock() defer r.m.Unlock() @@ -930,11 +937,6 @@ func (r *Repository) AddSnap(snapInfo *snap.Info) error { return fmt.Errorf("cannot register interfaces for snap %q more than once", snapName) } - bad := BadInterfacesError{ - snap: snapName, - issues: make(map[string]string), - } - for plugName, plugInfo := range snapInfo.Plugs { if r.plugs[snapName] == nil { r.plugs[snapName] = make(map[string]*Plug) @@ -950,10 +952,6 @@ func (r *Repository) AddSnap(snapInfo *snap.Info) error { slot := &Slot{SlotInfo: slotInfo} r.slots[snapName][slotName] = slot } - - if len(bad.issues) > 0 { - return &bad - } return nil } diff --git a/snap/info.go b/snap/info.go index 7e2353d5c7..bf83713064 100644 --- a/snap/info.go +++ b/snap/info.go @@ -627,8 +627,7 @@ func ReadInfo(name string, si *SideInfo) (*Info, error) { return nil, err } - err = SanitizePlugsSlots(info) - if err != nil { + if err := SanitizePlugsSlots(info); err != nil { return nil, err } From 46c79aed60e0b8a3cfe7d827a32af297f2d4950e Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Wed, 27 Sep 2017 09:44:35 -0500 Subject: [PATCH 06/13] Handle bad interfaces and log them via task.Logf again. --- interfaces/repo.go | 65 ++++----------------------------- interfaces/repo_test.go | 10 +++-- overlord/ifacestate/handlers.go | 10 ++--- overlord/snapstate/handlers.go | 1 + snap/info.go | 30 +++++++++++++++ snap/info_snap_yaml.go | 2 + 6 files changed, 51 insertions(+), 67 deletions(-) diff --git a/interfaces/repo.go b/interfaces/repo.go index 67975ae5a8..d4c7a64e0e 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -20,7 +20,6 @@ package interfaces import ( - "bytes" "fmt" "sort" "strings" @@ -55,14 +54,7 @@ func NewRepository() *Repository { backends: make(map[SecuritySystem]SecurityBackend), } snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { - if err := repo.SanitizePlugsSlots(snapInfo); err != nil { - if _, ok := err.(*BadInterfacesError); ok { - //task.Logf("%s", err) FIXME - } else { - return err - } - } - return nil + return repo.SanitizePlugsSlots(snapInfo) } return repo } @@ -826,66 +818,26 @@ func (r *Repository) SnapSpecification(securitySystem SecuritySystem, snapName s return spec, nil } -// BadInterfacesError is returned when some snap interfaces could not be registered. -// Those interfaces not mentioned in the error were successfully registered. -type BadInterfacesError struct { - snap string - issues map[string]string // slot or plug name => message -} - -func (e *BadInterfacesError) Error() string { - inverted := make(map[string][]string) - for name, reason := range e.issues { - inverted[reason] = append(inverted[reason], name) - } - var buf bytes.Buffer - fmt.Fprintf(&buf, "snap %q has bad plugs or slots: ", e.snap) - reasons := make([]string, 0, len(inverted)) - for reason := range inverted { - reasons = append(reasons, reason) - } - sort.Strings(reasons) - for _, reason := range reasons { - names := inverted[reason] - sort.Strings(names) - for i, name := range names { - if i > 0 { - buf.WriteString(", ") - } - buf.WriteString(name) - } - fmt.Fprintf(&buf, " (%s); ", reason) - } - return strings.TrimSuffix(buf.String(), "; ") -} - func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { err := snap.Validate(snapInfo) if err != nil { return err } - snapName := snapInfo.Name() - - bad := BadInterfacesError{ - snap: snapName, - issues: make(map[string]string), - } - for plugName, plugInfo := range snapInfo.Plugs { iface, ok := r.ifaces[plugInfo.Interface] if !ok { - bad.issues[plugName] = "unknown interface" + snapInfo.BadInterfaces[plugName] = "unknown interface" continue } // Reject plug with invalid name if err := ValidateName(plugName); err != nil { - bad.issues[plugName] = err.Error() + snapInfo.BadInterfaces[plugName] = err.Error() continue } plug := &Plug{PlugInfo: plugInfo} if err := plug.Sanitize(iface); err != nil { - bad.issues[plugName] = err.Error() + snapInfo.BadInterfaces[plugName] = err.Error() continue } } @@ -893,24 +845,21 @@ func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { for slotName, slotInfo := range snapInfo.Slots { iface, ok := r.ifaces[slotInfo.Interface] if !ok { - bad.issues[slotName] = "unknown interface" + snapInfo.BadInterfaces[slotName] = "unknown interface" continue } // Reject slot with invalid name if err := ValidateName(slotName); err != nil { - bad.issues[slotName] = err.Error() + snapInfo.BadInterfaces[slotName] = err.Error() continue } slot := &Slot{SlotInfo: slotInfo} if err := slot.Sanitize(iface); err != nil { - bad.issues[slotName] = err.Error() + snapInfo.BadInterfaces[slotName] = err.Error() continue } } - if len(bad.issues) > 0 { - return &bad - } return nil } diff --git a/interfaces/repo_test.go b/interfaces/repo_test.go index 144cce4431..94aa891212 100644 --- a/interfaces/repo_test.go +++ b/interfaces/repo_test.go @@ -1918,14 +1918,16 @@ func (s *RepositorySuite) TestSanitizeErrorsOnInvalidSlotNames(c *C) { c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) snapInfo := snaptest.MockInfo(c, testConsumerInvalidSlotNameYaml, nil) err := s.testRepo.SanitizePlugsSlots(snapInfo) - c.Assert(err, NotNil) - c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) + c.Assert(err, IsNil) + c.Assert(snapInfo.BadInterfaces, HasLen, 1) + c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) } func (s *RepositorySuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) snapInfo := snaptest.MockInfo(c, testConsumerInvalidPlugNameYaml, nil) err := s.testRepo.SanitizePlugsSlots(snapInfo) - c.Assert(err, NotNil) - c.Check(err, ErrorMatches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) + c.Assert(err, IsNil) + c.Assert(snapInfo.BadInterfaces, HasLen, 1) + c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) } diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index f9f7671d09..18f7ec7e66 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -148,12 +148,12 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, return err } if err := m.repo.AddSnap(snapInfo); err != nil { - if _, ok := err.(*interfaces.BadInterfacesError); ok { - task.Logf("%s", err) - } else { - return err - } + return err } + if len(snapInfo.BadInterfaces) > 0 { + task.Logf("%s", snapInfo.BadInterfacesInfoString()) + } + if err := m.reloadConnections(snapName); err != nil { return err } diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go index b71ecfdcaf..49ad878811 100644 --- a/overlord/snapstate/handlers.go +++ b/overlord/snapstate/handlers.go @@ -397,6 +397,7 @@ func (m *SnapManager) doMountSnap(t *state.Task, _ *tomb.Tomb) error { if err != nil { return err } + t.State().Lock() t.Set("snap-type", newInfo.Type) t.State().Unlock() diff --git a/snap/info.go b/snap/info.go index bf83713064..da3078fae2 100644 --- a/snap/info.go +++ b/snap/info.go @@ -20,6 +20,7 @@ package snap import ( + "bytes" "fmt" "io/ioutil" "os" @@ -165,6 +166,9 @@ type Info struct { Plugs map[string]*PlugInfo Slots map[string]*SlotInfo + // Plugs or slots with issues (they are not included in Plugs or Slots) + BadInterfaces map[string]string // slot or plug => message + // The information in all the remaining fields is not sourced from the snap blob itself. SideInfo @@ -331,6 +335,32 @@ func (s *Info) Services() []*AppInfo { return svcs } +func (s *Info) BadInterfacesInfoString() string { + inverted := make(map[string][]string) + for name, reason := range s.BadInterfaces { + inverted[reason] = append(inverted[reason], name) + } + var buf bytes.Buffer + fmt.Fprintf(&buf, "snap %q has bad plugs or slots: ", s.Name()) + reasons := make([]string, 0, len(inverted)) + for reason := range inverted { + reasons = append(reasons, reason) + } + sort.Strings(reasons) + for _, reason := range reasons { + names := inverted[reason] + sort.Strings(names) + for i, name := range names { + if i > 0 { + buf.WriteString(", ") + } + buf.WriteString(name) + } + fmt.Fprintf(&buf, " (%s); ", reason) + } + return strings.TrimSuffix(buf.String(), "; ") +} + // DownloadInfo contains the information to download a snap. // It can be marshalled. type DownloadInfo struct { diff --git a/snap/info_snap_yaml.go b/snap/info_snap_yaml.go index 7e53a800b2..ad6530b2c0 100644 --- a/snap/info_snap_yaml.go +++ b/snap/info_snap_yaml.go @@ -164,6 +164,8 @@ func InfoFromSnapYaml(yamlData []byte) (*Info, error) { // Rename specific plugs on the core snap. snap.renameClashingCorePlugs() + snap.BadInterfaces = make(map[string]string) + // FIXME: validation of the fields return snap, nil } From cfd3b5679cb6c4adbc28bbd9052887f1716868d0 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 3 Oct 2017 10:39:44 +0200 Subject: [PATCH 07/13] Removed commented test. --- interfaces/repo_test.go | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/interfaces/repo_test.go b/interfaces/repo_test.go index 94aa891212..adb1525ee0 100644 --- a/interfaces/repo_test.go +++ b/interfaces/repo_test.go @@ -1539,38 +1539,6 @@ func (s *AddRemoveSuite) SetUpTest(c *C) { c.Assert(err, IsNil) } -/*func (s *AddRemoveSuite) TestAddSnapComplexErrorHandling(c *C) { - err := s.repo.AddInterface(&ifacetest.TestInterface{ - InterfaceName: "invalid-plug-iface", - SanitizePlugCallback: func(plug *Plug) error { return fmt.Errorf("plug is invalid") }, - SanitizeSlotCallback: func(slot *Slot) error { return fmt.Errorf("slot is invalid") }, - }) - c.Assert(err, IsNil) - err = s.repo.AddInterface(&ifacetest.TestInterface{ - InterfaceName: "invalid-slot-iface", - SanitizePlugCallback: func(plug *Plug) error { return fmt.Errorf("plug is invalid") }, - SanitizeSlotCallback: func(slot *Slot) error { return fmt.Errorf("slot is invalid") }, - }) - c.Assert(err, IsNil) - snapInfo := snaptest.MockInfo(c, ` -name: complex -plugs: - invalid-plug-iface: - unknown-plug-iface: -slots: - invalid-slot-iface: - unknown-slot-iface: -`, nil) - err = s.repo.AddSnap(snapInfo) - c.Check(err, ErrorMatches, - `snap "complex" has bad plugs or slots: invalid-plug-iface \(plug is invalid\); invalid-slot-iface \(slot is invalid\); unknown-plug-iface, unknown-slot-iface \(unknown interface\)`) - // Nothing was added - c.Check(s.repo.Plug("complex", "invalid-plug-iface"), IsNil) - c.Check(s.repo.Plug("complex", "unknown-plug-iface"), IsNil) - c.Check(s.repo.Slot("complex", "invalid-slot-iface"), IsNil) - c.Check(s.repo.Slot("complex", "unknown-slot-iface"), IsNil) -}*/ - const testConsumerYaml = ` name: consumer apps: From ecff4ee50f20c8a37f57316aae38ed7bab4ea080 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 3 Oct 2017 10:40:10 +0200 Subject: [PATCH 08/13] Added SetSanitizePlugSlots to the repo. Call it from ifacemgr to ensure it's set only once in the real code. --- interfaces/repo.go | 10 +++++++--- overlord/ifacestate/ifacemgr.go | 2 ++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/interfaces/repo.go b/interfaces/repo.go index d4c7a64e0e..3e645fd287 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -53,12 +53,16 @@ func NewRepository() *Repository { plugSlots: make(map[*Plug]map[*Slot]*Connection), backends: make(map[SecuritySystem]SecurityBackend), } - snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { - return repo.SanitizePlugsSlots(snapInfo) - } + return repo } +func (r *Repository) SetSanitizePlugSlots() { + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { + return r.SanitizePlugsSlots(snapInfo) + } +} + // Interface returns an interface with a given name. func (r *Repository) Interface(interfaceName string) Interface { r.m.Lock() diff --git a/overlord/ifacestate/ifacemgr.go b/overlord/ifacestate/ifacemgr.go index 90f4cf2939..10411ac1f9 100644 --- a/overlord/ifacestate/ifacemgr.go +++ b/overlord/ifacestate/ifacemgr.go @@ -51,6 +51,8 @@ func Manager(s *state.State, hookManager *hookstate.HookManager, extraInterfaces runner: runner, repo: interfaces.NewRepository(), } + m.repo.SetSanitizePlugSlots() + if err := m.initialize(extraInterfaces, extraBackends); err != nil { return nil, err } From 3196ae35999ca600cc7da19dcb604786530a17fe Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 3 Oct 2017 11:32:07 +0200 Subject: [PATCH 09/13] Move snap.Validate back to repo.AddSnap. --- cmd/snap-exec/main.go | 3 ++- cmd/snap-exec/main_test.go | 2 +- cmd/snap/main.go | 3 ++- cmd/snap/main_test.go | 2 +- interfaces/repo.go | 14 +++++----- overlord/configstate/handler_test.go | 2 +- overlord/devicestate/devicestate_test.go | 2 +- overlord/hookstate/hookstate_test.go | 2 +- overlord/ifacestate/ifacestate_test.go | 2 +- overlord/snapstate/booted_test.go | 2 +- overlord/snapstate/check_snap_test.go | 2 +- overlord/snapstate/snapstate_test.go | 4 +-- snap/info.go | 8 +++--- snap/info_test.go | 33 ++++++++++++++---------- 14 files changed, 43 insertions(+), 38 deletions(-) diff --git a/cmd/snap-exec/main.go b/cmd/snap-exec/main.go index 90e7a804ed..0bb0e3d670 100644 --- a/cmd/snap-exec/main.go +++ b/cmd/snap-exec/main.go @@ -43,7 +43,8 @@ var opts struct { } func main() { - snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { return nil } + // plug/slot sanitization not used nor possible from snap-exec, make it no-op + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) {} if err := run(); err != nil { fmt.Fprintf(os.Stderr, "cannot snap-exec: %s\n", err) diff --git a/cmd/snap-exec/main_test.go b/cmd/snap-exec/main_test.go index faf8c74ccd..5038177bf8 100644 --- a/cmd/snap-exec/main_test.go +++ b/cmd/snap-exec/main_test.go @@ -50,7 +50,7 @@ func (s *snapExecSuite) SetUpTest(c *C) { opts.Command = "" opts.Hook = "" - s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) } func (s *snapExecSuite) TearDown(c *C) { diff --git a/cmd/snap/main.go b/cmd/snap/main.go index 5b1cbb4bbb..8d74786521 100644 --- a/cmd/snap/main.go +++ b/cmd/snap/main.go @@ -44,7 +44,8 @@ import ( func init() { // set User-Agent for when 'snap' talks to the store directly (snap download etc...) httputil.SetUserAgentFromVersion(cmd.Version, "snap") - snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { return nil } + // plug/slot sanitization not used nor possible from snap command, make it no-op + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) {} } // Standard streams, redirected for testing. diff --git a/cmd/snap/main_test.go b/cmd/snap/main_test.go index a0a1b96e6e..3fddd9e752 100644 --- a/cmd/snap/main_test.go +++ b/cmd/snap/main_test.go @@ -76,7 +76,7 @@ func (s *BaseSnapSuite) SetUpTest(c *C) { s.AuthFile = filepath.Join(c.MkDir(), "json") os.Setenv(TestAuthFileEnvKey, s.AuthFile) - snapdsnap.MockSanitizePlugsSlots(func(snapInfo *snapdsnap.Info) error { return nil }) + snapdsnap.MockSanitizePlugsSlots(func(snapInfo *snapdsnap.Info) {}) } func (s *BaseSnapSuite) TearDownTest(c *C) { diff --git a/interfaces/repo.go b/interfaces/repo.go index 3e645fd287..61e3c7f4bf 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -58,8 +58,8 @@ func NewRepository() *Repository { } func (r *Repository) SetSanitizePlugSlots() { - snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { - return r.SanitizePlugsSlots(snapInfo) + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) { + r.SanitizePlugsSlots(snapInfo) } } @@ -823,11 +823,6 @@ func (r *Repository) SnapSpecification(securitySystem SecuritySystem, snapName s } func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { - err := snap.Validate(snapInfo) - if err != nil { - return err - } - for plugName, plugInfo := range snapInfo.Plugs { iface, ok := r.ifaces[plugInfo.Interface] if !ok { @@ -881,6 +876,11 @@ func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { // Unknown interfaces and plugs/slots that don't validate are not added. // Information about those failures are returned to the caller. func (r *Repository) AddSnap(snapInfo *snap.Info) error { + err := snap.Validate(snapInfo) + if err != nil { + return err + } + r.m.Lock() defer r.m.Unlock() diff --git a/overlord/configstate/handler_test.go b/overlord/configstate/handler_test.go index 8f1ef13789..20f69e466d 100644 --- a/overlord/configstate/handler_test.go +++ b/overlord/configstate/handler_test.go @@ -53,7 +53,7 @@ func (s *configureHandlerSuite) SetUpTest(c *C) { s.state.Lock() defer s.state.Unlock() - s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) task := s.state.NewTask("test-task", "my test task") setup := &hookstate.HookSetup{Snap: "test-snap", Revision: snap.R(1), Hook: "test-hook"} diff --git a/overlord/devicestate/devicestate_test.go b/overlord/devicestate/devicestate_test.go index 2e226bc034..d8d670aa45 100644 --- a/overlord/devicestate/devicestate_test.go +++ b/overlord/devicestate/devicestate_test.go @@ -107,7 +107,7 @@ func (s *deviceMgrSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) os.MkdirAll(dirs.SnapRunDir, 0755) - s.restoreSanitize = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + s.restoreSanitize = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) s.bootloader = boottest.NewMockBootloader("mock", c.MkDir()) partition.ForceBootloader(s.bootloader) diff --git a/overlord/hookstate/hookstate_test.go b/overlord/hookstate/hookstate_test.go index 2833c98d63..b4be232961 100644 --- a/overlord/hookstate/hookstate_test.go +++ b/overlord/hookstate/hookstate_test.go @@ -94,7 +94,7 @@ func (s *hookManagerSuite) SetUpTest(c *C) { s.manager = manager s.o.AddManager(s.manager) - s.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil })) + s.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) hooksup := &hookstate.HookSetup{ Snap: "test-snap", diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go index 27076b1f37..2bd79880d3 100644 --- a/overlord/ifacestate/ifacestate_test.go +++ b/overlord/ifacestate/ifacestate_test.go @@ -78,7 +78,7 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { err = db.Add(s.storeSigning.StoreAccountKey("")) c.Assert(err, IsNil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) s.state.Lock() assertstate.ReplaceDB(s.state, s.db) diff --git a/overlord/snapstate/booted_test.go b/overlord/snapstate/booted_test.go index 0ed7479e2d..8ad0bfebfb 100644 --- a/overlord/snapstate/booted_test.go +++ b/overlord/snapstate/booted_test.go @@ -56,7 +56,7 @@ func (bs *bootedSuite) SetUpTest(c *C) { err := os.MkdirAll(filepath.Dir(dirs.SnapStateFile), 0755) c.Assert(err, IsNil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) bs.restore = func() { restoreSanitize() } diff --git a/overlord/snapstate/check_snap_test.go b/overlord/snapstate/check_snap_test.go index 00e2126cca..89f2293b71 100644 --- a/overlord/snapstate/check_snap_test.go +++ b/overlord/snapstate/check_snap_test.go @@ -46,7 +46,7 @@ var _ = Suite(&checkSnapSuite{}) func (s *checkSnapSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) s.st = state.New(nil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) s.restore = func() { restoreSanitize() } diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index 5b23093b95..424ee1c18b 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -83,7 +83,7 @@ func (s *snapmgrTestSuite) SetUpTest(c *C) { s.o = overlord.Mock() s.state = s.o.State() - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) s.fakeBackend = &fakeSnappyBackend{} s.fakeStore = &fakeStore{ @@ -4959,7 +4959,7 @@ func (s *snapmgrQuerySuite) SetUpTest(c *C) { st.Lock() defer st.Unlock() - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { return nil }) + restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) s.restore = func() { restoreSanitize() } diff --git a/snap/info.go b/snap/info.go index da3078fae2..ef76ab86ed 100644 --- a/snap/info.go +++ b/snap/info.go @@ -620,13 +620,13 @@ func (e NotFoundError) Error() string { return fmt.Sprintf("cannot find installed snap %q at revision %s", e.Snap, e.Revision) } -func MockSanitizePlugsSlots(f func(snapInfo *Info) error) (restore func()) { +func MockSanitizePlugsSlots(f func(snapInfo *Info)) (restore func()) { old := SanitizePlugsSlots SanitizePlugsSlots = f return func() { SanitizePlugsSlots = old } } -var SanitizePlugsSlots = func(snapInfo *Info) error { +var SanitizePlugsSlots = func(snapInfo *Info) { panic("SanitizePlugsSlots function not set") } @@ -657,9 +657,7 @@ func ReadInfo(name string, si *SideInfo) (*Info, error) { return nil, err } - if err := SanitizePlugsSlots(info); err != nil { - return nil, err - } + SanitizePlugsSlots(info) return info, nil } diff --git a/snap/info_test.go b/snap/info_test.go index 1d6155b8d3..c5b3a0c917 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -39,14 +39,30 @@ type infoSuite struct { restore func() } +type infoSimpleSuite struct{} + var _ = Suite(&infoSuite{}) +var _ = Suite(&infoSimpleSuite{}) + +func (s *infoSimpleSuite) SetUpTest(c *C) { + dirs.SetRootDir(c.MkDir()) +} + +func (s *infoSimpleSuite) TearDownTest(c *C) { + dirs.SetRootDir("") +} + +func (s *infoSimpleSuite) TestReadInfoPanicsIfSanitizeUnset(c *C) { + si := &snap.SideInfo{Revision: snap.R(1)} + snaptest.MockSnap(c, sampleYaml, sampleContents, si) + _, err := snap.ReadInfo("sample", si) + c.Assert(err, IsNil) +} func (s *infoSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) hookType := snap.NewHookType(regexp.MustCompile(".*")) - restore1 := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) error { - return nil - }) + restore1 := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) restore2 := snap.MockSupportedHookTypes([]*snap.HookType{hookType}) s.restore = func() { restore1() @@ -504,17 +520,6 @@ func (s *infoSuite) checkInstalledSnapAndSnapFile(c *C, yaml string, contents st checker(c, info) } -func (s *infoSuite) TestReadInfoErrorsOnPlugSlotValidation(c *C) { - snap.SanitizePlugsSlots = func(info *snap.Info) error { - return fmt.Errorf("ERROR") - } - si := &snap.SideInfo{Revision: snap.R(1)} - snaptest.MockSnap(c, sampleYaml, sampleContents, si) - _, err := snap.ReadInfo("sample", si) - c.Assert(err, NotNil) - c.Assert(err, ErrorMatches, "ERROR") -} - func (s *infoSuite) TestReadInfoNoHooks(c *C) { yaml := `name: foo version: 1.0` From 6e2c059626febb4e6837823faff4692893591d58 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Tue, 3 Oct 2017 12:27:50 +0200 Subject: [PATCH 10/13] Use BaseTest. Test for panic. --- overlord/ifacestate/ifacestate_test.go | 14 ++++++-------- overlord/snapstate/booted_test.go | 11 ++++++----- overlord/snapstate/check_snap_test.go | 13 ++++++------- overlord/snapstate/snapstate_test.go | 19 ++++++++----------- snap/info_test.go | 19 ++++++++----------- 5 files changed, 34 insertions(+), 42 deletions(-) diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go index 2bd79880d3..99fa37eca6 100644 --- a/overlord/ifacestate/ifacestate_test.go +++ b/overlord/ifacestate/ifacestate_test.go @@ -46,6 +46,7 @@ import ( func TestInterfaceManager(t *testing.T) { TestingT(t) } type interfaceManagerSuite struct { + testutil.BaseTest o *overlord.Overlord state *state.State db *asserts.Database @@ -54,7 +55,6 @@ type interfaceManagerSuite struct { extraIfaces []interfaces.Interface extraBackends []interfaces.SecurityBackend secBackend *ifacetest.TestSecurityBackend - restore func() mockSnapCmd *testutil.MockCmd storeSigning *assertstest.StoreStack } @@ -62,6 +62,7 @@ type interfaceManagerSuite struct { var _ = Suite(&interfaceManagerSuite{}) func (s *interfaceManagerSuite) SetUpTest(c *C) { + s.BaseTest.SetUpTest(c) s.storeSigning = assertstest.NewStoreStack("canonical", nil) s.mockSnapCmd = testutil.MockCommand(c, "snap", "") @@ -78,7 +79,7 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { err = db.Add(s.storeSigning.StoreAccountKey("")) c.Assert(err, IsNil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) + s.BaseTest.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) s.state.Lock() assertstate.ReplaceDB(s.state, s.db) @@ -92,21 +93,18 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { // TODO: transition this so that we don't load real backends and instead // just load the test backend here and this is nicely integrated with // extraBackends above. - restoreBackends := ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend}) - s.restore = func() { - restoreBackends() - restoreSanitize() - } + s.BaseTest.AddCleanup(ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend})) } func (s *interfaceManagerSuite) TearDownTest(c *C) { + s.BaseTest.TearDownTest(c) + s.mockSnapCmd.Restore() if s.privateMgr != nil { s.privateMgr.Stop() } dirs.SetRootDir("") - s.restore() } func (s *interfaceManagerSuite) manager(c *C) *ifacestate.InterfaceManager { diff --git a/overlord/snapstate/booted_test.go b/overlord/snapstate/booted_test.go index 8ad0bfebfb..9f916d71f6 100644 --- a/overlord/snapstate/booted_test.go +++ b/overlord/snapstate/booted_test.go @@ -37,9 +37,11 @@ import ( "github.com/snapcore/snapd/release" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" + "github.com/snapcore/snapd/testutil" ) type bootedSuite struct { + testutil.BaseTest bootloader *boottest.MockBootloader o *overlord.Overlord @@ -52,14 +54,13 @@ type bootedSuite struct { var _ = Suite(&bootedSuite{}) func (bs *bootedSuite) SetUpTest(c *C) { + bs.BaseTest.SetUpTest(c) + dirs.SetRootDir(c.MkDir()) err := os.MkdirAll(filepath.Dir(dirs.SnapStateFile), 0755) c.Assert(err, IsNil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) - bs.restore = func() { - restoreSanitize() - } + bs.BaseTest.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) // booted is not running on classic release.MockOnClassic(false) @@ -85,11 +86,11 @@ func (bs *bootedSuite) SetUpTest(c *C) { } func (bs *bootedSuite) TearDownTest(c *C) { + bs.BaseTest.TearDownTest(c) snapstate.AutoAliases = nil release.MockOnClassic(true) dirs.SetRootDir("") partition.ForceBootloader(nil) - bs.restore() } var osSI1 = &snap.SideInfo{RealName: "core", Revision: snap.R(1)} diff --git a/overlord/snapstate/check_snap_test.go b/overlord/snapstate/check_snap_test.go index 89f2293b71..d4db15409e 100644 --- a/overlord/snapstate/check_snap_test.go +++ b/overlord/snapstate/check_snap_test.go @@ -32,29 +32,28 @@ import ( "github.com/snapcore/snapd/release" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" + "github.com/snapcore/snapd/testutil" "github.com/snapcore/snapd/overlord/snapstate" ) type checkSnapSuite struct { - st *state.State - restore func() + testutil.BaseTest + st *state.State } var _ = Suite(&checkSnapSuite{}) func (s *checkSnapSuite) SetUpTest(c *C) { + s.BaseTest.SetUpTest(c) dirs.SetRootDir(c.MkDir()) s.st = state.New(nil) - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) - s.restore = func() { - restoreSanitize() - } + s.BaseTest.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) } func (s *checkSnapSuite) TearDownTest(c *C) { + s.BaseTest.TearDownTest(c) dirs.SetRootDir("") - s.restore() } func (s *checkSnapSuite) TestCheckSnapErrorOnUnsupportedArchitecture(c *C) { diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index 424ee1c18b..7e0e04bcd9 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -58,6 +58,7 @@ import ( func TestSnapManager(t *testing.T) { TestingT(t) } type snapmgrTestSuite struct { + testutil.BaseTest o *overlord.Overlord state *state.State snapmgr *snapstate.SnapManager @@ -66,8 +67,6 @@ type snapmgrTestSuite struct { fakeStore *fakeStore user *auth.UserState - - reset func() } func (s *snapmgrTestSuite) settle(c *C) { @@ -78,12 +77,13 @@ func (s *snapmgrTestSuite) settle(c *C) { var _ = Suite(&snapmgrTestSuite{}) func (s *snapmgrTestSuite) SetUpTest(c *C) { + s.BaseTest.SetUpTest(c) dirs.SetRootDir(c.MkDir()) s.o = overlord.Mock() s.state = s.o.State() - restoreSanitize := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) + s.BaseTest.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) s.fakeBackend = &fakeSnappyBackend{} s.fakeStore = &fakeStore{ @@ -109,19 +109,16 @@ func (s *snapmgrTestSuite) SetUpTest(c *C) { s.o.AddManager(s.snapmgr) - restore1 := snapstate.MockReadInfo(s.fakeBackend.ReadInfo) - restore2 := snapstate.MockOpenSnapFile(s.fakeBackend.OpenSnapFile) + s.BaseTest.AddCleanup(snapstate.MockReadInfo(s.fakeBackend.ReadInfo)) + s.BaseTest.AddCleanup(snapstate.MockOpenSnapFile(s.fakeBackend.OpenSnapFile)) - s.reset = func() { + s.BaseTest.AddCleanup(func() { snapstate.SetupInstallHook = oldSetupInstallHook snapstate.SetupAfterRefreshHook = oldSetupAfterRefreshHook snapstate.SetupRemoveHook = oldSetupRemoveHook - restore2() - restore1() - restoreSanitize() dirs.SetRootDir("/") - } + }) s.state.Lock() storestate.ReplaceStore(s.state, s.fakeStore) @@ -144,10 +141,10 @@ func (s *snapmgrTestSuite) SetUpTest(c *C) { } func (s *snapmgrTestSuite) TearDownTest(c *C) { + s.BaseTest.TearDownTest(c) snapstate.ValidateRefreshes = nil snapstate.AutoAliases = nil snapstate.CanAutoRefresh = nil - s.reset() } const ( diff --git a/snap/info_test.go b/snap/info_test.go index c5b3a0c917..0642a73023 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -33,10 +33,11 @@ import ( "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" "github.com/snapcore/snapd/snap/squashfs" + "github.com/snapcore/snapd/testutil" ) type infoSuite struct { - restore func() + testutil.BaseTest } type infoSimpleSuite struct{} @@ -55,24 +56,20 @@ func (s *infoSimpleSuite) TearDownTest(c *C) { func (s *infoSimpleSuite) TestReadInfoPanicsIfSanitizeUnset(c *C) { si := &snap.SideInfo{Revision: snap.R(1)} snaptest.MockSnap(c, sampleYaml, sampleContents, si) - _, err := snap.ReadInfo("sample", si) - c.Assert(err, IsNil) + c.Assert(func() { snap.ReadInfo("sample", si) }, Panics, `SanitizePlugsSlots function not set`) } func (s *infoSuite) SetUpTest(c *C) { + s.BaseTest.SetUpTest(c) dirs.SetRootDir(c.MkDir()) hookType := snap.NewHookType(regexp.MustCompile(".*")) - restore1 := snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) - restore2 := snap.MockSupportedHookTypes([]*snap.HookType{hookType}) - s.restore = func() { - restore1() - restore2() - } + s.BaseTest.AddCleanup(snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {})) + s.BaseTest.AddCleanup(snap.MockSupportedHookTypes([]*snap.HookType{hookType})) } func (s *infoSuite) TearDownTest(c *C) { + s.BaseTest.TearDownTest(c) dirs.SetRootDir("") - s.restore() } func (s *infoSuite) TestSideInfoOverrides(c *C) { @@ -554,7 +551,7 @@ version: 1.0` func (s *infoSuite) TestReadInfoInvalidImplicitHook(c *C) { hookType := snap.NewHookType(regexp.MustCompile("foo")) - s.restore = snap.MockSupportedHookTypes([]*snap.HookType{hookType}) + s.BaseTest.AddCleanup(snap.MockSupportedHookTypes([]*snap.HookType{hookType})) yaml := `name: foo version: 1.0` From efa9bf8906d958671e65de6294b6ae555da5fec3 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Mon, 9 Oct 2017 17:43:15 +0200 Subject: [PATCH 11/13] Removed SetSanitizePlugsSlots setter, sanitize against all builtin interfaces. --- interfaces/builtin/all.go | 43 ++++++++++++++++++++++++++++++ interfaces/builtin/all_test.go | 46 +++++++++++++++++++++++++++++++++ interfaces/repo.go | 46 --------------------------------- interfaces/repo_test.go | 18 ------------- overlord/ifacestate/ifacemgr.go | 1 - 5 files changed, 89 insertions(+), 65 deletions(-) diff --git a/interfaces/builtin/all.go b/interfaces/builtin/all.go index 6582df7203..9fe2c6ce1c 100644 --- a/interfaces/builtin/all.go +++ b/interfaces/builtin/all.go @@ -24,8 +24,13 @@ import ( "sort" "github.com/snapcore/snapd/interfaces" + "github.com/snapcore/snapd/snap" ) +func init() { + snap.SanitizePlugsSlots = SanitizePlugsSlots +} + var ( allInterfaces map[string]interfaces.Interface ) @@ -51,6 +56,44 @@ func registerIface(iface interfaces.Interface) { allInterfaces[iface.Name()] = iface } +func SanitizePlugsSlots(snapInfo *snap.Info) { + for plugName, plugInfo := range snapInfo.Plugs { + iface, ok := allInterfaces[plugInfo.Interface] + if !ok { + snapInfo.BadInterfaces[plugName] = "unknown interface" + continue + } + // Reject plug with invalid name + if err := interfaces.ValidateName(plugName); err != nil { + snapInfo.BadInterfaces[plugName] = err.Error() + continue + } + plug := &interfaces.Plug{PlugInfo: plugInfo} + if err := plug.Sanitize(iface); err != nil { + snapInfo.BadInterfaces[plugName] = err.Error() + continue + } + } + + for slotName, slotInfo := range snapInfo.Slots { + iface, ok := allInterfaces[slotInfo.Interface] + if !ok { + snapInfo.BadInterfaces[slotName] = "unknown interface" + continue + } + // Reject slot with invalid name + if err := interfaces.ValidateName(slotName); err != nil { + snapInfo.BadInterfaces[slotName] = err.Error() + continue + } + slot := &interfaces.Slot{SlotInfo: slotInfo} + if err := slot.Sanitize(iface); err != nil { + snapInfo.BadInterfaces[slotName] = err.Error() + continue + } + } +} + type byIfaceName []interfaces.Interface func (c byIfaceName) Len() int { return len(c) } diff --git a/interfaces/builtin/all_test.go b/interfaces/builtin/all_test.go index 36443f092a..4b665a5d8d 100644 --- a/interfaces/builtin/all_test.go +++ b/interfaces/builtin/all_test.go @@ -32,6 +32,8 @@ import ( "github.com/snapcore/snapd/interfaces/seccomp" "github.com/snapcore/snapd/interfaces/systemd" "github.com/snapcore/snapd/interfaces/udev" + "github.com/snapcore/snapd/snap" + "github.com/snapcore/snapd/snap/snaptest" . "gopkg.in/check.v1" ) @@ -307,3 +309,47 @@ func (s *AllSuite) TestRegisterIface(c *C) { // Duplicates are detected. c.Assert(func() { builtin.RegisterIface(iface) }, PanicMatches, `cannot register duplicate interface "foo"`) } + +const testConsumerInvalidSlotNameYaml = ` +name: consumer +slots: + ttyS5: + interface: iface +apps: + app: + slots: [iface] +` + +const testConsumerInvalidPlugNameYaml = ` +name: consumer +plugs: + ttyS3: + interface: iface +apps: + app: + plugs: [iface] +` + +func (s *AllSuite) TestSanitizeErrorsOnInvalidSlotNames(c *C) { + restore := builtin.MockInterfaces(map[string]interfaces.Interface{ + "iface": &ifacetest.TestInterface{InterfaceName: "iface"}, + }) + defer restore() + + snapInfo := snaptest.MockInfo(c, testConsumerInvalidSlotNameYaml, nil) + snap.SanitizePlugsSlots(snapInfo) + c.Assert(snapInfo.BadInterfaces, HasLen, 1) + c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) +} + +func (s *AllSuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { + restore := builtin.MockInterfaces(map[string]interfaces.Interface{ + "iface": &ifacetest.TestInterface{InterfaceName: "iface"}, + }) + defer restore() + + snapInfo := snaptest.MockInfo(c, testConsumerInvalidPlugNameYaml, nil) + snap.SanitizePlugsSlots(snapInfo) + c.Assert(snapInfo.BadInterfaces, HasLen, 1) + c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) +} diff --git a/interfaces/repo.go b/interfaces/repo.go index 61e3c7f4bf..024a1bb8af 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -57,12 +57,6 @@ func NewRepository() *Repository { return repo } -func (r *Repository) SetSanitizePlugSlots() { - snap.SanitizePlugsSlots = func(snapInfo *snap.Info) { - r.SanitizePlugsSlots(snapInfo) - } -} - // Interface returns an interface with a given name. func (r *Repository) Interface(interfaceName string) Interface { r.m.Lock() @@ -822,46 +816,6 @@ func (r *Repository) SnapSpecification(securitySystem SecuritySystem, snapName s return spec, nil } -func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { - for plugName, plugInfo := range snapInfo.Plugs { - iface, ok := r.ifaces[plugInfo.Interface] - if !ok { - snapInfo.BadInterfaces[plugName] = "unknown interface" - continue - } - // Reject plug with invalid name - if err := ValidateName(plugName); err != nil { - snapInfo.BadInterfaces[plugName] = err.Error() - continue - } - plug := &Plug{PlugInfo: plugInfo} - if err := plug.Sanitize(iface); err != nil { - snapInfo.BadInterfaces[plugName] = err.Error() - continue - } - } - - for slotName, slotInfo := range snapInfo.Slots { - iface, ok := r.ifaces[slotInfo.Interface] - if !ok { - snapInfo.BadInterfaces[slotName] = "unknown interface" - continue - } - // Reject slot with invalid name - if err := ValidateName(slotName); err != nil { - snapInfo.BadInterfaces[slotName] = err.Error() - continue - } - slot := &Slot{SlotInfo: slotInfo} - if err := slot.Sanitize(iface); err != nil { - snapInfo.BadInterfaces[slotName] = err.Error() - continue - } - } - - return nil -} - // AddSnap adds plugs and slots declared by the given snap to the repository. // // This function can be used to implement snap install or, when used along with diff --git a/interfaces/repo_test.go b/interfaces/repo_test.go index adb1525ee0..55317b36e2 100644 --- a/interfaces/repo_test.go +++ b/interfaces/repo_test.go @@ -1881,21 +1881,3 @@ slots: {Name: "i2", Summary: "i2 summary"}, }) } - -func (s *RepositorySuite) TestSanitizeErrorsOnInvalidSlotNames(c *C) { - c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) - snapInfo := snaptest.MockInfo(c, testConsumerInvalidSlotNameYaml, nil) - err := s.testRepo.SanitizePlugsSlots(snapInfo) - c.Assert(err, IsNil) - c.Assert(snapInfo.BadInterfaces, HasLen, 1) - c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) -} - -func (s *RepositorySuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { - c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "iface"}), IsNil) - snapInfo := snaptest.MockInfo(c, testConsumerInvalidPlugNameYaml, nil) - err := s.testRepo.SanitizePlugsSlots(snapInfo) - c.Assert(err, IsNil) - c.Assert(snapInfo.BadInterfaces, HasLen, 1) - c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) -} diff --git a/overlord/ifacestate/ifacemgr.go b/overlord/ifacestate/ifacemgr.go index 10411ac1f9..0f344d9fc4 100644 --- a/overlord/ifacestate/ifacemgr.go +++ b/overlord/ifacestate/ifacemgr.go @@ -51,7 +51,6 @@ func Manager(s *state.State, hookManager *hookstate.HookManager, extraInterfaces runner: runner, repo: interfaces.NewRepository(), } - m.repo.SetSanitizePlugSlots() if err := m.initialize(extraInterfaces, extraBackends); err != nil { return nil, err From 40cd900b97bc39fd94809cfbe8d7a248aa1e8952 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Thu, 19 Oct 2017 09:50:56 +0200 Subject: [PATCH 12/13] Review feedback: BadInterfacesSummary function, add interface name when logging bad interfaces. --- interfaces/builtin/all.go | 4 ++-- interfaces/builtin/all_test.go | 4 ++-- overlord/ifacestate/handlers.go | 2 +- snap/info.go | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/interfaces/builtin/all.go b/interfaces/builtin/all.go index 9fe2c6ce1c..3356742071 100644 --- a/interfaces/builtin/all.go +++ b/interfaces/builtin/all.go @@ -60,7 +60,7 @@ func SanitizePlugsSlots(snapInfo *snap.Info) { for plugName, plugInfo := range snapInfo.Plugs { iface, ok := allInterfaces[plugInfo.Interface] if !ok { - snapInfo.BadInterfaces[plugName] = "unknown interface" + snapInfo.BadInterfaces[plugName] = fmt.Sprintf("unknown interface %q", plugInfo.Interface) continue } // Reject plug with invalid name @@ -78,7 +78,7 @@ func SanitizePlugsSlots(snapInfo *snap.Info) { for slotName, slotInfo := range snapInfo.Slots { iface, ok := allInterfaces[slotInfo.Interface] if !ok { - snapInfo.BadInterfaces[slotName] = "unknown interface" + snapInfo.BadInterfaces[slotName] = fmt.Sprintf("unknown interface %q", slotInfo.Interface) continue } // Reject slot with invalid name diff --git a/interfaces/builtin/all_test.go b/interfaces/builtin/all_test.go index 4b665a5d8d..23a5072edb 100644 --- a/interfaces/builtin/all_test.go +++ b/interfaces/builtin/all_test.go @@ -339,7 +339,7 @@ func (s *AllSuite) TestSanitizeErrorsOnInvalidSlotNames(c *C) { snapInfo := snaptest.MockInfo(c, testConsumerInvalidSlotNameYaml, nil) snap.SanitizePlugsSlots(snapInfo) c.Assert(snapInfo.BadInterfaces, HasLen, 1) - c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) + c.Check(snap.BadInterfacesSummary(snapInfo), Matches, `snap "consumer" has bad plugs or slots: ttyS5 \(invalid interface name: "ttyS5"\)`) } func (s *AllSuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { @@ -351,5 +351,5 @@ func (s *AllSuite) TestSanitizeErrorsOnInvalidPlugNames(c *C) { snapInfo := snaptest.MockInfo(c, testConsumerInvalidPlugNameYaml, nil) snap.SanitizePlugsSlots(snapInfo) c.Assert(snapInfo.BadInterfaces, HasLen, 1) - c.Check(snapInfo.BadInterfacesInfoString(), Matches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) + c.Check(snap.BadInterfacesSummary(snapInfo), Matches, `snap "consumer" has bad plugs or slots: ttyS3 \(invalid interface name: "ttyS3"\)`) } diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index 18f7ec7e66..eecf03119c 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -151,7 +151,7 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, return err } if len(snapInfo.BadInterfaces) > 0 { - task.Logf("%s", snapInfo.BadInterfacesInfoString()) + task.Logf("%s", snap.BadInterfacesSummary(snapInfo)) } if err := m.reloadConnections(snapName); err != nil { diff --git a/snap/info.go b/snap/info.go index ef1bd2d9f7..f8ba57f819 100644 --- a/snap/info.go +++ b/snap/info.go @@ -327,13 +327,13 @@ func (s *Info) Services() []*AppInfo { return svcs } -func (s *Info) BadInterfacesInfoString() string { +func BadInterfacesSummary(snapInfo *Info) string { inverted := make(map[string][]string) - for name, reason := range s.BadInterfaces { + for name, reason := range snapInfo.BadInterfaces { inverted[reason] = append(inverted[reason], name) } var buf bytes.Buffer - fmt.Fprintf(&buf, "snap %q has bad plugs or slots: ", s.Name()) + fmt.Fprintf(&buf, "snap %q has bad plugs or slots: ", snapInfo.Name()) reasons := make([]string, 0, len(inverted)) for reason := range inverted { reasons = append(reasons, reason) From 85124e746b22ffd3c93151ff9ac52a836d181ad4 Mon Sep 17 00:00:00 2001 From: Pawel Stolowski Date: Thu, 19 Oct 2017 09:57:50 +0200 Subject: [PATCH 13/13] Review feedback: set SanitizePlugsAndSlots function in the init() of snap-exec. --- cmd/snap-exec/main.go | 4 +++- cmd/snap-exec/main_test.go | 6 +----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/cmd/snap-exec/main.go b/cmd/snap-exec/main.go index dfa1caa036..ee05a58866 100644 --- a/cmd/snap-exec/main.go +++ b/cmd/snap-exec/main.go @@ -42,10 +42,12 @@ var opts struct { Hook string `long:"hook" description:"hook to run" hidden:"yes"` } -func main() { +func init() { // plug/slot sanitization not used nor possible from snap-exec, make it no-op snap.SanitizePlugsSlots = func(snapInfo *snap.Info) {} +} +func main() { if err := run(); err != nil { fmt.Fprintf(os.Stderr, "cannot snap-exec: %s\n", err) os.Exit(1) diff --git a/cmd/snap-exec/main_test.go b/cmd/snap-exec/main_test.go index 9d450c16f2..ad75ddf5a5 100644 --- a/cmd/snap-exec/main_test.go +++ b/cmd/snap-exec/main_test.go @@ -40,9 +40,7 @@ import ( // Hook up check.v1 into the "go test" runner func Test(t *testing.T) { TestingT(t) } -type snapExecSuite struct { - restore func() -} +type snapExecSuite struct{} var _ = Suite(&snapExecSuite{}) @@ -50,12 +48,10 @@ func (s *snapExecSuite) SetUpTest(c *C) { // clean previous parse runs snapExec.SetOptsCommand("") snapExec.SetOptsHook("") - s.restore = snap.MockSanitizePlugsSlots(func(snapInfo *snap.Info) {}) } func (s *snapExecSuite) TearDown(c *C) { dirs.SetRootDir("/") - s.restore() } var mockYaml = []byte(`name: snapname