snap: add support for parsing snap layout section #3636

Merged
merged 4 commits into from Aug 14, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Jul 31, 2017

The layout section defines the "shape" of the runtime mount namespace
of a given snap. Layout is entirely optional and is empty by default.

Tree kind of layout entries can be defind:

  • bind mounts, designated by the presence of the "bind" field
  • tmpfs mounts, designated by the "type" field of "tmpfs"
  • symlinks, designated by the presence of the "symlink" field

In addition, all kinds can use the "user", "group" and "mode" fields
to define the ownership and permissions of any parent directories that
may need to be created. At present those are limited to "root", "nobody"
and at most "0755" permissions.

Any path designators can expand the following environment variables:
$SNAP, $SNAP_DATA and $SNAP_COMMON.

Forum: https://forum.snapcraft.io/t/feature-snap-layouts/1471
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

@zyga zyga requested a review from niemeyer Aug 3, 2017

snap: add support for parsing snap layout section
The layout section defines the "shape" of the runtime mount namespace
of a given snap. Layout is entirely optional and is empty by default.

Tree kind of layout entries can be defind:
 - bind mounts, designated by the presence of the "bind" field
 - tmpfs mounts, designated by the "type" field of "tmpfs"
 - symlinks, designated by the presence of the "symlink" field

In addition, all kinds can use the "user", "group" and "mode" fields
to define the ownership and permissions of any parent directories that
may need to be created. At present those are limited to "root", "nobody"
and at most "0755" permissions.

Any path designators can expand the following environment variables:
$SNAP, $SNAP_DATA and $SNAP_COMMON.

Forum: https://forum.snapcraft.io/t/feature-snap-layouts/1471
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

codecov-io commented Aug 3, 2017

Codecov Report

Merging #3636 into master will increase coverage by 0.03%.
The diff coverage is 91.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3636      +/-   ##
==========================================
+ Coverage   75.32%   75.36%   +0.03%     
==========================================
  Files         389      389              
  Lines       33666    33748      +82     
==========================================
+ Hits        25359    25434      +75     
- Misses       6489     6494       +5     
- Partials     1818     1820       +2
Impacted Files Coverage Δ
snap/info.go 88.62% <ø> (ø) ⬆️
snap/info_snap_yaml.go 94.05% <87.5%> (-0.5%) ⬇️
snap/validate.go 95.39% <93.22%> (-1.38%) ⬇️
interfaces/sorting.go 98.71% <0%> (-1.29%) ⬇️
cmd/snap/cmd_aliases.go 95% <0%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1398214...0faf6fd. Read the comment docs.

@zyga zyga requested a review from mvo5 Aug 4, 2017

mvo5 approved these changes Aug 7, 2017

Code looks good from my POV, I had a hunch that VlidatePathVariables() could be written in a simpler way but I played around a bit and my version was not easier to read so my hunch was probably wrong.

mostly ok, but needs a fix

snap/info_snap_yaml.go
+ for path, l := range y.Layout {
+ mode := 0755
+ if l.Mode != "" {
+ m, err := strconv.ParseInt(l.Mode, 8, 32)
@chipaca

chipaca Aug 7, 2017

Member

mode_t is unsigned, and os.FileMode is a uint32; math.MaxUint32 is a valid os.FileMode, and would fail to parse here. But also, os.ModeDir, a constant pre-defined probably-not-that-rare file mode, is not a valid int32.

@zyga

zyga Aug 8, 2017

Contributor

Done, I think.

@chipaca

chipaca Aug 8, 2017

Member

nope, you're still using ParseInt instead of ParseUint

@zyga

zyga Aug 8, 2017

Contributor

Ah, I didn't notice that the parsing function is incorrect too. Thank you!

@@ -170,3 +177,73 @@ func ValidateApp(app *AppInfo) error {
}
return nil
}
+
+// ValidatePathVariables ensures that given path contains only $SNAP, $SNAP_DATA or $SNAP_COMMON.
@chipaca

chipaca Aug 7, 2017

Member

interesting that we don't support having a $ that isn't interpolating a variable (i.e., no \$)

@zyga

zyga Aug 8, 2017

Contributor

Yeah, I kind of think this is a feature though :)

+ }
+ return nil
+}
+
@chipaca

chipaca Aug 7, 2017

Member
// ValidateLayout does unspeakable things to the layout
@zyga

zyga Aug 8, 2017

Contributor

ValidateLayout makes someone's hair gray ;-)

@zyga

zyga Aug 8, 2017

Contributor

Documented

zyga added some commits Aug 8, 2017

snap: use os.FileMode for Mode
Thanks to Chipaca for spotting the problem!

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: document ValidateLayout
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

chipaca approved these changes Aug 8, 2017

thank you

snap: use ParseUint to parse file mode
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga merged commit eb87e2d into snapcore:master Aug 14, 2017

6 of 7 checks passed

xenial-i386 autopkgtest finished (failure)
Details
artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:feature/layout-snap-yaml branch Aug 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment