/snapd

Add initial capability package #65

Merged
merged 4 commits into from Nov 12, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zyga @niemeyer @chipaca @mvo5
@zyga
Contributor

zyga commented Nov 9, 2015

This patch adds a trivial package for snappy capabilities. So far
there's just a set of basic types Capability, CapabilityType, one
function NewCapability and some tests.

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

zyga commented Nov 9, 2015

Repushed after fixing golint issues
capabilities/capabilities.go
+var validName *regexp.Regexp
+
+func init() {
+ validName = regexp.MustCompile("^[a-z][a-z0-9-]+$")
@mvo5

mvo5 Nov 9, 2015

Collaborator

You don't need to put this inside init(), you can write:

var validName = regexp.MustCompile("^[a-z][a-z0-9-]+$")
@zyga
Contributor

zyga commented Nov 9, 2015

Applied suggestion from @mvo5
@zyga
Contributor

zyga commented Nov 9, 2015

14:31 < roadmr> zyga: typo \o/ line 16 in capabilities.go has an extra leading space :P
14:31 < zyga> roadmr: looking
14:31 < zyga> roadmr: thanks, I'll fix that!
14:31 < zyga> roadmr: review FTW!
14:31 < roadmr> indeed :)
14:32 < zyga> roadmr: fixed

zyga added some commits Nov 9, 2015

@zyga
Add initial capability package 
This patch adds a trivial package for snappy capabilities. So far
there's just a set of basic types Capability, CapabilityType, one
function NewCapability and some tests.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
192087a
@zyga
Add the testutil package with the Contains Checker 
This checker is useful for writing tests that work with the three most
primitive collection types, arrays, slices and maps.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
f472677
@zyga
Add CapabilityRepository 
This patch adds a repository for capabilities. Each repository is just a
simple map[str]Capability with a few methods to add and remove elements.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
73c7314
@zyga
Track CapabilityRepository in the Daemon 
This patch adds a CapabilityRepository instance to the Daemon so that we
can start having APIs that interact with capabilities.

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

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.2%) to 65.798%
Details
c110d46
capabilities/capabilities.go
+ "regexp"
+)
+
+// Capability that expressess an instance Snappy Capability.
@niemeyer

niemeyer Nov 10, 2015

Contributor

Not sure I understand the sentence.. perhaps something along the lines of:

// Capability holds information about a capability that a snap may request
// from a snappy system to do its job while running on it.

capabilities/capabilities.go
+// Capability that expressess an instance Snappy Capability.
+type Capability struct {
+ // Name that is also used for programmatic access on command line and
+ // at application runtime. This is constrained to [a-z][a-z0-9-]+
@niemeyer

niemeyer Nov 10, 2015

Contributor

// Name is a key that identifies the capability. It must be unique within its context, which
// may be either a snap or a snappy runtime.

capabilities/capabilities.go
+ // at application runtime. This is constrained to [a-z][a-z0-9-]+
+ Name string
+ // Label meant for humans. This is the English version of this label.
+ // TODO: Add an i18n mechanism later.
@niemeyer

niemeyer Nov 10, 2015

Contributor

// Label provides an optional title for the capability to help a human tell which physical
// device this capability is referring to. It might say "Front USB", or "Green Serial Port",
// for example.

Please drop the TODO item for now. The translation here is special, because the physical label may not match the interface language, so let's not encourage passers-by to spend time on this before we understand what we actually want to do.

capabilities/capabilities.go
+ // TODO: Add an i18n mechanism later.
+ Label string
+ // CapabilityType describes a group of capabilities sharing some common
+ // traits. In particular, this is where security bits are coming from.
@niemeyer

niemeyer Nov 10, 2015

Contributor

// Type defines the type of this capability. The capability type defines the behavior allowed and
// expected from providers and consumers of that capability, and also which information should
// be exchanged by these parties.

capabilities/capabilities.go
+// All capability types are maintained with snappy source code.
+// In other words, there are no 3rd party capabilitites as they have
+// deep access to system security API promisses.
+type CapabilityType struct {
@niemeyer

niemeyer Nov 10, 2015

Contributor

We can name this simply as Type. This is already within the caps package, so it ends up as caps.Type.

capabilities/capabilities.go
+ *
+ */
+
+package capabilities
@niemeyer

niemeyer Nov 10, 2015

Contributor

I suggest naming this caps, for short. We'll unfortunately take over this convenient variable name, but it'll be boring to type and read the full fledged word behind every variable, type, method, etc, that we define here.

capabilities/capabilities.go
+ // as snaps will refer to capabilities types with a given name. Gadget
+ // snaps will also provide mechanisms to create capabilities with a given
+ // type name.
+ Name string
@niemeyer

niemeyer Nov 10, 2015

Contributor

Rather than a struct, I believe this should be simply:

// Type is the name of a capability type.
type Type string

We also don't need to re-describe this, since we just defined it inside Capability which is the more interesting type.

capabilities/capabilities.go
+// CapTypeFile is a basic capability vaguely expressing access to a specific
+// file. This single capability type is here just to help boostrap
+// the capability concept before we get to load capability interfaces from YAML.
+var CapTypeFile = CapabilityType{"file"}
@niemeyer

niemeyer Nov 10, 2015

Contributor

The idea described in the comment sounds good, but we should already be moving in that direction. If capabilities are externally defined, we shouldn't have the concept of Go types here, since we need it to be extensible, which we can't do if we need to recompile snappy to introduce new types. This should probably be something along those lines:

const (
FileType Type = "file"
)

So we get caps.FileType valued as "file". This means if we load an external capability type named "file", we can simply do caps.Type("file") and end up in a similar place.

capabilities/capabilities.go
+var CapTypeFile = CapabilityType{"file"}
+
+// Regular expression describing correct identifiers
+var validName = regexp.MustCompile("^[a-z][a-z0-9-]+$")
@niemeyer

niemeyer Nov 10, 2015

Contributor

"fo-" shouldn't be considered valid.

capabilities/capabilities.go
+var validName = regexp.MustCompile("^[a-z][a-z0-9-]+$")
+
+// NewCapability creates a new Capability object after validating arguments
+func NewCapability(Name, Label string, Type CapabilityType) (*Capability, error) {
@niemeyer

niemeyer Nov 10, 2015

Contributor

I'd prefer to not have this until we're really making good use of it, because this:

caps.Capability{Name: name, Type: type}

is much more readable and more flexible than this:

caps.New(name, type)
capabilities/capabilities.go
+// A CapabilityRepository stores all known snappy capabilities and types
+type CapabilityRepository struct {
+ // Map of capabilities, indexed by Capability.Name
+ Caps map[string]*Capability
@niemeyer

niemeyer Nov 10, 2015

Contributor

This should be caps.Repository, and we should try to make our implementations private whenever there's not a reason to expose it. In this case, you're designing a carefully crafted API, and the map here is just an implementation detail.

capabilities/capabilities.go
+}
+
+// NewCapabilityRepository creates an empty capability repository
+func NewCapabilityRepository() *CapabilityRepository {
@niemeyer

niemeyer Nov 10, 2015

Contributor

caps.NewRepository

capabilities/capabilities.go
+// Capability names must be unique within the repository.
+// An error is returned if this constraint is violated.
+func (r *CapabilityRepository) Add(cap *Capability) error {
+ if _, capPresent := r.Caps[cap.Name]; capPresent {
@niemeyer

niemeyer Nov 10, 2015

Contributor

Conventionally (and FYI mainly since you're just joining the Go party), most trivial uses of this expression use "ok" as the bool variable.

capabilities/capabilities.go
+// An error is returned if this constraint is violated.
+func (r *CapabilityRepository) Add(cap *Capability) error {
+ if _, capPresent := r.Caps[cap.Name]; capPresent {
+ return errors.New("Capability with that name already exists")
@niemeyer

niemeyer Nov 10, 2015

Contributor

Similarly, please start error sentences on lowercase, so we "Avoid: Messages: Like: This".

capabilities/capabilities.go
+
+// Remove a capability with a given name.
+// Removing a capability that doesn't exist silently does nothing
+func (r *CapabilityRepository) Remove(Name string) {
@niemeyer

niemeyer Nov 10, 2015

Contributor

s/Name/name/

capabilities/capabilities_test.go
+func (s *CapabilitySuite) TestNewCapabilityInvalidName(c *C) {
+ cap, err := NewCapability("name with space", "label", CapTypeFile)
+ c.Assert(cap, IsNil)
+ c.Assert(err, ErrorMatches, "Name is not a valid identifier")
@niemeyer

niemeyer Nov 10, 2015

Contributor

If we go with using the capability struct for now, this may become caps.ValidateName for the time being.

capabilities/capabilities_test.go
+
+func (s *CapabilitySuite) TestNewRepository(c *C) {
+ repo := NewCapabilityRepository()
+ c.Assert(len(repo.Caps), Equals, 0)
@niemeyer

niemeyer Nov 10, 2015

Contributor

I'd drop this test.

capabilities/capabilities_test.go
+}
+
+func (s *CapabilitySuite) TestAddCapabilityAllOk(c *C) {
+ cap, capErr := NewCapability("name", "label", CapTypeFile)
@niemeyer

niemeyer Nov 10, 2015

Contributor

s/capErr/err/

capabilities/capabilities_test.go
+func (s *CapabilitySuite) TestAddClash(c *C) {
+ repo := NewCapabilityRepository()
+ cap1, cap1Err := NewCapability("name", "label 1", CapTypeFile)
+ cap2, cap2Err := NewCapability("name", "label 2", CapTypeFile)
@niemeyer

niemeyer Nov 10, 2015

Contributor

s/cap1Err/err/, s/cap2Err/err, and then assert in place instead of postponing it.

capabilities/capabilities_test.go
+ c.Assert(cap1Err, IsNil)
+ c.Assert(cap2Err, IsNil)
+ err1 := repo.Add(cap1)
+ err2 := repo.Add(cap2)
@niemeyer

niemeyer Nov 10, 2015

Contributor

Same.

capabilities/capabilities_test.go
+
+func (s *CapabilitySuite) TestAdd(c *C) {
+ repo := NewCapabilityRepository()
+ cap, _ := NewCapability("name", "label", CapTypeFile)
@niemeyer

niemeyer Nov 10, 2015

Contributor 
cap, err := ...
c.Assert(err, IsNil)
...
capabilities/capabilities_test.go
+}
+
+func (s *CapabilitySuite) TestRemove(c *C) {
+ cap, _ := NewCapability("name", "label", CapTypeFile)
@niemeyer

niemeyer Nov 10, 2015

Contributor

Again, grab err and test.

daemon/daemon.go
@@ -41,6 +42,7 @@ type Daemon struct {
listener net.Listener
tomb tomb.Tomb
router *mux.Router
+ capRepo *capabilities.CapabilityRepository
@niemeyer

niemeyer Nov 10, 2015

Contributor

@chipaca Is this the right place to hook it in?

@chipaca

chipaca Nov 10, 2015

Member

@zyga checked with me before putting it here; I'll repeat here what I told him: without knowing how it's going to get used it's hard to give an answer with certainty, but you can't go wrong putting it in the daemon, as there'll only be one instance of it and every command has access to it.

testutil/checkers.go
+ case reflect.Slice, reflect.Array:
+ // Ensure that type of elements in haystack is compatible with needle
+ if needleV := reflect.ValueOf(needle); haystackV.Type().Elem() != needleV.Type() {
+ panic(fmt.Sprintf("haystack contains items of type %s but needle is a %s",
@niemeyer

niemeyer Nov 10, 2015

Contributor

This should return the error in the error string, rather than being a panic. It's the only reason why the that result exists. Have a look at use cases in checkers.go in check.v1 for examples.

@niemeyer

niemeyer Nov 10, 2015

Contributor

Also, let's please use generic names for the error message, specifically: "container has items of type %s but expected element is a %s". go-check has its own names for those variables, the call site will have its own local variable names too, so adding new analogies here won't help much.

testutil/checkers.go
+ // Ensure that type of elements in haystack is compatible with needle
+ if needleV := reflect.ValueOf(needle); haystackV.Type().Elem() != needleV.Type() {
+ panic(fmt.Sprintf("haystack contains items of type %s but needle is a %s",
+ haystackV.Type().Elem(), needleV.Type()))
@niemeyer

niemeyer Nov 10, 2015

Contributor

Same comments here about panic and error message.

testutil/checkers.go
+ }
+ return false, ""
+ case reflect.String:
+ // When haystack is a string, we expect needle to be a string as well
@niemeyer

niemeyer Nov 10, 2015

Contributor

If it's not, it should error out politely per above rather than panic.

testutil/checkers.go
+ haystack := params[0].(string)
+ return strings.Contains(haystack, needle), ""
+ default:
+ panic(fmt.Sprintf("haystack is of unsupported type %T", params[0]))
@niemeyer

niemeyer Nov 10, 2015

Contributor

This should also be an error, since the value is coming from developer code which is precisely being tested, so we should say politely that the test failed rather than tracking back.

@niemeyer
Contributor

niemeyer commented Nov 10, 2015

Thanks for the branch, Zygmunt. Looks great. There are a number of comments, but they are pretty much entirely about trivial details rather than something relevant. Feel free to merge this if you feel you've addressed the comments appropriately. If you'd like to raise something, feel free to poke me online.
@niemeyer
Contributor

niemeyer commented Nov 10, 2015

That is, assuming you get the +1 from someone else as well. I thought you did, but looking above I can't find any.
testutil/checkers.go
+// Contains is a Checker that looks for a needle in a haystack.
+// The needle can be any object. The haystack can be an array, slice or string.
+var Contains check.Checker = &containsChecker{
+ &check.CheckerInfo{Name: "Contains", Params: []string{"haystack", "needle"}},
@niemeyer

niemeyer Nov 10, 2015

Contributor

"container", "elem"

@niemeyer niemeyer referenced this pull request Nov 10, 2015

Closed

Add the testutil package with the Contains Checker #68

@chipaca
Member

chipaca commented Nov 12, 2015

LGTM.

chipaca added a commit that referenced this pull request Nov 12, 2015

@chipaca
Merge pull request #65 from zyga/caps 
Add initial capability package
3123ef9

@chipaca chipaca merged commit 3123ef9 into snapcore:master Nov 12, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 65.798%
Details

zyga pushed a commit to zyga/snapd that referenced this pull request Nov 7, 2016

@kyrofa
Merge pull request #65 from snapcore/towards-safe-mount 
Towards safe mount
938c748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment