interfaces/builtin: add support for content "source" section #4068

zyga · Oct 23, 2017

This branch alters the content interface to support the new syntax using
"source" section on content slots. This allows a content slot to share a
number of entries (both read and write at the same time).

When a "source" section is in use, the target directory on the plug
becomes a spool-like directory where content connections from various
connected slots is aggregated.

In addition, when entries would clash they are now automatically
renamed, which should help supporting plugin-type sharing.

This change itself is simple but will only work after the automatic
directory creation and hole poking patches are merged.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com
Forum: https://forum.snapcraft.io/t/improvements-in-the-content-interface/2387/4

codecov-io · Oct 23, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@54ded85). Click here to learn what that means.
The diff coverage is 89.65%.

@@            Coverage Diff            @@
##             master    #4068   +/-   ##
=========================================
  Coverage          ?   75.88%           
=========================================
  Files             ?      440           
  Lines             ?    38299           
  Branches          ?        0           
=========================================
  Hits              ?    29062           
  Misses            ?     7222           
  Partials          ?     2015

Impacted Files	Coverage Δ
interfaces/builtin/content.go	`86.71% <100%> (ø)`
interfaces/mount/spec.go	`85.41% <66.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 54ded85...c002b13. Read the comment docs.

stolowski · Oct 26, 2017

stolowski reviewed Oct 26, 2017

View changes

Quick first pass. Since you're on this, could you also address the problem of cleanSubPath which incorrectly rejects "$SNAP/" (but accepts "$SNAP") making sanitize fail, while resolveSpecialVariable would be fine with it?

I think it would be worth checking if the new syntax with "source" is not mixed with the old one ("read" or "write" at the same level as "source", in which case "source" would win and "old" ones would be silently ignored I think).

Done in 2777b37

stolowski · Oct 26, 2017

stolowski reviewed Oct 26, 2017

View changes

What happens if both read and write have same entries?

This is pretty interesting, we'll get a clash and those get de-conflicted. I'll add a test that shows what would happen.

Done in 06c8556

mvo5 · Nov 8, 2017

mvo5 reviewed Nov 8, 2017

View changes

Looks very nice, thanks for working on this. Tiny nitpick inside and one question about the clash handling.

Maybe a bit more context here? i.e. move the "read" attribute of slot %q into the "source" section and pass slot.Name in ? Same below.

Maybe a small comment like: // check if new style syntax is used ? Or maybe even a tiny helper. AIUI this check for the slot.Attr["source"].(map...) is just doing that, right?

This is nice - but I slightly wonder about the results here. In the corresponding test I see /var/snap/consumer/common/import/directory and /var/snap/consumer/common/import/directory-2 one is ro, one is rw. This makes me wonder two things: a) is the order stable, i.e. is directory-2 always the rw one? b) will that be useful for an application? i.e. what is the advantage over just rejecting clashing names?

I'll answer the ro/rw aspect separately.

The de-clashing allows us to have a system where plugin authors don't have to coordinate with each on directory names. The de-clashing aspect is also described on the forum (link is in the commit message).

zyga · Nov 9, 2017

I'm marking this as blocked for now. I only plan to land this after the bulk of the backend work in snap-update-ns allows this interface to operate (along with spread test to confirm that).

zyga added the Blocked label Nov 9, 2017

zyga referenced this pull request Jan 8, 2018
Open
cmd/snap-update-ns: enable writable mimic, allow content sharing to $SNAP #4452

@@ -124,7 +123,12 @@ func (iface *contentInterface) path(slot *interfaces.Slot, name string) []string
          		panic("internal error, path can only be used with read/write")
          	}
-        -	paths, ok := slot.Attrs[name].([]interface{})
+        +	source, ok := slot.Attrs["source"].(map[string]interface{})

+        +     - $SNAP/read-snap
+        +    write:
+        +     - $SNAP_COMMON/write-common
+        +     - $SNAP_DATA/write-data

+        +	// Error if "read" or "write" are present alongside "source".
+        +	if _, ok := slot.Attrs["source"]; ok {
+        +		if _, ok := slot.Attrs["read"]; ok {
+        +			return fmt.Errorf(`move the "read" attribute into the "source" section`)

@@ -165,9 +179,15 @@ func mountEntry(plug *interfaces.Plug, slot *interfaces.Slot, relSrc string, ext
          	options := make([]string, 0, len(extraOptions)+1)
          	options = append(options, "bind")
          	options = append(options, extraOptions...)
+        +	source := resolveSpecialVariable(relSrc, slot.Snap)
+        +	target := resolveSpecialVariable(plug.Attrs["target"].(string), plug.Snap)
+        +	if _, ok := slot.Attrs["source"].(map[string]interface{}); ok {

@@ -42,6 +44,16 @@ func (spec *Specification) AddMountEntry(e Entry) error {
          func (spec *Specification) MountEntries() []Entry {
          	result := make([]Entry, len(spec.mountEntries))
          	copy(result, spec.mountEntries)
+        +	// Number each entry, in case we get clashes this will automatically give
+        +	// them unique names.
+        +	count := make(map[string]int, len(spec.mountEntries))

snapcore/snapd

interfaces/builtin: add support for content "source" section #4068

zyga commented Oct 23, 2017 •

Edited 2 times

zyga
Oct 24, 2017

zyga
Oct 23, 2017

codecov-io commented Oct 23, 2017 •

Edited 5 times

codecov-io
Nov 16, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 23, 2017

zyga added some commits Oct 23, 2017

stolowski reviewed Oct 26, 2017

View changes

stolowski reviewed Oct 26, 2017

View changes

zyga added some commits Oct 30, 2017

mvo5 reviewed Nov 8, 2017

View changes

zyga added the Blocked label Nov 9, 2017

zyga commented Nov 9, 2017

zyga added some commits Nov 15, 2017

zyga referenced this pull request Jan 8, 2018

cmd/snap-update-ns: enable writable mimic, allow content sharing to $SNAP #4452

snapcore/snapd

Join GitHub today

interfaces/builtin: add support for content "source" section #4068

Conversation

zyga commented Oct 23, 2017 • edited Edited 2 times zyga Oct 24, 2017 zyga Oct 23, 2017

codecov-io commented Oct 23, 2017 • edited Edited 5 times codecov-io Nov 16, 2017 codecov-io Oct 30, 2017 codecov-io Oct 30, 2017 codecov-io Oct 30, 2017 codecov-io Oct 23, 2017

Codecov Report

zyga added some commits Oct 23, 2017

Some checks were not successful

All checks have passed

stolowski reviewed Oct 26, 2017 View changes

zyga Oct 30, 2017 • edited Edited 1 time zyga Oct 30, 2017

stolowski reviewed Oct 26, 2017 View changes

zyga Oct 30, 2017 • edited Edited 1 time zyga Oct 30, 2017

zyga added some commits Oct 30, 2017

All checks have passed

Some checks were not successful

mvo5 reviewed Nov 8, 2017 View changes

zyga added the Blocked label Nov 9, 2017

zyga commented Nov 9, 2017

zyga added some commits Nov 15, 2017

zyga referenced this pull request Jan 8, 2018

zyga commented Oct 23, 2017 •

Edited 2 times

zyga
Oct 24, 2017

zyga
Oct 23, 2017

codecov-io commented Oct 23, 2017 •

Edited 5 times

codecov-io
Nov 16, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 30, 2017

codecov-io
Oct 23, 2017

stolowski reviewed Oct 26, 2017

View changes

zyga Oct 30, 2017 •

Edited 1 time

zyga
Oct 30, 2017

stolowski reviewed Oct 26, 2017

View changes

zyga Oct 30, 2017 •

Edited 1 time

zyga
Oct 30, 2017

mvo5 reviewed Nov 8, 2017

View changes