interfaces: upower-observe: refactor to allow snaps to provide a slot #2469

Merged
merged 1 commit into from Jan 12, 2017

Projects

None yet

5 participants

@morphis
Contributor
morphis commented Dec 13, 2016 edited

With the upcoming upower snap we need to allow it to own a slot for upower-serve clients can connect to. However upower-observe will not do anything on the slot side still as we will implement a upower-control interface which will include the slot side part and write access to the upower service on the plug side.

@jhodapp

Looks good to me.

@mvo5 mvo5 added this to the 2.21 milestone Jan 2, 2017
@afrantzis
Contributor
afrantzis commented Jan 4, 2017 edited

Not sure if there is a policy for this, but, e.g., ofono.go allows apps running on classic to access both confined and unconfined ofono instances. Should we also support this for upower-observe?

@afrantzis
Contributor

The code you pointed to uses slotAppLabelExpr() on all-snap and "unconfined" on classic. This means that on classic, apps can't access upower services provided by a (confined) snap.

On the other hand, ofono.go on classic provides both rules at the same time: one with slotAppLabelExpr() and one with unconfined, so it allows apps to access ofono services regardless of whether they are provided by a confined or unconfined ofono instance. See https://github.com/snapcore/snapd/blob/master/interfaces/builtin/ofono.go#L267

Is there a policy about whether we want to go one way or the other?

@morphis
Contributor
morphis commented Jan 6, 2017

@afrantzis I think the rational behind this is that we provide upower on classic systems (Ubuntu Desktop) from a debian package and wont provide it from a snap. Because of that we only allow talking the plug side to always talk to the unconfined upower service. I am fine with adding both things to the ruleset. @jdstrand What do you think?

@jdstrand
Contributor
jdstrand commented Jan 9, 2017

@morphis (cc @afrantzis) - with ofono it was decided that an ofono snap on a classic system was a valid use case. For upower, it just depends on the use cases. IMHO, this implementation is fine as is since on classic upower is installed by default.

interfaces/builtin/upower_observe.go
"github.com/snapcore/snapd/interfaces"
+ "github.com/snapcore/snapd/release"
+ "github.com/snapcore/snapd/snap"
)
const upowerObserveConnectedPlugAppArmor = `
@jdstrand
jdstrand Jan 9, 2017 Contributor

I previously said 'LGTM' but that was premature. upower-observe allows a for the plug to connect to the slot, but there is no PermanentSlot policy or ConnectedSlot policy for this interface. Based on this interface and the upower-control PR (#2498), it appears you are trying to do something like with location-observe and location-control where the 'observe' interface allows GPS tracking (eg, reading) but the 'control' interface allows configuring the location service. Is this what you are planning for upower-observe and upower-control (where 'observe' has some safe subset of APIs and 'control' allows something else)? If so, you need to implement both interfaces with PermanentSlot and ConnectedSlot. If not, this should all be in upower-observe I think.

@morphis
morphis Jan 9, 2017 Contributor

@jdstrand Yes that is what I am doing here but I don't understand why we have to implement the same slot policy in both interfaces. A snap implementing the upower-observe slot should always implement the upower-observe interface as well. So both will always go together also because of the snaping (upower- prefix). It only differentiates on the plug side which one will be used.

@jdstrand
jdstrand Jan 9, 2017 Contributor

@morphis - I thought that this was likely the case but the current implementation isn't enforcing the invariant that every interface should work standalone and implement everything required (ie, there is no concept of interface dependencies). In other words, if you are splitting these, it is perfectly reasonable in terms of snapd for developers to only implement one, or to implement each with two different snaps, or to implement both in one snap. With location-* it was decided that the small bit of PermamentSlot policy overlap wasn't anything to worry about, but you could, if you wanted, abstract some of this out so it is shared (not required by this PR).

@morphis
Contributor
morphis commented Jan 11, 2017

Merge with upower-control interface.

@@ -495,6 +495,7 @@ slots:
allow-installation:
slot-snap-type:
- core
+ - app
@jdstrand
jdstrand Jan 11, 2017 Contributor

Please add:

deny-connection:                                                            
  on-classic: false

This makes it so a snap declaration is needed when on all snaps (ie, where upower isn't installed as a deb). We do something similar with network-manager, for example (but don't add anything for auto-connection for upower).

@morphis
morphis Jan 11, 2017 Contributor

Added.

+ path=/org/freedesktop/login1{,/**}
+ interface=org.freedesktop.login1.Manager
+ member={CanPowerOff,CanSuspend,CanHibernate,CanHybridSleep,PowerOff,Suspend,Hibernate,HybrisSleep}
+ peer=(label=unconfined),
@jdstrand
jdstrand Jan 11, 2017 Contributor

Should this be part of the 'shutdown' interface?

@morphis
morphis Jan 11, 2017 Contributor

We could add it there too but do we want to mix real 'shutdown' with 'suspend' and 'hibernate'? I fine to refactor this once we landed a first version of this. Are you ok with that?

@jdstrand
jdstrand Jan 11, 2017 Contributor

I think it is ok to have in the first version.

@jdstrand

Please make the test addition and this LGTM.

@@ -368,6 +368,7 @@ var (
"pulseaudio": {"app", "core"},
"serial-port": {"core", "gadget"},
"udisks2": {"app"},
+ "upower-observe": {"app", "core"},
// snowflakes
"docker": nil,
"lxd": nil,
@jdstrand
jdstrand Jan 11, 2017 Contributor

Can you also update TestConnectionOnClassic() to include upower-observe?

@morphis
morphis Jan 12, 2017 Contributor

Added

+ path=/org/freedesktop/login1{,/**}
+ interface=org.freedesktop.login1.Manager
+ member={CanPowerOff,CanSuspend,CanHibernate,CanHybridSleep,PowerOff,Suspend,Hibernate,HybrisSleep}
+ peer=(label=unconfined),
@jdstrand
jdstrand Jan 11, 2017 Contributor

I think it is ok to have in the first version.

Simon Fels interfaces: upower-observe: refactor to allow snaps to provide a slot
2d67ef7
@mvo5
Collaborator
mvo5 commented Jan 12, 2017

Looks good

@mvo5 mvo5 merged commit c4bb3e4 into snapcore:master Jan 12, 2017

6 checks passed

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment