daemon, o/i/apparmorprompting: implement ShutDown for InterfacesRequestsManager

daemon/api_prompting.go

@@ -429,7 +429,7 @@ func postInterfacesRequests(c *Command, r *http.Request, user *auth.UserState) R
 		return errorResp
 	}
 
-	outcome, err := getInterfaceManager(c).InterfacesRequestsManager().Ask(reqUID, postBody.Interface, snapName, postBody.PID, cgroupPath, c.d.tomb.Dying())
+	outcome, err := getInterfaceManager(c).InterfacesRequestsManager().Ask(reqUID, postBody.Interface, snapName, postBody.PID, cgroupPath)
 	if err != nil {
 		return promptingError(err)
 	}

daemon/api_prompting_test.go

@@ -58,7 +58,6 @@ type fakeInterfacesRequestsManager struct {
 	iface                string
 	pid                  int32
 	cgroup               string
-	snapdShuttingDown    <-chan struct{}
 	id                   prompting.IDType // used for prompt ID or rule ID
 	ruleConstraintsJSON  prompting.ConstraintsJSON
 	constraintsPatchJSON prompting.ConstraintsJSON
@@ -69,13 +68,12 @@ type fakeInterfacesRequestsManager struct {
 	clientActivity       bool
 }
 
-func (m *fakeInterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error) {
+func (m *fakeInterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error) {
 	m.userID = uid
 	m.iface = iface
 	m.snap = snap
 	m.pid = pid
 	m.cgroup = cgroup
-	m.snapdShuttingDown = snapdShuttingDown
 	return m.ask, m.err
 }
 
@@ -733,7 +731,6 @@ func (s *promptingSuite) TestPostInterfacesRequestsHappy(c *C) {
 	c.Check(s.manager.snap, Equals, expectedSnap)
 	c.Check(s.manager.pid, Equals, fakePID)
 	c.Check(s.manager.cgroup, Equals, fakeCgroup)
-	c.Check(s.manager.snapdShuttingDown, NotNil)
 
 	// Check return value
 	responseBody, ok := rsp.Result.(daemon.PostInterfacesRequestsResponse)

overlord/ifacestate/apparmorprompting/prompting.go

@@ -52,7 +52,7 @@ type listenerBackend interface {
 // A Manager holds outstanding prompts and mediates their replies, further it
 // stores and applies persistent rules.
 type Manager interface {
-	Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error)
+	Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error)
 	Prompts(userID uint32, clientActivity bool) ([]*requestprompts.Prompt, error)
 	PromptWithID(userID uint32, promptID prompting.IDType, clientActivity bool) (*requestprompts.Prompt, error)
 	HandleReply(userID uint32, promptID prompting.IDType, replyConstraintsJSON prompting.ConstraintsJSON, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string, clientActivity bool) ([]prompting.IDType, error)
@@ -85,6 +85,13 @@ type InterfacesRequestsManager struct {
 	// listener readiness.
 	listenerAlreadySignalled chan struct{}
 
+	// snapdShuttingDown is closed when overlord.ShutDown is called to
+	// signal that the daemon is going to be stopped and the
+	// InterfacesRequestsManager needs to stop receiving requests and
+	// finish handling existing requests.
+	snapdShuttingDown chan struct{}
+	shutDownOnce      sync.Once
+
 	askRequests chan *prompting.Request
 }
 
@@ -140,6 +147,7 @@ func New(noticeMgr *notices.NoticeManager) (m *InterfacesRequestsManager, retErr
 		prompts:                  promptsBackend,
 		rules:                    rulesBackend,
 		listenerAlreadySignalled: make(chan struct{}),
+		snapdShuttingDown:        make(chan struct{}),
 		askRequests:              make(chan *prompting.Request),
 	}
 
@@ -328,21 +336,18 @@ func (m *InterfacesRequestsManager) disconnect() error {
 //
 // If the given channel closes or the prompting subsystem is shutting down,
 // then returns [prompting_errors.ErrPromptingClosed].
-// TODO: replace this ad-hoc channel with a proper shutdown handler in the
-// manager itself, so this method will observe the shutdown from within the
-// manager and act accordingly.
 //
 // The given interface must be one for which we expect requests to be created
 // directly, rather than via AppArmor. The requested permissions will include
 // all available permissions for the given interface.
-func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error) {
+func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error) {
 	replyChan := make(chan []string)
 
 	reply := func(allowedPerms []string) error {
 		select {
 		case replyChan <- allowedPerms:
 			return nil
-		case <-snapdShuttingDown:
+		case <-m.snapdShuttingDown:
 			return prompting_errors.ErrPromptingClosed
 		case <-m.tomb.Dying():
 			return prompting_errors.ErrPromptingClosed
@@ -360,7 +365,7 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
 	select {
 	case m.askRequests <- req:
 		// request received and being processed
-	case <-snapdShuttingDown:
+	case <-m.snapdShuttingDown:
 		return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
 	case <-m.tomb.Dying():
 		return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
@@ -371,7 +376,7 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
 	select {
 	case allowedPermissions = <-replyChan:
 		// received reply
-	case <-snapdShuttingDown:
+	case <-m.snapdShuttingDown:
 		return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
 	case <-m.tomb.Dying():
 		return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
@@ -386,8 +391,20 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
 	return prompting.OutcomeAllow, nil
 }
 
+// ShutDown signals the manager to reject new and pending Ask() calls. It is
+// used to phase the closing process for the InterfacesRequestsManager. It
+// is not guaranteed to be called before Stop. ShutDown is idempotent.
+func (m *InterfacesRequestsManager) ShutDown() {
+	m.shutDownOnce.Do(func() {
+		close(m.snapdShuttingDown)
+	})
+
+}
+
 // Stop closes the listener, prompt DB, and rule DB. Stop is idempotent, and
-// the receiver cannot be started or used after it has been stopped.
+// the receiver cannot be started or used after it has been stopped. Stop will
+// successfully disconnects the InterfacesRequestsManager even when called
+// without first calling ShutDown.
 func (m *InterfacesRequestsManager) Stop() error {
 	m.tomb.Kill(nil)
 	// Kill causes the run loop to exit and call disconnect()

overlord/ifacestate/apparmorprompting/prompting_test.go

@@ -652,8 +652,7 @@ func (s *apparmorpromptingSuite) testAskWithOutcome(c *C, outcome prompting.Outc
 	outcomeChan := make(chan prompting.OutcomeType)
 	errChan := make(chan error)
 	go func() {
-		snapdShuttingDown := make(chan struct{})
-		out, err := mgr.Ask(uid, iface, snap, pid, cgroup, snapdShuttingDown)
+		out, err := mgr.Ask(uid, iface, snap, pid, cgroup)
 		logger.WithLoggerLock(func() {
 			c.Check(err, IsNil, Commentf(logbuf.String()))
 		})
@@ -741,26 +740,17 @@ func (s *apparmorpromptingSuite) TestAskShutdownBeforeSending(c *C) {
 		iface  = "audio-record"
 	)
 
-	// Stop the manager now so that it will not receive the request.
-	//
-	// Unfortunately, there's not a way to test the snapdShuttingDown channel
-	// closing as well, since if the manager has not stopped, there is a race
-	// where the run loop may receive the request. So we close the listener to
-	// ensure the run loop does not receive the request.
-	//
-	// XXX: in the future, when we remove the snapdShuttingDown channel in
-	// favor of a manager-level shutdown triggered by the daemon stopping, most
-	// of this comment can be removed.
-	c.Check(mgr.Stop(), IsNil)
+	// Shut down the manager now so that it will not receive the request.
+	mgr.ShutDown()
 
-	timeoutChan := make(chan struct{})
-	time.AfterFunc(time.Second, func() { close(timeoutChan) })
-	outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, timeoutChan)
+	outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
 	c.Check(outcome, Equals, prompting.OutcomeUnset)
 	c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
+
+	c.Check(mgr.Stop(), IsNil)
 }
 
-func (s *apparmorpromptingSuite) TestAskShutdownBeforeReply(c *C) {
+func (s *apparmorpromptingSuite) TestAskShutdownBeforeReplyWithStop(c *C) {
 	proceedWithClose, _, _, restore := apparmorprompting.MockListenerWithDelayedClose()
 	defer restore()
 
@@ -785,12 +775,10 @@ func (s *apparmorpromptingSuite) TestAskShutdownBeforeReply(c *C) {
 	mgr, err := apparmorprompting.New(s.noticeMgr)
 	c.Assert(err, IsNil)
 
-	neverClose := make(chan struct{})
-
 	// Call Ask, then signal when response has been validated
 	doneChan := make(chan struct{})
 	go func() {
-		outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, neverClose)
+		outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
 		c.Check(outcome, Equals, prompting.OutcomeUnset)
 		c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
 		close(doneChan)
@@ -861,11 +849,7 @@ func (s *apparmorpromptingSuite) TestAskShutdownBeforeReply(c *C) {
 	}
 }
 
-// XXX: this test only exists since there are currently two ways to tell Ask to
-// stop waiting: the manager closing, and the snapdShuttingDown channel closing.
-// Once the latter is removed in favor of a proper shutdown of the manager
-// triggered from the daemon, this test should be removed.
-func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
+func (s *apparmorpromptingSuite) TestAskShutdownBeforeReplyWithShutDown(c *C) {
 	_, _, restore := apparmorprompting.MockListener()
 	defer restore()
 
@@ -890,12 +874,10 @@ func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
 	mgr, err := apparmorprompting.New(s.noticeMgr)
 	c.Assert(err, IsNil)
 
-	snapdShuttingDown := make(chan struct{})
-
 	// Call Ask, then signal when response has been validated
 	doneChan := make(chan struct{})
 	go func() {
-		outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, snapdShuttingDown)
+		outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
 		c.Check(outcome, Equals, prompting.OutcomeUnset)
 		c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
 		close(doneChan)
@@ -909,14 +891,14 @@ func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
 		c.Errorf("manager failed to become ready after receiving request")
 	}
 
-	// Now Ask should be waiting for a reply. Close snapdShuttingDown instead.
-	close(snapdShuttingDown)
+	// Now Ask should be waiting for a reply. Shut down the manager instead.
+	mgr.ShutDown()
 
 	select {
 	case <-doneChan:
 		// all good
 	case <-time.After(time.Second):
-		c.Errorf("Ask failed to finish after closing snapdShuttingDown")
+		c.Errorf("Ask failed to finish after manager shutdown")
 	}
 
 	// Check that calls to Reply() also return immediately now that the shutdown
@@ -925,6 +907,8 @@ func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
 	clientActivity := false
 	_, err = mgr.PromptDB().Reply(uid, promptID, outcome, clientActivity)
 	c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
+
+	c.Check(mgr.Stop(), IsNil)
 }
 
 func (s *apparmorpromptingSuite) TestExistingRuleAllowsNewPrompt(c *C) {
@@ -1873,8 +1857,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyCausesPromptsHandleReadyingIfO
 	// Ask for other request in the background so we can see and respond to the prompt
 	whenSent := time.Now()
 	go func() {
-		snapdShuttingDown := make(chan struct{})
-		mgr.Ask(1000, "audio-record", "firefox", 1234, "some-cgroup", snapdShuttingDown)
+		mgr.Ask(1000, "audio-record", "firefox", 1234, "some-cgroup")
 	}()
 	// Wait for a notice
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
@@ -1976,11 +1959,10 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying
 
 	// Now add remaining API requests via Ask()
 
-	shutDownChan := make(chan struct{})
 	outcomeChan := make(chan prompting.OutcomeType)
 	errChan := make(chan error)
 	go func() {
-		outcome, err := mgr.Ask(1000, "audio-record", "obs-studio", 12345, "/cgroup-path/snap.obs-studio.obs-studio-someuuid.scope", shutDownChan)
+		outcome, err := mgr.Ask(1000, "audio-record", "obs-studio", 12345, "/cgroup-path/snap.obs-studio.obs-studio-someuuid.scope")
 		outcomeChan <- outcome
 		errChan <- err
 	}()
@@ -1994,7 +1976,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying
 	}
 
 	go func() {
-		outcome, err := mgr.Ask(1000, "audio-record", "signal-desktop", 67890, "/cgroup-path/snap.signal-desktop.signal-desktop.someuuid.scope", shutDownChan)
+		outcome, err := mgr.Ask(1000, "audio-record", "signal-desktop", 67890, "/cgroup-path/snap.signal-desktop.signal-desktop.someuuid.scope")
 		outcomeChan <- outcome
 		errChan <- err
 	}()
@@ -2018,7 +2000,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying
 	}
 
 	// Signal that snapd is shutting down and Ask calls should return
-	close(shutDownChan)
+	mgr.ShutDown()
 	for i := 0; i < 2; i++ {
 		select {
 		case outcome := <-outcomeChan:

overlord/ifacestate/export_test.go

@@ -140,6 +140,10 @@ func MockCreateInterfacesRequestsManager(new func(noticeMgr *notices.NoticeManag
 	return testutil.Mock(&createInterfacesRequestsManager, new)
 }
 
+func MockInterfacesRequestsManagerShutDown(new func(m *apparmorprompting.InterfacesRequestsManager)) (restore func()) {
+	return testutil.Mock(&interfacesRequestsManagerShutDown, new)
+}
+
 func MockInterfacesRequestsManagerStop(new func(m *apparmorprompting.InterfacesRequestsManager) error) (restore func()) {
 	return testutil.Mock(&interfacesRequestsManagerStop, new)
 }

overlord/ifacestate/ifacemgr.go

@@ -315,6 +315,26 @@ func (m *InterfaceManager) Ensure() error {
 	return nil
 }
 
+// interfacesRequestsManagerShutDown calls shutdown on the given manager.
+var interfacesRequestsManagerShutDown = func(interfacesRequestsManager *apparmorprompting.InterfacesRequestsManager) {
+	interfacesRequestsManager.ShutDown()
+}
+
+func (m *InterfaceManager) shutDownInterfacesRequestsManger() {
+	m.interfacesRequestsManagerMu.Lock()
+	defer m.interfacesRequestsManagerMu.Unlock()
+	if m.interfacesRequestsManager == nil {
+		return
+	}
+	interfacesRequestsManagerShutDown(m.interfacesRequestsManager)
+}
+
+// ShutDown implements ShutDowner. It prevents the manager from receiving
+// anymore new requests and reject pending ones.
+func (m *InterfaceManager) ShutDown() {
+	m.shutDownInterfacesRequestsManger()
+}
+
 // Stop implements StateStopper. It stops the udev monitor and prompting,
 // if running.
 func (m *InterfaceManager) Stop() {

overlord/ifacestate/ifacestate_test.go

@@ -5703,6 +5703,7 @@ slots:
 	change := s.addRemoveSnapSecurityChange("consumer")
 	s.se.Ensure()
 	s.se.Wait()
+	s.se.ShutDown()
 	s.se.Stop()
 
 	// Change succeeds
@@ -7156,6 +7157,7 @@ func (s *interfaceManagerSuite) TestManagerTransitionConnectionsCore(c *C) {
 	s.state.Unlock()
 	s.se.Ensure()
 	s.se.Wait()
+	s.se.ShutDown()
 	s.se.Stop()
 	s.state.Lock()
 
@@ -7904,6 +7906,40 @@ func (s *interfaceManagerSuite) TestInitInterfacesRequestsManagerError(c *C) {
 	c.Check(warns[0].String(), Matches, fmt.Sprintf(`cannot start prompting backend: %v; prompting will be inactive until snapd is restarted`, createError))
 }
 
+func (s *interfaceManagerSuite) TestShutDownInterfacesRequestsManager(c *C) {
+	shutDownCount := 0
+	restore := ifacestate.MockInterfacesRequestsManagerShutDown(func(m *apparmorprompting.InterfacesRequestsManager) {
+		shutDownCount++
+	})
+	defer restore()
+	mgr := ifacestate.NewInterfaceManagerWithAppArmorPrompting(true)
+	c.Check(mgr.InterfacesRequestsManager(), Equals, nil)
+	mgr.ShutDown()
+	c.Check(shutDownCount, Equals, 0)
+
+	restore = ifacestate.MockAssessAppArmorPrompting(func(m *ifacestate.InterfaceManager) bool {
+		return true
+	})
+	defer restore()
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		return true, nil
+	})
+	defer restore()
+	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
+	restore = ifacestate.MockCreateInterfacesRequestsManager(func(noticeMgr *notices.NoticeManager) (*apparmorprompting.InterfacesRequestsManager, error) {
+		return fakeManager, nil
+	})
+	defer restore()
+
+	mgr = s.manager(c)
+	c.Check(mgr.InterfacesRequestsManager(), Equals, fakeManager)
+
+	mgr.ShutDown()
+	c.Check(shutDownCount, Equals, 1)
+
+	mgr.Stop()
+}
+
 func (s *interfaceManagerSuite) TestStopInterfacesRequestsManagerError(c *C) {
 	restore := ifacestate.MockAssessAppArmorPrompting(func(m *ifacestate.InterfaceManager) bool {
 		return true
@@ -9168,6 +9204,7 @@ func (s *interfaceManagerSuite) TestUDevMonitorInit(c *C) {
 	for i := 0; i < 5; i++ {
 		c.Assert(s.se.Ensure(), IsNil)
 	}
+	s.se.ShutDown()
 	s.se.Stop()
 
 	c.Assert(u.ConnectCalls, Equals, 1)