interfaces: sanitize plugs and slots early in ReadInfo

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)

cmd/snap-exec/main_test.go

@@ -39,19 +39,24 @@ 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{})
 
 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

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.

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) {

interfaces/repo.go

@@ -20,7 +20,6 @@
 package interfaces
 
 import (
-	"bytes"
 	"fmt"
 	"sort"
 	"strings"
@@ -46,14 +45,18 @@ type Repository struct {
 
 // 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),
 		slotPlugs: make(map[*Slot]map[*Plug]*Connection),
 		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.
@@ -815,37 +818,49 @@ 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 (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error {
+	err := snap.Validate(snapInfo)
+	if err != nil {
+		return err
+	}
 
-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)
+	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
 		}
-		fmt.Fprintf(&buf, " (%s); ", reason)
 	}
-	return strings.TrimSuffix(buf.String(), "; ")
+
+	return nil
 }
 
 // AddSnap adds plugs and slots declared by the given snap to the repository.
@@ -862,11 +877,6 @@ func (e *BadInterfacesError) Error() string {
 // 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()
 
@@ -876,58 +886,21 @@ 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 {
-		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
 	}
-
-	if len(bad.issues) > 0 {
-		return &bad
-	}
 	return nil
 }
 

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,21 @@ 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"\)`)
+}

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()

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

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",

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
 	}