From 4b3890ccacb07f528d88636b1bf4988448f04195 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 21 Jun 2024 13:34:59 +0200 Subject: [PATCH] remote: fix incorrect CONTAINER_CONNECTION parsing When a user specifies a invalid connection in CONTAINER_CONNECTION then podman should return a proper error saying so. Currently it ignored the error and in rootFlags() just exited early with defining any flags. This caused a panic then when trying to use the flags later. In order to address this first store the connection error in the PodmanConfig struct and not abort right away during flag setup. This is important as the user might have specified a flag with a valid remote connection. As such we check all flags and only when none were given we return the connection error. Also while at it I noticed that the default connection reported via podman --help was wrong as it only used the old containers.conf field for it and did not consider the podman-connections.json default. New regression tests have been added to make sure it behaves correctly. This fixes the problem reported in the PR #22997. Signed-off-by: Paul Holzinger --- cmd/podman/root.go | 26 +++++++++++++++++--------- pkg/domain/entities/engine.go | 1 + test/system/272-system-connection.bats | 12 ++++++++++++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/cmd/podman/root.go b/cmd/podman/root.go index 57927ce670..a373a3aa18 100644 --- a/cmd/podman/root.go +++ b/cmd/podman/root.go @@ -175,6 +175,12 @@ func readRemoteCliFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) } case host.Changed: podmanConfig.URI = host.Value.String() + default: + // No cli options set, in case CONTAINER_CONNECTION was set to something + // invalid this contains the error, see setupRemoteConnection(). + // Important so that we can show a proper useful error message but still + // allow the cli overwrites (https://github.com/containers/podman/pull/22997). + return podmanConfig.ConnectionError } return nil } @@ -185,7 +191,8 @@ func readRemoteCliFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) // 2. Env variables (CONTAINER_HOST and CONTAINER_CONNECTION); // 3. ActiveService from containers.conf; // 4. RemoteURI; -func setupRemoteConnection(podmanConfig *entities.PodmanConfig) error { +// Returns the name of the default connection if any. +func setupRemoteConnection(podmanConfig *entities.PodmanConfig) string { conf := podmanConfig.ContainersConfDefaultsRO connEnv, hostEnv, sshkeyEnv := os.Getenv("CONTAINER_CONNECTION"), os.Getenv("CONTAINER_HOST"), os.Getenv("CONTAINER_SSHKEY") @@ -193,11 +200,13 @@ func setupRemoteConnection(podmanConfig *entities.PodmanConfig) error { case connEnv != "": con, err := conf.GetConnection(connEnv, false) if err != nil { - return err + podmanConfig.ConnectionError = err + return connEnv } podmanConfig.URI = con.URI podmanConfig.Identity = con.Identity podmanConfig.MachineMode = con.IsMachine + return con.Name case hostEnv != "": if sshkeyEnv != "" { podmanConfig.Identity = sshkeyEnv @@ -209,11 +218,11 @@ func setupRemoteConnection(podmanConfig *entities.PodmanConfig) error { podmanConfig.URI = con.URI podmanConfig.Identity = con.Identity podmanConfig.MachineMode = con.IsMachine - } else { - podmanConfig.URI = registry.DefaultAPIAddress() + return con.Name } + podmanConfig.URI = registry.DefaultAPIAddress() } - return nil + return "" } func persistentPreRunE(cmd *cobra.Command, args []string) error { @@ -463,9 +472,8 @@ func stdOutHook() { } func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) { - if err := setupRemoteConnection(podmanConfig); err != nil { - return - } + connectionName := setupRemoteConnection(podmanConfig) + lFlags := cmd.Flags() sshFlagName := "ssh" @@ -473,7 +481,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) { _ = cmd.RegisterFlagCompletionFunc(sshFlagName, common.AutocompleteSSH) connectionFlagName := "connection" - lFlags.StringP(connectionFlagName, "c", podmanConfig.ContainersConfDefaultsRO.Engine.ActiveService, "Connection to use for remote Podman service") + lFlags.StringP(connectionFlagName, "c", connectionName, "Connection to use for remote Podman service (CONTAINER_CONNECTION)") _ = cmd.RegisterFlagCompletionFunc(connectionFlagName, common.AutocompleteSystemConnections) urlFlagName := "url" diff --git a/pkg/domain/entities/engine.go b/pkg/domain/entities/engine.go index 688f3b4b8a..7fcbc64953 100644 --- a/pkg/domain/entities/engine.go +++ b/pkg/domain/entities/engine.go @@ -48,6 +48,7 @@ type PodmanConfig struct { Trace bool // Hidden: Trace execution URI string // URI to RESTful API Service FarmNodeName string // Name of farm node + ConnectionError error // Error when looking up the connection in setupRemoteConnection() Runroot string ImageStore string diff --git a/test/system/272-system-connection.bats b/test/system/272-system-connection.bats index f01d027fba..bc71cb14bb 100644 --- a/test/system/272-system-connection.bats +++ b/test/system/272-system-connection.bats @@ -230,6 +230,18 @@ $c2[ ]\+tcp://localhost:54321[ ]\+true[ ]\+true" \ CONTAINER_HOST=foo://124.com _run_podman_remote 125 --remote ps assert "$output" =~ "foo" "test env variable CONTAINER_HOST wrt config" + # There was a bug where this would panic instead of returning a proper error (#22997) + CONTAINER_CONNECTION=invalid-env _run_podman_remote 125 --remote ps + assert "$output" =~ "read cli flags: connection \"invalid-env\" not found" "connection error from env" + + # Check again with cli overwrite to ensure correct connection name in error is reported + CONTAINER_CONNECTION=invalid-env _run_podman_remote 125 --connection=invalid-cli ps + assert "$output" =~ "read cli flags: connection \"invalid-cli\" not found" "connection error from --connection cli" + + # Invalid env is fine if valid connection is given via cli + CONTAINER_CONNECTION=invalid-env _run_podman_remote 125 --connection=cli-override ps + assert "$output" =~ "/run/user/cli-override/podman/podman.sock" "no CONTAINER_CONNECTION connection error with valid --connection cli" + # Clean up run_podman system connection rm defaultconnection run_podman system connection rm env-override