Repository: moby/buildkit

Issue:
Dockerfile: "--unpack" option for ADD
## Description
The [documentation for the ADD directive](https://docs.docker.com/engine/reference/builder/#add) in a Dockerfile states:

> If <src> is a _local_ tar archive in a recognized compression format (identity, gzip, bzip2 or xz) then it is unpacked as a directory. Resources from remote URLs are not decompressed.

The word "_local_" is emphasized, and indeed, if the `<src>` is something like `https://github.com/NixOS/nixpkgs/archive/refs/heads/nixos-23.11.tar.gz`, i.e. is a **remote-URL** to a compressed archive, then said archive **is _not_ decompressed** when it would be if added as a local file. I would like to see either a **`--decompress` option to `ADD`**, that decompresses the archive, or make **decompression the default behavior and have a `--keep-compressed`**.

## Rationale
For images that start from scratch, one does not have decompression tools readily available, yet wants to keep their Dockerfile repositories free from binary files. For `ADD` to perform decompression for files from remote-URLs would be desirable in such cases.

Discussion:
@thaJeztah: The Dockerfile syntax is now maintained as part of the BuildKit project, so I'll transfer this ticket there.

There's been some prior discussion on this, and at the time, the suggestion was to use multi-stage builds for this (however, opinions may have changed); see (probably other tickets as well);

- https://github.com/moby/moby/issues/15934

As a workaround / alternative approach with the current features (but a slightly experimental approach), I wrote a comment on this ticket https://github.com/moby/moby/issues/2369#issuecomment-457219856 that (ab)uses the `--mount` option to extract content in a "FROM scratch" stage.
@tonistiigi: I guess it is not "decompress" really but more like "unpack/extract" as compressed and uncompressed tarballs should behave the same.

I guess if we add this then it should be both ways `ADD --unpack=true https://` `ADD --unpack=false local.tar `, and or backwards compatibility the default is different based on arguments.

I think this looks like a candidate for testing in dockerfile labs channel, @AkihiroSuda wdyt? @con-f-use: Good points, I agree, `--unpack` is shorter and less ambiguous and it should be backwards-compatible. I don't know why you need `true` / `false` tough. Presence of the flag alone implies `true`, its lack `false`.@thaJeztah: It could be a non-boolean option, to make it tri-state as starting point, but keeps it more flexible if more options need to be added later, e.g.;

- `auto` (or `local`) for current behaviour (default)
- `always` (or `enabled`) (always unpack)
- `disabled` (never unpack)

Boolean values (`true` as alias for `always`, `false` as alias for `disabled`) _could_ be added, but avoiding them emphasises that it's not just on/off@thaJeztah: A bit orthogonal, but perhaps something that should be looked into as well, as it's slightly related;

- https://github.com/moby/buildkit/issues/1547
- https://github.com/moby/buildkit/issues/3700@tonistiigi: >  I don't know why you need true / false tough. Presence of the flag alone implies true, its lack false.

Because iiuc the default behavior differs based on the input of the copy being remote or local path. Value allows to either enable, disable or keep default. Bool flag can be used without the value. In that case, it is `true`.

> It could be a non-boolean option, to make it tri-state as starting point, but keeps it more flexible if more options need to be added later, e.g.;

Unless we have concrete examples for this, I think it should be avoided. We already have many bool flags in Dockerfile and string values always require the user to know the exact set of valid strings for specific files.@fenollp: Hi! What can be done to help move this forward? Thanks @tonistiigi: This is marked "accepted". Waiting for a PR.

Solution patch:
diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
index b52c03155..9ba03f66a 100644
--- a/frontend/dockerfile/dockerfile2llb/convert.go
+++ b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -948,6 +948,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
 			link:            c.Link,
 			keepGitDir:      c.KeepGitDir,
 			checksum:        c.Checksum,
+			unpack:          c.Unpack,
 			location:        c.Location(),
 			ignoreMatcher:   opt.dockerIgnoreMatcher,
 			opt:             opt,
@@ -1554,9 +1555,15 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
 
 			st := llb.HTTP(src, llb.Filename(f), llb.WithCustomName(pgName), llb.Checksum(checksum), dfCmd(cfg.params))
 
+			var unpack bool
+			if cfg.unpack != nil {
+				unpack = *cfg.unpack
+			}
+
 			opts := append([]llb.CopyOption{&llb.CopyInfo{
 				Mode:           chopt,
 				CreateDestPath: true,
+				AttemptUnpack:  unpack,
 			}}, copyOpt...)
 
 			if a == nil {
@@ -1590,12 +1597,17 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
 				return errors.Wrap(err, "removing drive letter")
 			}
 
+			unpack := cfg.isAddCommand
+			if cfg.unpack != nil {
+				unpack = *cfg.unpack
+			}
+
 			opts := append([]llb.CopyOption{&llb.CopyInfo{
 				Mode:                chopt,
 				FollowSymlinks:      true,
 				CopyDirContentsOnly: true,
 				IncludePatterns:     patterns,
-				AttemptUnpack:       cfg.isAddCommand,
+				AttemptUnpack:       unpack,
 				CreateDestPath:      true,
 				AllowWildcard:       true,
 				AllowEmptyWildcard:  true,
@@ -1684,6 +1696,7 @@ type copyConfig struct {
 	location        []parser.Range
 	ignoreMatcher   *patternmatcher.PatternMatcher
 	opt             dispatchOpt
+	unpack          *bool
 }
 
 func dispatchMaintainer(d *dispatchState, c *instructions.MaintainerCommand) error {
diff --git a/frontend/dockerfile/instructions/commands.go b/frontend/dockerfile/instructions/commands.go
index bebce6361..f82f07f9d 100644
--- a/frontend/dockerfile/instructions/commands.go
+++ b/frontend/dockerfile/instructions/commands.go
@@ -246,6 +246,7 @@ type AddCommand struct {
 	ExcludePatterns []string
 	KeepGitDir      bool // whether to keep .git dir, only meaningful for git sources
 	Checksum        string
+	Unpack          *bool
 }
 
 func (c *AddCommand) Expand(expander SingleWordExpander) error {
diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go
index b44a6a384..ccb0820fa 100644
--- a/frontend/dockerfile/instructions/parse.go
+++ b/frontend/dockerfile/instructions/parse.go
@@ -338,6 +338,7 @@ func parseAdd(req parseRequest) (*AddCommand, error) {
 	flLink := req.flags.AddBool("link", false)
 	flKeepGitDir := req.flags.AddBool("keep-git-dir", false)
 	flChecksum := req.flags.AddString("checksum", "")
+	flUnpack := req.flags.AddBool("unpack", false)
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
@@ -347,6 +348,12 @@ func parseAdd(req parseRequest) (*AddCommand, error) {
 		return nil, err
 	}
 
+	var unpack *bool
+	if _, ok := req.flags.used["unpack"]; ok {
+		b := flUnpack.Value == "true"
+		unpack = &b
+	}
+
 	return &AddCommand{
 		withNameAndCode: newWithNameAndCode(req),
 		SourcesAndDest:  *sourcesAndDest,
@@ -356,6 +363,7 @@ func parseAdd(req parseRequest) (*AddCommand, error) {
 		KeepGitDir:      flKeepGitDir.Value == "true",
 		Checksum:        flChecksum.Value,
 		ExcludePatterns: stringValuesFromFlagIfPossible(flExcludes),
+		Unpack:          unpack,
 	}, nil
 }
 
diff --git a/solver/llbsolver/file/backend.go b/solver/llbsolver/file/backend.go
index 61550b84f..c4a512ffb 100644
--- a/solver/llbsolver/file/backend.go
+++ b/solver/llbsolver/file/backend.go
@@ -255,7 +255,7 @@ func docopy(ctx context.Context, src, dest string, action *pb.FileActionCopy, u
 
 	for _, s := range m {
 		if action.AttemptUnpackDockerCompatibility {
-			if ok, err := unpack(src, s, dest, destPath, ch, timestampToTime(action.Timestamp), idmap); err != nil {
+			if ok, err := unpack(src, s, dest, destPath, ch, u, timestampToTime(action.Timestamp), idmap); err != nil {
 				return errors.WithStack(err)
 			} else if ok {
 				continue
diff --git a/solver/llbsolver/file/unpack.go b/solver/llbsolver/file/unpack.go
index d8698b780..de2770500 100644
--- a/solver/llbsolver/file/unpack.go
+++ b/solver/llbsolver/file/unpack.go
@@ -13,7 +13,7 @@ import (
 	copy "github.com/tonistiigi/fsutil/copy"
 )
 
-func unpack(srcRoot string, src string, destRoot string, dest string, ch copy.Chowner, tm *time.Time, idmap *user.IdentityMapping) (bool, error) {
+func unpack(srcRoot string, src string, destRoot string, dest string, ch copy.Chowner, u *copy.User, tm *time.Time, idmap *user.IdentityMapping) (bool, error) {
 	src, err := fs.RootPath(srcRoot, src)
 	if err != nil {
 		return false, err
@@ -42,6 +42,12 @@ func unpack(srcRoot string, src string, destRoot string, dest string, ch copy.Ch
 	if idmap != nil {
 		opts.IDMap = *idmap
 	}
+	if u != nil {
+		opts.ChownOpts = &archive.ChownOpts{
+			UID: u.UID,
+			GID: u.GID,
+		}
+	}
 	return true, chrootarchive.Untar(file, dest, opts)
 }
 



Test patch:
diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go
index 1293044f3..0eb3e59ca 100644
--- a/frontend/dockerfile/dockerfile_test.go
+++ b/frontend/dockerfile/dockerfile_test.go
@@ -83,6 +83,7 @@ var allTests = integration.TestFuncs(
 	testDockerfileInvalidCommand,
 	testDockerfileADDFromURL,
 	testDockerfileAddArchive,
+	testDockerfileAddChownArchive,
 	testDockerfileScratchConfig,
 	testExportedHistory,
 	testExportedHistoryFlattenArgs,
@@ -3220,6 +3221,34 @@ ADD t.tar.gz /
 	require.NoError(t, err)
 	require.Equal(t, expectedContent, dt)
 
+	// add with unpack=false
+	dockerfile = fmt.Appendf(nil, `
+	FROM %s
+	ADD --unpack=false t.tar.gz /
+	`, baseImage)
+
+	dir = integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+		fstest.CreateFile("t.tar.gz", buf2.Bytes(), 0600),
+	)
+
+	args, trace = f.DFCmdArgs(dir.Name, dir.Name)
+	defer os.RemoveAll(trace)
+
+	destDir = t.TempDir()
+
+	cmd = sb.Cmd(args + fmt.Sprintf(" --output type=local,dest=%s", destDir))
+	require.NoError(t, cmd.Run())
+
+	_, err = os.Stat(filepath.Join(destDir, "foo"))
+	require.Error(t, err)
+	require.True(t, os.IsNotExist(err))
+
+	dt, err = os.ReadFile(filepath.Join(destDir, "t.tar.gz"))
+	require.NoError(t, err)
+	require.Equal(t, buf2.Bytes(), dt)
+
 	// COPY doesn't extract
 	dockerfile = fmt.Appendf(nil, `
 FROM %s
@@ -3277,6 +3306,29 @@ ADD %s /
 	require.NoError(t, err)
 	require.Equal(t, buf2.Bytes(), dt)
 
+	// ADD from URL with --unpack=true
+	dockerfile = fmt.Appendf(nil, `
+FROM %s
+ADD --unpack=true %s /dest/
+`, baseImage, server.URL+"/t.tar.gz")
+
+	dir = integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	args, trace = f.DFCmdArgs(dir.Name, dir.Name)
+	defer os.RemoveAll(trace)
+
+	destDir = t.TempDir()
+
+	cmd = sb.Cmd(args + fmt.Sprintf(" --output type=local,dest=%s", destDir))
+	require.NoError(t, cmd.Run())
+
+	dt, err = os.ReadFile(filepath.Join(destDir, "dest/foo"))
+	require.NoError(t, err)
+	require.Equal(t, expectedContent, dt)
+
 	// https://github.com/moby/buildkit/issues/386
 	dockerfile = fmt.Appendf(nil, `
 FROM %s
@@ -3301,6 +3353,70 @@ ADD %s /newname.tar.gz
 	require.Equal(t, buf2.Bytes(), dt)
 }
 
+func testDockerfileAddChownArchive(t *testing.T, sb integration.Sandbox) {
+	integration.SkipOnPlatform(t, "windows")
+	f := getFrontend(t, sb)
+
+	buf := bytes.NewBuffer(nil)
+	tw := tar.NewWriter(buf)
+	content := []byte("content0")
+	err := tw.WriteHeader(&tar.Header{
+		Name:     "foo",
+		Typeflag: tar.TypeReg,
+		Size:     int64(len(content)),
+		Mode:     0644,
+	})
+	require.NoError(t, err)
+	_, err = tw.Write(content)
+	require.NoError(t, err)
+	err = tw.Close()
+	require.NoError(t, err)
+
+	dockerfile := []byte(`
+FROM scratch
+ADD --chown=100:200 t.tar /out/
+`)
+
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+		fstest.CreateFile("t.tar", buf.Bytes(), 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	outBuf := &bytes.Buffer{}
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		Exports: []client.ExportEntry{
+			{
+				Type:   client.ExporterTar,
+				Output: fixedWriteCloser(&nopWriteCloser{outBuf}),
+			},
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, nil)
+	require.NoError(t, err)
+
+	m, err := testutil.ReadTarToMap(outBuf.Bytes(), false)
+	require.NoError(t, err)
+
+	mi, ok := m["out/foo"]
+	require.True(t, ok)
+	require.Equal(t, "content0", string(mi.Data))
+	require.Equal(t, 100, mi.Header.Uid)
+	require.Equal(t, 200, mi.Header.Gid)
+
+	mi, ok = m["out/"]
+	require.True(t, ok)
+	require.Equal(t, 100, mi.Header.Uid)
+	require.Equal(t, 200, mi.Header.Gid)
+}
+
 func testDockerfileAddArchiveWildcard(t *testing.T, sb integration.Sandbox) {
 	f := getFrontend(t, sb)