Refactor and expose private functions within `libcontainer/user`. #158

cyphar · 2014-08-15T07:07:23Z

This patchset refactors much of the code in user/ (such as using structs to return user data and general readability improvements), exposes several previously private functions (parsePasswdFile, parseGroupFile) to allow for callers to pass io.Readers as a data source (massively improving the usability of the package) and adds several exhaustive unit tests for RunUser parsing.

Also, the gargantuan function signature user.GetUserGroupSupplementaryHome was changed to the more readable and understandable user.GetExecUser.

This patch was created to assist the implementation of moby/moby#7537, in order to ensure the RunUser configuration parsing can be handled consistently both internally to libcontainer, as well as external to libcontainer.

Signed-off-by: Aleksa Sarai cyphar@cyphar.com (github: cyphar)

cyphar · 2014-08-15T07:13:14Z

/cc @tianon @crosbymichael

LK4D4 · 2014-08-15T07:21:18Z

namespaces/init.go

@@ -151,26 +151,47 @@ func RestoreParentDeathSignal(old int) error {

 // SetupUser changes the groups, gid, and uid for the user inside the container
 func SetupUser(u string) error {
-	uid, gid, suppGids, home, err := user.GetUserGroupSupplementaryHome(u, syscall.Getuid(), syscall.Getgid(), "/")
+	passwd, err := os.Open("/etc/passwd")
+	if !os.IsNotExist(err) {


I think this is true for err == nil.

EDIT: So it is. Good catch.

I don't follow:

!os.IsNotExist(err) == true

so you returning error for nil error. Or this is right logic?

Sorry, you're quite right. I misread the line. Pushing fixed version.

mrunalp · 2014-08-15T18:04:35Z

namespaces/init.go

+		Home: "/",
+	}
+
+	runUser, err := user.GetRunUser(u, defaultRunUser, passwd, group)


I like the idea of using Readers, however, do we have an actual use case where we won't be reading from a file? I ask because this requires additional work on the caller to open these files instead of just passing the paths down.

Using readers has three main benefits:

It makes the interface for user/ far more generalised. This is more of a subjective point, because it means that people can use the functions without the need for the library to use the filesystem (so if you have some generated data or are reading from a network connection which has sent the data you can use user/ without needing to write a temporary file, etc). This results in the library being more testable and other minute benefits.

It allows you to pass nil readers that will be used to ignore the existence of a passwd or group source and to try to carry on without them. The most obvious use of this is for files that do not exist (which was supported by the original design), but what about if you want to come up with some scheme where you can only refer to a group by its gid or you want to enforce that only uid and gid formats are allowed. Or maybe you want all errors when opening a file to be ignored (which was not supported by the original interface).

It also creates separation of concern. The user/ library doesn't need to know whether or not the file exists or where it is on the filesystem (if it even is on the filesystem) or if it has read access, etc. It means that the caller can do its job in getting the data and user/ can do its job in parsing the data.

I agree with your points and also that this is subjective. There is something to be said about simplicity of using the API, though. For e.g. http://golang.org/pkg/os/user/#Lookup. It does what is expected on Linux. IMO, we should only add features, if there is a solid requirement. I think at minimum a wrapper API that passes in Readers for /etc/passwd and /etc/group should be provided, so the clients have a simpler API to call when possible. I would defer to @tianon and @crosbymichael on this one. ( I don't have a strong objection, just a subjective opinion ;) )

I have no problem with adding wrappers (like ParsePasswdFileFilter or something similar), where you pass a file path instead of an io.Reader and it treats errors the same way user does currently.

Also, the main difference between os/user and libcontainer/user is that libcontainer deals with more than one system and deals with abstract representations of containers and implements them on Linux. While I do agree that passing file paths makes the interface look simpler, it removes some of the functionality gained by using Readers.

I'm +1 to implement os/user interface here. So we'll have os/user on pure Go.

@LK4D4 I've added an os/user interface in user/lookup.go (with os-specific stuff in user/lookup_unix.go).

Yeah, I would definitely love to see some wrappers on this, especially if we could get most callers of this code using a version of this that doesn't have to hard-code /etc/passwd (I love having that path be OS-specific like you've got below, even though this is a pretty linux-specific class/library right now). I think it's good to keep in mind that this is being used outside of libcontainer/Docker too, so having an interface that is simple to use in the default case is a good thing.

Regarding whether proliferation of simple wrapper functions is a bad thing, I think it's absolutely a great thing, as evidenced by their use within the Go standard library itself. So I think having a wrapper that lets us just pass in file paths would be good (obviously containing this boiler-plate code of opening the files etc.), and then also a wrapper that requires no filenames to be passed in at all and just uses the functions provided within the module to have sensible defaults.

@tianon: Sure. I'll write a some wrappers for ParsePasswdFilter, ParsePasswd, ParseGroupFilter and ParseGroup to take a file path (probably with a signature like ParsePasswdFilterFile(path string, filter func(u User) bool) ([]User, error)).

cyphar · 2014-08-23T03:01:49Z

/ping @tianon @crosbymichael @LK4D4

PTAL. I've added an os/user-like interface (making calling ParsePasswdFilter not necessary in common use-cases). Are there any outstanding issues with this patchset in its current state?

LK4D4 · 2014-08-23T11:42:24Z

user/lookup.go

+	}
+
+	// No user entries found.
+	if users == nil || len(users) < 1 {


This should be len(users) == 0

LK4D4 · 2014-08-23T15:08:34Z

LGTM

cyphar · 2014-08-23T16:11:04Z

Just took another look at it, and I added a set of os/user-like APIs for groups (they were missing in the Go library, so I added them [since some projects, like Docker, do group lookups]).

/ping @LK4D4 @tianon @crosbymichael

LK4D4 · 2014-08-25T03:45:25Z

LGTM group api

cyphar · 2014-08-26T05:44:44Z

@tianon @LK4D4 I've added a few more wrappers along the lines of @tianon's suggestions. PTAL, hopefully there's nothing left to add 😉.

LK4D4 · 2014-08-26T08:21:03Z

I would appreciate if you'll do separate commits for new changes and squash commits after review.
Now LGTM

cyphar · 2014-08-26T14:43:34Z

Rebase'd (and unsquashed the commits) and fixed a small bug (I didn't Close the file handles in the wrappers).

crosbymichael · 2014-08-26T17:39:04Z

user/user.go

-		gid      = defaultGid
-		suppGids = []int{}
-		home     = defaultHome
+type RunUser struct {


What do you mean by a RunUser here?

@crosbymichael It's a struct that contains all information relevant to running as a specific user. Hence RunUser. I couldn't think of a better name, if you've got a better one, I'd love to use it instead.

EDIT: There. Fixed and changed to ExecUser which kinda reveals more of what it is actually used for. PTAL?

cyphar · 2014-08-29T08:55:51Z

I've changed RunUser to ExecUser (since it sounds much better). PTAL.

/cc @tianon @crosbymichael

tianon · 2014-09-04T23:03:00Z

namespaces/init.go

+		Home: "/",
+	}
+
+	execUser, err := user.GetExecUserFile(u, defaultExecUser, "/etc/passwd", "/etc/group")


Why are /etc/passwd and /etc/group still hard-coded here as well as inside the user package? Isn't there a wrapper that allows these to just be defaulted properly as one might expect them to be so we don't repeat ourselves, especially with constants like this that are going to be fairly common for every potential user of the module?

Whoops. That's precisely the reason why I created the whole lookup_unix.go thing. I'll update it now.

tianon · 2014-09-04T23:27:16Z

Other than my one remaining gripe in the UX, this looks pretty nice to me at a high-level review.

cyphar · 2014-09-06T01:11:21Z

@tianon There. I've fixed that gripe in the UX (and made unsupported operating systems not give compile-time errors). PTAL (hopefully for the last time 😉)?

tianon · 2014-09-08T17:59:16Z

user/user.go

 		}
-		return 0, 0, nil, "", fmt.Errorf("Unable to find user %v: %v", userArg, err)
+		return ExecUser{}, fmt.Errorf("Unable to find user %v: %v", userArg, err)


Do you think it might be worth switching this return value to a *ExecUser so we can keep the super-simple nil returns? Maybe @crosbymichael can weigh in on his preference here. I'm good either way, but figured it's worth discussing briefly.

I think he has it this way because @LK4D4 said not to use pointers because it's a small type and we don't need the overhead but I think its a bad UI, you don't see many stdlib functions return something like this.

@crosbymichael @tianon: While I understand wanting nil returns (and that is fair enough), the reason I did it this way was because ExecUser struct values are just a representation of an abstract concept, and thus don't make sense to be pointer-ised (is that even a word). Basically, the way I've always learnt to use pointers is:

Only use pointers if the data it points to may change underneath you, and such changes make the old values invalid.

In other words, things like terminal or server structs might make sense to pointer-ise (the terminal or server may change states, and thus the old value is invalid) but when you're dealing with abstract representations of parsed user specifications it doesn't really (IMO) make sense to make them pointers (since the actual point of a pointer -- the ability to change the value it points to, and all users of that pointers now use the updated value -- is not used).

EDIT: Also, the old function didn't use nil returns -- it returned all of the struct fields as separate return values -- so we haven't really lost anything.

/ping @tianon @crosbymichael Any thoughts about my argument?

I don't think "status quo" is a very compelling argument here, especially
since improving this interface is the whole impetus of this PR, right? :)

That's fair enough, I'll make the change.

tianon · 2014-09-08T18:00:23Z

LGTM (with that one point I'd like @crosbymichael's thoughts on)

cyphar · 2014-10-21T21:05:13Z

/ping @tianon
Any updates / comments?

tianon · 2014-11-04T23:07:20Z

Sorry for the long tail; LGTM now

(love the extra testing; that's the biggest benefit from this PR, IMO, as a direct result of the cleaner interface)

LK4D4 · 2014-11-04T23:12:12Z

And now it needs rebase :)

cyphar · 2014-11-06T11:22:00Z

There, rebase'd. PTAL 😄.

/cc @tianon @LK4D4

tianon · 2014-11-06T17:07:28Z

Still LGTM; it's up to the other maintainers now.

cyphar · 2014-11-06T23:24:29Z

Whoops, forgot this patchset included changes to namespaces. Can the maintainers of namespaces PTAL at this patchset?

/cc @crosbymichael @rjnagal @vmarmol @mrunalp

This patch refactors most of GetUserGroupSupplementaryHome and its signature, to make using it much simpler. The private parsing ftunctions have also been exposed (parsePasswdFile, parseGroupFile) to allow custom data source to be used (increasing the versatility of the user/ tools). In addition, file path wrappers around the formerly private API functions have been added to make usage of the API for callers easier if the files that are being parsed are on the filesystem (while the io.Reader APIs are exposed for non-traditional usecases). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)

This patch adds an os/user-like user lookup API, implemented in pure Go. It also has some features not present in the standard library implementation (such as group lookups). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)

crosbymichael · 2014-11-07T00:17:12Z

LGTM

Refactor and expose private functions within `libcontainer/user`.

LK4D4 reviewed Aug 15, 2014
View reviewed changes

cyphar mentioned this pull request Aug 15, 2014

Transition usage of libcontainer/user to use new interface moby/moby#7591

Closed

mrunalp reviewed Aug 15, 2014
View reviewed changes

cyphar mentioned this pull request Aug 20, 2014

Proposal: Make USER change the uid and gid of COPY'd files moby/moby#7537

Closed

3 tasks

LK4D4 reviewed Aug 23, 2014
View reviewed changes

crosbymichael reviewed Aug 26, 2014
View reviewed changes

tianon reviewed Sep 4, 2014
View reviewed changes

tianon reviewed Sep 8, 2014
View reviewed changes

cyphar added 3 commits November 7, 2014 11:07

user: lookup: added os/user-like lookup API

195d8d5

This patch adds an os/user-like user lookup API, implemented in pure Go. It also has some features not present in the standard library implementation (such as group lookups). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)

user: add unit tests for GetExecUser

bf12e3c

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)

crosbymichael added a commit that referenced this pull request Nov 7, 2014

Merge pull request #158 from cyphar/refactor-expose-user

4ae31b6

Refactor and expose private functions within `libcontainer/user`.

crosbymichael merged commit 4ae31b6 into docker-archive:master Nov 7, 2014

cyphar deleted the refactor-expose-user branch November 7, 2014 07:50

cyphar mentioned this pull request Nov 13, 2014

Implement USER setting owner of COPY'd files. moby/moby#9046

Closed

Refactor and expose private functions within libcontainer/user. #158

Refactor and expose private functions within libcontainer/user. #158

Conversation

cyphar commented Aug 15, 2014

cyphar commented Aug 15, 2014

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

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

cyphar commented Aug 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented Aug 23, 2014

cyphar commented Aug 23, 2014

LK4D4 commented Aug 25, 2014

cyphar commented Aug 26, 2014

LK4D4 commented Aug 26, 2014

cyphar commented Aug 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Aug 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Sep 4, 2014

cyphar commented Sep 6, 2014

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

tianon commented Sep 8, 2014

cyphar commented Oct 21, 2014

tianon commented Nov 4, 2014

LK4D4 commented Nov 4, 2014

cyphar commented Nov 6, 2014

tianon commented Nov 6, 2014

cyphar commented Nov 6, 2014

crosbymichael commented Nov 7, 2014

Refactor and expose private functions within `libcontainer/user`. #158

Refactor and expose private functions within `libcontainer/user`. #158