Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overlord/ifacestate: remove connection state with discard-conns task, on the removal of last snap #958
Conversation
zyga
added some commits
Apr 14, 2016
niemeyer
reviewed
Apr 14, 2016
| @@ -336,6 +349,44 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecuirtyKeepsExistingConnectionSt | ||
| }) | ||
| } | ||
| +func (s *interfaceManagerSuite) TestDoRemoveSnapSecurityRemovesConnectionsFromStateWhenInvokedOnPlugSide(c *C) { |
niemeyer
Apr 14, 2016
Contributor
Okay, that is getting off limits. :) This is a function name, not documentation. Let's please come up with a name that is at most half the length of this one.
niemeyer
reviewed
Apr 14, 2016
| + s.testDoRemoveSnapSecurityRemovesConnectionsFromStateWhenInvokedOn(c, "consumer") | ||
| +} | ||
| + | ||
| +func (s *interfaceManagerSuite) TestDoRemoveSnapSecurityRemovesConnectionsFromStateWhenInvokedOnSlotSide(c *C) { |
niemeyer
reviewed
Apr 14, 2016
| + s.testDoRemoveSnapSecurityRemovesConnectionsFromStateWhenInvokedOn(c, "producer") | ||
| +} | ||
| + | ||
| +func (s *interfaceManagerSuite) testDoRemoveSnapSecurityRemovesConnectionsFromStateWhenInvokedOn(c *C, snapName string) { |
niemeyer
reviewed
Apr 14, 2016
| + conns, err := getConns(task.State()) |
niemeyer
Apr 14, 2016
Contributor
We cannot do this here. We need to be able to distinguish between removing the profiles, and discarding the connections. Otherwise we'll end up killing user data when deactivating a snap, or removing an old revision.
I suggest clarifying the security tasks we have, with the following set:
- setup-profiles
- remove-profiles
- discard-conns
niemeyer
Apr 14, 2016
Contributor
Then, on snapstate.Remove, distinguish between last snap being removed and obsolete snap being removed, and conditionally inject discard-conns after remove-profiles.
zyga
Apr 14, 2016
Contributor
This makes a lot of sense. Earlier when I was working on setup-snap-security I realized that it would be much easier if there were some more fine-grained tasks (e.g. setup that could be reused by connect) and it looks like the same is true for remove-snap-security.
So let's review how each of those would be used:
setup-profiles: on snap install, snap refresh and maybe snap activate
remove-profiles: on snap deactivate
discard-conns: on snap remove
niemeyer
Apr 14, 2016
Contributor
Almost.. snap remove may be used to remove one of several snaps. We cannot discard connections if we still have a snap with the same name installed (and perhaps even active!).
zyga
Apr 14, 2016
Contributor
I see now how there can be multiple snaps with the same name but I'm not sure this correctly translates that to security. Security identifiers don't contain snap revision so removing security for a given snap (at any revision) will impact the security for other snaps with the same name (but with different revision)
I'm not sure how security-affecting tasks and snap-affecting tasks are related wrt multiple revisions now.
zyga
added some commits
Apr 14, 2016
niemeyer
reviewed
Apr 14, 2016
| - affectedSnaps = append(affectedSnaps, snapInfo) | ||
| + | ||
| + if err == nil && len(snapState.Sequence) != 0 { | ||
| + return fmt.Errorf("cannot discard connections of a snap %q while it is present", snapName) |
niemeyer
reviewed
Apr 14, 2016
| + conns, err := getConns(st) | ||
| + if err != nil { | ||
| + return err | ||
| + } |
niemeyer
Apr 14, 2016
Contributor
// TODO Record removed connections in task state to recover it on undo.
zyga
Apr 14, 2016
Contributor
Oh, great idea. I was wondering how to make this robust and I forgot that there's a reason each task has a state! Will do.
zyga
Apr 15, 2016
Contributor
I've implemented a simple undo handler. I'm not sure if that's the right way so have look please.
|
Good direction. A couple of comments, and needs to update |
zyga
added some commits
Apr 15, 2016
|
retest this please |
zyga
added some commits
Apr 15, 2016
zyga
changed the title from
overlord/ifacestate: remove connection state on remove-snap-security
to
overlord/ifacestate: remove connection state with discard-conns task, on the removal of last snap
Apr 15, 2016
zyga
added some commits
Apr 15, 2016
niemeyer
reviewed
Apr 15, 2016
| addNext(discardSnap) | ||
| + if len(snapst.Sequence) == 1 { |
niemeyer
reviewed
Apr 15, 2016
| @@ -698,7 +698,9 @@ func (s *snapmgrTestSuite) TestRemoveIntegration(c *C) { | ||
| // verify snapSetup info | ||
| tasks := ts.Tasks() | ||
| - task := tasks[len(tasks)-1] | ||
| + // XXX -2 because the last task is discard-conns and snap-setup is in the | ||
| + // discard-snap, just before discard-conns. |
niemeyer
Apr 15, 2016
•
Edited 1 time
-
niemeyer
Apr 15, 2016
Contributor
-
Apr 15, 2016
Let's please replace the comment by:
// snap-setup is in discard-snap above discard-conns.
zyga
added some commits
Apr 15, 2016
|
@niemeyer This is now ready for final review and landing. Tested locally with |
zyga commentedApr 14, 2016
•
Edited 1 time
-
zyga
Apr 15, 2016
This branch changes profile setup/remove logic so that:
setup-profilessets up profiles (and handles autoconnect as before)remove-profilesjust removes profiles without touching persistent connectionsdicard-connsjust discards persistent connections (and supports undo)The snap manager now injects
discard-connswhen removing the last snap in the sequence (last as in the only one)