From 0b500515d8b8900851763a17b61609556f58d99c Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 22 Jan 2021 15:38:51 +0100 Subject: libpod: add (*Container).ResolvePath() Add an API to libpod to resolve a path on the container. We can refactor the code that was originally written for copy. Other functions are requiring a proper path resolution, so libpod seems like a reasonable home for sharing that code. Signed-off-by: Valentin Rothberg --- pkg/domain/infra/abi/archive.go | 14 +++- pkg/domain/infra/abi/containers_stat.go | 140 ++------------------------------ 2 files changed, 20 insertions(+), 134 deletions(-) (limited to 'pkg') diff --git a/pkg/domain/infra/abi/archive.go b/pkg/domain/infra/abi/archive.go index 809813756..c64dfb02a 100644 --- a/pkg/domain/infra/abi/archive.go +++ b/pkg/domain/infra/abi/archive.go @@ -26,13 +26,18 @@ func (ic *ContainerEngine) ContainerCopyFromArchive(ctx context.Context, nameOrI return nil, err } + containerMountPoint, err := container.Mount() + if err != nil { + return nil, err + } + unmount := func() { if err := container.Unmount(false); err != nil { logrus.Errorf("Error unmounting container: %v", err) } } - _, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerPath) + _, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerMountPoint, containerPath) if err != nil { unmount() return nil, err @@ -71,6 +76,11 @@ func (ic *ContainerEngine) ContainerCopyToArchive(ctx context.Context, nameOrID return nil, err } + containerMountPoint, err := container.Mount() + if err != nil { + return nil, err + } + unmount := func() { if err := container.Unmount(false); err != nil { logrus.Errorf("Error unmounting container: %v", err) @@ -83,7 +93,7 @@ func (ic *ContainerEngine) ContainerCopyToArchive(ctx context.Context, nameOrID containerPath = "/." } - _, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerPath) + _, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerMountPoint, containerPath) if err != nil { unmount() return nil, err diff --git a/pkg/domain/infra/abi/containers_stat.go b/pkg/domain/infra/abi/containers_stat.go index 931e77026..f3d0799a0 100644 --- a/pkg/domain/infra/abi/containers_stat.go +++ b/pkg/domain/infra/abi/containers_stat.go @@ -10,18 +10,11 @@ import ( "github.com/containers/podman/v2/libpod" "github.com/containers/podman/v2/pkg/copy" "github.com/containers/podman/v2/pkg/domain/entities" - securejoin "github.com/cyphar/filepath-securejoin" - "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) -func (ic *ContainerEngine) containerStat(container *libpod.Container, containerPath string) (*entities.ContainerStatReport, string, string, error) { - containerMountPoint, err := container.Mount() - if err != nil { - return nil, "", "", err - } - +func (ic *ContainerEngine) containerStat(container *libpod.Container, containerMountPoint string, containerPath string) (*entities.ContainerStatReport, string, string, error) { // Make sure that "/" copies the *contents* of the mount point and not // the directory. if containerPath == "/" { @@ -30,7 +23,7 @@ func (ic *ContainerEngine) containerStat(container *libpod.Container, containerP // Now resolve the container's path. It may hit a volume, it may hit a // bind mount, it may be relative. - resolvedRoot, resolvedContainerPath, err := resolveContainerPaths(container, containerMountPoint, containerPath) + resolvedRoot, resolvedContainerPath, err := container.ResolvePath(context.Background(), containerMountPoint, containerPath) if err != nil { return nil, "", "", err } @@ -94,138 +87,21 @@ func (ic *ContainerEngine) ContainerStat(ctx context.Context, nameOrID string, c return nil, err } + containerMountPoint, err := container.Mount() + if err != nil { + return nil, err + } + defer func() { if err := container.Unmount(false); err != nil { logrus.Errorf("Error unmounting container: %v", err) } }() - statReport, _, _, err := ic.containerStat(container, containerPath) + statReport, _, _, err := ic.containerStat(container, containerMountPoint, containerPath) return statReport, err } -// resolveContainerPaths resolves the container's mount point and the container -// path as specified by the user. Both may resolve to paths outside of the -// container's mount point when the container path hits a volume or bind mount. -// -// NOTE: We must take volumes and bind mounts into account as, regrettably, we -// can copy to/from stopped containers. In that case, the volumes and bind -// mounts are not present. For running containers, the runtime (e.g., runc or -// crun) takes care of these mounts. For stopped ones, we need to do quite -// some dance, as done below. -func resolveContainerPaths(container *libpod.Container, mountPoint string, containerPath string) (string, string, error) { - // Let's first make sure we have a path relative to the mount point. - pathRelativeToContainerMountPoint := containerPath - if !filepath.IsAbs(containerPath) { - // If the containerPath is not absolute, it's relative to the - // container's working dir. To be extra careful, let's first - // join the working dir with "/", and the add the containerPath - // to it. - pathRelativeToContainerMountPoint = filepath.Join(filepath.Join("/", container.WorkingDir()), containerPath) - } - resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint) - pathRelativeToContainerMountPoint = strings.TrimPrefix(pathRelativeToContainerMountPoint, mountPoint) - pathRelativeToContainerMountPoint = filepath.Join("/", pathRelativeToContainerMountPoint) - - // Now we have an "absolute container Path" but not yet resolved on the - // host (e.g., "/foo/bar/file.txt"). As mentioned above, we need to - // check if "/foo/bar/file.txt" is on a volume or bind mount. To do - // that, we need to walk *down* the paths to the root. Assuming - // volume-1 is mounted to "/foo" and volume-2 is mounted to "/foo/bar", - // we must select "/foo/bar". Once selected, we need to rebase the - // remainder (i.e, "/file.txt") on the volume's mount point on the - // host. Same applies to bind mounts. - - searchPath := pathRelativeToContainerMountPoint - for { - volume, err := findVolume(container, searchPath) - if err != nil { - return "", "", err - } - if volume != nil { - logrus.Debugf("Container path %q resolved to volume %q on path %q", containerPath, volume.Name(), searchPath) - - // TODO: We really need to force the volume to mount - // before doing this, but that API is not exposed - // externally right now and doing so is beyond the scope - // of this commit. - mountPoint, err := volume.MountPoint() - if err != nil { - return "", "", err - } - if mountPoint == "" { - return "", "", errors.Errorf("volume %s is not mounted, cannot copy into it", volume.Name()) - } - - // We found a matching volume for searchPath. We now - // need to first find the relative path of our input - // path to the searchPath, and then join it with the - // volume's mount point. - pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath) - absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume) - if err != nil { - return "", "", err - } - return mountPoint, absolutePathOnTheVolumeMount, nil - } - - if mount := findBindMount(container, searchPath); mount != nil { - logrus.Debugf("Container path %q resolved to bind mount %q:%q on path %q", containerPath, mount.Source, mount.Destination, searchPath) - // We found a matching bind mount for searchPath. We - // now need to first find the relative path of our - // input path to the searchPath, and then join it with - // the source of the bind mount. - pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath) - absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount) - if err != nil { - return "", "", err - } - return mount.Source, absolutePathOnTheBindMount, nil - - } - - if searchPath == "/" { - // Cannot go beyond "/", so we're done. - break - } - - // Walk *down* the path (e.g., "/foo/bar/x" -> "/foo/bar"). - searchPath = filepath.Dir(searchPath) - } - - // No volume, no bind mount but just a normal path on the container. - return mountPoint, resolvedPathOnTheContainerMountPoint, nil -} - -// findVolume checks if the specified container path matches a volume inside -// the container. It returns a matching volume or nil. -func findVolume(c *libpod.Container, containerPath string) (*libpod.Volume, error) { - runtime := c.Runtime() - cleanedContainerPath := filepath.Clean(containerPath) - for _, vol := range c.Config().NamedVolumes { - if cleanedContainerPath == filepath.Clean(vol.Dest) { - return runtime.GetVolume(vol.Name) - } - } - return nil, nil -} - -// findBindMount checks if the specified container path matches a bind mount -// inside the container. It returns a matching mount or nil. -func findBindMount(c *libpod.Container, containerPath string) *specs.Mount { - cleanedPath := filepath.Clean(containerPath) - for _, m := range c.Config().Spec.Mounts { - if m.Type != "bind" { - continue - } - if cleanedPath == filepath.Clean(m.Destination) { - mount := m - return &mount - } - } - return nil -} - // secureStat extracts file info for path in a chroot'ed environment in root. func secureStat(root string, path string) (*buildahCopiah.StatForItem, error) { var glob string -- cgit v1.2.3-54-g00ecf From c48753bfa0d56b855f71dc6ad62e8eff62cf98e1 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 21 Jan 2021 15:41:14 +0100 Subject: workdir presence checks A container's workdir can be specified via the CLI via `--workdir` and via an image config with the CLI having precedence. Since images have a tendency to specify workdirs without necessarily shipping the paths with the root FS, make sure that Podman creates the workdir. When specified via the CLI, do not create the path, but check for its existence and return a human-friendly error. NOTE: `crun` is performing a similar check that would yield exit code 127. With this change, however, Podman performs the check and yields exit code 126. Since this is specific to `crun`, I do not consider it to be a breaking change of Podman. Fixes: #9040 Signed-off-by: Valentin Rothberg --- libpod/container_internal_linux.go | 63 +++++++++++++++++++++++++------- libpod/container_path_resolution.go | 47 ++++++++++++++++++++++-- pkg/specgen/generate/container.go | 14 ------- pkg/specgen/generate/container_create.go | 12 +++++- test/e2e/run_working_dir_test.go | 6 +-- test/system/030-run.bats | 21 +++++++++++ test/system/070-build.bats | 4 +- 7 files changed, 127 insertions(+), 40 deletions(-) (limited to 'pkg') diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 0553cc59c..9c5989d23 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -174,25 +174,60 @@ func (c *Container) prepare() error { return err } - // Ensure container entrypoint is created (if required) - if c.config.CreateWorkingDir { - workdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir()) - if err != nil { - return errors.Wrapf(err, "error creating path to container %s working dir", c.ID()) - } - rootUID := c.RootUID() - rootGID := c.RootGID() + // Make sure the workdir exists + if err := c.resolveWorkDir(); err != nil { + return err + } - if err := os.MkdirAll(workdir, 0755); err != nil { - if os.IsExist(err) { - return nil + return nil +} + +// resolveWorkDir resolves the container's workdir and, depending on the +// configuration, will create it, or error out if it does not exist. +// Note that the container must be mounted before. +func (c *Container) resolveWorkDir() error { + workdir := c.WorkingDir() + + // If the specified workdir is a subdir of a volume or mount, + // we don't need to do anything. The runtime is taking care of + // that. + if isPathOnVolume(c, workdir) || isPathOnBindMount(c, workdir) { + logrus.Debugf("Workdir %q resolved to a volume or mount", workdir) + return nil + } + + _, resolvedWorkdir, err := c.resolvePath(c.state.Mountpoint, workdir) + if err != nil { + return err + } + logrus.Debugf("Workdir %q resolved to host path %q", workdir, resolvedWorkdir) + + // No need to create it (e.g., `--workdir=/foo`), so let's make sure + // the path exists on the container. + if !c.config.CreateWorkingDir { + if _, err := os.Stat(resolvedWorkdir); err != nil { + if os.IsNotExist(err) { + return errors.Errorf("workdir %q does not exist on container %s", workdir, c.ID()) } - return errors.Wrapf(err, "error creating container %s working dir", c.ID()) + // This might be a serious error (e.g., permission), so + // we need to return the full error. + return errors.Wrapf(err, "error detecting workdir %q on container %s", workdir, c.ID()) } + } + + // Ensure container entrypoint is created (if required). + rootUID := c.RootUID() + rootGID := c.RootGID() - if err := os.Chown(workdir, rootUID, rootGID); err != nil { - return errors.Wrapf(err, "error chowning container %s working directory to container root", c.ID()) + if err := os.MkdirAll(resolvedWorkdir, 0755); err != nil { + if os.IsExist(err) { + return nil } + return errors.Wrapf(err, "error creating container %s workdir", c.ID()) + } + + if err := os.Chown(resolvedWorkdir, rootUID, rootGID); err != nil { + return errors.Wrapf(err, "error chowning container %s workdir to container root", c.ID()) } return nil diff --git a/libpod/container_path_resolution.go b/libpod/container_path_resolution.go index 68f174278..805b3b947 100644 --- a/libpod/container_path_resolution.go +++ b/libpod/container_path_resolution.go @@ -13,6 +13,11 @@ import ( // resolveContainerPaths resolves the container's mount point and the container // path as specified by the user. Both may resolve to paths outside of the // container's mount point when the container path hits a volume or bind mount. +// +// It returns a bool, indicating whether containerPath resolves outside of +// mountPoint (e.g., via a mount or volume), the resolved root (e.g., container +// mount, bind mount or volume) and the resolved path on the root (absolute to +// the host). func (container *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) { // Let's first make sure we have a path relative to the mount point. pathRelativeToContainerMountPoint := containerPath @@ -97,8 +102,8 @@ func (container *Container) resolvePath(mountPoint string, containerPath string) return mountPoint, resolvedPathOnTheContainerMountPoint, nil } -// findVolume checks if the specified container path matches a volume inside -// the container. It returns a matching volume or nil. +// findVolume checks if the specified containerPath matches the destination +// path of a Volume. Returns a matching Volume or nil. func findVolume(c *Container, containerPath string) (*Volume, error) { runtime := c.Runtime() cleanedContainerPath := filepath.Clean(containerPath) @@ -110,8 +115,25 @@ func findVolume(c *Container, containerPath string) (*Volume, error) { return nil, nil } -// findBindMount checks if the specified container path matches a bind mount -// inside the container. It returns a matching mount or nil. +// isPathOnVolume returns true if the specified containerPath is a subdir of any +// Volume's destination. +func isPathOnVolume(c *Container, containerPath string) bool { + cleanedContainerPath := filepath.Clean(containerPath) + for _, vol := range c.Config().NamedVolumes { + if cleanedContainerPath == filepath.Clean(vol.Dest) { + return true + } + for dest := vol.Dest; dest != "/"; dest = filepath.Dir(dest) { + if cleanedContainerPath == dest { + return true + } + } + } + return false +} + +// findBindMounts checks if the specified containerPath matches the destination +// path of a Mount. Returns a matching Mount or nil. func findBindMount(c *Container, containerPath string) *specs.Mount { cleanedPath := filepath.Clean(containerPath) for _, m := range c.Config().Spec.Mounts { @@ -125,3 +147,20 @@ func findBindMount(c *Container, containerPath string) *specs.Mount { } return nil } + +/// isPathOnBindMount returns true if the specified containerPath is a subdir of any +// Mount's destination. +func isPathOnBindMount(c *Container, containerPath string) bool { + cleanedContainerPath := filepath.Clean(containerPath) + for _, m := range c.Config().Spec.Mounts { + if cleanedContainerPath == filepath.Clean(m.Destination) { + return true + } + for dest := m.Destination; dest != "/"; dest = filepath.Dir(dest) { + if cleanedContainerPath == dest { + return true + } + } + } + return false +} diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index cc3f7928c..31d317bf8 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -203,20 +203,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat } s.Annotations = annotations - // workdir - if s.WorkDir == "" { - if newImage != nil { - workingDir, err := newImage.WorkingDir(ctx) - if err != nil { - return nil, err - } - s.WorkDir = workingDir - } - } - if s.WorkDir == "" { - s.WorkDir = "/" - } - if len(s.SeccompProfilePath) < 1 { p, err := libpod.DefaultSeccompPath() if err != nil { diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 4f36744ca..1bc050b00 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -272,10 +272,18 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen. if s.Entrypoint != nil { options = append(options, libpod.WithEntrypoint(s.Entrypoint)) } - // If the user did not set an workdir but the image did, ensure it is - // created. + // If the user did not specify a workdir on the CLI, let's extract it + // from the image. if s.WorkDir == "" && img != nil { options = append(options, libpod.WithCreateWorkingDir()) + wd, err := img.WorkingDir(ctx) + if err != nil { + return nil, err + } + s.WorkDir = wd + } + if s.WorkDir == "" { + s.WorkDir = "/" } if s.StopSignal != nil { options = append(options, libpod.WithStopSignal(*s.StopSignal)) diff --git a/test/e2e/run_working_dir_test.go b/test/e2e/run_working_dir_test.go index 7d8db361c..59538448e 100644 --- a/test/e2e/run_working_dir_test.go +++ b/test/e2e/run_working_dir_test.go @@ -2,7 +2,6 @@ package integration import ( "os" - "strings" . "github.com/containers/podman/v2/test/utils" . "github.com/onsi/ginkgo" @@ -41,12 +40,9 @@ var _ = Describe("Podman run", func() { }) It("podman run a container using non existing --workdir", func() { - if !strings.Contains(podmanTest.OCIRuntime, "crun") { - Skip("Test only works on crun") - } session := podmanTest.Podman([]string{"run", "--workdir", "/home/foobar", ALPINE, "pwd"}) session.WaitWithDefaultTimeout() - Expect(session.ExitCode()).To(Equal(127)) + Expect(session.ExitCode()).To(Equal(126)) }) It("podman run a container on an image with a workdir", func() { diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 29dc95dc3..dcf1da370 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -589,4 +589,25 @@ json-file | f is "${lines[1]}" "$rand" "Container runs successfully despite warning" } +@test "podman run - check workdir" { + # Workdirs specified via the CLI are not created on the root FS. + run_podman 126 run --rm --workdir /i/do/not/exist $IMAGE pwd + # Note: remote error prepends an attach error. + is "$output" "Error: .*workdir \"/i/do/not/exist\" does not exist on container.*" + + testdir=$PODMAN_TMPDIR/volume + mkdir -p $testdir + randomcontent=$(random_string 10) + echo "$randomcontent" > $testdir/content + + # Workdir does not exist on the image but is volume mounted. + run_podman run --rm --workdir /IamNotOnTheImage -v $testdir:/IamNotOnTheImage $IMAGE cat content + is "$output" "$randomcontent" "cat random content" + + # Workdir does not exist on the image but is created by the runtime as it's + # a subdir of a volume. + run_podman run --rm --workdir /IamNotOntheImage -v $testdir/content:/IamNotOntheImage/foo $IMAGE cat foo + is "$output" "$randomcontent" "cat random content" +} + # vim: filetype=sh diff --git a/test/system/070-build.bats b/test/system/070-build.bats index 0e83a184b..bf9fa789c 100644 --- a/test/system/070-build.bats +++ b/test/system/070-build.bats @@ -145,10 +145,12 @@ EOF https_proxy=https-proxy-in-env-file EOF + # NOTE: it's important to not create the workdir. + # Podman will make sure to create a missing workdir + # if needed. See #9040. cat >$tmpdir/Containerfile < Date: Mon, 25 Jan 2021 15:29:00 -0500 Subject: Ensure shutdown handler access is syncronized There was a potential race where two handlers could be added at the same time. Go Maps are not thread-safe, so that could do unpleasant things. Add a mutex to keep things safe. Also, swap the order or Register and Start for the handlers in Libpod runtime created. As written, there was a small gap between Start and Register where SIGTERM/SIGINT would be completely ignored, instead of stopping Podman. Swapping the two closes this gap. Signed-off-by: Matthew Heon --- libpod/runtime.go | 14 +++++++------- libpod/shutdown/handler.go | 10 ++++++++++ pkg/api/server/server.go | 8 ++++---- 3 files changed, 21 insertions(+), 11 deletions(-) (limited to 'pkg') diff --git a/libpod/runtime.go b/libpod/runtime.go index 34c737a67..0dc220b52 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -180,6 +180,13 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R } } + if err := shutdown.Register("libpod", func(sig os.Signal) error { + os.Exit(1) + return nil + }); err != nil && errors.Cause(err) != shutdown.ErrHandlerExists { + logrus.Errorf("Error registering shutdown handler for libpod: %v", err) + } + if err := shutdown.Start(); err != nil { return nil, errors.Wrapf(err, "error starting shutdown signal handler") } @@ -188,13 +195,6 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R return nil, err } - if err := shutdown.Register("libpod", func(sig os.Signal) error { - os.Exit(1) - return nil - }); err != nil && errors.Cause(err) != shutdown.ErrHandlerExists { - logrus.Errorf("Error registering shutdown handler for libpod: %v", err) - } - return runtime, nil } diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go index f0f228b19..ac1d33910 100644 --- a/libpod/shutdown/handler.go +++ b/libpod/shutdown/handler.go @@ -18,6 +18,8 @@ var ( stopped bool sigChan chan os.Signal cancelChan chan bool + // Syncronize accesses to the map + handlerLock sync.Mutex // Definitions of all on-shutdown handlers handlers map[string]func(os.Signal) error // Ordering that on-shutdown handlers will be invoked. @@ -50,6 +52,7 @@ func Start() error { case sig := <-sigChan: logrus.Infof("Received shutdown signal %v, terminating!", sig) shutdownInhibit.Lock() + handlerLock.Lock() for _, name := range handlerOrder { handler, ok := handlers[name] if !ok { @@ -61,6 +64,7 @@ func Start() error { logrus.Errorf("Error running shutdown handler %s: %v", name, err) } } + handlerLock.Unlock() shutdownInhibit.Unlock() return } @@ -97,6 +101,9 @@ func Uninhibit() { // by a signal. Handlers are invoked LIFO - the last handler registered is the // first run. func Register(name string, handler func(os.Signal) error) error { + handlerLock.Lock() + defer handlerLock.Unlock() + if handlers == nil { handlers = make(map[string]func(os.Signal) error) } @@ -113,6 +120,9 @@ func Register(name string, handler func(os.Signal) error) error { // Unregister un-registers a given shutdown handler. func Unregister(name string) error { + handlerLock.Lock() + defer handlerLock.Unlock() + if handlers == nil { return nil } diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go index 046f6561c..d612041f6 100644 --- a/pkg/api/server/server.go +++ b/pkg/api/server/server.go @@ -179,15 +179,15 @@ func setupSystemd() { func (s *APIServer) Serve() error { setupSystemd() - // Start the shutdown signal handler. - if err := shutdown.Start(); err != nil { - return err - } if err := shutdown.Register("server", func(sig os.Signal) error { return s.Shutdown() }); err != nil { return err } + // Start the shutdown signal handler. + if err := shutdown.Start(); err != nil { + return err + } errChan := make(chan error, 1) -- cgit v1.2.3-54-g00ecf From b111370306bca917d77d3b84352791f157e09983 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 20 Jan 2021 14:30:42 +0100 Subject: Make generate systemd --new robust against double curly braces If the container create command contains an argument with double curly braces the golang template parsing can fail since it tries to interpret the value as variable. To fix this change the default delimiter for the internal template to `{{{{`. Fixes #9034 Signed-off-by: Paul Holzinger --- pkg/systemd/generate/common.go | 12 ++++---- pkg/systemd/generate/containers.go | 48 +++++++++++++++--------------- pkg/systemd/generate/containers_test.go | 39 +++++++++++++++++++++++++ pkg/systemd/generate/pods.go | 52 ++++++++++++++++----------------- pkg/systemd/generate/pods_test.go | 43 +++++++++++++++++++++++++++ test/e2e/generate_systemd_test.go | 21 +++++++++++++ 6 files changed, 159 insertions(+), 56 deletions(-) (limited to 'pkg') diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go index 8901298db..de6751a17 100644 --- a/pkg/systemd/generate/common.go +++ b/pkg/systemd/generate/common.go @@ -30,14 +30,14 @@ func validateRestartPolicy(restart string) error { return errors.Errorf("%s is not a valid restart policy", restart) } -const headerTemplate = `# {{.ServiceName}}.service -# autogenerated by Podman {{.PodmanVersion}} -{{- if .TimeStamp}} -# {{.TimeStamp}} -{{- end}} +const headerTemplate = `# {{{{.ServiceName}}}}.service +# autogenerated by Podman {{{{.PodmanVersion}}}} +{{{{- if .TimeStamp}}}} +# {{{{.TimeStamp}}}} +{{{{- end}}}} [Unit] -Description=Podman {{.ServiceName}}.service +Description=Podman {{{{.ServiceName}}}}.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go index b64b2593c..5f52b0a77 100644 --- a/pkg/systemd/generate/containers.go +++ b/pkg/systemd/generate/containers.go @@ -72,22 +72,22 @@ type containerInfo struct { } const containerTemplate = headerTemplate + ` -{{- if .BoundToServices}} -BindsTo={{- range $index, $value := .BoundToServices -}}{{if $index}} {{end}}{{ $value }}.service{{end}} -After={{- range $index, $value := .BoundToServices -}}{{if $index}} {{end}}{{ $value }}.service{{end}} -{{- end}} +{{{{- if .BoundToServices}}}} +BindsTo={{{{- range $index, $value := .BoundToServices -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}.service{{{{end}}}} +After={{{{- range $index, $value := .BoundToServices -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}.service{{{{end}}}} +{{{{- end}}}} [Service] -Environment={{.EnvVariable}}=%n -Restart={{.RestartPolicy}} -TimeoutStopSec={{.TimeoutStopSec}} -{{- if .ExecStartPre}} -ExecStartPre={{.ExecStartPre}} -{{- end}} -ExecStart={{.ExecStart}} -ExecStop={{.ExecStop}} -ExecStopPost={{.ExecStopPost}} -PIDFile={{.PIDFile}} +Environment={{{{.EnvVariable}}}}=%n +Restart={{{{.RestartPolicy}}}} +TimeoutStopSec={{{{.TimeoutStopSec}}}} +{{{{- if .ExecStartPre}}}} +ExecStartPre={{{{.ExecStartPre}}}} +{{{{- end}}}} +ExecStart={{{{.ExecStart}}}} +ExecStop={{{{.ExecStop}}}} +ExecStopPost={{{{.ExecStopPost}}}} +PIDFile={{{{.PIDFile}}}} Type=forking [Install] @@ -173,9 +173,9 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst } info.EnvVariable = EnvVariable - info.ExecStart = "{{.Executable}} start {{.ContainerNameOrID}}" - info.ExecStop = "{{.Executable}} stop {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}} {{.ContainerNameOrID}}" - info.ExecStopPost = "{{.Executable}} stop {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}} {{.ContainerNameOrID}}" + info.ExecStart = "{{{{.Executable}}}} start {{{{.ContainerNameOrID}}}}" + info.ExecStop = "{{{{.Executable}}}} stop {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} {{{{.ContainerNameOrID}}}}" + info.ExecStopPost = "{{{{.Executable}}}} stop {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} {{{{.ContainerNameOrID}}}}" // Assemble the ExecStart command when creating a new container. // @@ -209,8 +209,8 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst } startCommand = append(startCommand, "run", - "--conmon-pidfile", "{{.PIDFile}}", - "--cidfile", "{{.ContainerIDFile}}", + "--conmon-pidfile", "{{{{.PIDFile}}}}", + "--cidfile", "{{{{.ContainerIDFile}}}}", "--cgroups=no-conmon", ) // If the container is in a pod, make sure that the @@ -281,10 +281,10 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst startCommand = append(startCommand, remainingCmd...) startCommand = quoteArguments(startCommand) - info.ExecStartPre = "/bin/rm -f {{.PIDFile}} {{.ContainerIDFile}}" + info.ExecStartPre = "/bin/rm -f {{{{.PIDFile}}}} {{{{.ContainerIDFile}}}}" info.ExecStart = strings.Join(startCommand, " ") - info.ExecStop = "{{.Executable}} {{if .RootFlags}}{{ .RootFlags}} {{end}}stop --ignore --cidfile {{.ContainerIDFile}} {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}}" - info.ExecStopPost = "{{.Executable}} {{if .RootFlags}}{{ .RootFlags}} {{end}}rm --ignore -f --cidfile {{.ContainerIDFile}}" + info.ExecStop = "{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}stop --ignore --cidfile {{{{.ContainerIDFile}}}} {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}}" + info.ExecStopPost = "{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}rm --ignore -f --cidfile {{{{.ContainerIDFile}}}}" } info.TimeoutStopSec = minTimeoutStopSec + info.StopTimeout @@ -307,7 +307,7 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst // generation. That's especially needed for embedding the PID and ID // files in other fields which will eventually get replaced in the 2nd // template execution. - templ, err := template.New("container_template").Parse(containerTemplate) + templ, err := template.New("container_template").Delims("{{{{", "}}}}").Parse(containerTemplate) if err != nil { return "", errors.Wrap(err, "error parsing systemd service template") } @@ -318,7 +318,7 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst } // Now parse the generated template (i.e., buf) and execute it. - templ, err = template.New("container_template").Parse(buf.String()) + templ, err = template.New("container_template").Delims("{{{{", "}}}}").Parse(buf.String()) if err != nil { return "", errors.Wrap(err, "error parsing systemd service template") } diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go index c8e65bfe3..96d95644b 100644 --- a/pkg/systemd/generate/containers_test.go +++ b/pkg/systemd/generate/containers_test.go @@ -329,6 +329,29 @@ Type=forking WantedBy=multi-user.target default.target ` + goodNewWithJournaldTag := `# jadda-jadda.service +# autogenerated by Podman CI + +[Unit] +Description=Podman jadda-jadda.service +Documentation=man:podman-generate-systemd(1) +Wants=network.target +After=network-online.target + +[Service] +Environment=PODMAN_SYSTEMD_UNIT=%n +Restart=always +TimeoutStopSec=70 +ExecStartPre=/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id +ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon -d --replace --name test --log-driver=journald --log-opt=tag={{.Name}} awesome-image:latest +ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10 +ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id +PIDFile=%t/jadda-jadda.pid +Type=forking + +[Install] +WantedBy=multi-user.target default.target +` tests := []struct { name string info containerInfo @@ -608,6 +631,22 @@ WantedBy=multi-user.target default.target true, false, }, + {"good with journald log tag (see #9034)", + containerInfo{ + Executable: "/usr/bin/podman", + ServiceName: "jadda-jadda", + ContainerNameOrID: "jadda-jadda", + RestartPolicy: "always", + PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", + StopTimeout: 10, + PodmanVersion: "CI", + CreateCommand: []string{"I'll get stripped", "create", "--name", "test", "--log-driver=journald", "--log-opt=tag={{.Name}}", "awesome-image:latest"}, + EnvVariable: EnvVariable, + }, + goodNewWithJournaldTag, + true, + false, + }, } for _, tt := range tests { test := tt diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go index 7678a240f..c7e3aa955 100644 --- a/pkg/systemd/generate/pods.go +++ b/pkg/systemd/generate/pods.go @@ -72,23 +72,23 @@ type podInfo struct { ExecStopPost string } -const podTemplate = headerTemplate + `Requires={{- range $index, $value := .RequiredServices -}}{{if $index}} {{end}}{{ $value }}.service{{end}} -Before={{- range $index, $value := .RequiredServices -}}{{if $index}} {{end}}{{ $value }}.service{{end}} +const podTemplate = headerTemplate + `Requires={{{{- range $index, $value := .RequiredServices -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}.service{{{{end}}}} +Before={{{{- range $index, $value := .RequiredServices -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}.service{{{{end}}}} [Service] -Environment={{.EnvVariable}}=%n -Restart={{.RestartPolicy}} -TimeoutStopSec={{.TimeoutStopSec}} -{{- if .ExecStartPre1}} -ExecStartPre={{.ExecStartPre1}} -{{- end}} -{{- if .ExecStartPre2}} -ExecStartPre={{.ExecStartPre2}} -{{- end}} -ExecStart={{.ExecStart}} -ExecStop={{.ExecStop}} -ExecStopPost={{.ExecStopPost}} -PIDFile={{.PIDFile}} +Environment={{{{.EnvVariable}}}}=%n +Restart={{{{.RestartPolicy}}}} +TimeoutStopSec={{{{.TimeoutStopSec}}}} +{{{{- if .ExecStartPre1}}}} +ExecStartPre={{{{.ExecStartPre1}}}} +{{{{- end}}}} +{{{{- if .ExecStartPre2}}}} +ExecStartPre={{{{.ExecStartPre2}}}} +{{{{- end}}}} +ExecStart={{{{.ExecStart}}}} +ExecStop={{{{.ExecStop}}}} +ExecStopPost={{{{.ExecStopPost}}}} +PIDFile={{{{.PIDFile}}}} Type=forking [Install] @@ -236,9 +236,9 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) } info.EnvVariable = EnvVariable - info.ExecStart = "{{.Executable}} start {{.InfraNameOrID}}" - info.ExecStop = "{{.Executable}} stop {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}} {{.InfraNameOrID}}" - info.ExecStopPost = "{{.Executable}} stop {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}} {{.InfraNameOrID}}" + info.ExecStart = "{{{{.Executable}}}} start {{{{.InfraNameOrID}}}}" + info.ExecStop = "{{{{.Executable}}}} stop {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} {{{{.InfraNameOrID}}}}" + info.ExecStopPost = "{{{{.Executable}}}} stop {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} {{{{.InfraNameOrID}}}}" // Assemble the ExecStart command when creating a new pod. // @@ -278,8 +278,8 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) startCommand = append(startCommand, podRootArgs...) startCommand = append(startCommand, []string{"pod", "create", - "--infra-conmon-pidfile", "{{.PIDFile}}", - "--pod-id-file", "{{.PodIDFile}}"}...) + "--infra-conmon-pidfile", "{{{{.PIDFile}}}}", + "--pod-id-file", "{{{{.PodIDFile}}}}"}...) // Presence check for certain flags/options. fs := pflag.NewFlagSet("args", pflag.ContinueOnError) @@ -308,11 +308,11 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) startCommand = append(startCommand, podCreateArgs...) startCommand = quoteArguments(startCommand) - info.ExecStartPre1 = "/bin/rm -f {{.PIDFile}} {{.PodIDFile}}" + info.ExecStartPre1 = "/bin/rm -f {{{{.PIDFile}}}} {{{{.PodIDFile}}}}" info.ExecStartPre2 = strings.Join(startCommand, " ") - info.ExecStart = "{{.Executable}} {{if .RootFlags}}{{ .RootFlags}} {{end}}pod start --pod-id-file {{.PodIDFile}}" - info.ExecStop = "{{.Executable}} {{if .RootFlags}}{{ .RootFlags}} {{end}}pod stop --ignore --pod-id-file {{.PodIDFile}} {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}}" - info.ExecStopPost = "{{.Executable}} {{if .RootFlags}}{{ .RootFlags}} {{end}}pod rm --ignore -f --pod-id-file {{.PodIDFile}}" + info.ExecStart = "{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod start --pod-id-file {{{{.PodIDFile}}}}" + info.ExecStop = "{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod stop --ignore --pod-id-file {{{{.PodIDFile}}}} {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}}" + info.ExecStopPost = "{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod rm --ignore -f --pod-id-file {{{{.PodIDFile}}}}" } info.TimeoutStopSec = minTimeoutStopSec + info.StopTimeout @@ -334,7 +334,7 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) // generation. That's especially needed for embedding the PID and ID // files in other fields which will eventually get replaced in the 2nd // template execution. - templ, err := template.New("pod_template").Parse(podTemplate) + templ, err := template.New("pod_template").Delims("{{{{", "}}}}").Parse(podTemplate) if err != nil { return "", errors.Wrap(err, "error parsing systemd service template") } @@ -345,7 +345,7 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) } // Now parse the generated template (i.e., buf) and execute it. - templ, err = template.New("pod_template").Parse(buf.String()) + templ, err = template.New("pod_template").Delims("{{{{", "}}}}").Parse(buf.String()) if err != nil { return "", errors.Wrap(err, "error parsing systemd service template") } diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go index 1c6330160..2b430226b 100644 --- a/pkg/systemd/generate/pods_test.go +++ b/pkg/systemd/generate/pods_test.go @@ -139,6 +139,33 @@ ExecStopPost=/usr/bin/podman pod rm --ignore -f --pod-id-file %t/pod-123abc.pod- PIDFile=%t/pod-123abc.pid Type=forking +[Install] +WantedBy=multi-user.target default.target +` + + podNewLabelWithCurlyBraces := `# pod-123abc.service +# autogenerated by Podman CI + +[Unit] +Description=Podman pod-123abc.service +Documentation=man:podman-generate-systemd(1) +Wants=network.target +After=network-online.target +Requires=container-1.service container-2.service +Before=container-1.service container-2.service + +[Service] +Environment=PODMAN_SYSTEMD_UNIT=%n +Restart=on-failure +TimeoutStopSec=70 +ExecStartPre=/bin/rm -f %t/pod-123abc.pid %t/pod-123abc.pod-id +ExecStartPre=/usr/bin/podman pod create --infra-conmon-pidfile %t/pod-123abc.pid --pod-id-file %t/pod-123abc.pod-id --name foo --label key={{someval}} --replace +ExecStart=/usr/bin/podman pod start --pod-id-file %t/pod-123abc.pod-id +ExecStop=/usr/bin/podman pod stop --ignore --pod-id-file %t/pod-123abc.pod-id -t 10 +ExecStopPost=/usr/bin/podman pod rm --ignore -f --pod-id-file %t/pod-123abc.pod-id +PIDFile=%t/pod-123abc.pid +Type=forking + [Install] WantedBy=multi-user.target default.target ` @@ -230,6 +257,22 @@ WantedBy=multi-user.target default.target true, false, }, + {"pod --new with double curly braces", + podInfo{ + Executable: "/usr/bin/podman", + ServiceName: "pod-123abc", + InfraNameOrID: "jadda-jadda-infra", + RestartPolicy: "on-failure", + PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", + StopTimeout: 10, + PodmanVersion: "CI", + RequiredServices: []string{"container-1", "container-2"}, + CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "--label", "key={{someval}}"}, + }, + podNewLabelWithCurlyBraces, + true, + false, + }, } for _, tt := range tests { diff --git a/test/e2e/generate_systemd_test.go b/test/e2e/generate_systemd_test.go index be9727591..606d756b0 100644 --- a/test/e2e/generate_systemd_test.go +++ b/test/e2e/generate_systemd_test.go @@ -355,4 +355,25 @@ var _ = Describe("Podman generate systemd", func() { Expect(session.ExitCode()).To(Equal(0)) Expect(session.IsJSONOutputValid()).To(BeTrue()) }) + + It("podman generate systemd --new create command with double curly braces", func() { + // Regression test for #9034 + session := podmanTest.Podman([]string{"create", "--name", "foo", "--log-driver=journald", "--log-opt=tag={{.Name}}", ALPINE}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"generate", "systemd", "--new", "foo"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(session.OutputToString()).To(ContainSubstring(" --log-opt=tag={{.Name}} ")) + + session = podmanTest.Podman([]string{"pod", "create", "--name", "pod", "--label", "key={{someval}}"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"generate", "systemd", "--new", "pod"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(session.OutputToString()).To(ContainSubstring(" --label key={{someval}}")) + }) }) -- cgit v1.2.3-54-g00ecf