osutil: pass --extrausers option to groupdel

[mardy]

Do not shy off from deleting groups using the --extrausers option, if
needed.

See https://bugs.launchpad.net/bugs/1840375

Needs someone (@pedronis ?) to decide what to do on systems where groupdel does not support the --extrausers option: this branch currently silently ignores the error (meaning that groupdel has failed but the user is not aware of it), because this leads to the same outcome we had before this branch. But we might want to return an error instead?

[mardy]

Well, actually, the code is returning an error in any case here, so I guess we can simplify the code and always return the error describing the groupdel failure.

[anonymouse64]

lgtm, but I think we could at least log in the case where it fails with the --extrausers flag

[anonymouse64]

probably IMHO we should just log an error and return nil. The previous code gave no indication anything was wrong, whereas if we at least log the user will see that some part of the group deletion failed.

[anonymouse64]

after reads a bit weird here, not sure the right way to phrase it is, but usually we will use the pattern while doing x, error foo happened, but really these two errors are orthogonal to each other IMHO, so maybe the thing to do here is to make them separate errors, one each line:

errors encountered ensuring group foo exists:
- groupdel does not support --extrausers option
- useradd failed with foo

[mardy]

I've read about go's support for nested errors (with the %w modifier passed to Errorf); do you think it makes sense to use it here?

[anonymouse64]

As we discussed in the meeting, %w is only meant to be used in the case where callers want to inspect an error type, if we did something simple like:

return fmt.Errorf("some syscall failed while doing foo: %v",err)

then if err was say a os.PathError, the standard library says you can check for that specific error with os.IsNotExists, but now we created a new error type from Errorf which doesn't match the os.PathError, so that check would fail. The solution from the Go team to this was to introduce %w which wraps errors so that if we did this:

return fmt.Errorf("some syscall failed while doing foo: %w",err)

now the error returned by Errorf will be wrapped such that the os.IsNotExists function would return true since the IsNotExists function now can use the Unwrap() and As() or Is() functions from go's errors package (see https://golang.org/pkg/errors/).

As was pointed out though, since we use an old go version for some distros, we cannot use the errors package directly, and instead we have to do sneaky tricks like use the xerrors package and define our format strings as constants, since old Go does not know about the %w verb. See here for an example: https://github.com/snapcore/snapd/blob/master/bootloader/lkenv/lkenv.go#L264-L275

[anonymouse64]

but also as explained wrapped errors is not what we want here since:

• the callers of this function probably don't care to inspect the different error cases
• the errors are not related to each other

[mardy]

Done!

[anonymouse64]

+1 from me now on the error message, but bonus points if you fix the existing confusing error message too 😄

[anonymouse64]

not a big deal, but would be nice to change this error message too, I just noticed that it was like this originally that's probably where you got your error message format from 😄

[mardy]

Indeed it was :-)

I updated the branch now.

[anonymouse64]

still LGTM, thanks for fixing that other error message

[mvo5]

Looks fine, just one suggestion for simplification

[mvo5]

This is tricky - if this code runs localized we will not detect the error. We could always run userdel with "LC_ALL=C.UTF-8" but then the error could be confusing to the user. My feeling is we should just remove the if extraUsers special case and always create the "OutputErr". I.e. something like:

diff --git a/osutil/user.go b/osutil/user.go
index 851d268033..6ebf45e714 100644
--- a/osutil/user.go
+++ b/osutil/user.go
@@ -20,7 +20,6 @@
 package osutil
 
 import (
-       "bytes"
        "fmt"
        "os"
        "os/exec"
@@ -154,13 +153,7 @@ func EnsureUserGroup(name string, id uint32, extraUsers bool) error {
                delCmdStr = append(delCmdStr, name)
                cmd = exec.Command(delCmdStr[0], delCmdStr[1:]...)
                if output2, err2 := cmd.CombinedOutput(); err2 != nil {
-                       var groupdelErrStr string
-                       if extraUsers && bytes.Contains(output2, []byte("unrecognized option")) {
-                               // We have hit https://bugs.launchpad.net/bugs/1840375
-                               groupdelErrStr = "groupdel does not support '--extrausers' option"
-                       } else {
-                               groupdelErrStr = OutputErr(output2, err2).Error()
-                       }
+                       groupdelErrStr := OutputErr(output2, err2).Error()
                        return fmt.Errorf(`errors encountered ensuring user %s exists:
 - %s
 - %s`, name, useraddErrStr, groupdelErrStr)
diff --git a/osutil/user_test.go b/osutil/user_test.go
index 3227670025..996f899137 100644
--- a/osutil/user_test.go
+++ b/osutil/user_test.go
@@ -567,7 +567,7 @@ func (s *ensureUserSuite) TestEnsureUserGroupFailedUseraddCoreNoExtra(c *check.C
        err := osutil.EnsureUserGroup("lakatos", 123456, true)
        c.Assert(err, check.ErrorMatches, `errors encountered ensuring user lakatos exists:
 - useradd failed with: some error
-- groupdel does not support '--extrausers' option`)
+- groupdel: unrecognized option '--extrauser'`)
 
        c.Check(mockGroupDel.Calls(), check.DeepEquals, [][]string{
                {"groupdel", "--extrausers", "lakatos"},

and it seems the error is still readable enough(?)

[mardy]

Oh, right! I could just search for the --extrausers string, which should be printed regardless of the locale, but indeed all this if block is a bit too much for just getting a slightly better error message. So I'll happily apply your suggestion :-)

[mardy]

Done!