Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

container: workdir resolution must consider symlink if explicitly configured #13399

Merged

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Mar 2, 2022

While resolving workdir we mostly create a workdir when stat
fails with ENOENT or ErrNotExist however following cases are not
true when user explicitly specifies a workdir while running using
--workdir which tells podman to only use workdir if its exists on
the container. Following configuration is implicity set with other
run mechanism like podman play kube

Problem with explicit --workdir or similar implicit config in podman play kube is that currently podman ignores the fact that workdir can also be
a symlink and actual link could be valid.

Hence following commit ensures that in such scenarios when a workdir
is not found and we cannot create a workdir podman must perform a
check to ensure that if workdir is a symlink and link is resolved
successfully and resolved link is present on the container then we
return as it is.

Docker performs a similar behavior.

Closes: #13346

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2022
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch 2 times, most recently from cc10e2a to 841dbf3 Compare March 2, 2022 10:00
@flouthoc
Copy link
Collaborator Author

flouthoc commented Mar 2, 2022

Failing reproducer without this PR would be

FROM alpine

RUN mkdir /tmp/destination
RUN ln -s /tmp/destination /tmp/link

WORKDIR /tmp/link

CMD ["echo", "hello"]
$ podman build -t test .
$ podman run --workdir /tmp/link test

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some inline comments.

Please also fix the commit message, the first line is too long

libpod/container_internal_linux.go Outdated Show resolved Hide resolved
libpod/container_internal_linux.go Outdated Show resolved Hide resolved
libpod/container_internal_linux.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch from 841dbf3 to dc26ed7 Compare March 2, 2022 10:28
@flouthoc flouthoc changed the title container: workdir resolution must consider symlink if explicitly configured using --workdir or implicity using others e.g podman play kube container: workdir resolution must consider symlink if explicitly configured using --workdir Mar 2, 2022
@flouthoc flouthoc changed the title container: workdir resolution must consider symlink if explicitly configured using --workdir container: workdir resolution must consider symlink if explicitly configured Mar 2, 2022
@flouthoc flouthoc requested a review from giuseppe March 2, 2022 10:32
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch from dc26ed7 to ffad3fc Compare March 2, 2022 10:38
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if the target is another symlink?

There is some duplication here we could probably avoid. What do you think about keep calling c.resolvePath() until the result is not a symlink (up to a limit to avoid loops)?

libpod/container_internal_linux.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch from ffad3fc to 58dbb61 Compare March 2, 2022 11:09
@flouthoc
Copy link
Collaborator Author

flouthoc commented Mar 2, 2022

what happens if the target is another symlink?

There is some duplication here we could probably avoid. What do you think about keep calling c.resolvePath() until the result is not a symlink (up to a limit to avoid loops)?

@giuseppe This is a valid concern, I have addressed this in new commit and can be verified with symlink chain.

FROM alpine

RUN mkdir /tmp/destination
RUN ln -s /tmp/destination /tmp/link
RUN ln -s /tmp/link /tmp/link2
RUN ln -s /tmp/link2 /tmp/link3

WORKDIR /tmp/link3

CMD ["echo", "hello"]
$ podman run --workdir /tmp/link3 test

@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch 2 times, most recently from 8a9a7d2 to 569256e Compare March 2, 2022 11:31
@flouthoc flouthoc requested a review from giuseppe March 2, 2022 11:33
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit, could we have this functionality in a separate function?

Beside that, LGTM

libpod/container_internal_linux.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch from 569256e to 93a3888 Compare March 2, 2022 12:15
…figured

While resolving `workdir` we mostly create a `workdir` when `stat`
fails with `ENOENT` or `ErrNotExist` however following cases are not
true when user explicitly specifies a `workdir` while `running` using
`--workdir` which tells `podman` to only use workdir if its exists on
the container. Following configuration is implicity set with other
`run` mechanism like `podman play kube`

Problem with explicit `--workdir` or similar implicit config in `podman play
kube` is that currently podman ignores the fact that workdir can also be
a `symlink` and actual `link` could be valid.

Hence following commit ensures that in such scenarios when a `workdir`
is not found and we cannot create a `workdir` podman must perform a
check to ensure that if `workdir` is a `symlink` and `link` is resolved
successfully and resolved link is present on the container then we
return as it is.

Docker performs a similar behviour.

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the resolve-workdir-symlink branch from 93a3888 to 0bd0ad5 Compare March 2, 2022 13:32
@rhatdan
Copy link
Member

rhatdan commented Mar 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit ed59b89 into containers:main Mar 2, 2022
@github-actions 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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

play kube with symlinked working directory in container
4 participants