mz560: fix handling of ignoreList

[mzihlmann]

In preparation for #560

Description
The handling of ignoreLists was a bit confused. There is a global variable util.ignoreList that gets initialized on bootup and is basically used for most tasks, but even functions that accept a ignorelist as a parameter would just at some depth fall back to read the global variable instead, clearly a bug. Fixing that bug however uncovered another incosistency. There are two functions to check whether a given path is in the ignoreList or not.

• IsInProvidedIgnoreList checks whether that path is in the ignoreList literally, it is used for managing the list, as in check whether for my specific path an entry already exists or not.
• CheckCleanedPathAgainstProvidedIgnoreList checks whether a given path should be ignored based on the ignoreList, this is different to the above, as /dev/sda1 should be ignored if there is an entry for /dev.

The code did use the former function when the latter should be used, there is no single code section where we have to manage the ignoreList so the former function can be dropped altogether.

[mzihlmann]

if you need further proof that unittests are useless....

well at least the wrong tests let me research into why they are wrong, uncovering a larger issue in the code in the process, so helpful?

[mzihlmann]

AI summary review

Overview

Fixes a long-standing bug where functions accepting a wl []IgnoreListEntry parameter
silently fell back to the global util.ignorelist variable at some call depth, making the
parameter useless. Alongside this, replaces all uses of IsInProvidedIgnoreList
(exact-match only) with CheckCleanedPathAgainstProvidedIgnoreList (prefix/subtree match),
which is the semantically correct check for "should this path be excluded from the layer?"

---

What Changed

File	Change
pkg/util/fs_util.go	Remove IsInProvidedIgnoreList / IsInIgnoreList; fix CheckCleanedPathAgainstProvidedIgnoreList to use the passed-in wl instead of global; add wl param to WalkFS / gowalkDir
pkg/snapshot/snapshot.go	Pass wl into WalkFS; replace post-filter CheckIgnoreList with CheckCleanedPathAgainstProvidedIgnoreList + logrus.Panic unreachable guard
pkg/snapshot/snapshot_test.go	Update NewSnapshotter call-site
pkg/filesystem/resolve.go	Replace IsInProvidedIgnoreList with prefix-aware check
pkg/filesystem/resolve_test.go	Mirror the same replacement in test helpers
pkg/executor/build.go	Pass util.IgnoreList() explicitly to NewSnapshotter
NewSnapshotter signature	Now takes wl []util.IgnoreListEntry instead of hard-coding global
Integration tests	Two new Dockerfiles + two new test functions covering subtree exclusion and PrefixMatchOnly semantics

---

Code Quality

Good:

• The root cause (parameter shadowed by global) is fixed cleanly throughout the call chain.
• Removing IsInProvidedIgnoreList / IsInIgnoreList eliminates the confusing dual-function
  API — one correct function is better than two with subtle semantic differences.
• filepath.Clean is applied consistently at each call-site before the check.
• The logrus.Panic("Unreachable Code: …") guard in scanFullFilesystem makes the
  double-filter explicit and will surface regressions immediately.
• Integration tests cover both scenarios described in the bug.

Concerns / Suggestions:

1. logrus.Panic before logrus.Debugf (snapshot.go)
   The Panic fires before the Debugf line beneath it ever runs. The debug message is
   dead code. Remove it, or swap the two lines if the intent was to log then panic.

2. logrus.Panic is very loud for a guard
   Using panic (even via logrus) for an "unreachable" invariant in production code is
   aggressive. A logrus.Warn + continue would be safer for users hitting unexpected
   edge cases, or use a build-tag-gated assertion. Alternatively, if the invariant truly
   holds, remove the dead check entirely.

3. NewSnapshotter callers outside this PR
   The signature change is a breaking API change. Any external caller or test that
   constructs a Snapshotter directly will now fail to compile. A quick grep confirms
   snapshot_test.go was updated; worth verifying no other call-site was missed.

4. CheckCleanedPathAgainstProvidedIgnoreList name
   Now that IsInProvidedIgnoreList is gone this is the only ignore-list checker, so the
   "Cleaned…Against…" qualifier is verbose. Consider PathMatchesIgnoreList for
   readability (non-blocking).

5. Integration test isolation (TestPrefixMatchOnlyIgnoreList)
   The test relies on getLastLayerFiles returning the last layer of a freshly built
   kaniko image. If a previous test leaves state or the layer ordering shifts, results
   could be flaky. Consider asserting on the full layer set rather than only the last layer
   (non-blocking; depends on existing test infrastructure conventions).

---

Correctness

• The core fix (wl parameter now actually flows through WalkFS → gowalkDir → CheckCleanedPathAgainstProvidedIgnoreList) is correct.
• Replacing exact-match with prefix-match for ResolvePaths is correct: --ignore-path=/dest
  must exclude /dest/subdir/file.txt.
• No obvious race conditions introduced; wl is passed by value (slice header) and the
  underlying array is read-only after initialization.

---

Verdict

Approve with minor fixes. The logrus.Panic / dead logrus.Debugf ordering bug
(point 1) should be fixed before merge. The rest are style or robustness suggestions.

[mzihlmann]

now that we verified the panic doesn't pull, the redundant code can be removed

[0hlov3]

Using the local wl in CheckCleanedPathAgainstProvidedIgnoreList and removing the unused exact-match helpers makes sense.

One blocker: this introduces a regression for VOLUME (and anything calling AddToIgnoreList after the snapshotter is created).

s.ignorelist is captured once at construction:

snapshot.NewSnapshotter(..., util.IgnoreList())

But VOLUME later appends to the global ignore list. Since slices are copied by header, s.ignorelist becomes stale.

Before this PR the walk still consulted the live global list; now it only uses s.ignorelist, so paths added later (e.g. /data) may incorrectly end up in layers.

Suggested fix: fetch the ignore list at snapshot time instead of storing it once, e.g. call util.IgnoreList() inside TakeSnapshot / scanFullFilesystem, or pass a getter function.

What do you think?

[mzihlmann]

weird this should have been caught by Dockerfile_test_volume_2
it almost looks like buildkit doesn't care about VOLUMES either?

moby/buildkit#880 (comment)

Oh, volumes only affect the output image, not the build context

Also a single global is anyways not sufficient, as we do build multistage builds.

current behaviour (main) is to ignore files immediately, target behaviour (buildkit) is to do so in downstream images only.
let's keep the current behaviour for now.

   # main
    cmd.go:42: TYPE     NAME                   INPUT-0                                      INPUT-1
        Layer    ctx:/layers-1/layer    length mismatch (1 vs 0)                     
        Layer    ctx:/layers-1/layer    name "foo1/" only appears in input 0         
        Layer    ctx:/layers-2/layer    length mismatch (2 vs 1)                     
        Layer    ctx:/layers-2/layer    name "foo1/hello" only appears in input 0    
        Layer    ctx:/layers-4/layer    length mismatch (2 vs 1)                     
        Layer    ctx:/layers-4/layer    name "foo1/foo" only appears in input 0      
        Layer    ctx:/layers-5/layer    length mismatch (2 vs 1)                     
        Layer    ctx:/layers-5/layer    name "foo1/foo2" only appears in input 0     
        Layer    ctx:/layers-6/layer    length mismatch (1 vs 2)                     
        Layer    ctx:/layers-6/layer    name "foo1/" only appears in input 1         
        Layer    ctx:/layers-7/layer    length mismatch (0 vs 3)                     
        Layer    ctx:/layers-7/layer    name "foo3/" only appears in input 1         
        Layer    ctx:/layers-7/layer    name "foo1/" only appears in input 1         
        Layer    ctx:/layers-7/layer    name "foo2/" only appears in input 1         
        Layer    ctx:/layers-8/layer    length mismatch (1 vs 3)                     
        Layer    ctx:/layers-8/layer    name "foo1/" only appears in input 1         
        Layer    ctx:/layers-8/layer    name "foo4/" only appears in input 1     

hmmmm even if I address the "regression" here, behaviour would still change

with the regression fix:
    cmd.go:42: TYPE     NAME                   INPUT-0                                      INPUT-1
        Layer    ctx:/layers-1/layer    length mismatch (1 vs 0)                     
        Layer    ctx:/layers-1/layer    name "foo1/" only appears in input 0         
        Layer    ctx:/layers-2/layer    length mismatch (2 vs 1)                     
        Layer    ctx:/layers-2/layer    name "foo1/hello" only appears in input 0    
        Layer    ctx:/layers-3/layer    length mismatch (2 vs 1)                     
        Layer    ctx:/layers-3/layer    name "foo1/bar/" only appears in input 0     
        Layer    ctx:/layers-4/layer    length mismatch (2 vs 0)                     
        Layer    ctx:/layers-4/layer    name "foo1/" only appears in input 0         
        Layer    ctx:/layers-4/layer    name "foo1/foo" only appears in input 0      
        Layer    ctx:/layers-5/layer    length mismatch (2 vs 0)                     
        Layer    ctx:/layers-5/layer    name "foo1/" only appears in input 0         
        Layer    ctx:/layers-5/layer    name "foo1/foo2" only appears in input 0     
        Layer    ctx:/layers-7/layer    length mismatch (0 vs 2)                     
        Layer    ctx:/layers-7/layer    name "foo2/" only appears in input 1         
        Layer    ctx:/layers-7/layer    name "foo3/" only appears in input 1         
        Layer    ctx:/layers-8/layer    length mismatch (1 vs 2)                     
        Layer    ctx:/layers-8/layer    name "foo4/" only appears in input 1  

note that in main, for whatever reason, foo1 is snapshotted in layer 8. I think it makes sense to address these functional fixes separately first, so they are intentional and well documented in a separate PR.

There seems to be an entire treasure trove of buggy behaviour around VOLUME