Merge pull request #9168 from jhenstridge/snapctl-is-connected-bad-pl…

…ug-or-slot

o/hookstate/ctlcmd: make is-connected check whether the plug or slot exists

overlord/hookstate/ctlcmd/is_connected.go

@@ -25,6 +25,7 @@ import (
 	"github.com/snapcore/snapd/i18n"
 	"github.com/snapcore/snapd/interfaces"
 	"github.com/snapcore/snapd/overlord/ifacestate"
+	"github.com/snapcore/snapd/overlord/snapstate"
 )
 
 type isConnectedCommand struct {
@@ -68,14 +69,24 @@ func (c *isConnectedCommand) Execute(args []string) error {
 	st.Lock()
 	defer st.Unlock()
 
+	info, err := snapstate.CurrentInfo(st, snapName)
+	if err != nil {
+		return fmt.Errorf("internal error: cannot get snap info: %s", err)
+	}
+
+	// XXX: This will fail for implicit slots.  In practice, this
+	// would only affect calls that used the "core" snap as
+	// context.  That snap does not have any hooks using
+	// is-connected, so the limitation is probably moot.
+	if info.Plugs[plugOrSlot] == nil && info.Slots[plugOrSlot] == nil {
+		return fmt.Errorf("snap %q has no plug or slot named %q", snapName, plugOrSlot)
+	}
+
 	conns, err := ifacestate.ConnectionStates(st)
 	if err != nil {
 		return fmt.Errorf("internal error: cannot get connections: %s", err)
 	}
 
-	// XXX should we check if plug/slot exists? We don't differentiate between
-	// non-connected/not-existing at the moment.
-
 	// snapName is the name of the snap executing snapctl command, it's
 	// obtained from the context (ephemeral if run by apps, or full if run by
 	// hooks). plug and slot names are unique within a snap, so there is no

overlord/hookstate/ctlcmd/is_connected_test.go

@@ -20,23 +20,31 @@
 package ctlcmd_test
 
 import (
+	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/overlord/hookstate"
 	"github.com/snapcore/snapd/overlord/hookstate/ctlcmd"
 	"github.com/snapcore/snapd/overlord/hookstate/hooktest"
+	"github.com/snapcore/snapd/overlord/snapstate"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/snap/snaptest"
+	"github.com/snapcore/snapd/testutil"
 
 	. "gopkg.in/check.v1"
 )
 
 type isConnectedSuite struct {
+	testutil.BaseTest
 	st          *state.State
 	mockHandler *hooktest.MockHandler
 }
 
 var _ = Suite(&isConnectedSuite{})
 
 func (s *isConnectedSuite) SetUpTest(c *C) {
+	s.BaseTest.SetUpTest(c)
+	dirs.SetRootDir(c.MkDir())
+	s.AddCleanup(func() { dirs.SetRootDir("/") })
 	s.st = state.New(nil)
 	s.mockHandler = hooktest.NewMockHandler()
 }
@@ -61,14 +69,41 @@ var isConnectedTests = []struct {
 	args:     []string{"is-connected", "plug3"},
 	exitCode: 1,
 }, {
-	args:     []string{"is-connected", "slot2"},
-	exitCode: 1,
+	args: []string{"is-connected", "slot2"},
+	err:  `snap "snap1" has no plug or slot named "slot2"`,
 }, {
-	args:     []string{"is-connected", "foo"},
-	exitCode: 1,
+	args: []string{"is-connected", "foo"},
+	err:  `snap "snap1" has no plug or slot named "foo"`,
 }}
 
+func mockInstalledSnap(c *C, st *state.State, snapYaml string) {
+	info := snaptest.MockSnapCurrent(c, snapYaml, &snap.SideInfo{Revision: snap.R(1)})
+	snapstate.Set(st, info.InstanceName(), &snapstate.SnapState{
+		Active: true,
+		Sequence: []*snap.SideInfo{
+			{
+				RealName: info.SnapName(),
+				Revision: info.Revision,
+				SnapID:   info.InstanceName() + "-id",
+			},
+		},
+		Current: info.Revision,
+	})
+}
+
 func (s *isConnectedSuite) testIsConnected(c *C, context *hookstate.Context) {
+	mockInstalledSnap(c, s.st, `name: snap1
+plugs:
+  plug1:
+    interface: x11
+  plug2:
+    interface: x11
+  plug3:
+    interface: x11
+slots:
+  slot1:
+    interface: x11`)
+
 	s.st.Set("conns", map[string]interface{}{
 		"snap1:plug1 snap2:slot2": map[string]interface{}{},
 		"snap1:plug2 snap3:slot3": map[string]interface{}{"undesired": true},
@@ -81,20 +116,19 @@ func (s *isConnectedSuite) testIsConnected(c *C, context *hookstate.Context) {
 
 	for _, test := range isConnectedTests {
 		stdout, stderr, err := ctlcmd.Run(context, test.args, 0)
+		comment := Commentf("%s", test.args)
 		if test.exitCode > 0 {
-			unsuccessfulErr, ok := err.(*ctlcmd.UnsuccessfulError)
-			c.Assert(ok, Equals, true)
-			c.Check(unsuccessfulErr.ExitCode, Equals, test.exitCode)
+			c.Check(err, DeepEquals, &ctlcmd.UnsuccessfulError{ExitCode: test.exitCode}, comment)
 		} else {
 			if test.err == "" {
-				c.Check(err, IsNil)
+				c.Check(err, IsNil, comment)
 			} else {
-				c.Check(err, ErrorMatches, test.err)
+				c.Check(err, ErrorMatches, test.err, comment)
 			}
 		}
 
-		c.Check(string(stdout), Equals, test.stdout, Commentf("%s\n", test.args))
-		c.Check(string(stderr), Equals, "")
+		c.Check(string(stdout), Equals, test.stdout, comment)
+		c.Check(string(stderr), Equals, "", comment)
 	}
 }
 
@@ -136,6 +170,11 @@ func (s *isConnectedSuite) TestNoContextError(c *C) {
 func (s *isConnectedSuite) TestGetRegularUser(c *C) {
 	s.st.Lock()
 
+	mockInstalledSnap(c, s.st, `name: snap1
+plugs:
+  plug1:
+    interface: x11`)
+
 	s.st.Set("conns", map[string]interface{}{
 		"snap1:plug1 snap2:slot2": map[string]interface{}{},
 	})