diff --git a/msix/UbuntuProForWSL/Package.appxmanifest b/msix/UbuntuProForWSL/Package.appxmanifest index d3df54ead..cf5204e8d 100644 --- a/msix/UbuntuProForWSL/Package.appxmanifest +++ b/msix/UbuntuProForWSL/Package.appxmanifest @@ -1,5 +1,5 @@  - + Ubuntu Pro for WSL @@ -33,6 +33,11 @@ + + + + + diff --git a/windows-agent/cmd/ubuntu-pro-agent/agent/agent.go b/windows-agent/cmd/ubuntu-pro-agent/agent/agent.go index 4c1af5073..19ac0d87c 100644 --- a/windows-agent/cmd/ubuntu-pro-agent/agent/agent.go +++ b/windows-agent/cmd/ubuntu-pro-agent/agent/agent.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "github.com/canonical/ubuntu-pro-for-wsl/common" @@ -86,15 +87,29 @@ func New(o ...option) *App { return nil }, RunE: func(cmd *cobra.Command, args []string) error { + var opt options + for _, f := range o { + f(&opt) + } + + cleanup, err := a.ensureSingleInstance(opt) + if err != nil { + // We won't serve(), so let's close the ready channel right now. + // Otherwise callers of WaitReady will block forever. + close(a.ready) + return err + } + defer cleanup() + ctx := context.Background() - cleanup, err := a.setUpLogger(ctx) + cleanup, err = a.setUpLogger(ctx) if err != nil { log.Warningf(ctx, "could not set logger output: %v", err) } defer cleanup() - return a.serve(ctx, o...) + return a.serve(ctx, opt) }, // We display usage error ourselves SilenceErrors: true, @@ -112,12 +127,7 @@ func New(o ...option) *App { } // serve creates new GRPC services and listen on a TCP socket. This call is blocking until we quit it. -func (a *App) serve(ctx context.Context, args ...option) error { - var opt options - for _, f := range args { - f(&opt) - } - +func (a *App) serve(ctx context.Context, opt options) error { publicDir, err := a.publicDir(opt) if err != nil { close(a.ready) @@ -266,3 +276,32 @@ func (a *App) setUpLogger(ctx context.Context) (func(), error) { return func() { _ = f.Close() }, nil } + +// ensureSingleInstance creates a lock file to ensure that only one instance of the agent is running. +// It returns a cleanup function to release that file or an error if the lock file could not be flushed to disk. +func (a *App) ensureSingleInstance(opt options) (cleanup func(), err error) { + priv, err := a.privateDir(opt) + if err != nil { + return nil, fmt.Errorf("could not access the agent's private dir: %v", err) + } + + // We deliberately create a new file instead of reusing the address file, for example, because that file has many other reasons for being recreated. + // No other file the agent creates match the semantics of exclusive ownership needed here. + path := filepath.Join(priv, "ubuntu-pro-agent.lock") + f, err := createLockFile(path) + if err != nil { + return nil, err + } + + pid := strconv.Itoa(os.Getpid()) + if _, err := f.WriteString(pid); err != nil { + return nil, fmt.Errorf("could not write PID to lock file %s: %v", path, errors.Join(err, f.Close())) + } + if err := f.Sync(); err != nil { + return nil, fmt.Errorf("could not flush lock file %s: %v", path, errors.Join(err, f.Close())) + } + + return func() { + log.Warningf(context.Background(), "when releasing the lock file: %v", f.Close()) + }, nil +} diff --git a/windows-agent/cmd/ubuntu-pro-agent/agent/agent_test.go b/windows-agent/cmd/ubuntu-pro-agent/agent/agent_test.go index 4c091dbe8..1d3973ed2 100644 --- a/windows-agent/cmd/ubuntu-pro-agent/agent/agent_test.go +++ b/windows-agent/cmd/ubuntu-pro-agent/agent/agent_test.go @@ -2,6 +2,7 @@ package agent_test import ( "bytes" + "errors" "fmt" "io" "os" @@ -23,6 +24,64 @@ func init() { daemontestutils.DefaultNetworkDetectionToMock() } +func TestSingleInstance(t *testing.T) { + t.Parallel() + + testcases := map[string]struct { + args []string + anotherInstanceRunning bool + breakLockFile bool + + wantError bool + }{ + "Success when single instance": {args: []string{"version"}}, + + // Testing the 'clean' verb cannot run in parallel because it tries to delete files still opened by other test cases. + "Completion succeeds with another instance running": {args: []string{"completion", "bash"}, anotherInstanceRunning: true}, + "Completion succeeds with broken lock file": {args: []string{"completion", "bash"}, breakLockFile: true}, + "Help succeeds with another instance running": {args: []string{"help"}, anotherInstanceRunning: true}, + "Help succeeds with broken lock file": {args: []string{"help"}, breakLockFile: true}, + "Version succeeds with another instance running": {args: []string{"version"}, anotherInstanceRunning: true}, + "Version succeeds with broken lock file": {args: []string{"version"}, breakLockFile: true}, + + "Default (serve) fails with another instance running": {anotherInstanceRunning: true, wantError: true}, + "Default (serve) fails with broken lock file": {breakLockFile: true, wantError: true}, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + private := t.TempDir() + if tc.anotherInstanceRunning { + path := filepath.Join(private, "ubuntu-pro-agent.lock") + + f, err := agent.CreateLockFile(path) + require.NoError(t, err, "Setup: couldn't create lock file") + + defer f.Close() + } + + if tc.breakLockFile { + path := filepath.Join(private, "ubuntu-pro-agent.lock") + err := os.MkdirAll(path, 0700) + err2 := os.WriteFile(filepath.Join(path, "breaking-lock.txt"), []byte{}, 0600) + require.NoError(t, errors.Join(err, err2), "Setup: couldn't break the lock file") + } + + a := agent.NewForTesting(t, "", private) + a.SetArgs(tc.args...) + + err := a.Run() + if tc.wantError { + require.Error(t, err, "Run should return an error") + return + } + require.NoError(t, err, "Run should not return an error") + }) + } +} + func TestHelp(t *testing.T) { a := agent.NewForTesting(t, "", "") a.SetArgs("--help") @@ -218,8 +277,8 @@ func TestAppRunFailsOnComponentsCreationAndQuit(t *testing.T) { invalidLocalAppData bool invalidUserProfile bool }{ - "Invalid private directory": {invalidPublicDir: true}, - "Invalid public directory": {invalidPrivateDir: true}, + "Invalid private directory": {invalidPrivateDir: true}, + "Invalid public directory": {invalidPublicDir: true}, "Invalid LocalAppData": {invalidLocalAppData: true}, "Invalid UserProfile": {invalidUserProfile: true}, } @@ -248,12 +307,12 @@ func TestAppRunFailsOnComponentsCreationAndQuit(t *testing.T) { privateDir = badDir } - a := agent.New(agent.WithPublicDir(publicDir), agent.WithPrivateDir(privateDir), agent.WithRegistry(registry.NewMock())) - a.SetArgs() - err := os.WriteFile(badDir, []byte("I'm here to break the service"), 0600) require.NoError(t, err, "Failed to write file") + a := agent.New(agent.WithPublicDir(publicDir), agent.WithPrivateDir(privateDir), agent.WithRegistry(registry.NewMock())) + a.SetArgs("") + err = a.Run() require.Error(t, err, "Run should exit with an error") a.Quit() @@ -372,6 +431,8 @@ func TestLogs(t *testing.T) { }() a.WaitReady() + // TODO: Implement the real fix for WaitReady() per UDENG-4900 + <-time.After(1 * time.Second) select { case <-ch: @@ -407,12 +468,14 @@ func TestClean(t *testing.T) { // Not parallel because we modify the environment testCases := map[string]struct { - emptyUserProfile bool - emptyLocalAppDir bool + emptyUserProfile bool + emptyLocalAppDir bool + anotherInstanceRunning bool wantErr bool }{ - "Success": {}, + "Success": {}, + "Success with another instance running": {anotherInstanceRunning: true}, "Error when %UserProfile% is empty": {emptyUserProfile: true, wantErr: true}, "Error when %LocalAppData% is empty": {emptyLocalAppDir: true, wantErr: true}, @@ -457,6 +520,17 @@ func TestClean(t *testing.T) { require.NoError(t, err, "Setup: could not write file outside the private directory") } + if tc.anotherInstanceRunning { + path := filepath.Join(appData, common.LocalAppDataDir, "ubuntu-pro-agent.lock") + + f, err := agent.CreateLockFile(path) + require.NoError(t, err, "Setup: couldn't create lock file") + // Since the 'clean' verb kills all other agent processes, by the time the agent attempts to clean the private directory the lock file would have been released. + // So for this test case the only thing that matters is whether the agent proceeds with cleaning (as it should) or not. + // We don't want the test to fail because the file is still open. + require.NoError(t, f.Close(), "Setup: couldn't close the fake lock file") + } + a := agent.New(agent.WithRegistry(registry.NewMock())) a.SetArgs("clean") diff --git a/windows-agent/cmd/ubuntu-pro-agent/agent/export_test.go b/windows-agent/cmd/ubuntu-pro-agent/agent/export_test.go index a925c0ae0..95edd5946 100644 --- a/windows-agent/cmd/ubuntu-pro-agent/agent/export_test.go +++ b/windows-agent/cmd/ubuntu-pro-agent/agent/export_test.go @@ -46,3 +46,6 @@ func NewForTesting(t *testing.T, publicDir, privateDir string) *App { func (a App) Config() daemonConfig { return a.config } + +// CreateLockFile tries to create or open an empty file with given name with exclusive access. +var CreateLockFile = createLockFile diff --git a/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_linux.go b/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_linux.go new file mode 100644 index 000000000..991973bab --- /dev/null +++ b/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_linux.go @@ -0,0 +1,28 @@ +package agent + +import ( + "errors" + "fmt" + "os" + "syscall" + + "github.com/ubuntu/decorate" +) + +// createLockFile tries to create or open an empty file with given name with exclusive access. +// If the file already exists AND is still locked, it will fail. +func createLockFile(path string) (f *os.File, err error) { + decorate.OnError(&err, "could not create lock file %s: %v", path, err) + + f, err = os.OpenFile(path, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0600) + if err != nil { + return nil, err + } + // This would only fail if the file is locked by another process. + err = syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err != nil { + return nil, fmt.Errorf("could not lock file: %v", errors.Join(err, f.Close())) + } + + return f, nil +} diff --git a/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_windows.go b/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_windows.go new file mode 100644 index 000000000..8cfe64e36 --- /dev/null +++ b/windows-agent/cmd/ubuntu-pro-agent/agent/lockfile_windows.go @@ -0,0 +1,19 @@ +package agent + +import ( + "os" + + "github.com/ubuntu/decorate" +) + +// createLockFile tries to create or open an empty file with given name with exclusive access. +func createLockFile(path string) (f *os.File, err error) { + decorate.OnError(&err, "could not create lock file %s: %v", path, err) + + // On Windows removing fails if the file is opened by another process with ERROR_SHARING_VIOLATION. + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return nil, err + } + // If this process is the only instance of this program, then the file won't exist. + return os.OpenFile(path, os.O_CREATE|os.O_EXCL, 0600) +}