Merge pull request #23945 from jakecorrenti/default-conn

Handle default system connection transfer properly on machine removal

11 files changed, 146 insertions, 47 deletions

diff --git a/cmd/podman/machine/init.go b/cmd/podman/machine/init.go
index 6f0579a97..69afdc02c 100644
--- a/cmd/podman/machine/init.go
+++ b/cmd/podman/machine/init.go
@@ -11,7 +11,6 @@ import (
 	"github.com/containers/podman/v5/cmd/podman/registry"
 	ldefine "github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/libpod/events"
-	"github.com/containers/podman/v5/pkg/machine"
 	"github.com/containers/podman/v5/pkg/machine/define"
 	"github.com/containers/podman/v5/pkg/machine/shim"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
@@ -34,7 +33,7 @@ var (
 
 	initOpts           = define.InitOptions{}
 	initOptionalFlags  = InitOptionalFlags{}
-	defaultMachineName = machine.DefaultMachineName
+	defaultMachineName = define.DefaultMachineName
 	now                bool
 )
 
diff --git a/cmd/podman/machine/os/manager.go b/cmd/podman/machine/os/manager.go
index 1e028be23..234246cf1 100644
--- a/cmd/podman/machine/os/manager.go
+++ b/cmd/podman/machine/os/manager.go
@@ -9,7 +9,7 @@ import (
 	"strings"
 
 	machineconfig "github.com/containers/common/pkg/machine"
-	pkgMachine "github.com/containers/podman/v5/pkg/machine"
+	"github.com/containers/podman/v5/pkg/machine/define"
 	"github.com/containers/podman/v5/pkg/machine/env"
 	pkgOS "github.com/containers/podman/v5/pkg/machine/os"
 	"github.com/containers/podman/v5/pkg/machine/provider"
@@ -47,7 +47,7 @@ func guestOSManager() (pkgOS.Manager, error) {
 func machineOSManager(opts ManagerOpts, _ vmconfigs.VMProvider) (pkgOS.Manager, error) {
 	vmName := opts.VMName
 	if opts.VMName == "" {
-		vmName = pkgMachine.DefaultMachineName
+		vmName = define.DefaultMachineName
 	}
 	p, err := provider.Get()
 	if err != nil {
diff --git a/cmd/podman/system/reset_machine.go b/cmd/podman/system/reset_machine.go
index 35430dfb7..2c49e1758 100644
--- a/cmd/podman/system/reset_machine.go
+++ b/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)
 		}
 
diff --git a/pkg/machine/config.go b/pkg/machine/config.go
index 25a9aae69..046b94b0f 100644
--- a/pkg/machine/config.go
+++ b/pkg/machine/config.go
@@ -20,10 +20,7 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-const (
-	DefaultMachineName string = "podman-machine-default"
-	apiUpTimeout              = 20 * time.Second
-)
+const apiUpTimeout = 20 * time.Second
 
 var (
 	ForwarderBinaryName = "gvproxy"
diff --git a/pkg/machine/connection/connection.go b/pkg/machine/connection/connection.go
index 469dadcb1..9c77ddeef 100644
--- a/pkg/machine/connection/connection.go
+++ b/pkg/machine/connection/connection.go
@@ -7,10 +7,9 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"os"
 
 	"github.com/containers/common/pkg/config"
-	"github.com/sirupsen/logrus"
+	"github.com/containers/podman/v5/pkg/machine/define"
 )
 
 const LocalhostIP = "127.0.0.1"
@@ -76,46 +75,74 @@ func UpdateConnectionPairPort(name string, port, uid int, remoteUsername string,
 // Returns true if it modified the default
 func UpdateConnectionIfDefault(rootful bool, name, rootfulName string) error {
 	return config.EditConnectionConfig(func(cfg *config.ConnectionsFile) error {
-		if name == cfg.Connection.Default && rootful {
-			cfg.Connection.Default = rootfulName
-		} else if rootfulName == cfg.Connection.Default && !rootful {
-			cfg.Connection.Default = name
-		}
+		updateConnection(cfg, rootful, name, rootfulName)
 		return nil
 	})
 }
 
-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 updateConnection(cfg *config.ConnectionsFile, rootful bool, name, rootfulName string) {
+	if name == cfg.Connection.Default && rootful {
+		cfg.Connection.Default = rootfulName
+	} else if rootfulName == cfg.Connection.Default && !rootful {
+		cfg.Connection.Default = name
+	}
+}
 
-			if cfg.Connection.Default == name {
-				cfg.Connection.Default = ""
-			}
+func RemoveConnections(machines map[string]bool, names ...string) error {
+	var dest config.Destination
+	var service string
+
+	if err := config.EditConnectionConfig(func(cfg *config.ConnectionsFile) error {
+		err := setNewDefaultConnection(cfg, &dest, &service, names...)
+		if err != nil {
+			return err
 		}
-		for service := range cfg.Connection.Connections {
-			cfg.Connection.Default = service
-			break
+
+		rootful, ok := machines[service]
+		if dest.IsMachine && ok {
+			updateConnection(cfg, rootful, service, service+"-root")
 		}
 		return nil
-	})
+	}); err != nil {
+		return err
+	}
+
+	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 available system connections and
+// sets the first available connection as the new default
+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)
+
+	// If there is a podman-machine-default system connection, immediately set that as the new default
+	if c, ok := cfg.Connection.Connections[define.DefaultMachineName]; ok {
+		cfg.Connection.Default = define.DefaultMachineName
+		*dest = c
+		*service = define.DefaultMachineName
+		return nil
+	}
+
+	// set the new default system connection to the first in the map
+	for con, d := range cfg.Connection.Connections {
+		cfg.Connection.Default = con
+		*dest = d
+		*service = con
+		break
 	}
+	return nil
 }
 
 // makeSSHURL creates a URL from the given input
diff --git a/pkg/machine/define/config.go b/pkg/machine/define/config.go
index 90135f76f..b6c07342c 100644
--- a/pkg/machine/define/config.go
+++ b/pkg/machine/define/config.go
@@ -2,8 +2,11 @@ package define
 
 import "os"
 
-const UserCertsTargetPath = "/etc/containers/certs.d"
-const DefaultIdentityName = "machine"
+const (
+	UserCertsTargetPath = "/etc/containers/certs.d"
+	DefaultIdentityName = "machine"
+	DefaultMachineName  = "podman-machine-default"
+)
 
 // MountTag is an identifier to mount a VirtioFS file system tag on a mount point in the VM.
 // Ref: https://developer.apple.com/documentation/virtualization/running_intel_binaries_in_linux_vms_with_rosetta
diff --git a/pkg/machine/e2e/rm_test.go b/pkg/machine/e2e/rm_test.go
index 210e6ed09..119c5b4b7 100644
--- a/pkg/machine/e2e/rm_test.go
+++ b/pkg/machine/e2e/rm_test.go
@@ -43,6 +43,33 @@ 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}}--{{.Default}}"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sysConnOutput.outputToString()).To(ContainSubstring(name + "--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}}--{{.Default}}"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sysConnOutput.outputToString()).To(ContainSubstring(name2 + "-root--true"))
 	})
 
 	It("Remove running machine", func() {
diff --git a/pkg/machine/machine_common.go b/pkg/machine/machine_common.go
index 1afc3d15b..d2ba418b0 100644
--- a/pkg/machine/machine_common.go
+++ b/pkg/machine/machine_common.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/containers/podman/v5/pkg/machine/connection"
+	"github.com/containers/podman/v5/pkg/machine/define"
 	"github.com/containers/storage/pkg/ioutils"
 )
 
@@ -36,7 +37,7 @@ func WaitAPIAndPrintInfo(forwardState APIForwardingState, name, helper, forwardS
 	suffix := ""
 	var fmtString string
 
-	if name != DefaultMachineName {
+	if name != define.DefaultMachineName {
 		suffix = " " + name
 	}
 
@@ -95,7 +96,7 @@ following command in your terminal session:
 
 func PrintRootlessWarning(name string) {
 	suffix := ""
-	if name != DefaultMachineName {
+	if name != define.DefaultMachineName {
 		suffix = " " + name
 	}
 
diff --git a/pkg/machine/provider/platform.go b/pkg/machine/provider/platform.go
index af2f3436a..e04251896 100644
--- a/pkg/machine/provider/platform.go
+++ b/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
+}
diff --git a/pkg/machine/shim/host.go b/pkg/machine/shim/host.go
index 49bcf01b8..948ebe11a 100644
--- a/pkg/machine/shim/host.go
+++ b/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/shim/diskpull"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
@@ -231,7 +232,11 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 	}
 
 	cleanup := func() error {
-		return connection.RemoveConnections(mc.Name, mc.Name+"-root")
+		machines, err := provider.GetAllMachinesAndRootfulness()
+		if err != nil {
+			return err
+		}
+		return connection.RemoveConnections(machines, mc.Name, mc.Name+"-root")
 	}
 	callbackFuncs.Add(cleanup)
 
@@ -584,7 +589,12 @@ 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
 	}
@@ -659,12 +669,17 @@ func Reset(mps []vmconfigs.VMProvider, opts machine.ResetOptions) error {
 		}
 		removeDirs = append(removeDirs, d)
 
+		machines, err := provider.GetAllMachinesAndRootfulness()
+		if err != nil {
+			return err
+		}
+
 		for _, mc := range mcs {
 			err := Stop(mc, p, d, true)
 			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)
 			}
diff --git a/pkg/machine/vmconfigs/machine.go b/pkg/machine/vmconfigs/machine.go
index bbd0f6cc9..f47fce845 100644
--- a/pkg/machine/vmconfigs/machine.go
+++ b/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)
 		}