From a158eae7ff188102e54ec0a7ca570c3746fedefa Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 24 Jun 2024 17:11:56 +0200 Subject: [PATCH] podman run use pod userns even with --pod-id-file The pod was set after we checked the namespace and the namespace code only checked the --pod flag but didn't consider --pod-id-file option. As such fix the check to first set the pod option on the spec then use that for the namespace. Also make sure we always use an empty default otherwise it would be impossible in the backend to know if a user requested a specific userns or not, i.e. even in case of a set PODMAN_USERNS env a container should still get the userns from the pod and not use the var in this case. Therefore unset it from the default cli value. There are more issues here around --pod-id-file and cli validation that does not consider the option as conflicting with --userns like --pod does but I decided to fix the bug at hand and don't try to fix the entire mess which most likely would take days. Fixes #22931 Signed-off-by: Paul Holzinger --- cmd/podman/common/create.go | 4 +--- cmd/podman/kube/play.go | 2 +- pkg/specgenutil/specgen.go | 34 ++++++++++++++++++---------------- test/e2e/run_userns_test.go | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index 3d2dc42d59..6de40987ee 100644 --- a/cmd/podman/common/create.go +++ b/cmd/podman/common/create.go @@ -1,8 +1,6 @@ package common import ( - "os" - "github.com/containers/common/pkg/auth" "github.com/containers/common/pkg/completion" commonFlag "github.com/containers/common/pkg/flag" @@ -723,7 +721,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, usernsFlagName := "userns" createFlags.String( - usernsFlagName, os.Getenv("PODMAN_USERNS"), + usernsFlagName, "", "User namespace to use", ) _ = cmd.RegisterFlagCompletionFunc(usernsFlagName, AutocompleteUserNamespace) diff --git a/cmd/podman/kube/play.go b/cmd/podman/kube/play.go index 908e0077b0..eba54f0193 100644 --- a/cmd/podman/kube/play.go +++ b/cmd/podman/kube/play.go @@ -134,7 +134,7 @@ func playFlags(cmd *cobra.Command) { _ = cmd.RegisterFlagCompletionFunc(logOptFlagName, common.AutocompleteLogOpt) usernsFlagName := "userns" - flags.StringVar(&playOptions.Userns, usernsFlagName, os.Getenv("PODMAN_USERNS"), + flags.StringVar(&playOptions.Userns, usernsFlagName, "", "User namespace to use", ) _ = cmd.RegisterFlagCompletionFunc(usernsFlagName, common.AutocompleteUserNamespace) diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index 912193b453..c9dc0775d5 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -222,7 +222,8 @@ func setNamespaces(rtc *config.Config, s *specgen.SpecGenerator, c *entities.Con } } userns := c.UserNS - if userns == "" && c.Pod == "" { + // caller must make sure s.Pod is set before calling this function. + if userns == "" && s.Pod == "" { if ns, ok := os.LookupEnv("PODMAN_USERNS"); ok { userns = ns } else { @@ -388,6 +389,22 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions s.StartupHealthConfig.Successes = int(c.StartupHCSuccesses) } + if len(s.Pod) == 0 || len(c.Pod) > 0 { + s.Pod = c.Pod + } + + if len(c.PodIDFile) > 0 { + if len(s.Pod) > 0 { + return errors.New("cannot specify both --pod and --pod-id-file") + } + podID, err := ReadPodIDFile(c.PodIDFile) + if err != nil { + return err + } + s.Pod = podID + } + + // Important s.Pod must be set above here. if err := setNamespaces(rtc, s, c); err != nil { return err } @@ -408,21 +425,6 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions s.PublishExposedPorts = &c.PublishAll } - if len(s.Pod) == 0 || len(c.Pod) > 0 { - s.Pod = c.Pod - } - - if len(c.PodIDFile) > 0 { - if len(s.Pod) > 0 { - return errors.New("cannot specify both --pod and --pod-id-file") - } - podID, err := ReadPodIDFile(c.PodIDFile) - if err != nil { - return err - } - s.Pod = podID - } - expose, err := CreateExpose(c.Expose) if err != nil { return err diff --git a/test/e2e/run_userns_test.go b/test/e2e/run_userns_test.go index 311a6f1c41..811f0a4620 100644 --- a/test/e2e/run_userns_test.go +++ b/test/e2e/run_userns_test.go @@ -422,4 +422,41 @@ var _ = Describe("Podman UserNS support", func() { podmanTest.RestartRemoteService() } }) + + It("podman pod userns inherited for containers", func() { + podName := "testPod" + podIDFile := filepath.Join(podmanTest.TempDir, "podid") + podCreate := podmanTest.Podman([]string{"pod", "create", "--pod-id-file", podIDFile, "--uidmap", "0:0:1000", "--name", podName}) + podCreate.WaitWithDefaultTimeout() + Expect(podCreate).Should(ExitCleanly()) + + // The containers should not use PODMAN_USERNS as they must inherited the userns from the pod. + os.Setenv("PODMAN_USERNS", "keep-id") + defer os.Unsetenv("PODMAN_USERNS") + + expectedMapping := ` 0 0 1000 + 0 0 1000 +` + // rootless mapping is split in two ranges + if isRootless() { + expectedMapping = ` 0 0 1 + 1 1 999 + 0 0 1 + 1 1 999 +` + } + + session := podmanTest.Podman([]string{"run", "--pod", podName, ALPINE, "cat", "/proc/self/uid_map", "/proc/self/gid_map"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + output := string(session.Out.Contents()) + Expect(output).To(Equal(expectedMapping)) + + // https://github.com/containers/podman/issues/22931 + session = podmanTest.Podman([]string{"run", "--pod-id-file", podIDFile, ALPINE, "cat", "/proc/self/uid_map", "/proc/self/gid_map"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + output = string(session.Out.Contents()) + Expect(output).To(Equal(expectedMapping)) + }) })