many: refactor fdstore for easier test mocking

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

Author: ZeyadYasser

netutil/activation.go

@@ -73,7 +73,8 @@ func GetListener(socketPath string, listenerMap map[string]net.Listener) (net.Li
 // ActivationListeners builds a map of addresses to listeners that were passed
 // during systemd activation
 func ActivationListeners() (listenerByAddr map[string]net.Listener, err error) {
-	listeners, err := fdstore.ActivationListeners()
+	fds := fdstore.New()
+	listeners, err := fds.ActivationListeners()
 	if err != nil {
 		return nil, err
 	}

overlord/fdestate/backend/export_test.go

@@ -20,11 +20,8 @@
 package backend
 
 import (
-	"os"
-
 	"github.com/snapcore/snapd/boot"
 	"github.com/snapcore/snapd/secboot"
-	"github.com/snapcore/snapd/systemd/fdstore"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -66,14 +63,6 @@ func MockSecbootPCRPolicyCounterHandles(f func(uk secboot.UpdatedKeys) []uint32)
 	}
 }
 
-func MockFdstoreAdd(f func(name fdstore.FdName, f *os.File) error) (restore func()) {
-	return testutil.Mock(&fdstoreAdd, f)
-}
-
-func MockFdstoreGet(f func(name fdstore.FdName) (*os.File, error)) (restore func()) {
-	return testutil.Mock(&fdstoreGet, f)
-}
-
 func MockUnixMmap(f func(fd int, offset int64, length int, prot int, flags int) ([]byte, error)) (restore func()) {
 	return testutil.Mock(&unixMmap, f)
 }

overlord/fdestate/backend/secret_state.go

@@ -31,6 +31,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/strutil"
 	"github.com/snapcore/snapd/systemd/fdstore"
@@ -43,9 +44,7 @@ var (
 )
 
 var (
-	fdstoreAdd = fdstore.Add
-	fdstoreGet = fdstore.Get
-
+	fdstoreNew      = fdstore.New
 	unixMemfdSecret = unix.MemfdSecret
 	unixMemfdCreate = unix.MemfdCreate
 	unixMmap        = unix.Mmap
@@ -282,7 +281,9 @@ func (s *secretState) closeLocked() error {
 }
 
 func openSecretStateFile() (*os.File, error) {
-	f, err := fdstoreGet(fdstore.FdNameMemfdSecretState)
+	fds := fdstoreNew()
+
+	f, err := fds.Get(fdstore.FdNameMemfdSecretState)
 	if errors.Is(err, fdstore.ErrNotFound) || errors.Is(err, fdstore.ErrUnsupportedSystemdVersion) {
 		fdstoreSupported := !errors.Is(err, fdstore.ErrUnsupportedSystemdVersion)
 		fd, err := unixMemfdSecret(0)
@@ -309,7 +310,7 @@ func openSecretStateFile() (*os.File, error) {
 			// version is too old, we will just use the memfd without adding
 			// it to the fdstore, persistence across snapd restarts will be
 			// lost but it is better than crashing.
-			if err := fdstoreAdd(fdstore.FdNameMemfdSecretState, f); err != nil {
+			if err := fds.Add(fdstore.FdNameMemfdSecretState, f); err != nil {
 				return nil, fmt.Errorf("cannot add secret state to fdstore: %w", err)
 			}
 		} else {
@@ -396,3 +397,12 @@ func OpenSecretState(stateChecker stateLockChecker) (retState SecretState, retEr
 	runtime.SetFinalizer(s, (*secretState).closeLocked)
 	return s, nil
 }
+
+func MockFdstoreNew(f func() fdstore.Store) (restore func()) {
+	osutil.MustBeTestBinary("fdstoreNew only can be mocked in tests")
+	old := fdstoreNew
+	fdstoreNew = f
+	return func() {
+		fdstoreNew = old
+	}
+}

overlord/fdestate/backend/secret_state_test.go

@@ -22,6 +22,7 @@ package backend_test
 import (
 	"errors"
 	"fmt"
+	"net"
 	"os"
 
 	"golang.org/x/sys/unix"
@@ -42,6 +43,7 @@ type secretStateSuite struct {
 	ops              []string
 	failOn           map[string]error
 	stateLockChecker *mockStateLockChecker
+	fdstore          fdstore.Store
 }
 
 var _ = Suite(&secretStateSuite{})
@@ -56,6 +58,33 @@ func (m *mockStateLockChecker) EnsureLocked() {
 	}
 }
 
+type mockFdstore struct {
+	get func(name fdstore.FdName) (*os.File, error)
+	add func(name fdstore.FdName, f *os.File) error
+}
+
+func (m *mockFdstore) Add(name fdstore.FdName, f *os.File) error {
+	if m.add == nil {
+		return errors.New("mockFdstore.Add() not implemented")
+	}
+	return m.add(name, f)
+}
+
+func (m *mockFdstore) Get(name fdstore.FdName) (*os.File, error) {
+	if m.get == nil {
+		return nil, errors.New("mockFdstore.Get() not implemented")
+	}
+	return m.get(name)
+}
+
+func (m *mockFdstore) Remove(name fdstore.FdName) error {
+	return errors.New("mockFdstore.Remove() not implemented")
+}
+
+func (m *mockFdstore) ActivationListeners() ([]net.Listener, error) {
+	return nil, errors.New("mockFdstore.ActivationListeners() not implemented")
+}
+
 func dupFile(name fdstore.FdName, f *os.File) (*os.File, error) {
 	duplicatedFd, err := unix.Dup(int(f.Fd()))
 	if err != nil {
@@ -148,8 +177,12 @@ func (s *secretStateSuite) SetUpTest(c *C) {
 	s.failOn = make(map[string]error)
 	s.stateLockChecker = &mockStateLockChecker{}
 
-	s.AddCleanup(backend.MockFdstoreGet(s.fdstoreGet))
-	s.AddCleanup(backend.MockFdstoreAdd(s.fdstoreAdd))
+	s.AddCleanup(backend.MockFdstoreNew(func() fdstore.Store {
+		return &mockFdstore{
+			get: s.fdstoreGet,
+			add: s.fdstoreAdd,
+		}
+	}))
 	s.AddCleanup(backend.MockUnixMmap(s.mmap))
 	s.AddCleanup(backend.MockUnixMunmap(s.munmap))
 	s.AddCleanup(backend.MockUnixMemfdSecret(s.memfdSecret))

systemd/fdstore/fdstore.go

@@ -56,6 +56,43 @@ func (name FdName) isSocket() bool {
 	return strings.HasSuffix(string(name), ".socket")
 }
 
+type Store interface {
+	// Add passes a file descriptor to systemd associated with a name
+	// to reuse it across snapd restarts.
+	//
+	//   - The file descriptors can be retrieved by calling Get().
+	//   - Only a single file descriptor can associated with a FdName.
+	//
+	// Maintains a copy of the underlying file descriptor internally. It
+	// is the caller's responsibility to close f when finished.
+	Add(name FdName, f *os.File) error
+	// Get retrieves a duplicate of the file descriptor passed from systemd by
+	// its name. close-on-exec is set on the returned file descriptor. An error
+	// matching ErrNotFound is returned if no matching file descriptor is found.
+	// Passed name cannot be a socket (i.e. cannot end in ".socket"), for
+	// activation sockets use ActivationListeners() instead.
+	//
+	// The fdstore holds a copy of the file descriptor, the caller needs to
+	// call Remove() on top of closing all privately held references in order
+	// to release all resources associated with a given fd.
+	Get(name FdName) (*os.File, error)
+	// Remove removes file descriptors from systemd given their name.
+	// Remove cannot remove activation sockets.
+	Remove(name FdName) error
+	// ActivationListeners returns activation listeners that were passed
+	// from systemd. Only sockets whose name has a ".socket" suffix are
+	// returned. Order of returned listeners is not deterministic.
+	//
+	// It is the caller's responsibility to close returned listeners when finished.
+	ActivationListeners() ([]net.Listener, error)
+}
+
+type store struct{}
+
+func New() Store {
+	return &store{}
+}
+
 var (
 	osGetpid        = os.Getpid
 	osFileClose     = (*os.File).Close
@@ -76,7 +113,7 @@ var mu sync.RWMutex
 // passed from systemd.
 const sd_LISTEN_FDS_START = 3
 
-func initFdstore() {
+func (s *store) initFdstore() {
 	mu.Lock()
 	defer mu.Unlock()
 
@@ -149,7 +186,7 @@ func initFdstore() {
 		}
 		if shouldRemove {
 			logger.Noticef("removing unexpected fdstore entry %q", name)
-			if err := remove(name); err != nil {
+			if err := s.remove(name); err != nil {
 				logger.Noticef("internal error: cannot remove fdstore entry %q: %v", name, err)
 				continue
 			}
@@ -172,10 +209,8 @@ func checkSystemdVersion() error {
 	return nil
 }
 
-// Remove removes file descriptors from systemd given their name.
-// Remove cannot remove activation sockets.
-func Remove(name FdName) error {
-	initFdstore()
+func (s *store) Remove(name FdName) error {
+	s.initFdstore()
 
 	if err := checkSystemdVersion(); err != nil {
 		return fmt.Errorf("cannot remove file descriptor from fdstore: %w", err)
@@ -194,13 +229,13 @@ func Remove(name FdName) error {
 		return fmt.Errorf("cannot remove file descriptor from fdstore: %w", ErrNotFound)
 	}
 
-	return remove(name)
+	return s.remove(name)
 }
 
 // remove file descriptors from systemd given their name.
 //
 // Caller must hold the fdstore lock.
-func remove(name FdName) error {
+func (s *store) remove(name FdName) error {
 	state := fmt.Sprintf("FDSTOREREMOVE=1\nFDNAME=%s", name)
 	if err := sdNotify(state); err != nil {
 		return err
@@ -226,17 +261,8 @@ func duplicateFile(name FdName, f *os.File) (*os.File, error) {
 	return os.NewFile(uintptr(duplicatedFd), string(name)), nil
 }
 
-// Get retrieves a duplicate of the file descriptor passed from systemd by
-// its name. close-on-exec is set on the returned file descriptor. An error
-// matching ErrNotFound is returned if no matching file descriptor is found.
-// Passed name cannot be a socket (i.e. cannot end in ".socket"), for
-// activation sockets use ActivationListeners() instead.
-//
-// The fdstore holds a copy of the file descriptor, the caller needs to
-// call Remove() on top of closing all privately held references in order
-// to release all resources associated with a given fd.
-func Get(name FdName) (*os.File, error) {
-	initFdstore()
+func (s *store) Get(name FdName) (*os.File, error) {
+	s.initFdstore()
 
 	if err := checkSystemdVersion(); err != nil {
 		return nil, fmt.Errorf("cannot get file descriptor from fdstore: %w", err)
@@ -261,16 +287,8 @@ func Get(name FdName) (*os.File, error) {
 	return duplicateFile(name, fds[0])
 }
 
-// Add passes a file descriptor to systemd associated with a name
-// to reuse it across snapd restarts.
-//
-//   - The file descriptors can be retrieved by calling Get().
-//   - Only a single file descriptor can associated with a FdName.
-//
-// Maintains a copy of the underlying file descriptor internally. It
-// is the caller's responsibility to close f when finished.
-func Add(name FdName, f *os.File) error {
-	initFdstore()
+func (s *store) Add(name FdName, f *os.File) error {
+	s.initFdstore()
 
 	if err := checkSystemdVersion(); err != nil {
 		return fmt.Errorf("cannot add file descriptor to fdstore: %w", err)
@@ -307,13 +325,8 @@ func Add(name FdName, f *os.File) error {
 	return nil
 }
 
-// ActivationListeners returns activation listeners that were passed
-// from systemd. Only sockets whose name has a ".socket" suffix are
-// returned. Order of returned listeners is not deterministic.
-//
-// It is the caller's responsibility to close returned listeners when finished.
-func ActivationListeners() (retListeners []net.Listener, retErr error) {
-	initFdstore()
+func (s *store) ActivationListeners() (retListeners []net.Listener, retErr error) {
+	s.initFdstore()
 
 	mu.RLock()
 	defer mu.RUnlock()