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.app
FROM scratch
COPY foo /foo
EOT
$ docker buildx bake https://github.com/crazy-max/buildx-buildkit-tests.git#:buildx-1627

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

bake/bake.go

+			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.

tests/bake.go

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

bake/bake.go

+	} 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

tests/bake.go

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

bake/bake.go

 	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

bake/bake.go

+		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

bake/bake.go

+		}
+	} 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

tests/bake.go Show resolved Hide resolved

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

crazy-max commented Aug 24, 2023

View reviewed changes

jedevc reviewed Aug 24, 2023

View reviewed changes

tests/bake.go Outdated Show resolved Hide resolved

crazy-max force-pushed the fix-bake-cwd-dockerfile branch from 88009ba to 509439a Compare 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 5b65cfb Compare 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 7497e64 Compare October 19, 2023 15:41

tonistiigi approved these changes Oct 19, 2023

View reviewed changes

tonistiigi merged commit 31d021a into docker:master Oct 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

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-max Oct 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-max Oct 11, 2023 •

edited

Loading