Rework users.GetUID #4262
Rework users.GetUID #4262
Conversation
| var uid string | ||
|
|
||
| if entry, err := user.Lookup(name); err != nil { | ||
| if !errors.Is(err, user.UnknownUserError(name)) { |
There was a problem hiding this comment.
errors.Is(err, user.UnknownUserError(name)) looks weird but seems to work.
Maybe returning a locally defined UnknownUserError = errors.New("unknown user") would make the function more pleasant to use.
There was a problem hiding this comment.
I was also pondering about this. The original code re-used this standard library error in this way. Changing the returned error would trickle down to callers, which are relying on this as well.
What about having a function for this instead, e.g.
func IsNotExist(err error) bool {
var UnknownUserError user.UnknownUserError
return errors.As(err, &UnknownUserError)
}Callers could then use if users.IsNotExist(err) { ... } ... Which would mimic the design of the standard library's os.Is... functions.
There was a problem hiding this comment.
Sounds reasonable. The os.Is... function docs now do say new code should use errors.Is(err, fs.ErrNotExist) but they're not going anywhere.
There was a problem hiding this comment.
Oh. Right. I'll have a look at your initial proposal first, then.
|
This pull request has merge conflicts that need to be resolved. |
|
This pull request has merge conflicts that need to be resolved. |
|
This pull request has merge conflicts that need to be resolved. |
|
This pull request has merge conflicts that need to be resolved. |
internal/pkg/users/users.go
Outdated
| firstLine, _, _ := bytes.Cut(exitErr.Stderr, []byte{'\n'}) | ||
| return UnknownUID, fmt.Errorf("%w (%w: %s)", err, idErr, string(firstLine)) | ||
| } | ||
| return UnknownUID, fmt.Errorf("%w (%w)", err, idErr) | ||
| } | ||
|
|
||
| uid = string(bytes.TrimSuffix(out, []byte{'\n'})) |
There was a problem hiding this comment.
Two different ways to get the first line here :) Maybe the stderr shouldn't be cut anyway?
There was a problem hiding this comment.
To be precise, this removes a trailing newline, and doesn't return only the first line, but all of them, if any. id is expected to only write a single line to stdout, though.
The stderr truncation is different. In case of an error, we never know how many lines will be written. Having only the first line in the error message seemed like a good idea to me, to not potentially pollute the error message but still be helpful. The standard error message of *exec.ExitError is not overly helpful on its own, it's usually just "exit status 1" and that's it.
|
The PR is marked as stale since no activity has been recorded in 30 days |
This allows for a more obvious fallback handling in case certain users don't exist. Add a special RootUID constant that can be used to explicitly assign a fallback UID. Also add log statements to every fallback assignment. Signed-off-by: Tom Wieczorek <[email protected]>
Signed-off-by: Tom Wieczorek <[email protected]>
There are quite some failure modes for GetUID. Sort them out by providing hopefully helpful error messages. Signed-off-by: Tom Wieczorek <[email protected]>
The `id` program is not always installed to /usr/bin. Rely on k0s's PATH to find the right executable. Signed-off-by: Tom Wieczorek <[email protected]>
The new name more closely matches the standard library function name. Also, "lookup" sounds less lightweight than "get", which gives a better sense of the actual runtime cost. Signed-off-by: Tom Wieczorek <[email protected]>
The zero value for a UID happens to be root's UID. Assuming root may or may not be a good default, depending on the use case. Setting file ownership to root is a restrictive and safe default, running programs as root is not. By returning "UnknownUID" as a constant to -1, callers must explicitly decide on the fallback instead of accidentally assuming root. Signed-off-by: Tom Wieczorek <[email protected]>
The etcd user ID is already stored in the etcd struct, so it can be reused. Signed-off-by: Tom Wieczorek <[email protected]>
Makes the errors.Is() checks nicer. On the contrary, the error message won't contain the user name anymore. Wrap the error accordingly on the caller side instead. Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
explicit-getuid
branch
from
1676c13 to
64dd416
Compare
Description
Rename to LookupUID
The new name more closely matches the standard library function name. Also, "lookup" sounds less lightweight than "get", which gives a better sense of the actual runtime cost.
Explicit usage and fallback handling
Prefer to call utility functions directly with a UID, rather than having them accept a username and call
LookupUIDinternally. This allows for a more obvious fallback handling in case certain users don't exist. Add a specialRootUIDconstant that can be used to explicitly fallback to the root user. Also add log statements to every fallback assignment. Use -1 as UID return value in case of errors. The zero value for a UID happens to be root's UID. Assuming root may or may not be a good default, depending on the use case. Setting file ownership to root is a restrictive and safe default, running programs as root is not. By returningUnknownUIDas a constant to -1, callers must explicitly decide on the fallback instead of accidentally assuming root.Use id program from PATH
The
idprogram is not always installed to/usr/bin. Rely on k0s'sPATHto find the right executable.More context for errors
There are quite some failure modes. Sort them out by providing hopefully helpful error messages. This might help when debugging things. Introduce
users.ErrNotFound, which makes theerrors.Is()checks nicer. On the contrary, the error message won't contain the user name anymore. Wrap the error accordingly on the caller side instead.Type of change
How Has This Been Tested?
Checklist: