Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/mount: add support for parsing x-snapd.{mode,uid,gid}= #3965
Conversation
zyga
added some commits
Sep 26, 2017
zyga
requested a review
from
mvo5
Sep 26, 2017
Some checks were not successful
|
Drat, I used golang 1.8-only library. Let me improve that. |
|
This will be improved once #3966 lands |
zyga
added some commits
Oct 2, 2017
All checks have failed
zyga
changed the title from
interfaces/mount: add support for parsing x-snapd-{mode,user,group}=
to
interfaces/mount: add support for parsing x-snapd-mkdir-{mode,uid,gid}=
Oct 2, 2017
|
This should now work on older version of golang and has streamlined API that's more clear about the purpose. |
All checks have failed
stolowski
requested changes
Oct 2, 2017
Looks good overall, some quick feedback, will take another look later.
| @@ -192,8 +195,63 @@ func OptsToFlags(opts []string) (flags int, err error) { | ||
| case "strictatime": | ||
| flags |= syscall.MS_STRICTATIME | ||
| default: | ||
| - return 0, fmt.Errorf("unsupported mount option: %q", opt) | ||
| + if !strings.HasPrefix(opt, "x-snapd-") { |
zyga
Oct 2, 2017
Contributor
Just future proofing. I can make this an explicit list but not sure if we need that.
| + return uid, nil | ||
| + } | ||
| + } | ||
| + return uid, nil |
stolowski
Oct 2, 2017
Contributor
I think what you did below in XSnapdMkdirGid returning (0, nil) in such case is better and more readable.
| + gid, err = osutil.FindGid(kv[1]) | ||
| + if err != nil { | ||
| + // The error message is not very useful so just skip it. | ||
| + return gid, fmt.Errorf("cannot resolve group name %q", kv[1]) |
stolowski
Oct 2, 2017
Contributor
Looking at osutil.FindGid (in particular at strconv.ParseUint) it's not totally clear that gid will be sane in case of error. I think it would make sense to play safe and return (0, err) here.
Same comment applies to FindUid above.
All checks have failed
|
Hmm, it seems we now cannot build snapd without cgo. Let me rethink this branch. |
|
Aha, I figured out a way to make this work. I just need to move this to snap-update-ns which is already using cgo and where this limitation does not apply. Fortunately I have a PR for parts of this already. |
zyga
added some commits
Oct 5, 2017
Some checks were not successful
codecov-io
commented
Oct 5, 2017
•
Edited 2 times
-
codecov-io
Oct 25, 2017
-
codecov-io
Oct 5, 2017
-
Oct 25, 2017 -
Oct 5, 2017
Codecov Report
@@ Coverage Diff @@
## master #3965 +/- ##
==========================================
+ Coverage 75.77% 75.81% +0.03%
==========================================
Files 433 434 +1
Lines 37293 37341 +48
==========================================
+ Hits 28259 28309 +50
+ Misses 7059 7057 -2
Partials 1975 1975
Continue to review full report at Codecov.
|
|
Oh, I still have one issue on golang 1.6 - |
zyga
added some commits
Oct 5, 2017
Some checks were not successful
Some checks were not successful
zyga
changed the title from
interfaces/mount: add support for parsing x-snapd-mkdir-{mode,uid,gid}=
to
interfaces/mount: add support for parsing x-snapd.{mode,uid,gid}=
Oct 5, 2017
Some checks were not successful
stolowski
approved these changes
Oct 5, 2017
Thanks for addressing my previous comments. Please add test checks for sane uid/gid on error, and then +1.
| + // Unknown group names are invalid. | ||
| + e = &mount.Entry{Options: []string{"x-snapd.gid=.bogus"}} | ||
| + _, err = update.XSnapdGid(e) | ||
| + c.Assert(err, ErrorMatches, `cannot resolve group name ".bogus"`) |
stolowski
Oct 5, 2017
Contributor
Can you also add a check that the returned gid is 0 on error? Same for uid test above.
All checks have passed
zyga
referenced this pull request
Oct 5, 2017
Merged
cmd/snap-update-ns: create missing mount points automatically. #4008
zyga
requested a review
from
jdstrand
Oct 11, 2017
| +} | ||
| + | ||
| +// XSnapdUid returns the user associated with x-snapd-user mount option. If | ||
| +// the mode is not specified explicitly then a default "root" use is |
mvo5
Oct 16, 2017
Collaborator
Maybe: then a default "0" (root) id is returned ? Or use->user at least :)
| +} | ||
| + | ||
| +// XSnapdGid returns the user associated with x-snapd-user mount option. If | ||
| +// the mode is not specified explicitly then a default "root" use is |
| + c.Assert(uid, Equals, uint64(0)) | ||
| + | ||
| + // User is parsed from the x-snapd-user= option. | ||
| + daemonUid, err := osutil.FindUid("daemon") |
mvo5
Oct 16, 2017
Collaborator
(nitpick) I would this below the XSnapUid() to make it more obvious that this is part of the "check" part of the "prepare, run, check" part of the test. Or maybe add a small helper findUid(c *C, name string) to avoid having to deal with error?
| + c.Assert(gid, Equals, uint64(0)) | ||
| + | ||
| + // Group is parsed from the x-snapd-group= option. | ||
| + daemonGid, err := osutil.FindGid("daemon") |
|
Done, tests will fail until #4048 is merged though. |
jdstrand
approved these changes
Oct 24, 2017
Looks fine overall. Approving in the interest of time, but please see the inline comments for a couple things to consider and comment changes.
| + if strings.HasPrefix(opt, "x-snapd.mode=") { | ||
| + kv := strings.SplitN(opt, "=", 2) | ||
| + var mode os.FileMode | ||
| + n, err := fmt.Sscanf(kv[1], "%o", &mode) |
jdstrand
Oct 24, 2017
Contributor
The Sscanf here is fine but I wonder if it would be more robust with a regex to validate the input?
| + for _, opt := range e.Options { | ||
| + if strings.HasPrefix(opt, "x-snapd.uid=") { | ||
| + kv := strings.SplitN(opt, "=", 2) | ||
| + uid, err = osutil.FindUid(kv[1]) |
jdstrand
Oct 24, 2017
Contributor
This and XsnapGid do not support specifying by uid number (ie, x-snapd.uid=123 will fail). If this is intentional, can you please add a comment?
zyga
Oct 25, 2017
Contributor
Interesting. Since numbers are not valid user or group names we could extend this code to just return that number directly. I don't think we need this just yet but it's a natural extension to how this could work.
| + kv := strings.SplitN(opt, "=", 2) | ||
| + uid, err = osutil.FindUid(kv[1]) | ||
| + if err != nil { | ||
| + // The error message is not very useful so just skip it. |
jdstrand
Oct 24, 2017
Contributor
I guess you mean 'The error message from FindUid is not very useful so just skip it?` Also see XSnapGid.
| + if err != nil { | ||
| + // The error message is not very useful so just skip it. | ||
| + return 0, fmt.Errorf("cannot resolve user name %q", kv[1]) | ||
| + } |
jdstrand
Oct 24, 2017
Contributor
The code will return '0' as the uid if it can't find the uid, which seems to be papering over the error. Perhaps return math.MaxUint64? So long as consumers always check the error, not a blocker.
| @@ -161,4 +161,10 @@ func (s *entrySuite) TestOptsToFlags(c *C) { | ||
| c.Assert(flags, Equals, syscall.MS_RDONLY|syscall.MS_NODEV|syscall.MS_NOSUID) | ||
| _, err = mount.OptsToFlags([]string{"bogus"}) | ||
| c.Assert(err, ErrorMatches, `unsupported mount option: "bogus"`) | ||
| + // The x-snapd-prefix is reserved for non-kernel parameters that do not | ||
| + // translate to kernel level mount flags. This is similar to systemd or | ||
| + // udisks use fstab options to convey additional data. |
zyga
added some commits
Oct 25, 2017
zyga commentedSep 26, 2017
This branch adds support for parsing three additional fstab options that are
specific to snapd's layout feature. The three options in combination shall
allow snap-update-ns to create the correct mount point on demand.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com