Skip to content

bake: fix BAKE_CMD_CONTEXT relative path resolution#1840

Merged
jedevc merged 8 commits intodocker:masterfrom
jedevc:fix-check-path-for-bake-cmd-context
Jun 6, 2023
Merged

bake: fix BAKE_CMD_CONTEXT relative path resolution#1840
jedevc merged 8 commits intodocker:masterfrom
jedevc:fix-check-path-for-bake-cmd-context

Conversation

@jedevc
Copy link
Copy Markdown
Collaborator

@jedevc jedevc commented May 25, 2023

🛠️ Fixes path resolution issue with BAKE_CMD_CONTEXT.

In https://docs.docker.com/build/bake/remote-definition/, the following snippet no longer works:

$ docker buildx bake "https://github.com/tonistiigi/buildx.git#remote-test" --print
{
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "dockerfile-inline": "FROM alpine\nWORKDIR /src\nCOPY . .\nRUN ls -l \u0026\u0026 stop\n"
    }
  }
}

Instead returning the following error:

$ docker buildx bake "https://github.com/tonistiigi/buildx.git#remote-test" --print
ERROR: Rel: can't make . relative to /home/jedevc/Documents/Docker/buildx

This issue comes from

buildx/bake/bake.go

Lines 1007 to 1010 in 341fb65

rel, err := filepath.Rel(wd, p)
if err != nil {
return err
}

To resolve this issue, we need to ensure that the passed path is an absolute path, instead of the relative path ..

To prevent further regression, I add a couple of tests to build against a remote git repo (which is where most of the code in this PR comes from 😱).

cc @crazy-max @dvdksn

@jedevc jedevc requested review from crazy-max and tonistiigi May 25, 2023 13:07
@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from c7647f4 to 41c613d Compare May 25, 2023 14:31
@jedevc jedevc mentioned this pull request May 25, 2023
Merged
@jedevc jedevc added area/bake kind/bug Something isn't working labels May 25, 2023
@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch 2 times, most recently from 5296eaa to cf100e3 Compare May 25, 2023 16:55
@jedevc jedevc added this to the v0.11.0 milestone May 31, 2023
crazy-max
crazy-max reviewed Jun 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Can we have a test case for

buildx/bake/bake.go

Lines 1011 to 1013 in 7cef021

if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return errors.Errorf("path %s is outside of the working directory, please set BAKE_ALLOW_REMOTE_FS_ACCESS=1", p)
}
?

@jedevc
Copy link
Copy Markdown
Collaborator Author

jedevc commented Jun 6, 2023

Can we have a test case

@crazy-max sure, will work on this.

@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from efe200a to 537a643 Compare June 6, 2023 15:03
jedevc added 8 commits June 6, 2023 17:15
@jedevc
test: tmpdir can be a test helper
d03e93f 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
git: update gitutil test utilities
48d7daf 
- Adds a new GitServeHTTP function to start an http server to serve a
  target git repository.
- Adds a new GitDir helper method to get the path to the .git
  directory
- Updates the GitAdd method to take a variable number of files

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
tests: add basic remote bake context test
76c9634 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
bake: fix BAKE_CMD_CONTEXT relative path resolution
ef997fd 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
tests: add bake test for remote cmd context override
e859ebc 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
tests: add bake test for remote subdir context
61a7854 
Fixed in 12b6a3a, but now we have
regression tests! So we can add a check that we don't break this
behavior again.

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
tests: refactor cmd helpers to allow configuring cwd, etc
c820350 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
bake: fix potential context entitlements escape
d34103b 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from 537a643 to d34103b Compare June 6, 2023 15:19
tonistiigi
tonistiigi approved these changes Jun 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Iiuc the util/gitutil serves dual purposes atm and is used for reading VCS info for builds as well as test utilities. This isn't ideal and these responsibilities should be split. Not needed in this PR though.

@jedevc jedevc requested a review from crazy-max June 6, 2023 15:35
@jedevc jedevc merged commit 696770d into docker:master Jun 6, 2023
@jedevc jedevc deleted the fix-check-path-for-bake-cmd-context branch June 6, 2023 15:49
@jedevc jedevc mentioned this pull request Jun 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@crazy-max crazy-max crazy-max approved these changes

Assignees

No one assigned

Labels

area/bake kind/bug Something isn't working

Projects

None yet

Milestone

v0.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jedevc @tonistiigi @crazy-max