bake: local dockerfile support for remote definition by crazy-max · Pull Request #2015 · docker/buildx

crazy-max · 2023-08-23T15:42:01Z

Allow to use a local dockerfile with a remote bake definition like we do for context using cwd:// protocol.

Can be tested with https://github.com/crazy-max/buildx-buildkit-tests/tree/main/buildx-1627:

$ cd /tmp
$ cat <<EOT >> Dockerfile.appFROM scratchCOPY foo /fooEOT
$ docker buildx bake https://github.com/crazy-max/buildx-buildkit-tests.git#:buildx-1627

crazy-max · 2023-08-23T15:47:17Z

+ return nil, err
+ }
+ var err error
+ bi.DockerfilePath, err = filepath.Abs(bi.DockerfilePath)


We need to return the absolute representation of the dockerfile path after checking its a valid one like we do for context so we can set the right dockerfile dir if context is a remote URL during build.
I was also thinking reading the file in bi.DockerfileInline and then set bi.DockerfilePath = "-" but that's hacky and would be confusing.

jedevc · 2023-08-23T16:19:38Z

Should we add a test to make sure that #1880 (comment) still applies? So in the test, if Dockerfile.app appears without the cwd:// prefix, we should error.

Either - that case should error like in v0.11 (which reverted #1880), or it should pull the docker file from the remote.

crazy-max · 2023-08-24T08:45:45Z

Should we add a test to make sure that #1880 (comment) still applies? So in the test, if Dockerfile.app appears without the cwd:// prefix, we should error.
Either - that case should error like in v0.11 (which reverted #1880), or it should pull the docker file from the remote.

Yes so for a remote bake definition like:

target"default" {
context=BAKE_CMD_CONTEXT
dockerfile="Dockerfile.app"
}

Invoked with:

docker buildx bake https://github.com/foo/bar.git

We should use local context but we don't support reading remote Dockerfile as expected:

#0 building with "default" instance using docker driver
#1 [internal] load git source https://github.com/foo/bar.git
#1 0.480 ref: refs/heads/main HEAD
#1 0.480 dea0f6334d719a494a384a13812f604b6b79fed3 HEAD
#1 0.916 dea0f6334d719a494a384a13812f604b6b79fed3 refs/heads/main
#1 CACHED
#2 [internal] load .dockerignore
#2 transferring context:
#2 transferring context: 2B done
#2 DONE 0.0s
#3 [internal] load build definition from Dockerfile.app
#3 transferring dockerfile: 2B done
#3 DONE 0.0s
ERROR: failed to solve: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount3004544897/Dockerfile.app: no such file or directory

crazy-max · 2023-08-24T10:46:57Z

+	} else if !build.IsRemoteURL(bi.DockerfilePath) && strings.HasPrefix(bi.ContextPath, "cwd://") && (inp != nil && build.IsRemoteURL(inp.URL)) {
+ if _, err := os.Stat(filepath.Join(path.Clean(strings.TrimPrefix(bi.ContextPath, "cwd://")), bi.DockerfilePath)); err == nil {
+ return nil, errors.Errorf("reading a local dockerfile for a remote build invocation is not allowed when using a local context")
+ }


Deny access to a local dockerfile for remote invocation with a local context. We need to check if a local Dockerfile exists early at the moment but in follow-up we should support reading a remote Dockerfile. Probably having a DockerfileState input like ContextState one.

Just want to check - isn't this independent of what the Context is set to? Isn't it instead dependent on whether the bake file is coming from a remote?

It's dependent on remote bake but also if context is not local

tonistiigi · 2023-10-10T22:15:37Z

 	if t.Dockerfile != nil {
 dockerfilePath = *t.Dockerfile
 	}
+	if !strings.HasPrefix(dockerfilePath, "cwd://") {


follow-up: this should work with remote URLs as well

you mean similar to what has been said in #2015 (comment) so we support reading a remote Dockerfile with a local context?

tonistiigi · 2023-10-10T22:20:43Z

+ if err != nil {
+ return nil, err
+ }
+	} else if !build.IsRemoteURL(bi.DockerfilePath) && strings.HasPrefix(bi.ContextPath, "cwd://") && (inp != nil && build.IsRemoteURL(inp.URL)) {


I'm not sure I understand this condition:
!build.IsRemoteURL(bi.DockerfilePath) && strings.HasPrefix(bi.ContextPath, "cwd://")
If Dockerfile is not remote and context is based on local dir then why would that be invalid config?
Maybe a comment would help.

tonistiigi · 2023-10-10T22:22:34Z

+ }
+	} else if !build.IsRemoteURL(bi.DockerfilePath) && strings.HasPrefix(bi.ContextPath, "cwd://") && (inp != nil && build.IsRemoteURL(inp.URL)) {
+ if _, err := os.Stat(filepath.Join(path.Clean(strings.TrimPrefix(bi.ContextPath, "cwd://")), bi.DockerfilePath)); err == nil {
+ return nil, errors.Errorf("reading a dockerfile for a remote build invocation is currently not supported")


Is this for security?

No, just not supported atm, see #2015 (comment)

Added a comment to better explain the issue.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

…l context we don't currently support reading a remote Dockerfile with a local context when doing a remote invocation because we automatically derive the dockerfile from the context atm. To avoid mistakenly reading a local Dockerfile, we check if the Dockerfile exists locally and if so, we error out. Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max commented Aug 23, 2023
View reviewed changes

crazy-max requested review from jedevc, jsternberg and tonistiigi August 23, 2023 15:47

crazy-max marked this pull request as ready for review August 23, 2023 15:47

jedevc reviewed Aug 23, 2023
View reviewed changes

Comment threadtests/bake.go

crazy-max force-pushed the fix-bake-cwd-dockerfile branch from 83646f5 to 88009baCompare August 24, 2023 10:40

crazy-max commented Aug 24, 2023
View reviewed changes

jedevc reviewed Aug 24, 2023
View reviewed changes

Comment threadtests/bake.go Outdated

crazy-max force-pushed the fix-bake-cwd-dockerfile branch from 88009ba to 509439aCompare August 24, 2023 11:17

crazy-max added this to the v0.12.0 milestone Sep 12, 2023

tonistiigi reviewed Oct 10, 2023
View reviewed changes

crazy-max force-pushed the fix-bake-cwd-dockerfile branch 2 times, most recently from 91e3978 to 5b65cfbCompare October 19, 2023 11:30

crazy-max added 2 commits October 19, 2023 17:41

bake: local dockerfile support for remote definition
de9d253
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the fix-bake-cwd-dockerfile branch from 5b65cfb to 7497e64Compare October 19, 2023 15:41

tonistiigi approved these changes Oct 19, 2023
View reviewed changes

tonistiigi merged commit 31d021a into docker:masterOct 19, 2023

crazy-max added the status/needs-follow-up label Oct 19, 2023

crazy-max deleted the fix-bake-cwd-dockerfile branch October 19, 2023 22:46

Navigation Menu

Uh oh!

bake: local dockerfile support for remote definition - #2015

bake: local dockerfile support for remote definition#2015
tonistiigi merged 2 commits into
docker:masterfrom
crazy-max:fix-bake-cwd-dockerfile

crazy-max commented Aug 23, 2023

Uh oh!

crazy-maxAug 23, 2023

Uh oh!

Uh oh!

jedevc commented Aug 23, 2023

Uh oh!

crazy-max commented Aug 24, 2023

Uh oh!

crazy-maxAug 24, 2023

Uh oh!

jedevcAug 24, 2023

Uh oh!

crazy-maxAug 24, 2023

Uh oh!

Uh oh!

tonistiigiOct 10, 2023

Uh oh!

crazy-maxOct 11, 2023

Uh oh!

tonistiigiOct 10, 2023

Uh oh!

tonistiigiOct 10, 2023

Uh oh!

crazy-maxOct 11, 2023•
edited
Loading

Uh oh!

crazy-maxOct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

crazy-max commented Aug 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jedevc commented Aug 23, 2023

Uh oh!

crazy-max commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazy-maxOct 11, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crazy-maxOct 11, 2023•
edited
Loading