overlord/snapshotstate/backend: fall back on sudo when no runuser #5957
Conversation
chipaca
force-pushed
the
fallback-to-sudo
branch
from
2bb7c1d
to
c4836cc
Compare
Without this change, snapshots don't work on 14.04.
chipaca
force-pushed
the
fallback-to-sudo
branch
from
c4836cc
to
99c2c73
Compare
| // runuser and sudo happen to work the same way in this case. | ||
| // The main reason to prefer runuser over sudo is that runuser | ||
| // is part of util-linux, which is considered essential, | ||
| // whereas sudo is an addon which could be removed. |
There was a problem hiding this comment.
Thanks for adding a note in case we forget the reasoning again.
| // is part of util-linux, which is considered essential, | ||
| // whereas sudo is an addon which could be removed. | ||
| for _, cmd := range []string{"runuser", "sudo"} { | ||
| if lp, err := execLookPath(cmd); err == nil { |
There was a problem hiding this comment.
Nitpick, looking for the actual command could be done in sync.Once(), eg.
var cmdSeekedFor sync.Once
var sudoOrRunuser string
func pickUserWrapper() {
for _, cmd := range []string{"runuser", "sudo"} {
if lp, err := execLookPath(cmd); err != nil {
sudoOrRunuser = lp
break
}
}
}
func maybeRunuserCommand(.....) {
cmdSeekedFor.Do(pickUserWrapper)
if sysGeteuid() == 0 && username != "root" && sudoOnRunuser != "" {
....
}
return exec.Command(args[0], args[1:]...)
}
There was a problem hiding this comment.
hm, might as well do it in init, n'eh?
| // The main reason to prefer runuser over sudo is that runuser | ||
| // is part of util-linux, which is considered essential, | ||
| // whereas sudo is an addon which could be removed. | ||
| for _, cmd := range []string{"runuser", "sudo"} { |
There was a problem hiding this comment.
Should we log an error/warning when neither is found?
Also, log when neither sudo nor runuser is available for use.
chipaca
force-pushed
the
fallback-to-sudo
branch
from
e2920f2
to
6038f4a
Compare
| // runuser and sudo happen to work the same way in this case. | ||
| // The main reason to prefer runuser over sudo is that runuser | ||
| // is part of util-linux, which is considered essential, | ||
| // whereas sudo is an addon which could be removed. |
There was a problem hiding this comment.
Hold on, but the rationale for adding this branch is that we ... don't have runuser everywhere, right?
There was a problem hiding this comment.
Correct, 14.04 has an old util-linux that doesn't have runuser.
There was a problem hiding this comment.
May be worth mentioning in the comment that runuser is avaialble since util-linux 2.23 and some old systems that we support are on an even older version.
| return "" | ||
| } | ||
|
|
||
| var sudo = pickUserWrapper() |
There was a problem hiding this comment.
I think the variable name is no longer appropriate. Perhaps launcher?
There was a problem hiding this comment.
launcher says nothing about what the thing is though...
There was a problem hiding this comment.
I could unify things:
pickUserWrapperis alright, i think- so
sudo->userWrapper - so
maybeRunuserCommand->userWrapCommand?
| // runuser only works for euid 0, and doesn't make sense for root | ||
| return exec.Command(args[0], args[1:]...) | ||
| if sysGeteuid() == 0 && username != "root" && sudo != "" { | ||
| sudoArgs := make([]string, len(args)+4) |
There was a problem hiding this comment.
While perhaps a little bit YAGNI for now, since I assume the code works, I would consider having the launcher be an interface with a function RunWithArgs() that encapsulates this kind of knowledge. Right now there's a hidden assumption that "sudo" behaves like sudo, down the the command line handling.
There was a problem hiding this comment.
indeed, YAGNI. If they needed different handling (e.g., if we were trying to use su or sudo), then it'd do something different. Still probably not an interface thing.
|
|
||
| return exec.Command("runuser", sudoArgs...) | ||
| // TODO: use warnings instead | ||
| logger.Noticef("No user wrapper found; running tar for user data as root. Please make sure 'sudo' or 'runuser' (from util-linux) is on $PATH to avoid this.") |
There was a problem hiding this comment.
Hmm, when this is the case what would happen? Should things just fail loudly?
There was a problem hiding this comment.
I'm not sure. It'll break on NFS and you'll get an error. Other than that it should be fine.
There was a problem hiding this comment.
Ah, that's a good thing to know. We can already check if home is using NFS so perhaps all of this can be optional eventually?
…ew concerns This change renames the maybeRunUser helper to be tarAsUser, and specializes it to _only_ run tar instead of arbitrary commands. This makes it that little bit clearer why the fail-open case is Ok, and why it's not generally useful as-is. Also unifies sudo/runuser variables to be userWrapper. Thank you @zyga, I think it is clearer this way.
| // runuser and sudo happen to work the same way in this case. | ||
| // The main reason to prefer runuser over sudo is that runuser | ||
| // is part of util-linux, which is considered essential, | ||
| // whereas sudo is an addon which could be removed. |
There was a problem hiding this comment.
May be worth mentioning in the comment that runuser is avaialble since util-linux 2.23 and some old systems that we support are on an even older version.
| return "", errors.New("no such thing") | ||
| })() | ||
|
|
||
| c.Check(backend.PickUserWrapper(), check.Equals, "") |
There was a problem hiding this comment.
Interesting, I would expect this to return an error and not just an empty string. Reading on.
|
|
||
| c.Check(backend.TarAsUser("test", "--bar"), check.DeepEquals, &exec.Cmd{ | ||
| Path: "/bin/tar", | ||
| Args: []string{"tar", "--bar"}, |
| Args: uwArgs, | ||
| } | ||
| } | ||
| // TODO: use warnings instead |
There was a problem hiding this comment.
Well, do use them :) any reason not to yet?
There was a problem hiding this comment.
After writing this I realised it would be nice to have a helper to write tests like c.Assert(..., WarnsAbout, ...)
There was a problem hiding this comment.
to add a warning you need a handle on state, which is four calls up and to the lest of this. Needs some work to have a helper.
| if sysGeteuid() == 0 && username != "root" { | ||
| if userWrapper != "" { | ||
| uwArgs := make([]string, len(args)+5) | ||
| copy(uwArgs[5:], args) |
There was a problem hiding this comment.
Nitpick, it would read more naturally, with progressing index variable, if copy was after uwArgs[4] = ... assignment below.
Without this change, snapshots don't work on 14.04.