overlord/ifacestate: setup security of snaps affected by auto-connection #2652
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch prevents auto-connect from clobbering the connState.Auto flag when considering auto-connection candidates. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This patch fixes a bug where snaps that were auto-connected did not have their security setup done. The only reason that this went unnoticed for so long is that the main snap (the one that triggered the operation) was handled separately and that most slots don't need a security setup for an active connection. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
chipaca
approved these changes
Jan 18, 2017
niemeyer
approved these changes
Jan 18, 2017
| @@ -102,7 +102,7 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, | |||
| // - restore connections based on what is kept in the state | |||
| // - if a connection cannot be restored then remove it from the state | |||
| // - setup the security of all the affected snaps | |||
| affectedSnaps, err := m.repo.DisconnectSnap(snapName) | |||
| disconnectAffected, err := m.repo.DisconnectSnap(snapName) | |||
There was a problem hiding this comment.
Before I could tell this was a list of snaps that were touched. Now it can be pretty much anything.
How about disconnectedSnaps and connectedSnaps below?
There was a problem hiding this comment.
ah, nicer names, thank you.
| @@ -200,19 +200,20 @@ func (c *autoConnectChecker) check(plug *interfaces.Plug, slot *interfaces.Slot) | |||
| return ic.CheckAutoConnect() == nil | |||
| } | |||
|
|
|||
| func (m *InterfaceManager) autoConnect(task *state.Task, snapName string, blacklist map[string]bool) error { | |||
| func (m *InterfaceManager) autoConnect(task *state.Task, snapName string, blacklist map[string]bool) ([]string, error) { | |||
There was a problem hiding this comment.
We need a comment for the function, as that first result is impossible to guess.
There was a problem hiding this comment.
Agreed, I'm sorry for not documenting it earlier.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch corrects a bug where auto-connection would not trigger setup of security for snaps that are on the "far side" of the connection. This was mostly unnoticed because typically those would include the core snap and its various slots that don't have any connection side-effects for the core itself.
Thanks to @albaguirre for highlighting the problem and working on the solution.