make sure the workdir exists on container mount #9054

vrothberg · 2021-01-21T14:47:22Z

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.

Fixes: #9040
Signed-off-by: Valentin Rothberg [email protected]

openshift-ci-robot · 2021-01-21T14:47:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg · 2021-01-21T14:47:32Z

@mheon PTAL

vrothberg · 2021-01-21T14:47:50Z

@edsantiago PTAL

edsantiago

LGTM, with one suggestion. I'm always trying to encourage people to piggyback on existing tests. Every second we save is valuable, and podman build takes a long long time.

test/system/030-run.bats

edsantiago · 2021-01-21T14:54:29Z

Thinking back... wasn't there a good reason why podman decided not to implicitly create workdirs? @mheon @rhatdan @baude ?

mheon · 2021-01-21T14:57:37Z

I'm a little reluctant here - @giuseppe made this change in crun with the intention that user-specified workdirs that do not exist would cause errors.

mheon · 2021-01-21T14:58:52Z

@edsantiago Logic was to catch user error - a workdir not specified by the image, that does not exist, is probably a typo.

edsantiago · 2021-01-21T15:00:30Z

Thanks @mheon. I remember that now.

Whichever way we go, we need to embed that decision in the code. If we go this route, please document it. If we reject this PR, I will add a system test that explicitly checks for failure and explains the reason in a test comment.

vrothberg · 2021-01-21T15:14:32Z

Notes from the meeting: Consensus was on only creating the workdir if specified by the image. When specified by the user, Podman should error out if it does not exist.

vrothberg · 2021-01-21T16:51:11Z

Updated. PTanotherL

vrothberg · 2021-01-21T17:36:27Z

Integration failure looks legit. I will have a look tomorrow morning.

giuseppe · 2021-01-22T07:50:27Z

libpod/container_internal_linux.go

@@ -174,24 +174,35 @@ func (c *Container) prepare() error {
 		return err
 	}

-	// Ensure container entrypoint is created (if required)
+	workdir := c.WorkingDir()
+	resolvedWorkdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir())


how would it work with volumes?

if we have something like podman run -v /foo:/foo -w /foo/bar ... the working dir must be looked up in the volume mount

Very good point.

To make this work, we can reuse the copy code in pkg/domain/infra/abi/containers_stat.go and move it into libpod. But I think that should be a separate PR.

@mheon WDYT?

Actually, we need to tackle this now to make system tests happy.

Would your intention be to replace the copy-up code wholesale, or just move the bit that determines where the path in a container lives?

Just the latter.

rhatdan · 2021-01-22T09:59:17Z

Just out of curiosity, could you try this with a --read-only container?

vrothberg · 2021-01-22T11:57:57Z

Just out of curiosity, could you try this with a --read-only container?

# With --workdir
podman (fix-9040) $ ./bin/podman run  --rm --workdir=/foo/bar --read-only alpine pwd
Error: workdir "/foo/bar" does not exist on container a94d26e8b4fe7d29ca49395b4c9ff0edcdfa25010257642f749530b92d9a4e4d

# With workdir from the image
podman (fix-9040) $ ./bin/podman run  --rm --read-only test pwd
/i/do/not/exist

rhatdan · 2021-01-22T14:14:08Z

Excellent.

libpod/container_internal_linux.go

vrothberg · 2021-01-25T16:56:37Z

Merge me :) (assuming I am pleasing)

mheon · 2021-01-25T17:01:55Z

libpod/container_api.go

+// resolve to the container's mount point or to a volume or bind mount.  It
+// returns the root (e.g., container, volume or bind mount) and the fully
+// resolved path which is either equal to or a subdirectory of the root.
+func (c *Container) ResolvePath(ctx context.Context, path string) (string, string, error) {


This should do the standard lock + sync if it's a public function

It's safe to use concurrently/

Or shall I lock and then mount and unmount?

My concern isn't really concurrency so much as removed containers - we could have the container removed out from under us midway through this, between Mount and Unmount, for example. Preference would be lock then mount() / unmount()

Although, looking at it more, I think a lot of the logic in Unmount() isn't duplicated in unmount()... I don't think it can be safely moved in there, either (we have two cases for unmounting - cases like this, where an external user wants to access the container's root filesystem - the code in Unmount() works there - and cases where Libpod wants to stop the container - the code in Unmount works there).

@vrothberg What would you think about making ResolvePath() be contingent on the container already being mounted? It seems like it would simplify the code, and potentially make things a bit more performant (we wouldn't need to call Mount more than once in the Copy code).

@mheon sounds good to me. One early version of the change was exactly like that but I preferred to play it safe. I'll go as suggested and repush.

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 <[email protected]>

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: containers#9040 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2021-01-26T10:39:41Z

Ready to go

vrothberg · 2021-01-26T10:40:16Z

Marking as backport for 3.0. Please raise your voice in case you have concerns.

rhatdan · 2021-01-26T11:53:20Z

I want @mheon to approve and merge.

giuseppe · 2021-01-26T13:40:33Z

libpod/container_path_resolution.go

+		if m.Type != "bind" {
+			continue
+		}


to check for a bind mount, we'd need to look for bind or rbind in the options, the type is ignored by the runtime

I think we require that Type also be set for Libpod - at the very least, all of our existing code is guaranteed to do so.

Currently, i's detecting correctly when using --mount=type=bind etc. It's pretty the code from cp.

If there's something missing, let's fix that in a separate PR.

mheon · 2021-01-26T15:59:22Z

/lgtm

mheon · 2021-01-26T15:59:37Z

/hold

mheon · 2021-01-26T15:59:51Z

Oops, I think I was the only LGTM... Need one more to merge

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2021

edsantiago reviewed Jan 21, 2021

View reviewed changes

test/system/030-run.bats Outdated Show resolved Hide resolved

vrothberg force-pushed the fix-9040 branch from 0fb90ba to ed4ca88 Compare January 21, 2021 16:50

giuseppe reviewed Jan 22, 2021

View reviewed changes

vrothberg force-pushed the fix-9040 branch 2 times, most recently from a56bc4f to 019791b Compare January 22, 2021 09:35

vrothberg force-pushed the fix-9040 branch from 019791b to 1a6d07c Compare January 22, 2021 12:21

TomSweeneyRedHat reviewed Jan 22, 2021

View reviewed changes

libpod/container_internal_linux.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Jan 22, 2021

View reviewed changes

libpod/container_internal_linux.go Show resolved Hide resolved

TomSweeneyRedHat reviewed Jan 22, 2021

View reviewed changes

libpod/container_internal_linux.go Show resolved Hide resolved

vrothberg force-pushed the fix-9040 branch 4 times, most recently from 2100b7e to 0a50092 Compare January 25, 2021 15:04

mheon reviewed Jan 25, 2021

View reviewed changes

vrothberg force-pushed the fix-9040 branch from 0a50092 to 0f668aa Compare January 26, 2021 08:04

vrothberg added the Backport to v3.0 label Jan 26, 2021

giuseppe reviewed Jan 26, 2021

View reviewed changes

openshift-ci-robot assigned mheon Jan 26, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2021

openshift-merge-robot merged commit ad1e0bb into containers:master Jan 26, 2021

vrothberg deleted the fix-9040 branch January 26, 2021 16:01

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

make sure the workdir exists on container mount #9054

make sure the workdir exists on container mount #9054

Conversation

vrothberg commented Jan 21, 2021 • edited Loading

openshift-ci-robot commented Jan 21, 2021

vrothberg commented Jan 21, 2021

vrothberg commented Jan 21, 2021

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago commented Jan 21, 2021

mheon commented Jan 21, 2021

mheon commented Jan 21, 2021

edsantiago commented Jan 21, 2021

vrothberg commented Jan 21, 2021

vrothberg commented Jan 21, 2021

vrothberg commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jan 22, 2021

vrothberg commented Jan 22, 2021 • edited Loading

rhatdan commented Jan 22, 2021

vrothberg commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Jan 26, 2021

vrothberg commented Jan 26, 2021

rhatdan commented Jan 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jan 26, 2021

mheon commented Jan 26, 2021

mheon commented Jan 26, 2021

vrothberg commented Jan 21, 2021 •

edited

Loading

vrothberg commented Jan 22, 2021 •

edited

Loading