filter: fix a bug with parent dirs #203

nicks · 2024-06-26T02:45:14Z

if the Map() function excludes a directory,
but includes a file inside that directory,
then the walker must backtrack and include
the directory.

otherwise, buildkit gets confused and
sends "changes out of order" errors.

part of fixing
tilt-dev/tilt#6393

tonistiigi · 2024-07-02T17:22:27Z

This seems to revert changes from #183 (d9cfebb) . From an initial look, the argumentation in #183 makes more sense to me. If Map skips the parent dir then it should skip the children of that dir as well, while in the new test, one could make an argument that this is an invalid Map function and the function itself should also allow parents if it wants to allow a specific subpath.

@jedevc @aaronlehmann

nicks · 2024-07-02T17:59:56Z

re: "the function itself should also allow parents if it wants to allow a specific subpath"

how would this work? The subpaths are requested from the buildkit server. The Map() function runs on the client, and doesn't know if the subpaths will be requested or not, right?

Our current workaround is to ALWAYS include the directory if the Map() doesn't know if the subpaths will be requested. The bug downstream (tilt-dev/tilt#6393) is that this means there are sometimes empty folders lying around, which creates problems in python.

tonistiigi · 2024-07-02T19:04:20Z

Maybe that case should use IncludePatterns then instead of Map. As Map is for filtering out files and directories(with their subfiles). Yes, in Map if you check a || a/b || a/b/c you may end up with a and a/b without any a/b/c being present. If you set includepatterns to a/b/c then you will not.

nicks · 2024-07-02T19:41:35Z

honestly, poking around at this, I feel like the FilterOpt API doesn't make much sense anymore. it's vestigial from when Map() was a property of DirSource.

Now that the API supports composable FS implementations, maybe we should just implement our own FS with the Map() semantics we need.

I don't really understand why you like the current Map() semantics -- according to what you're saying, the only way to implement a valid Map() is to have Map() crawl the subdirectories itself, which means it's duplicating all the logic in Walk().

is there an example of a project that uses it how you think it should work that you could point me to?

nicks · 2024-07-02T19:59:42Z

ah, poking around at the downstream Dagger issue, i have an idea!

if a Map() returns SkipDir, that should be a "big hammer" that excludes everything underneath. If a subpath of the directory is included by a pattern, the subpath should be ignored.

if a Map() returns Exclude, then that should be softer. If a subpath of the directory is included by a pattern, then the subpath should be included, and the parent backtracker should pick up the parent directory.

i pushed up some tests to demo the new behavior, what do you think?

jedevc · 2024-07-03T11:30:59Z

honestly, poking around at this, I feel like the FilterOpt API doesn't make much sense anymore. it's vestigial from when Map() was a property of DirSource.

Does Exclude even make sense to apply to non-directories? As you mention, the current behavior is bad, and it's possible to end up with an invalid list of files (where you have a/x.txt, but not its parent a/). Is there any case where you wouldn't just use SkipDir instead today?

I guess, the idea of this PR is to try and have the map's Exclude return value act more inline with the ExcludePatterns? If so, I think that seems reasonable.

The change doesn't change the behavior of any existing valid map functions, but simply makes some previously invalid map functions valid (e.g. in the above, we'd get the parent a/).

TL;DR - I think the current state of this PR makes sense to me - but I think we definitely need to annotate the return values for how they behave, the current doc comments aren't very descriptive for this newer behavior.

jedevc · 2024-07-03T11:36:44Z

filter.go

@@ -372,6 +372,8 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
 					return err
 				}
 			}
+
+			dir.calledFn = true


👀 this feels unrelated, were we not setting this before? Was there a case where we were calling the walk target function multiple times? Kinda confused as to why this shows up just here (definitely worth fixing).

ya, i moved this line.

Before this pr, the logic went:

check if the directory is skipped by includePatterns or excludePatterns

set calledFn to true

check if the directory is skipped by mapFn

call fn

now, i only set calledFn to true directly before calling fn

if the Map() function excludes a directory, but includes a file inside that directory, then the walker must backtrack and include the directory. otherwise, buildkit gets confused and sends "changes out of order" errors. part of fixing tilt-dev/tilt#6393 Signed-off-by: Nick Santos <nick.santos@docker.com>

nicks · 2024-07-03T13:44:05Z

@jedevc i added some comments on the MapResult options to make the behavior a bit clearer. what do you think?

nicks · 2024-07-09T13:24:23Z

@tonistiigi is this PR ok to merge? i'll need a maintainer to merge it for me 🙏

tonistiigi

Does this mean that https://github.com/moby/buildkit/blob/v0.15.0-rc1/exporter/local/fs.go#L116 is wrong (or at least needs to be updated after this PR)?

tonistiigi · 2024-07-09T19:37:38Z

filter.go

@@ -197,7 +206,7 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
 			isDir = dirEntry.IsDir()
 		}

-		if fs.includeMatcher != nil || fs.excludeMatcher != nil {
+		if fs.includeMatcher != nil || fs.excludeMatcher != nil || fs.mapFn != nil {


One thing I'm not a big fan of is that everything that gets a map goes to this more complicated code-path to support the backtracking, while most of the time the map is really used to reset uid/gid or skip paths recursively.

Would be really ugly, but smth. like MapAllowBacktrack bool would solve that.

I don't know how big issue this is in practice though. It might be that most of our requests set includematches/excludematcher anyway.

is your concern more about performance or debuggability?

i'd be in favor of adding a much simpler FS implementation purely focused on the reset uid/gid case.

Performance a bit maybe, but mostly keeping the default code path simpler and preventing potential stability issues.

nicks · 2024-07-09T22:09:53Z

re: https://github.com/moby/buildkit/blob/v0.15.0-rc1/exporter/local/fs.go#L116 -- i'm not really sure what the intended behavior of that code is, but i think you would want the backtracking there, no?

tonistiigi · 2024-07-09T22:29:24Z

, but i think you would want the backtracking there, no?

I don't think so. If a directory causes a remap error then we shouldn't just add it (with wrong permissions) if it has some child under it.

jedevc · 2024-07-10T10:38:20Z

re: moby/buildkit@v0.15.0-rc1/exporter/local/fs.go#L116

The current behavior also seems quite odd to me - if a directory is excluded here, then the files will still be added underneath under the current behavior: which would cause an error on transmission.

Currently directories (with valid files underneath) in this case aren't excluded (at least in practice, or we'd see those weird transmission errors everywhere). I think the "right" thing to do in this case would be to use MapResultSkipDir here for directories, I'm not quite sure why it isn't already.

nicks · 2024-07-11T14:03:58Z

in theory, i feel like that code should be using SkipDir -- it'd be faster to skip the whole directory, rather than continuing to traverse it

in practice...i couldn't even figure out how to get that codepath to trigger? i traced back to this 5 year old comment about it being an incomplete feature 👀 https://github.com/moby/buildkit/blob/e83d79a51fb49aeb921d8a2348ae14a58701c98c/cmd/buildkitd/config/config.go#L101, but i'm not sure if that comment is still true

tonistiigi · 2024-09-19T16:05:49Z

in practice...i couldn't even figure out how to get that codepath to trigger?

This is enabled by the --userns-remap feature of dockerd.

this test demonstrates the bug called out in tonistiigi/fsutil#203 Currently, if a directory's UID can't be mapped to the host, buildkit will export an invalid tarball. Signed-off-by: Nick Santos <nick.santos@docker.com>

nicks · 2024-10-09T01:59:14Z

sorry, have been a bit busy the last couple weeks and finally got back to this.

i think the new behavior i'm introducing in this PR is the correct behavior for tarball exports. I opened a PR in buildkit that demonstrates the issue:
moby/buildkit#5411

this test demonstrates the bug called out in tonistiigi/fsutil#203 Currently, if a directory's UID can't be mapped to the host, buildkit will export an invalid tarball. Signed-off-by: Nick Santos <nick.santos@docker.com>

nicks force-pushed the nicks/map branch 2 times, most recently from 2ea0aea to c9de5ba Compare July 2, 2024 20:01

jedevc reviewed Jul 3, 2024

View reviewed changes

nicks force-pushed the nicks/map branch from c9de5ba to 020c217 Compare July 3, 2024 14:15

nicks requested a review from jedevc July 3, 2024 14:15

jedevc approved these changes Jul 3, 2024

View reviewed changes

tonistiigi reviewed Jul 9, 2024

View reviewed changes

nicks requested a review from tonistiigi July 11, 2024 14:04

nicks mentioned this pull request Oct 9, 2024

exporter: add tests for a directory with an idmap error moby/buildkit#5411

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter: fix a bug with parent dirs #203

filter: fix a bug with parent dirs #203

nicks commented Jun 26, 2024

tonistiigi commented Jul 2, 2024

nicks commented Jul 2, 2024 •

edited

Loading

tonistiigi commented Jul 2, 2024

nicks commented Jul 2, 2024

nicks commented Jul 2, 2024 •

edited

Loading

jedevc commented Jul 3, 2024 •

edited

Loading

jedevc Jul 3, 2024

nicks Jul 3, 2024

nicks commented Jul 3, 2024

nicks commented Jul 9, 2024

tonistiigi left a comment

tonistiigi Jul 9, 2024

nicks Jul 9, 2024

tonistiigi Jul 9, 2024

nicks commented Jul 9, 2024

tonistiigi commented Jul 9, 2024

jedevc commented Jul 10, 2024 •

edited

Loading

nicks commented Jul 11, 2024

tonistiigi commented Sep 19, 2024

nicks commented Oct 9, 2024

filter: fix a bug with parent dirs #203

Are you sure you want to change the base?

filter: fix a bug with parent dirs #203

Conversation

nicks commented Jun 26, 2024

tonistiigi commented Jul 2, 2024

nicks commented Jul 2, 2024 • edited Loading

tonistiigi commented Jul 2, 2024

nicks commented Jul 2, 2024

nicks commented Jul 2, 2024 • edited Loading

jedevc commented Jul 3, 2024 • edited Loading

jedevc Jul 3, 2024

Choose a reason for hiding this comment

nicks Jul 3, 2024

Choose a reason for hiding this comment

nicks commented Jul 3, 2024

nicks commented Jul 9, 2024

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi Jul 9, 2024

Choose a reason for hiding this comment

nicks Jul 9, 2024

Choose a reason for hiding this comment

tonistiigi Jul 9, 2024

Choose a reason for hiding this comment

nicks commented Jul 9, 2024

tonistiigi commented Jul 9, 2024

jedevc commented Jul 10, 2024 • edited Loading

nicks commented Jul 11, 2024

tonistiigi commented Sep 19, 2024

nicks commented Oct 9, 2024

nicks commented Jul 2, 2024 •

edited

Loading

nicks commented Jul 2, 2024 •

edited

Loading

jedevc commented Jul 3, 2024 •

edited

Loading

jedevc commented Jul 10, 2024 •

edited

Loading