Implement readUser on Windows #3518

gabriel-samfira · 2023-01-18T23:18:09Z

This change adds the ability to resolve usernames to SIDs in the FileOpSolver. This enables WORKDIR, ADD and COPY to properly set permissions on Windows.

Depends on:

[Windows] Implement executor on Windows #3517

Signed-off-by: Gabriel Adrian Samfira [email protected]

gabriel-samfira · 2023-08-30T21:14:55Z

With the dependency merged, this PR is now ready for review. This is the last PR in this series. It enables useful definitions to be built natively on Windows.

CC @tonistiigi @AkihiroSuda

tonistiigi · 2023-08-30T21:43:49Z

solver/llbsolver/file/backend.go

@@ -251,6 +212,7 @@ func docopy(ctx context.Context, src, dest string, action pb.FileActionCopy, u *
 }

 type Backend struct {
+ Executor executor.Executor


Wdyt if we put separate type definitions here for UNIX and windows? Logically the executor dependency really does not belong here, so would like to avoid it for UNIX case.

Another option would be to leave this type as-is and in solve() optionally pass in some option that does the user conversion. This would then only be passed in case of windows and linux can use the default implementation.

Logically the executor dependency really does not belong here

It felt a bit yucky to add the executor there. I changed it and added a new function NewFileOpBackend() that sets a readUserFn in the Backend{}. For Windows it wraps the readUser() function which uses the executor. For everything else we just return the usual readUser() function. This way we don't set an exported Executor field in the Backend{} struct.

Does that sound acceptable?

tonistiigi · 2023-09-01T16:15:16Z

solver/llbsolver/file/backend.go

@@ -250,7 +211,16 @@ func docopy(ctx context.Context, src, dest string, action pb.FileActionCopy, u *
 return nil
 }

+// NewFileOpBackend returns a new file operation backend. The executor is currently only used for Windows,
+// and it is used to construct the readUserFn field set in the returned Backend.
+func NewFileOpBackend(exec executor.Executor) *Backend {


I'd like to avoid passing this Executor in here (and import on buildkit/executor). And instead pass a function/interface only for readuser (ideally only on windows, in linux you can just pass nil).

This will mean having code that reads the user defined in more than one place (this package and somewhere else). Also, if we only pass in that function/interface for one OS, it creates a special case that will add cognitive burden for future readers of this code (one more thing to keep track of). I can do that if you wish, but it might be better to pass in that function/interface for all OSs, just to keep things consistent and in one place.

If we decide to go that route, we can define a new interface in solver/llbsolver/file, implement it in solver/llbsolver/ops package (the Windows version will call into the executor which is already imported in exec.go), and pass that implementation into Backend{}`.

How does that sound?

I can do that if you wish, but it might be better to pass in that function/interface for all OSs, just to keep things consistent and in one place.

Fine by me if you prefer that. I thought about it from the mindset that file can do its job without special configuration but if user wants to pass in custom readUser they can do this. This doesn't necessarily need to be just OS-based in the future but using /etc/passwd from some other snapshot or some other user database.

we can define a new interface

If it is just one function it can just be a callback as well.

I moved the readUser() code and defined a callback type. Have a look and let me know if you would prefer it done differently.

solver/llbsolver/file/backend.go

tonistiigi · 2023-09-06T20:19:17Z

solver/llbsolver/file/backend.go

@@ -266,7 +237,8 @@ func (fb *Backend) Mkdir(ctx context.Context, m, user, group fileoptypes.Mount,
 }
 defer lm.Unmount()

- u, err := readUser(action.Owner, user, group)
+ // u, err := readUser(action.Owner, user, group, fb.Executor)
+ u, err := fb.readUserFn(action.Owner, user, group)


This should not panic on nil.

What do you think if we unexport Backend{} and have NewFileOpBackend() return an error if we pass in a nil callback. I think we should always pass in a callback even if it's a noop. This way we can avoid having to check if the read user function is nill whenever we want to use it, and simply check once in the "constructor".

Just a nil check should do I think. Or defaulting to noop function.

tonistiigi · 2023-09-06T20:19:39Z

solver/llbsolver/file/backend.go

@@ -250,7 +210,18 @@ func docopy(ctx context.Context, src, dest string, action pb.FileActionCopy, u *
 return nil
 }

+// NewFileOpBackend returns a new file operation backend. The executor is currently only used for Windows,
+// and it is used to construct the readUserFn field set in the returned Backend.
+func NewFileOpBackend(readUserFn ReadUserCallback) *Backend {


Maybe makes sense to put this into opt stuct.

solver/llbsolver/ops/file.go

solver/llbsolver/ops/user_windows.go

tonistiigi

Looks like some comments are marked as resolved but I think you forgot to push your updates.

gabriel-samfira · 2023-10-06T07:48:24Z

Changes made. Apologies for the delay. I've been sidetracked for the past couple of weeks, and may be sparse for the next 2 weeks as well. I changed the readUser function to accept snapshot.Mountable and the conversion remains in the files package.

The Backend{} type has been un-exported and we now check that the readUser callback is not nil. From what I understand, that can't be a noop, as it will mean ignoring the USER/GROUP stanzas. It can be unsupported on a particular OS, but not nil. Correct me if I'm wrong on this 😄 .

gabriel-samfira · 2023-10-06T08:00:15Z

failures seem to be due to rate limits(?)

slonopotamus · 2023-10-06T09:54:25Z

failures seem to be due to rate limits(?)

Uh?

https:/moby/buildkit/actions/runs/6428415521/job/17455566735?pr=3518#step:4:446

264.5 solver/llbsolver/file/backend.go:215:51: unexported-return: exported func NewFileOpBackend returns unexported type *file.backend, which can be annoying to use (revive)
264.5 func NewFileOpBackend(readUser ReadUserCallback) (*backend, error) {
264.5

gabriel-samfira · 2023-10-06T09:56:58Z

failures seem to be due to rate limits(?)

Uh?

https:/moby/buildkit/actions/runs/6428415521/job/17455566735?pr=3518#step:4:446
264.5 solver/llbsolver/file/backend.go:215:51: unexported-return: exported func NewFileOpBackend returns unexported type *file.backend, which can be annoying to use (revive)
264.5 func NewFileOpBackend(readUser ReadUserCallback) (*backend, error) {
264.5  

Will fix later today. Apologies for the noise.

gabriel-samfira · 2023-10-06T14:07:01Z

@slonopotamus I had a quick look at one of the tests and stumbled across:

https:/moby/buildkit/actions/runs/6428415520/job/17455724114?pr=3518#step:8:1798

Which led me to believe it was a rate limit issue. I stopped there.

As for the linting issue, fixing that now.

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

gabriel-samfira · 2023-10-17T12:55:46Z

The failing test seems to be due to vagrant failing. @tonistiigi if you have a few minutes, let me know if the changes here are fine.

tonistiigi · 2023-10-20T23:46:08Z

solver/llbsolver/file/backend.go

+func (fb *Backend) readUserWrapper(owner *pb.ChownOpt, user, group fileoptypes.Mount) (*copy.User, error) {
+ var userMountable, groupMountable snapshot.Mountable
+ if user != nil {
+ usr, ok := user.(*Mount)


There is still something iffy about these type casts but looks like the previous code had a similar problem, so no update is needed with this PR.

I agree. There are a few architectural boundaries being broken here which could do with some refactor, but it will require touching a lot of code. This pattern is present in a number of places and should probably be tackled at some point as part of a broader cleanup.

Thanks for the review and the merge!

gabriel-samfira force-pushed the fileop-read-user branch from 9f84471 to 508b7fb Compare January 18, 2023 23:23

crazy-max added the area/windows label Jan 19, 2023

gabriel-samfira mentioned this pull request Feb 6, 2023

Implement readUser on Windows #3248

Closed

gabriel-samfira mentioned this pull request Feb 23, 2023

As a Developer I want to be able to build Windows Containers with BuildKit microsoft/Windows-Containers#34

Open

gabriel-samfira force-pushed the fileop-read-user branch from 508b7fb to cf6e4af Compare August 15, 2023 10:38

gabriel-samfira force-pushed the fileop-read-user branch 3 times, most recently from 0355063 to aca323e Compare August 30, 2023 20:54

gabriel-samfira marked this pull request as ready for review August 30, 2023 21:11

tonistiigi reviewed Aug 30, 2023

View reviewed changes

tonistiigi reviewed Sep 1, 2023

View reviewed changes

tonistiigi reviewed Sep 6, 2023

View reviewed changes

tonistiigi reviewed Sep 26, 2023

View reviewed changes

gabriel-samfira force-pushed the fileop-read-user branch from 0d782c2 to 8ddc21f Compare October 6, 2023 06:47

gabriel-samfira force-pushed the fileop-read-user branch from 8836e0f to 3339003 Compare October 10, 2023 07:22

gabriel-samfira requested a review from tonistiigi October 10, 2023 07:56

gabriel-samfira force-pushed the fileop-read-user branch from 3339003 to 120f330 Compare October 12, 2023 07:55

gabriel-samfira added 5 commits October 17, 2023 15:06

Implement readUser on Windows

b8c7bd5

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

Remove the need for an exported Executor field

8a369a9

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

Move readUser code outside of the file package

fe3ca93

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

Use snapshot.Mountable as an argument type to readUser

2f3bda8

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

Fix linting issue

2585dd9

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

gabriel-samfira force-pushed the fileop-read-user branch from 120f330 to 2585dd9 Compare October 17, 2023 12:06

tonistiigi approved these changes Oct 20, 2023

View reviewed changes

tonistiigi merged commit 762b262 into moby:master Oct 20, 2023
54 of 55 checks passed

TBBle mentioned this pull request Oct 22, 2023

Unable to use Buildkit with Windows containers #616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement readUser on Windows #3518

Implement readUser on Windows #3518

gabriel-samfira commented Jan 18, 2023 •

edited

Loading

gabriel-samfira commented Aug 30, 2023

tonistiigi Aug 30, 2023

gabriel-samfira Aug 31, 2023

tonistiigi Sep 1, 2023

gabriel-samfira Sep 5, 2023 •

edited

Loading

tonistiigi Sep 5, 2023

gabriel-samfira Sep 6, 2023

tonistiigi Sep 6, 2023

gabriel-samfira Sep 23, 2023

tonistiigi Sep 26, 2023

tonistiigi Sep 6, 2023

tonistiigi left a comment

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

slonopotamus commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 17, 2023

tonistiigi Oct 20, 2023

gabriel-samfira Oct 21, 2023

Implement readUser on Windows #3518

Implement readUser on Windows #3518

Conversation

gabriel-samfira commented Jan 18, 2023 • edited Loading

gabriel-samfira commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriel-samfira Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

slonopotamus commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 6, 2023

gabriel-samfira commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriel-samfira commented Jan 18, 2023 •

edited

Loading

gabriel-samfira Sep 5, 2023 •

edited

Loading