run: use internal.GetTempDir with os.MkdirTemp for root and bundle path
#5084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[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 |
flouthoc
commented
Oct 11, 2023
| @@ -499,7 +504,7 @@ func setupSlirp4netnsNetwork(config *config.Config, netns, cid string, options [ | |||
| Mask: res.Subnet.Mask, | |||
| }} | |||
| netStatus := map[string]nettypes.StatusBlock{ | |||
| slirp4netns.BinaryName: nettypes.StatusBlock{ | |||
| slirp4netns.BinaryName: { | |||
There was a problem hiding this comment.
gofmt did this change and it not relevant to this PR
|
Ephemeral COPR build failed. @containers/packit-build please check. |
flouthoc
force-pushed
the
run-use-gettempdir
branch
from
da06ca9 to
66c0f74
Compare
flouthoc
changed the title
internal.GetTempDir instead of os.MkdirTemp for root and bundle pathinternal.GetTempDir with os.MkdirTemp for root and bundle path
flouthoc
force-pushed
the
run-use-gettempdir
branch
from
66c0f74 to
0e3f189
Compare
|
LGTM |
|
Does this also need to touch call sites in image.go and sshagent.go? |
Projects which are using buildah as a library and set `TMPDIR` manually can stumble upon a use-case where `TMPDIR` was set to a relative path. Such as `export TMPDIR=.` in such case buildah will try to create a temporary root using `Mkdirtemp` leading to a point where bundle is not generated correctly since path was relative. Following use case can be resolved by making sure that buildah always converts relative path to absolute path and `GetTempDir` does it well. Example reproducer with podman ```Dockerfile FROM alpine RUN echo hello ``` ```console export TMPDIR=. podman build --no-cache -t test . ``` Expected failure ```console STEP 1/2: FROM alpine STEP 2/2: RUN echo hello error running container: checking permissions on "buildah2341274198": stat buildah2341274198: no such file or directory ERRO[0000] did not get container create message from subprocess: EOF Error: building at STEP "RUN echo hello": while running runtime: exit status 1 ``` Closes: RHEL-2598 Signed-off-by: Aditya R <[email protected]>
flouthoc
force-pushed
the
run-use-gettempdir
branch
from
0e3f189 to
9c99d7c
Compare
|
@nalind Good point. Done. |
|
LGTM |
|
/hold |
|
LGTM |
|
/hold cancel |
|
Late for the review, but a very nice change @flouthoc |
flouthoc
added a commit
to flouthoc/podman
that referenced
this pull request
Oct 16, 2023
Test edge-case described here: containers/buildah#5084 Test case in: RHEL-2598 Signed-off-by: Aditya R <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Projects which are using buildah as a library and set
TMPDIRmanually can stumble upon a use-case whereTMPDIRwas set to a relative path.Such as
export TMPDIR=.in such case buildah will try to create a temporary root usingMkdirtempleading to a point where bundle is not generated correctly since path was relative.Following use case can be resolved by making sure that buildah always converts relative path to absolute path and
GetTempDirdoes it well.Example reproducer with podman
Expected failure
Closes: RHEL-2598
What type of PR is this?
What this PR does / why we need it:
How to verify it
Integration test cannot be added as it can be only reproduced by projects which are using buildah as a library ( such as podman ) however reproducer for podman is given to verify the bug.
Unit test added for
GetTempDirto make sure it is converting relative values toTMPDIRto absoluteWhich issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?