interfaces: add common support for udev #3658

Merged
merged 1 commit into from Aug 4, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Aug 4, 2017

Many interfaces will need to support udev-based device tagging. To
counter the explosion of custom interface types the common interface can
be grown to support per-app udev tags.

There's also some helper for testing, placed in export_test.go, since
common_test is not using builtin_test package. I'll clean that up
separately.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

interfaces: add common support for udev
Many interfaces will need to support udev-based device tagging. To
counter the explosion of custom interface types the common interface can
be grown to support per-app udev tags.

There's also some helper for testing, placed in export_test.go, since
common_test is not using builtin_test package. I'll clean that up
separately.

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

Codecov Report

Merging #3658 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3658      +/-   ##
==========================================
+ Coverage   75.16%   75.18%   +0.01%     
==========================================
  Files         388      388              
  Lines       33628    33637       +9     
==========================================
+ Hits        25278    25289      +11     
+ Misses       6534     6532       -2     
  Partials     1816     1816
Impacted Files Coverage Δ
interfaces/builtin/common.go 62.29% <100%> (+6.52%) ⬆️
interfaces/sorting.go 100% <0%> (+1.28%) ⬆️
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 8880f59...a370824. Read the comment docs.

@@ -142,3 +145,15 @@ func (iface *commonInterface) SecCompConnectedPlug(spec *seccomp.Specification,
}
return nil
}
+
+func (iface *commonInterface) UDevConnectedPlug(spec *udev.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
+ old := "###SLOT_SECURITY_TAGS###"
@adglkh

adglkh Aug 4, 2017

Contributor

I prefer to use the following code for a few reasons here, instead of introducing tags and doing the string.Replace here.

  1. less code for consistency with other functions in commonInterface
  2. In favor of "format" here rather than "replace" for performance consideration (nitpicking, we have a large code snippet udev rules for udisk2, modemManager, maybe more in the future)
if iface.connectedPlugUdev != "" {
        for appName := range plug.Apps {
            tag := udevSnapSecurityName(plug.Snap.Name(), appName)
            spec.AddSnippet(fmt.Sprintf(iface.connectedPlugUdev, tag))
        } 
}

People could use %[1]s if there're multiple udev entries there or simply use "%s" if only one entry exists.

I'll leave the decision to you though.

@zyga

zyga Aug 4, 2017

Contributor

I used format first but then changed this so that it is consistent with ###replaceme### markers we use elsewhere in the interface code. We can revisit that but then we should do it consistently in the whole stack (all of builtin/*.go)

@adglkh

adglkh Aug 4, 2017

Contributor

Okay. Sounds good!

@@ -142,3 +145,15 @@ func (iface *commonInterface) SecCompConnectedPlug(spec *seccomp.Specification,
}
return nil
}
+
+func (iface *commonInterface) UDevConnectedPlug(spec *udev.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
+ old := "###SLOT_SECURITY_TAGS###"
@adglkh

adglkh Aug 4, 2017

Contributor

I prefer to use the following code for a few reasons here, instead of introducing tags and doing the string.Replace here.

  1. less code for consistency with other functions in commonInterface
  2. In favor of "format" here rather than "replace" for performance consideration (nitpicking, we have a large code snippet udev rules for udisk2, modemManager, maybe more in the future)
if iface.connectedPlugUdev != "" {
        for appName := range plug.Apps {
            tag := udevSnapSecurityName(plug.Snap.Name(), appName)
            spec.AddSnippet(fmt.Sprintf(iface.connectedPlugUdev, tag))
        } 
}

People could use %[1]s if there're multiple udev entries there or simply use "%s" if only one entry exists.

I'll leave the decision to you though.

@zyga

zyga Aug 4, 2017

Contributor

I used format first but then changed this so that it is consistent with ###replaceme### markers we use elsewhere in the interface code. We can revisit that but then we should do it consistently in the whole stack (all of builtin/*.go)

@adglkh

adglkh Aug 4, 2017

Contributor

Okay. Sounds good!

@zyga zyga merged commit d5e9fb9 into snapcore:master Aug 4, 2017

7 checks passed

artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 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/common-interface-udev-support branch Aug 4, 2017

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