Fix system connections on machine removal

When a machine is removed, it removes the current
system connection and will select a new one from
the top of the alphabetical list. However, when
doing this there is no guarantee that, if the new
system connection is associated with a machine, the
new system connection will have the right rootfulness.
This will ensure that the new connection, if associated
with a machine, will have proper rootfulness.

Fixes: containers#22577

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

cmd/podman/system/reset_machine.go

@@ -30,6 +30,11 @@ func resetMachine() error {
 		logrus.Errorf("unable to load machines: %q", err)
 	}
 
+	machines, err := p.GetAllMachinesAndRootfulness()
+	if err != nil {
+		return err
+	}
+
 	for _, mc := range mcs {
 		state, err := provider.State(mc, false)
 		if err != nil {
@@ -42,7 +47,7 @@ func resetMachine() error {
 			}
 		}
 
-		if err := connection.RemoveConnections(mc.Name, mc.Name+"-root"); err != nil {
+		if err := connection.RemoveConnections(machines, mc.Name, mc.Name+"-root"); err != nil {
 			logrus.Error(err)
 		}
 

pkg/machine/connection/connection.go

@@ -7,10 +7,8 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"os"
 
 	"github.com/containers/common/pkg/config"
-	"github.com/sirupsen/logrus"
 )
 
 const LocalhostIP = "127.0.0.1"
@@ -85,37 +83,51 @@ func UpdateConnectionIfDefault(rootful bool, name, rootfulName string) error {
 	})
 }
 
-func RemoveConnections(names ...string) error {
-	return config.EditConnectionConfig(func(cfg *config.ConnectionsFile) error {
-		for _, name := range names {
-			if _, ok := cfg.Connection.Connections[name]; ok {
-				delete(cfg.Connection.Connections, name)
-			} else {
-				return fmt.Errorf("unable to find connection named %q", name)
-			}
+func RemoveConnections(machines map[string]bool, names ...string) error {
+	var dest config.Destination
+	var service string
 
-			if cfg.Connection.Default == name {
-				cfg.Connection.Default = ""
-			}
-		}
-		for service := range cfg.Connection.Connections {
-			cfg.Connection.Default = service
-			break
-		}
-		return nil
-	})
+	if err := config.EditConnectionConfig(func(cfg *config.ConnectionsFile) error {
+		return setNewDefaultConnection(cfg, &dest, &service, names...)
+	}); err != nil {
+		return err
+	}
+
+	rootful, ok := machines[service]
+	if dest.IsMachine && ok {
+		return UpdateConnectionIfDefault(rootful, service, service+"-root")
+	}
+
+	return nil
 }
 
-// removeFilesAndConnections removes any files and connections with the given names
-func RemoveFilesAndConnections(files []string, names ...string) {
-	for _, f := range files {
-		if err := os.Remove(f); err != nil && !errors.Is(err, os.ErrNotExist) {
-			logrus.Error(err)
+// setNewDefaultConnection iterates through the list of system connections and sets the new default as the very first one
+func setNewDefaultConnection(cfg *config.ConnectionsFile, dest *config.Destination, service *string, names ...string) error {
+	// delete the connection associated with the names and if that connection is
+	// the default, reset the default connection
+	for _, name := range names {
+		if _, ok := cfg.Connection.Connections[name]; ok {
+			delete(cfg.Connection.Connections, name)
+		} else {
+			return fmt.Errorf("unable to find connection named %q", name)
+		}
+
+		if cfg.Connection.Default == name {
+			cfg.Connection.Default = ""
 		}
 	}
-	if err := RemoveConnections(names...); err != nil {
-		logrus.Error(err)
+
+	// set the new default system connection to the first in the map
+	for con := range cfg.Connection.Connections {
+		cfg.Connection.Default = con
+		if c, ok := cfg.Connection.Connections[con]; ok {
+			*dest = c
+			*service = con
+		}
+		break
 	}
+
+	return nil
 }
 
 // makeSSHURL creates a URL from the given input

pkg/machine/e2e/rm_test.go

@@ -43,6 +43,49 @@ var _ = Describe("podman machine rm", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(removeSession2).To(Exit(125))
 		Expect(removeSession2.errorToString()).To(ContainSubstring(fmt.Sprintf("%s: VM does not exist", name)))
+
+		// Ensure that the system connections have the right rootfulness
+		name = randomString()
+		i = new(initMachine)
+		session, err = mb.setName(name).setCmd(i.withImage(mb.imagePath)).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(0))
+
+		name2 := randomString()
+		i = new(initMachine)
+		session, err = mb.setName(name2).setCmd(i.withImage(mb.imagePath).withRootful(true)).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(0))
+
+		bm := basicMachine{}
+		sysConnOutput, err := mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Name}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		names := sysConnOutput.outputToStringSlice()
+		sysConnOutput, err = mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Default}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		defaults := sysConnOutput.outputToStringSlice()
+		for idx, n := range names {
+			if n == name {
+				Expect(defaults[idx]).To(Equal("true"))
+			}
+		}
+
+		rm = rmMachine{}
+		removeSession, err = mb.setName(name).setCmd(rm.withForce()).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(removeSession).To(Exit(0))
+
+		sysConnOutput, err = mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Name}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		names = sysConnOutput.outputToStringSlice()
+		sysConnOutput, err = mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Default}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		defaults = sysConnOutput.outputToStringSlice()
+		for idx, n := range names {
+			if n == name2+"-root" {
+				Expect(defaults[idx]).To(Equal("true"))
+			}
+		}
 	})
 
 	It("Remove running machine", func() {

pkg/machine/provider/platform.go

@@ -2,6 +2,8 @@ package provider
 
 import (
 	"github.com/containers/podman/v5/pkg/machine/define"
+	"github.com/containers/podman/v5/pkg/machine/env"
+	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
 )
 
 func InstalledProviders() ([]define.VMType, error) {
@@ -18,3 +20,26 @@ func InstalledProviders() ([]define.VMType, error) {
 	}
 	return installedTypes, nil
 }
+
+// GetAllMachinesAndRootfulness collects all podman machine configs and returns
+// a map in the format: { machineName: isRootful }
+func GetAllMachinesAndRootfulness() (map[string]bool, error) {
+	providers := GetAll()
+	machines := map[string]bool{}
+	for _, provider := range providers {
+		dirs, err := env.GetMachineDirs(provider.VMType())
+		if err != nil {
+			return nil, err
+		}
+		providerMachines, err := vmconfigs.LoadMachinesInDir(dirs)
+		if err != nil {
+			return nil, err
+		}
+
+		for n, m := range providerMachines {
+			machines[n] = m.HostUser.Rootful
+		}
+	}
+
+	return machines, nil
+}

pkg/machine/shim/host.go

@@ -16,6 +16,7 @@ import (
 	"github.com/containers/podman/v5/pkg/machine/env"
 	"github.com/containers/podman/v5/pkg/machine/ignition"
 	"github.com/containers/podman/v5/pkg/machine/lock"
+	"github.com/containers/podman/v5/pkg/machine/provider"
 	"github.com/containers/podman/v5/pkg/machine/proxyenv"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
 	"github.com/containers/podman/v5/utils"
@@ -229,8 +230,12 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 		return err
 	}
 
+	machines, err := provider.GetAllMachinesAndRootfulness()
+	if err != nil {
+		return err
+	}
 	cleanup := func() error {
-		return connection.RemoveConnections(mc.Name, mc.Name+"-root")
+		return connection.RemoveConnections(machines, mc.Name, mc.Name+"-root")
 	}
 	callbackFuncs.Add(cleanup)
 
@@ -580,7 +585,11 @@ func Remove(mc *vmconfigs.MachineConfig, mp vmconfigs.VMProvider, dirs *machineD
 		}
 	}
 
-	rmFiles, genericRm, err := mc.Remove(opts.SaveIgnition, opts.SaveImage)
+	machines, err := provider.GetAllMachinesAndRootfulness()
+	if err != nil {
+		return err
+	}
+	rmFiles, genericRm, err := mc.Remove(machines, opts.SaveIgnition, opts.SaveImage)
 	if err != nil {
 		return err
 	}
@@ -642,6 +651,11 @@ func Reset(mps []vmconfigs.VMProvider, opts machine.ResetOptions) error {
 	var resetErrors *multierror.Error
 	removeDirs := []*machineDefine.MachineDirs{}
 
+	machines, err := provider.GetAllMachinesAndRootfulness()
+	if err != nil {
+		return err
+	}
+
 	for _, p := range mps {
 		d, err := env.GetMachineDirs(p.VMType())
 		if err != nil {
@@ -660,7 +674,7 @@ func Reset(mps []vmconfigs.VMProvider, opts machine.ResetOptions) error {
 			if err != nil {
 				resetErrors = multierror.Append(resetErrors, err)
 			}
-			_, genericRm, err := mc.Remove(false, false)
+			_, genericRm, err := mc.Remove(machines, false, false)
 			if err != nil {
 				resetErrors = multierror.Append(resetErrors, err)
 			}

pkg/machine/vmconfigs/machine.go

@@ -153,7 +153,7 @@ func (mc *MachineConfig) updateLastBoot() error { //nolint:unused
 	return mc.Write()
 }
 
-func (mc *MachineConfig) Remove(saveIgnition, saveImage bool) ([]string, func() error, error) {
+func (mc *MachineConfig) Remove(machines map[string]bool, saveIgnition, saveImage bool) ([]string, func() error, error) {
 	ignitionFile, err := mc.IgnitionFile()
 	if err != nil {
 		return nil, nil, err
@@ -195,7 +195,7 @@ func (mc *MachineConfig) Remove(saveIgnition, saveImage bool) ([]string, func()
 
 	mcRemove := func() error {
 		var errs []error
-		if err := connection.RemoveConnections(mc.Name, mc.Name+"-root"); err != nil {
+		if err := connection.RemoveConnections(machines, mc.Name, mc.Name+"-root"); err != nil {
 			errs = append(errs, err)
 		}