/snapd

interfaces/udev,osutil: avoid doubled rules and put all in a per snap file #1623

Merged
merged 1 commit into from Aug 8, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Reviewed
Projects
None yet
Milestone
No milestone
4 participants
@morphis @snappy-m-o @niemeyer @kyrofa
@morphis
Contributor

morphis commented Aug 3, 2016

This reworks the udev backend to overcome the following problem: Interfaces can only add rules for all applications a plug belongs to which results in doubled rules in multiple files as the result. Like with the current implementation if an interface generates the following udev rules on plug connection

IMPORT{builtin}="usb_id"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app-with-specific-plug"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app2-with-specific-plug"

the same rules end up in a rule file for each app the plug is used in

$ cat /etc/udev/rules.d/70-snap.zigbee-test.app-with-specific-plug.rules
IMPORT{builtin}="usb_id"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app-with-specific-plug"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app2-with-specific-plug"
$ cat /etc/udev/rules.d/70-snap.zigbee-test.app2-with-specific-plug.rules
IMPORT{builtin}="usb_id"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app-with-specific-plug"
SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idProduct}=="1111", ATTRS{idVendor}=="2222", SYMLINK+="zigbee/$env{ID_SERIAL}", TAG+="snap_zigbee-test_app2-with-specific-plug"

To overcome this problem all udev rules will be stored in a rule file per snap (/etc/udev/rules.d/snap..rules) and not per app (/etc/udev/rules.d/snap...rules). Furthermore the udev backend will detect identical snippets and just adds them once to the snap rules file.

Putting this here for a first early review. Unit tests are still failing and need to be fixed.
@snappy-m-o
Contributor

snappy-m-o commented Aug 3, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Aug 3, 2016

Can one of the admins verify this patch?
@niemeyer
Contributor

niemeyer commented Aug 3, 2016

I've discussed this issue with @zyga during the Snappy sprint two weeks ago. Are you in sync with him?
@morphis
Contributor

morphis commented Aug 3, 2016

@niemeyer Yes. Based on what I got from him I've implemented this now.
@morphis
Contributor

morphis commented Aug 3, 2016

@niemeyer Can you give me an initial review on this today?
interfaces/udev/backend.go
@@ -59,7 +60,7 @@ func (b *Backend) Setup(snapInfo *snap.Info, devMode bool, repo *interfaces.Repo
if err != nil {
return fmt.Errorf("cannot obtain expected udev rules for snap %q: %s", snapName, err)
}
- glob := fmt.Sprintf("70-%s.rules", interfaces.SecurityTagGlob(snapName))
+ glob := fmt.Sprintf("70-%s.rules", snap.SecurityTag(snapName))
@niemeyer

niemeyer Aug 3, 2016

Contributor

Something here is bogus. The content map has data both for hooks and for a global "snap-wide" rules file.

Do we want to merge the rules for the individual hooks in the same file as well? If so, that needs doing and consideration below. If not, this is broken because the glob now misses entries which may be written by ensureDirState.

@morphis

morphis Aug 3, 2016

Contributor

Lets do that if nothing else speaks against this. Wanted to have some initial comments on this if I am right or not.

interfaces/udev/backend.go
@@ -91,22 +92,46 @@ func ensureDirState(dir, glob string, content map[string]*osutil.FileState, snap
return errReload
}
+func generateHash(bytes []byte) uint32 {
+ h := fnv.New32a()
@niemeyer

niemeyer Aug 3, 2016

Contributor

This hash isn't strong enough to handle uniqueness as intended here.

I suggest going with a simpler implementation:

var unique map[string][]byte
...
for _, snippet := range ... {
        unique[string(snippet)] = snippet
}

No need to test for containment. At the end of the process unique will have all the snippets in it.

@morphis

morphis Aug 3, 2016

Contributor

Will fix that.

interfaces/udev/backend.go
@@ -74,7 +75,7 @@ func (b *Backend) Setup(snapInfo *snap.Info, devMode bool, repo *interfaces.Repo
//
// If the method fails it should be re-tried (with a sensible strategy) by the caller.
func (b *Backend) Remove(snapName string) error {
- glob := fmt.Sprintf("70-%s.rules", interfaces.SecurityTagGlob(snapName))
+ glob := fmt.Sprintf("70-%s.rules", snap.SecurityTag(snapName))
return ensureDirState(dirs.SnapUdevRulesDir, glob, nil, snapName)
@niemeyer

niemeyer Aug 3, 2016

Contributor

Same as above. Note that if this is all just a single file, ensureDirState is a huge red-herring. This would all be much simpler.

@morphis

morphis Aug 3, 2016

Contributor

Absolutely. Didn't wanted to change to much until we agreed on the right way forward. Lets drop ensureDirState here if we keep the single file per snap.

interfaces/udev/backend.go
}
+ snapSecurityTag := snap.SecurityTag(snapInfo.Name())
+ addContent(snapSecurityTag, finalSnapSnippets, content)
+
for _, hookInfo := range snapInfo.Hooks {
securityTag := hookInfo.SecurityTag()
hookSnippets := snippets[securityTag]
@niemeyer

niemeyer Aug 3, 2016

Contributor

Per note above, this needs more consideration. We need to take a higher level view on how to do udev properly, and squash it all into a much simpler implementation if merging all snippets is really what we want.

@morphis

morphis Aug 3, 2016

Contributor

From the discussions we had at the sprint (you with zyga and I with zyga) this is the way forward and also @jdstrand was ok with keep a rules file per snap.

@kyrofa

kyrofa Aug 3, 2016

Member

Indeed. As it is now hooks still look like they'll cause duplication-- you probably want to squash the entire thing, not just apps.

@morphis

morphis via email Aug 3, 2016

Contributor
@kyrofa

kyrofa Aug 3, 2016

Member

Yeah I think that makes the most sense. Apps and hooks are pretty much the same, it's just that hooks are typically run by snapd instead of directly by the user.

snap/info.go
+// SecurityTag returns the snap-specific security tag.
+func SecurityTag(snapName string) string {
+ return fmt.Sprintf("snap.%s", snapName)
+}
@kyrofa

kyrofa Aug 3, 2016

Member

If we're going to have a function that does this, can we make sure we use it for the AppSecurityTag() and HookSecurityTag() functions so they can't diverge?

@kyrofa

kyrofa Aug 3, 2016

Member

Or at least put them all together so it's obvious.

@kyrofa

kyrofa Aug 3, 2016

Member

Oh you did, I'm blind 😄 .

@morphis

morphis via email Aug 3, 2016

Contributor

@niemeyer niemeyer added the Reviewed label Aug 3, 2016

@morphis
Contributor

morphis commented Aug 4, 2016

@niemeyer @kyrofa Updated implementation. Need to see where I add additional unit tests now.
interfaces/udev/backend.go
+ // EnsureFileState will make sure the file will be only updated when its content
+ // has changed and will otherwise return an error which prevents us from reloading
+ // udev rules when not needed.
+ if err := osutil.EnsureFileState(rulesFilePath, rulesFileState); err != nil {
@niemeyer

niemeyer Aug 4, 2016

Contributor

This looks more clear if broken on a separate line and aggregated:

err := ...
if err != nil && err != osutil.ErrSameState {
        return err
}
interfaces/udev/backend.go
@@ -55,16 +62,43 @@ func (b *Backend) Setup(snapInfo *snap.Info, devMode bool, repo *interfaces.Repo
if err != nil {
return fmt.Errorf("cannot obtain udev security snippets for snap %q: %s", snapName, err)
}
- content, err := b.combineSnippets(snapInfo, snippets)
+ combinedSnippets, err := b.combineSnippets(snapInfo, snippets)
@niemeyer

niemeyer Aug 4, 2016

Contributor

Let's please keep this variable named content as that's what is used everywhere else around this concept.

interfaces/udev/backend.go
dir := dirs.SnapUdevRulesDir
if err := os.MkdirAll(dir, 0755); err != nil {
return fmt.Errorf("cannot create directory for udev rules %q: %s", dir, err)
}
- return ensureDirState(dir, glob, content, snapName)
+
+ if len(combinedSnippets) == 0 {
@niemeyer

niemeyer Aug 4, 2016

Contributor

What happens on removals? The original logic used ensureDirState, which deletes all files matching the glob which are not provided for creation inside the content map. The new logic seems bogus in that regard.

interfaces/udev/backend.go
+ buffer.WriteString("# This file is automatically generated.\n")
+ for _, snippet := range combinedSnippets {
+ buffer.Write(snippet)
+ buffer.WriteRune('\n')
@niemeyer

niemeyer Aug 4, 2016

Contributor

WriteByte

interfaces/udev/backend.go
- // Try reload the rules regardless of errEnsure.
- errReload = ReloadRules()
+ rulesFilePath := snapRulesFilePath(snapName)
+ if _, err := os.Stat(rulesFilePath); os.IsNotExist(err) {
@niemeyer

niemeyer Aug 4, 2016 •
Edited 1 time

Contributor

Checking for existence before removal is unnecessary. Just remove and check the error there (...; err != nil && !os.IsNotExist {).

@niemeyer

niemeyer Aug 4, 2016

Contributor

Actually, if os.IsNotExist, no need to ReloadRules. Still, no need to check for existence ahead of the removal.

@niemeyer niemeyer changed the title from interfaces: udev: avoid doubled rules and put all in a per snap file to interfaces/udev,osutil: avoid doubled rules and put all in a per snap file Aug 4, 2016

@niemeyer
Contributor

niemeyer commented Aug 4, 2016

Looks reasonable, but needs testing fixes covering all these changes, including the particular issues uncovered in the review.

@niemeyer niemeyer referenced this pull request Aug 8, 2016

Closed

Merge master onto #1623 to check tests. #1650

@niemeyer
Contributor

niemeyer commented Aug 8, 2016

Tests confirmed passing after merge in #1650.

@niemeyer niemeyer merged commit 2d1ee2f into snapcore:master Aug 8, 2016
1 check failed

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment