interfaces: fix udev tagging for hooks (2.29) #4146

zyga · 2017-11-03T14:37:54Z

This is a backport of #4144 for 2.29; Original description follows.

This branch aims to fix udev tagging to support hooks in addition to apps that were added earlier.

There are three main patches here:

add spec.TagDevice method to udev.Specification, this automatically tags both apps and hooks
switch non-common interfaces to TagDevice, with exception of some special uses of AddSnippet
switch common interfaces to TagDevice, with some trivial changes to how that is defined.

The only remaining uses of spec.AddSnippet are special-cases that add large chunk of arbitrary
udev code that is not specifically aiming to tag a device.

This passes local unit testing, no testing was done on any hardware.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

This will just allow us to add apps/hooks easily later. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This patch introduces functionality similar to what other specification types allow: the snippet is automatically added to all the apps and hooks that affect the given plug or slot. Udev is a bit special in that it allows arbitrary udev programs to be injected (for instance, the network manager interface requires this) but most interfaces don't need that generic functionality and instead just want to tag a device so that all the apps associated with the plug or slot have acess to it. When we added hooks the udev backend was never updated to handle them so we now noticed that hooks don't have any udev tagging at all. With this new method we can use a much simpler API to do the right thing while still retain the ability to add arbitrary free-form snippets everywhere. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This automatically handles both apps and hooks, leading to simpler and more correct code. The only cases that still use AddSnippet are injecting non-tagging expressions (udisks, modem-manager, etc). Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

The "new" udev is where the TAG += ... part is handled automatically by the common interface and now contains support for both apps and hooks. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5

This looks good and simplifies things which is great. OTOH quite a bit of churn so I have some questions inline.

mvo5 · 2017-11-09T08:28:22Z

interfaces/udev/spec.go

@@ -56,6 +70,8 @@ func (spec *Specification) AddConnectedPlug(iface interfaces.Interface, plug *in
 		UDevConnectedPlug(spec *Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error
 	}
 	if iface, ok := iface.(definer); ok {
+		spec.securityTags = plug.SecurityTags()
+		defer func() { spec.securityTags = nil }()


Why are we doing this? This feels strange (and dangerous), I mean, if its something that should only be valid during the iface.UDevConnectedPlug() call maybe it should be an input parameter there instead? Actually we already pass "plug" in there, so why is UdevConntecedPlug not just using plug.SecurityTags() ?

We do this in all the other backends. This is just making the API changes smaller and consistent with other backends.

I agree, it merits a rethink at some point, but yes, I saw the other backends work like this as well

mvo5 · 2017-11-09T08:37:56Z

interfaces/builtin/mir_test.go

@@ -139,8 +139,8 @@ func (s *MirInterfaceSuite) TestSecCompOnClassic(c *C) {
 func (s *MirInterfaceSuite) TestUDevSpec(c *C) {
 	udevSpec := &udev.Specification{}
 	c.Assert(udevSpec.AddPermanentSlot(s.iface, s.coreSlot), IsNil)
-	c.Assert(udevSpec.Snippets(), HasLen, 1)
-	c.Assert(udevSpec.Snippets()[0], testutil.Contains, `KERNEL=="event[0-9]*", TAG+="snap_mir-server_mir"`)
+	c.Assert(udevSpec.Snippets(), HasLen, 5)


Why did this change from 1 snippet to 5 snippets?

Each snippet that tags a device is now a separate entry. Before they were one big blob.

The test is a bit relaxed now with checking length only. I think it would be nice to have a 'Contains' check, like we do for others.

mvo5 · 2017-11-09T08:38:49Z

interfaces/builtin/modem_manager_test.go

@@ -207,9 +207,9 @@ func (s *ModemManagerInterfaceSuite) TestUsedSecuritySystems(c *C) {

 	udevSpec := &udev.Specification{}
 	c.Assert(udevSpec.AddPermanentSlot(s.iface, s.slot), IsNil)
-	c.Assert(udevSpec.Snippets(), HasLen, 1)
+	c.Assert(udevSpec.Snippets(), HasLen, 2)


Why did this change to two? One extra for a hook?

Same as above. In the past this was one multi-line snippet.

pedronis

lgtm, some questions about SUBSYSTEM vs SUBSYSTEMS

pedronis · 2017-11-09T08:54:11Z

interfaces/builtin/hidraw.go

+		spec.TagDevice(fmt.Sprintf(`SUBSYSTEM=="hidraw", KERNEL=="%s"`, strings.TrimPrefix(path, "/dev/")))
+	} else {
+		spec.TagDevice(fmt.Sprintf(`IMPORT{builtin}="usb_id"
+SUBSYSTEM=="hidraw", SUBSYSTEMS=="usb", ATTRS{idVendor}=="%04x", ATTRS{idProduct}=="%04x"`, usbVendor, usbProduct))


is SUBSYSTEMS="usb" correct here? I remember jdstrand changed recently SUBSYSTEMS to SUBSYSTEM in some cases

for other reasons I was reading the udev docs, it seems this means the hidraw subsystem under the usb devices, so it seems plausible

pedronis · 2017-11-09T08:54:39Z

interfaces/builtin/serial_port.go

+		spec.TagDevice(fmt.Sprintf(`SUBSYSTEM=="tty", KERNEL=="%s"`, strings.TrimPrefix(path, "/dev/")))
+	} else {
+		spec.TagDevice(fmt.Sprintf(`IMPORT{builtin}="usb_id"
+SUBSYSTEM=="tty", SUBSYSTEMS=="usb", ATTRS{idVendor}=="%04x", ATTRS{idProduct}=="%04x"`, usbVendor, usbProduct))


is SUBSYSTEMS="usb" correct here? I remember jdstrand changed recently SUBSYSTEMS to SUBSYSTEM in some cases

pedronis · 2017-11-09T08:57:29Z

interfaces/udev/spec.go

@@ -56,6 +70,8 @@ func (spec *Specification) AddConnectedPlug(iface interfaces.Interface, plug *in
 		UDevConnectedPlug(spec *Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error
 	}
 	if iface, ok := iface.(definer); ok {
+		spec.securityTags = plug.SecurityTags()
+		defer func() { spec.securityTags = nil }()


I agree, it merits a rethink at some point, but yes, I saw the other backends work like this as well

The test relies on the "old" snapd-xdg-open deb package. However with the promotion of snapd 2.28.5 into xenial-updates the pervious snapd-xdg-open version 0.0.0~16.04 is no longer available to download. This means we can not run the test. Disable for now until we find a way to fix it.

stolowski

LGTM, with two remarks.

stolowski · 2017-11-09T09:32:31Z

interfaces/builtin/mir_test.go

@@ -139,8 +139,8 @@ func (s *MirInterfaceSuite) TestSecCompOnClassic(c *C) {
 func (s *MirInterfaceSuite) TestUDevSpec(c *C) {
 	udevSpec := &udev.Specification{}
 	c.Assert(udevSpec.AddPermanentSlot(s.iface, s.coreSlot), IsNil)
-	c.Assert(udevSpec.Snippets(), HasLen, 1)
-	c.Assert(udevSpec.Snippets()[0], testutil.Contains, `KERNEL=="event[0-9]*", TAG+="snap_mir-server_mir"`)
+	c.Assert(udevSpec.Snippets(), HasLen, 5)


The test is a bit relaxed now with checking length only. I think it would be nice to have a 'Contains' check, like we do for others.

stolowski · 2017-11-09T09:35:41Z

interfaces/builtin/hidraw.go

-			spec.AddSnippet(fmt.Sprintf("SUBSYSTEM==\"hidraw\", KERNEL==\"%s\", TAG+=\"%s\"", strings.TrimPrefix(path, "/dev/"), tag))
-
-		} else {
-			spec.AddSnippet(udevUsbDeviceSnippet("hidraw", usbVendor, usbProduct, "TAG", tag))


Why keeping udevUsbDeviceSnippet in utils, can we remove it as in #4144? Did you forget about cherry-picking something from the other branch?

I wanted to stay minimal in this approach. I can pull more from 4144 if necessary.

codecov-io · 2017-11-09T11:49:58Z

Codecov Report

Merging #4146 into release/2.29 will decrease coverage by 0.02%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           release/2.29    #4146      +/-   ##
================================================
- Coverage          75.6%   75.58%   -0.03%     
================================================
  Files               433      433              
  Lines             37277    37253      -24     
================================================
- Hits              28182    28156      -26     
- Misses             7124     7126       +2     
  Partials           1971     1971

Impacted Files	Coverage Δ
interfaces/builtin/kvm.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/bluetooth_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/kernel_module_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/raw_usb.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/hardware_random_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/fuse_support.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/optical_drive.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/physical_memory_observe.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/time_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/framebuffer.go	`100% <ø> (ø)`	⬆️
... and 29 more

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 01ac6ba...1892106. Read the comment docs.

zyga added 4 commits November 3, 2017 15:33

interfaces/udev: refactor acquisition of test data

db92cf9

This will just allow us to add apps/hooks easily later. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

interfaces/builtin: convert common interfaces to new udev

143d2d6

The "new" udev is where the TAG += ... part is handled automatically by the common interface and now contains support for both apps and hooks. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga added this to the 2.29 milestone Nov 3, 2017

zyga added 2 commits November 8, 2017 19:20

Merge branch 'release/2.29' into backport/udev-hooks-2.29

ad5d488

Merge branch 'release/2.29' into backport/udev-hooks-2.29

b029890

mvo5 reviewed Nov 9, 2017

View reviewed changes

pedronis reviewed Nov 9, 2017

View reviewed changes

stolowski reviewed Nov 9, 2017

View reviewed changes

mvo5 merged commit c8fb68f into snapcore:release/2.29 Nov 9, 2017

pedronis mentioned this pull request Nov 10, 2017

interfaces: fix udev tagging for hooks #4144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces: fix udev tagging for hooks (2.29) #4146

interfaces: fix udev tagging for hooks (2.29) #4146

zyga commented Nov 3, 2017

mvo5 left a comment

mvo5 Nov 9, 2017

zyga Nov 9, 2017

pedronis Nov 9, 2017

mvo5 Nov 9, 2017

zyga Nov 9, 2017

stolowski Nov 9, 2017

mvo5 Nov 9, 2017

zyga Nov 9, 2017

pedronis left a comment

pedronis Nov 9, 2017

pedronis Nov 10, 2017

pedronis Nov 9, 2017

pedronis Nov 9, 2017

stolowski left a comment

stolowski Nov 9, 2017

stolowski Nov 9, 2017

zyga Nov 9, 2017

codecov-io commented Nov 9, 2017 •

edited

interfaces: fix udev tagging for hooks (2.29) #4146

interfaces: fix udev tagging for hooks (2.29) #4146

Conversation

zyga commented Nov 3, 2017

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 9, 2017 • edited

Codecov Report

codecov-io commented Nov 9, 2017 •

edited