overlord/ifacestate: fix auto-connect during core snap transition

interfaces/repo.go

@@ -537,17 +537,24 @@ func (r *Repository) connected(snapName, plugOrSlotName string) ([]ConnRef, erro
 	if r.plugs[snapName][plugOrSlotName] == nil && r.slots[snapName][plugOrSlotName] == nil {
 		return nil, fmt.Errorf("snap %q has no plug or slot named %q", snapName, plugOrSlotName)
 	}
-	// Collect all the relevant connections
+	// Collect all the relevant connections but be on the lookout for duplicates.
+	seen := make(map[ConnRef]bool)
 	if plug, ok := r.plugs[snapName][plugOrSlotName]; ok {
 		for _, slotRef := range plug.Connections {
 			connRef := ConnRef{PlugRef: plug.Ref(), SlotRef: slotRef}
-			conns = append(conns, connRef)
+			if !seen[connRef] {
+				conns = append(conns, connRef)
+				seen[connRef] = true
+			}
 		}
 	}
 	if slot, ok := r.slots[snapName][plugOrSlotName]; ok {
 		for _, plugRef := range slot.Connections {
 			connRef := ConnRef{PlugRef: plugRef, SlotRef: slot.Ref()}
-			conns = append(conns, connRef)
+			if !seen[connRef] {
+				conns = append(conns, connRef)
+				seen[connRef] = true
+			}
 		}
 	}
 	return conns, nil

interfaces/repo_test.go

@@ -1208,6 +1208,29 @@ func (s *RepositorySuite) TestConnectedFindsCoreSnap(c *C) {
 	})
 }
 
+// Connected de-duplicates results
+func (s *RepositorySuite) TestConnectedDeduplicates(c *C) {
+	coreSnapInfo := &snap.Info{SuggestedName: "core", Type: snap.TypeOS}
+	plug := &Plug{PlugInfo: &snap.PlugInfo{
+		Snap:      coreSnapInfo,
+		Name:      "network-bind",
+		Interface: "network-bind",
+	}}
+	slot := &Slot{SlotInfo: &snap.SlotInfo{
+		Snap:      coreSnapInfo,
+		Name:      "network-bind",
+		Interface: "network-bind",
+	}}
+	c.Assert(s.testRepo.AddInterface(&ifacetest.TestInterface{InterfaceName: "network-bind"}), IsNil)
+	c.Assert(s.testRepo.AddPlug(plug), IsNil)
+	c.Assert(s.testRepo.AddSlot(slot), IsNil)
+	c.Assert(s.testRepo.Connect(ConnRef{PlugRef: plug.Ref(), SlotRef: slot.Ref()}), IsNil)
+
+	conns, err := s.testRepo.Connected("core", "network-bind")
+	c.Assert(err, IsNil)
+	c.Check(conns, DeepEquals, []ConnRef{{plug.Ref(), slot.Ref()}})
+}
+
 // Tests for Repository.DisconnectAll()
 
 func (s *RepositorySuite) TestDisconnectAll(c *C) {

overlord/ifacestate/helpers.go

@@ -50,6 +50,9 @@ func (m *InterfaceManager) initialize(extraInterfaces []interfaces.Interface, ex
 	if err := m.reloadConnections(""); err != nil {
 		return err
 	}
+	if err := m.fixDisconnectedCorePlugs(); err != nil {
+		return err
+	}
 	if err := m.regenerateAllSecurityProfiles(); err != nil {
 		return err
 	}
@@ -98,6 +101,28 @@ func (m *InterfaceManager) addSnaps() error {
 	return nil
 }
 
+// fixDisconnectedCorePlugs will re-connect the network-bind-plug and
+// core-support-plug to the corresponding slots on the core snap.
+// This cures the effects of bug https://bugs.launchpad.net/snappy/+bug/1680097
+func (m *InterfaceManager) fixDisconnectedCorePlugs() error {
+	const coreName = "core"
+	for _, slotName := range []string{"network-bind", "core-support"} {
+		plugName := fmt.Sprintf("%s-plug", slotName)
+		// If the core snap has the plug
+		if m.repo.Plug(coreName, plugName) != nil {
+			// connect it to the slot (connect is a no-op if connected)
+			connRef := interfaces.ConnRef{
+				PlugRef: interfaces.PlugRef{Snap: coreName, Name: plugName},
+				SlotRef: interfaces.SlotRef{Snap: coreName, Name: slotName},
+			}
+			if err := m.repo.Connect(connRef); err != nil {
+				logger.Noticef("%s", err)
+			}
+		}
+	}
+	return nil
+}
+
 // regenerateAllSecurityProfiles will regenerate the security profiles
 // for apparmor and seccomp. This is needed because:
 // - for seccomp we may have "terms" on disk that the current snap-confine
@@ -300,6 +325,18 @@ func (m *InterfaceManager) autoConnect(task *state.Task, snapName string, blackl
 			continue
 		}
 		candidates := m.repo.AutoConnectCandidateSlots(snapName, plug.Name, autochecker.check)
+		// If we are in a core transition we may have both the old ubuntu-core
+		// snap and the new core snap providing the same interface. In that
+		// situation we want to ignore any candidates in ubuntu-core and simply
+		// go with those from the new core snap.
+		if len(candidates) == 2 {
+			switch {
+			case candidates[0].Snap.Name() == "ubuntu-core" && candidates[1].Snap.Name() == "core":
+				candidates = candidates[1:2]
+			case candidates[1].Snap.Name() == "ubuntu-core" && candidates[0].Snap.Name() == "core":
+				candidates = candidates[0:1]
+			}
+		}
 		if len(candidates) != 1 {
 			continue
 		}

overlord/ifacestate/ifacestate_test.go

@@ -667,12 +667,18 @@ func (s *interfaceManagerSuite) addDiscardConnsChange(c *C, snapName string) *st
 	return change
 }
 
-var osSnapYaml = `
+var ubuntuCoreSnapYaml = `
 name: ubuntu-core
 version: 1
 type: os
 `
 
+var coreSnapYaml = `
+name: core
+version: 1
+type: os
+`
+
 var sampleSnapYaml = `
 name: snap
 version: 1
@@ -704,13 +710,20 @@ slots:
   attr2: value2
 `
 
+var httpdSnapYaml = `name: httpd
+version: 1
+plugs:
+ network:
+  interface: network
+`
+
 // The setup-profiles task will not auto-connect an plug that was previously
 // explicitly disconnected by the user.
 func (s *interfaceManagerSuite) TestDoSetupSnapSecurityHonorsDisconnect(c *C) {
 	c.Skip("feature disabled until redesign/reimpl")
 	// Add an OS snap as well as a sample snap with a "network" plug.
 	// The plug is normally auto-connected.
-	s.mockSnap(c, osSnapYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
 	snapInfo := s.mockSnap(c, sampleSnapYaml)
 
 	// Initialize the manager. This registers the two snaps.
@@ -749,7 +762,7 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityHonorsDisconnect(c *C) {
 // The setup-profiles task will auto-connect plugs with viable candidates.
 func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsPlugs(c *C) {
 	// Add an OS snap.
-	s.mockSnap(c, osSnapYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
 
 	// Initialize the manager. This registers the OS snap.
 	mgr := s.manager(c)
@@ -796,7 +809,7 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityAutoConnectsSlots(c *C) {
 	// Mock the interface that will be used by the test
 	s.mockIface(c, &ifacetest.TestInterface{InterfaceName: "test"})
 	// Add an OS snap.
-	s.mockSnap(c, osSnapYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
 	// Add a consumer snap with unconnect plug (interface "test")
 	s.mockSnap(c, consumerYaml)
 
@@ -920,7 +933,7 @@ slots:
 // operates on or auto-connects to and will leave other state intact.
 func (s *interfaceManagerSuite) TestDoSetupSnapSecuirtyKeepsExistingConnectionState(c *C) {
 	// Add an OS snap in place.
-	s.mockSnap(c, osSnapYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
 
 	// Initialize the manager. This registers the two snaps.
 	mgr := s.manager(c)
@@ -976,7 +989,7 @@ func (s *interfaceManagerSuite) TestDoSetupProfilesAddsImplicitSlots(c *C) {
 	mgr := s.manager(c)
 
 	// Add an OS snap.
-	snapInfo := s.mockSnap(c, osSnapYaml)
+	snapInfo := s.mockSnap(c, ubuntuCoreSnapYaml)
 
 	// Run the setup-profiles task and let it finish.
 	change := s.addSetupSnapSecurityChange(c, &snapstate.SnapSetup{
@@ -1096,7 +1109,7 @@ func (s *interfaceManagerSuite) TestSetupProfilesHonorsDevMode(c *C) {
 // of the affected set.
 func (s *interfaceManagerSuite) TestSetupProfilesUsesFreshSnapInfo(c *C) {
 	// Put the OS and the sample snaps in place.
-	coreSnapInfo := s.mockSnap(c, osSnapYaml)
+	coreSnapInfo := s.mockSnap(c, ubuntuCoreSnapYaml)
 	oldSnapInfo := s.mockSnap(c, sampleSnapYaml)
 
 	// Put connection information between the OS snap and the sample snap.
@@ -1151,7 +1164,7 @@ func (s *interfaceManagerSuite) TestSetupProfilesUsesFreshSnapInfo(c *C) {
 // setup-profiles needs to setup security for connected slots after autoconnection
 func (s *interfaceManagerSuite) TestAutoConnectSetupSecurityForConnectedSlots(c *C) {
 	// Add an OS snap.
-	coreSnapInfo := s.mockSnap(c, osSnapYaml)
+	coreSnapInfo := s.mockSnap(c, ubuntuCoreSnapYaml)
 
 	// Initialize the manager. This registers the OS snap.
 	mgr := s.manager(c)
@@ -1646,7 +1659,7 @@ slots:
 }
 
 func (s *interfaceManagerSuite) TestCheckInterfacesConsidersImplicitSlots(c *C) {
-	snapInfo := s.mockSnap(c, osSnapYaml)
+	snapInfo := s.mockSnap(c, ubuntuCoreSnapYaml)
 
 	s.state.Lock()
 	defer s.state.Unlock()
@@ -1740,26 +1753,9 @@ func (s *interfaceManagerSuite) TestUndoSetupProfilesOnRefresh(c *C) {
 	c.Check(s.secBackend.SetupCalls[0].Options, Equals, interfaces.ConfinementOptions{})
 }
 
-var ubuntuCoreYaml = `name: ubuntu-core
-version: 1
-type: os
-`
-
-var coreYaml = `name: ubuntu-core
-version: 1
-type: os
-`
-
-var httpdSnapYaml = `name: httpd
-version: 1
-plugs:
- network:
-  interface: network
-`
-
 func (s *interfaceManagerSuite) TestManagerTransitionConnectionsCore(c *C) {
-	s.mockSnap(c, ubuntuCoreYaml)
-	s.mockSnap(c, coreYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
+	s.mockSnap(c, coreSnapYaml)
 	s.mockSnap(c, httpdSnapYaml)
 
 	mgr := s.manager(c)
@@ -1797,8 +1793,8 @@ func (s *interfaceManagerSuite) TestManagerTransitionConnectionsCore(c *C) {
 }
 
 func (s *interfaceManagerSuite) TestManagerTransitionConnectionsCoreUndo(c *C) {
-	s.mockSnap(c, ubuntuCoreYaml)
-	s.mockSnap(c, coreYaml)
+	s.mockSnap(c, ubuntuCoreSnapYaml)
+	s.mockSnap(c, coreSnapYaml)
 	s.mockSnap(c, httpdSnapYaml)
 
 	mgr := s.manager(c)
@@ -1841,3 +1837,82 @@ func (s *interfaceManagerSuite) TestManagerTransitionConnectionsCoreUndo(c *C) {
 		},
 	})
 }
+
+func (s *interfaceManagerSuite) TestAutoConnectDuringCoreTransition(c *C) {
+	// Add both the old and new core snaps
+	s.mockSnap(c, ubuntuCoreSnapYaml)
+	s.mockSnap(c, coreSnapYaml)
+
+	// Initialize the manager. This registers both of the core snaps.
+	mgr := s.manager(c)
+
+	// Add a sample snap with a "network" plug which should be auto-connected.
+	// Normally it would not be auto connected because there are multiple
+	// provides but we have special support for this case so the old
+	// ubuntu-core snap is ignored and we pick the new core snap.
+	snapInfo := s.mockSnap(c, sampleSnapYaml)
+
+	// Run the setup-snap-security task and let it finish.
+	change := s.addSetupSnapSecurityChange(c, &snapstate.SnapSetup{
+		SideInfo: &snap.SideInfo{
+			RealName: snapInfo.Name(),
+			Revision: snapInfo.Revision,
+		},
+	})
+	mgr.Ensure()
+	mgr.Wait()
+	mgr.Stop()
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	// Ensure that the task succeeded.
+	c.Assert(change.Status(), Equals, state.DoneStatus)
+
+	// Ensure that "network" is now saved in the state as auto-connected and
+	// that it is connected to the new core snap rather than the old
+	// ubuntu-core snap.
+	var conns map[string]interface{}
+	err := s.state.Get("conns", &conns)
+	c.Assert(err, IsNil)
+	c.Check(conns, DeepEquals, map[string]interface{}{
+		"snap:network core:network": map[string]interface{}{
+			"interface": "network", "auto": true,
+		},
+	})
+
+	// Ensure that "network" is really connected.
+	repo := mgr.Repository()
+	plug := repo.Plug("snap", "network")
+	c.Assert(plug, Not(IsNil))
+	c.Check(plug.Connections, HasLen, 1)
+}
+
+func (s *interfaceManagerSuite) TestManagerFixesNetworkBindConnectionOnInit(c *C) {
+	s.mockSnap(c, coreSnapYaml+`
+plugs:
+  network-bind-plug:
+    interface: network-bind
+  core-support-plug:
+    interface: core-support
+slots:
+  network-bind:
+  core-support:
+`)
+	mgr := s.manager(c)
+	// Check that network-bind and core-support are connected to their -plugs
+	connRefs, err := mgr.Repository().Connected("core", "network-bind")
+	c.Assert(err, IsNil)
+	c.Assert(connRefs, HasLen, 1)
+	c.Assert(connRefs, DeepEquals, []interfaces.ConnRef{{
+		PlugRef: interfaces.PlugRef{Snap: "core", Name: "network-bind-plug"},
+		SlotRef: interfaces.SlotRef{Snap: "core", Name: "network-bind"},
+	}})
+	connRefs, err = mgr.Repository().Connected("core", "core-support")
+	c.Assert(err, IsNil)
+	c.Assert(connRefs, HasLen, 1)
+	c.Assert(connRefs, DeepEquals, []interfaces.ConnRef{{
+		PlugRef: interfaces.PlugRef{Snap: "core", Name: "core-support-plug"},
+		SlotRef: interfaces.SlotRef{Snap: "core", Name: "core-support"},
+	}})
+}

tests/main/classic-ubuntu-core-transition/task.yaml

@@ -43,7 +43,8 @@ execute: |
 
     snap install --${CORE_CHANNEL} ubuntu-core
     snap install test-snapd-python-webserver
-    snap interfaces |MATCH ":network.*test-snapd-python-webserver"
+    snap interfaces | MATCH ":network +test-snapd-python-webserver"
+    snap interfaces | MATCH ":network-bind +test-snapd-python-webserver"
 
     echo "Ensure the webserver is working"
     wait_for_service snap.test-snapd-python-webserver.test-snapd-python-webserver
@@ -69,7 +70,8 @@ execute: |
         echo "ubuntu-core still installed, transition failed"
         exit 1
     fi
-    snap interfaces |MATCH ":network.*test-snapd-python-webserver"
+    snap interfaces | MATCH ":network +test-snapd-python-webserver"
+    snap interfaces | MATCH ":network-bind +core,test-snapd-python-webserver"
     echo "Ensure the webserver is still working"
     wait_for_service snap.test-snapd-python-webserver.test-snapd-python-webserver
     curl http://localhost | MATCH "XKCD rocks"