Recent Change to Dockerignore Parsing

[BusterNeece]

One of my GitHub Actions involves a build which uses a .dockerignore file that basically looks like this:

**
!web
!util/docker/web
!composer.json

The intent is to automatically ignore anything that isn't expressly specified later in the file.

Up until today, on my GitHub Actions, this worked without any issue. Today, however, I started getting failed builds:

#7 [2/8] COPY ./util/docker/web /bd_build/
#7 DONE 0.4s

#8 [3/8] RUN chmod a+x /bd_build/*.sh     && /bd_build/prepare.sh     && /bd_build/add_user.sh     && /bd_build/setup.sh     && /bd_build/cleanup.sh     && rm -rf /bd_build
#8 0.070 chmod: cannot access '/bd_build/*.sh': No such file or directory

Removing the .dockerignore file caused the problem to be resolved and the build to continue as normal. Something has changed in the last 24 hours with some part of the stack that enables building Docker images via Buildkit on GitHub Actions that has affected how the .dockerignore file is parsed.

My GitHub Actions report that my current version of buildkit is moby/buildkit:buildx-stable-1 => buildkitd github.com/moby/buildkit v0.9.3 8d2625494a6a3d413e3d875a2ff7dd9b1ed1b1a9

[slavovojacek]

Had the same issue, everything good locally but GH actions were failing for us. Figured it was .dockerignore related, we did the below to fix.

  **/*
  !*.json
- !prisma
- !src
+ !prisma/**
+ !src/**

[tonistiigi]

Could either of you post a full example?

[crazy-max]

@SlvrEagle23 If you have a link to your repo that would be great!

Ok I was able to repro with buildx 0.7.0. In the meantime you can use the previous buildx release in your workflow with:

      -
        name: Set up Docker Buildx
        uses: docker/setup-buildx-action@v1
        with:
          version: v0.6.3

[tonistiigi]

@aaronlehmann This looks to be related to your * updates in patternmatcher. The vendor of buildkit&moby seems to be regression point #775

[aaronlehmann]

This is related to moby/moby#42676

@tonistiigi: I want to confirm the expected behavior here. Given the .dockerignore file:

**
!util/docker/web

It sounds like we expect a file util/docker/web/foo not to be ignored because of the exclusion.

What if .dockerignore was this instead?

**
!util/docker/web
util/docker/web/foo

In that case, would we expect the file to be ignored because there is a more specific exclude pattern?

The logic currently getting applied is to use the parent dir's match result as a default if none of the patterns directly match a file. In the case here, the ** matches all files, so the parent dir result never comes into play.

If we want the behavior I described above, it sounds like we need to keep track of these parent results at a per-pattern level. For example, with the .dockerignore:

**
!util/docker/web
util/docker/web/foo

...when checking util/docker/web/foo:

• ** matched parent dir util/docker/web => we don't care if ** matches util/docker/web/foo; default to whatever we decided for util/docker/web
• !util/docker/web matched parent dir util/docker/web => we don't care if !util/docker/web matches util/docker/web/foo; continue with the default
• util/docker/web/foo did not match parent dir util/docker/web => therefore we check this pattern, and since it matches, we must exclude util/docker/web/foo

[tonistiigi]

In that case, would we expect the file to be ignored because there is a more specific exclude pattern?

I think logically this would be correct. But priority would be to fix up the regressions from the old behavior. If these cases didn't work before as well then they are not that time-critical.

[thaJeztah]

I think this matches .gitignore behavior; https://git-scm.com/docs/gitignore

It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

mkdir test && cd test
mkdir -p util/docker/web
touch util/docker/web/foo
touch util/docker/web/bar
git init

Without .gitignore

git status

On branch main

No commits yet

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	util/

nothing added to commit but untracked files present (use "git add" to track)

Adding a .gitignore:

cat > ./.gitignore <<'EOF'
**
!util/docker/web
util/docker/web/foo
EOF

git status

On branch main

No commits yet

nothing to commit (create/copy files and use "git add" to track)

Checking with git check-ignore:

git check-ignore -v util/docker/web
.gitignore:1:**	util/docker/web

git check-ignore -v util/docker/web/foo
.gitignore:1:**	util/docker/web/foo

git check-ignore -v util/docker/web/bar
.gitignore:1:**	foo/bar/baz

So to re-include a file, the parent directory that contains it must be included first, which can become complex quickly. The gitignore docs has an example to "exclude everything, except for some files, but things get complex quickly;

cat > ./.gitignore <<'EOF'
/*
!/util
/util/*
!util/docker
/util/docker/web/*
!/util/docker/web/foo
EOF

git check-ignore -v util/docker/web/foo
.gitignore:6:!/util/docker/web/foo	util/docker/web/foo

git check-ignore -v util/docker/web/bar
.gitignore:5:/util/docker/web/*	util/docker/web/bar

git status -uall
On branch main

No commits yet

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	util/docker/web/foo

nothing added to commit but untracked files present (use "git add" to track)

Changing that .gitignore to use ** shows that everything is ignored again;

cat > ./.gitignore <<'EOF'
/**
!/util
/util/*
!util/docker
/util/docker/web/*
!/util/docker/web/foo
EOF

git status -uall
On branch main

No commits yet

nothing to commit (create/copy files and use "git add" to track)

[thaJeztah]

That said, with BuildKit only sending files that are included in the COPY, perhaps a better pattern would be to

COPY util/docker/web

Unlike the classic (non-buildkit) builder, that should not send anything except for that path

[tonistiigi]

That said, with BuildKit only sending files that are included in the COPY, p

These paths still go through dockerignore filtering.

[thaJeztah]

Yes, so I meant that the .dockerignore file then wouldn't be needed in that case

…

On 19 Nov 2021, at 21:24, Tõnis Tiigi ***@***.***> wrote:  That said, with BuildKit only sending files that are included in the COPY, p These paths still go through dockerignore filtering. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[aaronlehmann]

Opened moby/moby#43037 and tonistiigi/fsutil#114 to restore the old behavior.

[abhinavsingh]

jFYI, We getting hit by this too. All of a sudden our CI is not passing with similar docker buildx errors https://github.com/abhinavsingh/proxy.py/runs/4517435825?check_suite_focus=true

Will try the .dockerignore hack listed above by other folks.

[abhinavsingh]

This change completely breaks our CI. We tried the workaround but nothing seems to work. Point is, COPY directive in our Dockerfile uses an ARG as source. Per, #850 (comment), neither does the .dockerignore workaround work nor does buildx can identify the file from COPY directive.

Repository is here https://github.com/abhinavsingh/proxy.py. We are currently unable to submit new PRs due to broken build.

[abhinavsingh]

I think this matches .gitignore behavior; https://git-scm.com/docs/gitignore

It might be consistent but has broken a lot of builds around the world and wasted so many unnecessary man power digging into this. Can there be a better channel to communicate such breaking changes?

[tonistiigi]

@abhinavsingh Set a custom buildx version as a workaround. This issue in here should be fixed in v0.7.1 but looks like this is a different case.

@aaronlehmann This looks like another case. I can repro locally. Looks like it is related to the complicated name?

This fails:

 # touch dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl
 # docker buildx build \
        --platform linux/386 \
        -t abhinavsingh/proxy.py:2.4.0rc3.dev36.g08acad9 \
        --build-arg PROXYPY_PKG_PATH=dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl .
WARN[0000] No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
[+] Building 1.1s (8/10)
 => [internal] load build definition from Dockerfile                                                           0.0s
 => => transferring dockerfile: 1.21kB                                                                         0.0s
 => [internal] load .dockerignore                                                                              0.0s
 => => transferring context: 87B                                                                               0.0s
 => [internal] load metadata for docker.io/library/python:3.10-alpine                                          1.0s
 => [auth] library/python:pull token for registry-1.docker.io                                                  0.0s
 => [1/5] FROM docker.io/library/python:3.10-alpine@sha256:dce56d40d885d2c8847aa2a278a29d50450c8e3d10f9d7ffeb  0.0s
 => => resolve docker.io/library/python:3.10-alpine@sha256:dce56d40d885d2c8847aa2a278a29d50450c8e3d10f9d7ffeb  0.0s
 => [internal] load build context                                                                              0.0s
 => => transferring context: 32B                                                                               0.0s
 => CACHED [2/5] COPY README.md /                                                                              0.0s
 => ERROR [3/5] COPY dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl /                                  0.0s
------
 > [3/5] COPY dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl /:
------
Dockerfile:14
--------------------
  12 |
  13 |     COPY README.md /
  14 | >>> COPY $PROXYPY_PKG_PATH /
  15 |     RUN pip install --upgrade pip && \
  16 |       pip install \
--------------------
error: failed to solve: failed to compute cache key: "/dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl": not found

This passes

 # touch dist/proxy.whl
 # docker buildx build \
        --platform linux/386 \
        -t abhinavsingh/proxy.py:2.4.0rc3.dev36.g08acad9 \
        --build-arg PROXYPY_PKG_PATH=dist/proxy.whl .

WARN[0000] No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
[+] Building 1.7s (8/9)
 => [internal] load build definition from Dockerfile                                                           0.0s
 => => transferring dockerfile: 1.21kB                                                                         0.0s
 => [internal] load .dockerignore                                                                              0.0s
 => => transferring context: 87B                                                                               0.0s
 => [internal] load metadata for docker.io/library/python:3.10-alpine                                          0.4s
 => [1/5] FROM docker.io/library/python:3.10-alpine@sha256:dce56d40d885d2c8847aa2a278a29d50450c8e3d10f9d7ffeb  0.0s
 => => resolve docker.io/library/python:3.10-alpine@sha256:dce56d40d885d2c8847aa2a278a29d50450c8e3d10f9d7ffeb  0.0s
 => [internal] load build context                                                                              0.0s
 => => transferring context: 91B                                                                               0.0s
 => CACHED [2/5] COPY README.md /                                                                              0.0s
 => CACHED [3/5] COPY dist/proxy.whl /                                                                         0.0s
 => CANCELED [4/5] RUN pip install --upgrade pip &&   pip install   --no-index   --find-links file:///   prox  1.1s
error: failed to solve: Canceled: context canceled

 # cat .dockerignore
# Ignore everything
**

!dist/*.whl
!README.md

[abhinavsingh]

FWIW, in our scenario, builds are broken even after removing .dockerignore completely. buildx simply doesn't seem to pick up on the file.

[abhinavsingh]

Looks like it is related to the complicated name?

Thank you @tonistiigi Didn't realize complex name can be the culprit here. Will try to rename artifact and see if it works.

Set a custom buildx version as a workaround

Gotcha.

Trying these workaround shortly, will revert back with findings. Thanks again.

[tonistiigi]

FWIW, in our scenario, builds are broken even after removing .dockerignore completely. buildx simply doesn't seem to pick up on the file.

Indeed, I can confirm that. So could be something very different. Both v0.6.3 and v0.7.0 seem to work fine (notice v0.7.0 does have the issue originally reported here). v0.7.1 does not.

@crazy-max Unrelated, but I noticed I can't see the buildx version on the action's output. Maybe we can improve on that for future debugging needs.

[abhinavsingh]

v0.7.0

Thank you @tonistiigi , this worked out. Ref https://github.com/abhinavsingh/proxy.py/runs/4556154387?check_suite_focus=true

[tonistiigi]

The issue in here is the + sign that is not escaped in the regexp ^dist/proxy\.py-2\.4\.0rc3\.dev36+g08acad9-py3-none-any\.whl$ so it doesn't match the filename. But I don't understand why this is a regexp at all as there are no wildcards anywhere in this path. Afaics it should just be a string comparison.

If I view the diff then previously I think it did use a much simpler comparator with the prefix.Match. Additionally, prefix.Match that now seems to be gone was filepath.Match based that is of course a different set of rules(that also match the Dockerfile path rules). I'm not sure if it is really that easy to guarantee no behavior changes when switching this directly to regexp.

Pretty bad that this complete logic switch was in a patch release. I assumed we were only fixing regressions here.

[abhinavsingh]

+ sign is an artifact generated by standard Python automatic version detection system (based upon version control) and is in compliance with PEP standards.

• https://www.python.org/dev/peps/pep-0440/#local-version-identifiers

If this becomes a problem we could get rid of + sign for Docker side of things. But because standard versioning system is generating such names for the artifact, we can expects similar cases out in the wild too.

[aaronlehmann]

If I view the diff then previously I think it did use a much simpler comparator with the prefix.Match. Additionally, prefix.Match that now seems to be gone was filepath.Match based that is of course a different set of rules(that also match the Dockerfile path rules). I'm not sure if it is really that easy to guarantee no behavior changes when switching this directly to regexp.

Pretty bad that this complete logic switch was in a patch release. I assumed we were only fixing regressions here.

The PR in question is tonistiigi/fsutil#108. This wasn't meant as a regression fix; the intent was to add support for ** in include patterns and simplify the code to standardize on one pattern matching implementation. Before this, different matching implementations were used for include patterns and exclude patterns, which meant ** patterns were only supported as part of exclude patterns. This was very inconsistent.

I suspect what happened is that fsutil was upgraded to master in the patch release instead of cherry-picking specific fixes - or if cherry-picks were done, this one was picked up accidentally.

The issue in here is the + sign that is not escaped in the regexp ^dist/proxy\.py-2\.4\.0rc3\.dev36+g08acad9-py3-none-any\.whl$ so it doesn't match the filename.

I am confused about where this incorrect regexp is coming from. I assume you're talking about the code in (*Pattern).compile (pkg/fileutils) that transforms a path pattern into a regexp. But I see this code does escape + characters, and I added some test cases to confirm it:

diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go
index b24d82ebd2..c1b720a0db 100644
--- a/pkg/fileutils/fileutils_test.go
+++ b/pkg/fileutils/fileutils_test.go
@@ -382,10 +382,14 @@ func TestMatches(t *testing.T) {
                {"a(b)c/def", "a(b)c/def", true},
                {"a(b)c/def", "a(b)c/xyz", false},
                {"a.|)$(}+{bc", "a.|)$(}+{bc", true},
+               {"dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl", "dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl", true},
+               {"dist/*.whl", "dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl", true},
        }
        multiPatternTests := []multiPatternTestCase{
                {[]string{"**", "!util/docker/web"}, "util/docker/web/foo", false},
                {[]string{"**", "!util/docker/web", "util/docker/web/foo"}, "util/docker/web/foo", true},
+               {[]string{"**", "!dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl"}, "dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl", false},
+               {[]string{"**", "!dist/*.whl"}, "dist/proxy.py-2.4.0rc3.dev36+g08acad9-py3-none-any.whl", false},
        }

        if runtime.GOOS != "windows" {

Can you elaborate on where you are seeing this?

But I don't understand why this is a regexp at all as there are no wildcards anywhere in this path. Afaics it should just be a string comparison.

I agree, it seems like there should be a fast path to avoid compiling a regexp when there are no wildcards.

[tonistiigi]

I am confused about where this incorrect regexp is coming from.

You might be not seeing it because of another recent PR in moby master. moby/moby@195558d#diff-709ecb416d43d86b2812c0eda71926f0af93cb7ef5f0486067a48e1e7ef90521

[crazy-max]

Seems to be solved since last buildkit vendoring #889