From 54b71b637c2453a389f99946aea383344d0df34f Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Thu, 12 Sep 2024 14:31:55 -0400 Subject: [PATCH 1/6] Add `GetAllMachinesAndRootfulness` Adds the function `GetAllMachinesAndRootfulness` which creates a map of all podman machines, of any supported provider, on the system and whether it is rootful or not. Signed-off-by: Jake Correnti --- pkg/machine/provider/platform.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/machine/provider/platform.go b/pkg/machine/provider/platform.go index af2f3436a771..e04251896898 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 +} From e041f2a7b781ed93e18ef3db69eec4aeca50c18f Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Thu, 12 Sep 2024 14:40:24 -0400 Subject: [PATCH 2/6] Remove `RemoveFilesAndConnections` The `RemoveFilesAndConnections` function is not being used, so its safe to remove it and not carry unnecessary code that we need to maintain. Signed-off-by: Jake Correnti --- pkg/machine/connection/connection.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/pkg/machine/connection/connection.go b/pkg/machine/connection/connection.go index 469dadcb1e5d..58560df50762 100644 --- a/pkg/machine/connection/connection.go +++ b/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" @@ -106,18 +104,6 @@ func RemoveConnections(names ...string) error { }) } -// 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) - } - } - if err := RemoveConnections(names...); err != nil { - logrus.Error(err) - } -} - // makeSSHURL creates a URL from the given input func makeSSHURL(host, path, port, userName string) *url.URL { var hostname string From 9febd2c27ad6708f1b49326ab00758e499a66617 Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Tue, 24 Sep 2024 15:33:33 -0400 Subject: [PATCH 3/6] Move `DefaultMachineName` to `pkg/machine/define` Moves the `DefaultMachineName` constant out of `pkg/machine` and into `pkg/machine/define`. Signed-off-by: Jake Correnti --- cmd/podman/machine/init.go | 3 +-- cmd/podman/machine/os/manager.go | 4 ++-- pkg/machine/config.go | 5 +---- pkg/machine/define/config.go | 7 +++++-- pkg/machine/machine_common.go | 5 +++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/podman/machine/init.go b/cmd/podman/machine/init.go index 6f0579a97a03..69afdc02c6aa 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 1e028be23845..234246cf1dde 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/pkg/machine/config.go b/pkg/machine/config.go index 25a9aae69bd4..046b94b0f1c5 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/define/config.go b/pkg/machine/define/config.go index 90135f76fd48..b6c07342c15c 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/machine_common.go b/pkg/machine/machine_common.go index 1afc3d15b369..d2ba418b04fd 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 } From c709be3a2952226e40d7caba0ac89ffe1c28144c Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Thu, 12 Sep 2024 14:50:53 -0400 Subject: [PATCH 4/6] Simplify `RemoveConnections` Takes the code inside the closure in the function `RemoveConnections` and makes it a separate function to increase readability. Signed-off-by: Jake Correnti --- pkg/machine/connection/connection.go | 61 +++++++++++++++++++++------- 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/pkg/machine/connection/connection.go b/pkg/machine/connection/connection.go index 58560df50762..8ed9fdafc9ca 100644 --- a/pkg/machine/connection/connection.go +++ b/pkg/machine/connection/connection.go @@ -9,6 +9,7 @@ import ( "net/url" "github.com/containers/common/pkg/config" + "github.com/containers/podman/v5/pkg/machine/define" ) const LocalhostIP = "127.0.0.1" @@ -84,24 +85,56 @@ 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) - } + 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 + } + + return nil + }); err != nil { + return err + } - if cfg.Connection.Default == name { - cfg.Connection.Default = "" - } + return nil +} + +// 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) } - for service := range cfg.Connection.Connections { - cfg.Connection.Default = service - break + + if cfg.Connection.Default == name { + cfg.Connection.Default = "" } + } + + // 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 From 24deec835ca2367bee8fca2bb7898aedfbe1db0e Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Thu, 12 Sep 2024 15:03:03 -0400 Subject: [PATCH 5/6] Update connection on removal Modify `RemoveConnections` to verify the new default system connection's rootful state matches the rootful-ness of the podman machine it is associated with. Signed-off-by: Jake Correnti --- cmd/podman/system/reset_machine.go | 7 ++++++- pkg/machine/connection/connection.go | 30 ++++++++++++++++++---------- pkg/machine/shim/host.go | 21 ++++++++++++++++--- pkg/machine/vmconfigs/machine.go | 4 ++-- 4 files changed, 45 insertions(+), 17 deletions(-) diff --git a/cmd/podman/system/reset_machine.go b/cmd/podman/system/reset_machine.go index 35430dfb7d72..2c49e1758af3 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/connection/connection.go b/pkg/machine/connection/connection.go index 8ed9fdafc9ca..9c77ddeeffb6 100644 --- a/pkg/machine/connection/connection.go +++ b/pkg/machine/connection/connection.go @@ -75,26 +75,34 @@ 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 { +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 + } +} + +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 - } + err := setNewDefaultConnection(cfg, &dest, &service, names...) + if err != nil { + return err + } - return nil + rootful, ok := machines[service] + if dest.IsMachine && ok { + updateConnection(cfg, rootful, service, service+"-root") + } + return nil }); err != nil { return err } diff --git a/pkg/machine/shim/host.go b/pkg/machine/shim/host.go index e1c1834f60db..5664d6baf454 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/vmconfigs" "github.com/containers/podman/v5/utils" @@ -230,7 +231,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) @@ -580,7 +585,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 } @@ -655,12 +665,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 bbd0f6cc9be4..f47fce845fbd 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) } From e9b8564690d49c54a4d32c85161776e06bede250 Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Thu, 12 Sep 2024 15:08:25 -0400 Subject: [PATCH 6/6] Modify machine "Remove machine" test Modifies the "Remove machine" test to verify the system connections are handled properly on removal. Signed-off-by: Jake Correnti --- pkg/machine/e2e/rm_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/machine/e2e/rm_test.go b/pkg/machine/e2e/rm_test.go index 210e6ed09f8d..119c5b4b7ad4 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() {