Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snapstate: auto install default-providers for content snaps #4103
Conversation
mvo5
added some commits
Sep 25, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
codecov-io
commented
Nov 7, 2017
•
Edited 7 times
-
codecov-io
Nov 22, 2017
-
codecov-io
Nov 22, 2017
-
codecov-io
Nov 20, 2017
-
codecov-io
Nov 13, 2017
-
codecov-io
Nov 13, 2017
-
codecov-io
Nov 13, 2017
-
codecov-io
Nov 7, 2017
-
Nov 22, 2017 -
Nov 22, 2017 -
Nov 20, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 7, 2017
Codecov Report
@@ Coverage Diff @@
## master #4103 +/- ##
==========================================
+ Coverage 75.39% 75.87% +0.48%
==========================================
Files 436 442 +6
Lines 37735 38493 +758
==========================================
+ Hits 28450 29208 +758
+ Misses 7296 7265 -31
- Partials 1989 2020 +31
Continue to review full report at Codecov.
|
pedronis
added
the
Blocked
label
Nov 9, 2017
mvo5
added some commits
Nov 13, 2017
mvo5
removed
the
Blocked
label
Nov 13, 2017
zyga
requested changes
Nov 16, 2017
A few comments inline.
I know the test snap names are congested but I also found that part a little confusing. Perhaps we can name the content snaps a little bit differently (like test-snapd-content-provider-{a,b}). Just a suggestion though.
| @@ -166,8 +169,23 @@ func (m *SnapManager) doPrerequisites(t *state.Task, _ *tomb.Tomb) error { | ||
| prereqName = snapsup.Base | ||
| } | ||
| + if err := m.installPrereq(t, prereqName, snapsup.UserID); err != nil { |
zyga
Nov 16, 2017
Contributor
A comment here would be nice. Is this trying to get prerequisites and prerequisites of prerequisites?
| + Channel string `json:"channel,omitempty"` | ||
| + UserID int `json:"user-id,omitempty"` | ||
| + Base string `json:"base,omitempty"` | ||
| + Prereq []string `json:"prereq,omitempty"` |
zyga
Nov 16, 2017
Contributor
Can you document what the content of Prereq is? Are those snap names / snap IDs?
| +func contentAttr(attrs map[string]interface{}) string { | ||
| + // we always have a "content" attr in plug/slots of interface | ||
| + // type "content" | ||
| + s, _ := attrs["content"].(string) |
mvo5
Nov 20, 2017
Collaborator
I could, however it would panic if "content" is not defined in the code. This will just return an empty string. It should not be needed as we always create a content attr for type content but its slightly more defensive.
niemeyer
Nov 30, 2017
Contributor
To be pedantic, it would actually panic if it's not a string, whether it's defined or not. It's the type assertion that panics. The map will gladly return whatever it has or nil.
| + // fill in the plug/slot data | ||
| + if rawYamlInfo, err := snap.InfoFromSnapYaml([]byte(d.SnapYAML)); err == nil { | ||
| + if info.Plugs == nil { | ||
| + info.Plugs = make(map[string]*snap.PlugInfo) |
zyga
Nov 16, 2017
•
Edited 1 time
-
zyga
Nov 16, 2017
Contributor
-
Nov 16, 2017
Note that plugs refer to apps (and vice versa). Is this code correctly linking plugs/slots/apps/hooks from one info object together? Are we mixing two disconnected objects here?
EDIT: this is the reason for "changes requested" as everything else I remarked upon is cosmetic.
mvo5
Nov 20, 2017
Collaborator
Indeed, nice catch. I added a test and ensure now that the right *Snap is set (before it was wrong :(
| + for _, plug := range info.Plugs { | ||
| + if plug.Interface == "content" { | ||
| + if !contentIfaceAvailable(st, contentAttr(plug.Attrs)) { | ||
| + dprovider, ok := plug.Attrs["default-provider"].(string) |
stolowski
Nov 17, 2017
Contributor
I think we should make sure this attribute is sane in Sanitize method of the plug (after you've read the snap yaml the plugs/slots are already sanitized).
mvo5
Nov 20, 2017
Collaborator
I think this makes sense but unfortunately we have apparently already snaps in the wild that have strange default provider names. So we need to think how we sanitize (warn ideally) without breaking snaps that exist and used to work.
niemeyer
Nov 30, 2017
Contributor
I think this makes sense but unfortunately we have apparently already snaps in the wild that have strange default provider names. So we need to think how we sanitize (warn ideally) without breaking snaps that exist and used to work.
Agreed, we can phase in the implementation without breaking. That said, there's still a point to be sad about not taking user data blindly.
I've never reviewed our internal snapstate functions considering that completely corrupted data could be injected into it, which means we can have serious security issues if we do allow that to happen.
We really must sanitize, even if we don't error out. We can just drop invalid entries, and also fix foo:bar entries so they're just foo.
mvo5
added some commits
Nov 20, 2017
| + info.Slots[k] = v | ||
| + info.Slots[k].Snap = info | ||
| + } | ||
| + } |
zyga
Nov 22, 2017
Contributor
This looks better but I wonder if you need look into info.{Plugs,Slots}.{Apps,Hooks}?
mvo5
Nov 22, 2017
Collaborator
this is done via snap.InfoFromSnapYaml([]byte(d.SnapYAML) automatically AFAICT. I added an extra test for this now just to be sure.
mvo5
added this to the 2.30 milestone
Nov 24, 2017
| for _, chg := range st.Changes() { | ||
| if chg.Status().Ready() { | ||
| continue | ||
| } | ||
| + if myChgID == chg.ID() { |
pedronis
Nov 29, 2017
Contributor
I don't understand why we need this change? don't we end up adding the same thing again otherwise?
niemeyer
Nov 30, 2017
Contributor
I don't understand why we need this change? don't we end up adding the same thing again otherwise?
Yes, this looks error prone. If we ask whether a change is in flight, we should get a correct answer for it no matter what. If the call site doesn't care about some changes, then it shouldn't ask the question in the first place.
niemeyer
Nov 30, 2017
Contributor
This point makes me realize that there's a good chance we were talking across each other good part of the time in our call today.
mvo5
modified the milestones:
2.30,
2.31
Nov 30, 2017
niemeyer
approved these changes
Nov 30, 2017
A few points, and this one LGTM. We need to follow up on the terms conversation.
| for _, chg := range st.Changes() { | ||
| if chg.Status().Ready() { | ||
| continue | ||
| } | ||
| + if myChgID == chg.ID() { |
pedronis
Nov 29, 2017
Contributor
I don't understand why we need this change? don't we end up adding the same thing again otherwise?
niemeyer
Nov 30, 2017
Contributor
I don't understand why we need this change? don't we end up adding the same thing again otherwise?
Yes, this looks error prone. If we ask whether a change is in flight, we should get a correct answer for it no matter what. If the call site doesn't care about some changes, then it shouldn't ask the question in the first place.
niemeyer
Nov 30, 2017
Contributor
This point makes me realize that there's a good chance we were talking across each other good part of the time in our call today.
| + // Prereq is a list of snap-names that need to get installed | ||
| + // together with this snap. Typically used when installing | ||
| + // content-snaps with default-providers. | ||
| + Prereq []string `json:"prereq,omitempty"` |
niemeyer
Nov 30, 2017
Contributor
This would be more like "Recommends", or perhaps "Wants". As discussed we also want to change the task name so it aligns with its purpose and with the usual conventions.
This is probably best for a separate PR, though. Let's please catch up so we decide on good terms we're happy with.
| +func contentIfaceAvailable(st *state.State, contentTag string) bool { | ||
| + repo := ifacerepo.Get(st) | ||
| + for _, slot := range repo.AllSlots("content") { | ||
| + if contentAttr(slot.Attrs) == contentTag { |
niemeyer
Nov 30, 2017
Contributor
That implies two content interfaces with the "content" attribute missing ("") would match each other. Do we want that?
mvo5
Dec 11, 2017
Collaborator
The contentInterface.Sanitize{Plug,Slot}() will ensure it is not empty, it is set to {plug,slot}.Name when empty. However I agree the code should be defensive about it.
| + for _, plug := range info.Plugs { | ||
| + if plug.Interface == "content" { | ||
| + if !contentIfaceAvailable(st, contentAttr(plug.Attrs)) { | ||
| + dprovider, ok := plug.Attrs["default-provider"].(string) |
stolowski
Nov 17, 2017
Contributor
I think we should make sure this attribute is sane in Sanitize method of the plug (after you've read the snap yaml the plugs/slots are already sanitized).
mvo5
Nov 20, 2017
Collaborator
I think this makes sense but unfortunately we have apparently already snaps in the wild that have strange default provider names. So we need to think how we sanitize (warn ideally) without breaking snaps that exist and used to work.
niemeyer
Nov 30, 2017
Contributor
I think this makes sense but unfortunately we have apparently already snaps in the wild that have strange default provider names. So we need to think how we sanitize (warn ideally) without breaking snaps that exist and used to work.
Agreed, we can phase in the implementation without breaking. That said, there's still a point to be sad about not taking user data blindly.
I've never reviewed our internal snapstate functions considering that completely corrupted data could be injected into it, which means we can have serious security issues if we do allow that to happen.
We really must sanitize, even if we don't error out. We can just drop invalid entries, and also fix foo:bar entries so they're just foo.
| + if err := m.installPrereq(t, prereqName, snapsup.UserID); err != nil { | ||
| + return err | ||
| + } | ||
| + } |
pedronis
Nov 30, 2017
Contributor
I'm a bit worried that the Retry returned from installPrereq and this sort of looping is not compatible. If we keep the retry approach we should not add tasks to the change if we are not sure we are about to be able to be done and get out of the prerequisite task step. Otherwise we might get cross waits that will never succeed if we are not careful. These are similar problems to trying to get multiple locks at the same time, that is always delicate.
| + return nil | ||
| + } | ||
| + } | ||
| + } |
pedronis
Nov 30, 2017
Contributor
I missed understanding this bit, so for link-snap we return nil, as we discussed then there's no waiting setup though which is wrong... also I'm not sure this bit is fully right if there's also a discard-snap in the change
pedronis
requested changes
Nov 30, 2017
•
as I wrote I think we need to add all tasks or retry, not a mix of those two, with the code as it stands, also the special casing about the same change isn't waiting in any form (retry or task waits) at all atm afaict
mvo5 commentedOct 30, 2017
This is the first iteration to install the
default-providersfor content snaps.One missing feature is the check if the interface is already connected and to skip the default provider if that is the case. This requires some extra work to refactor the ifacestate repository information which is needed for another PR too so it will happen very soon.