Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces: sanitize plugs and slots early in ReadInfo #3972
Conversation
stolowski
added some commits
Sep 22, 2017
All checks have passed
codecov-io
commented
Sep 27, 2017
•
Edited 1 time
-
codecov-io
Sep 27, 2017
-
Sep 27, 2017
Codecov Report
@@ Coverage Diff @@
## master #3972 +/- ##
=========================================
+ Coverage 75.71% 75.8% +0.08%
=========================================
Files 429 433 +4
Lines 36709 37174 +465
=========================================
+ Hits 27793 28178 +385
- Misses 6964 7024 +60
- Partials 1952 1972 +20
Continue to review full report at Codecov.
|
| ifaces: make(map[string]Interface), | ||
| plugs: make(map[string]map[string]*Plug), | ||
| slots: make(map[string]map[string]*Slot), | ||
| slotPlugs: make(map[*Slot]map[*Plug]*Connection), | ||
| plugSlots: make(map[*Plug]map[*Slot]*Connection), | ||
| backends: make(map[SecuritySystem]SecurityBackend), | ||
| } | ||
| + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) error { |
zyga
Sep 28, 2017
Contributor
Could you please add a check that snap.SanitizePlugsSlots is not set yet? I worry that we may end up with sanitizer that checks against some ghost repository for whatever reason later.
stolowski
Oct 3, 2017
Contributor
I tried a couple of different approches revolving around such check, but it turned out to be impossible due to the fact that in some tests we create two instances of repo side-by-side, or two daemons (with repos created implicitely).
Anyway, I changed this to an explicit setup call from ifacemgr, that way we know we do this only once in real code and there is no possiblity to mess it up. And ReadInfo will panic if the callback is not set.
| - issues map[string]string // slot or plug name => message | ||
| -} | ||
| +func (r *Repository) SanitizePlugsSlots(snapInfo *snap.Info) error { | ||
| + err := snap.Validate(snapInfo) |
zyga
Sep 28, 2017
Contributor
Is there any reason to validate the snap info at this step? Also, without this the whole method returns nothing.
| @@ -862,11 +877,6 @@ func (e *BadInterfacesError) Error() string { | ||
| // Unknown interfaces and plugs/slots that don't validate are not added. | ||
| // Information about those failures are returned to the caller. | ||
| func (r *Repository) AddSnap(snapInfo *snap.Info) error { | ||
| - err := snap.Validate(snapInfo) |
zyga
Sep 28, 2017
Contributor
If you choose to keep snap validation out of interface validation then please this code here.
| @@ -1539,7 +1539,7 @@ func (s *AddRemoveSuite) SetUpTest(c *C) { | ||
| c.Assert(err, IsNil) | ||
| } | ||
| -func (s *AddRemoveSuite) TestAddSnapComplexErrorHandling(c *C) { | ||
| +/*func (s *AddRemoveSuite) TestAddSnapComplexErrorHandling(c *C) { |
zyga
Sep 28, 2017
Contributor
Can you please remove the affected test function instead of commenting it out?
| - } | ||
| + return err | ||
| + } | ||
| + if len(snapInfo.BadInterfaces) > 0 { |
| @@ -90,7 +92,11 @@ func (s *interfaceManagerSuite) SetUpTest(c *C) { | ||
| // TODO: transition this so that we don't load real backends and instead | ||
| // just load the test backend here and this is nicely integrated with | ||
| // extraBackends above. | ||
| - s.restoreBackends = ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend}) | ||
| + restoreBackends := ifacestate.MockSecurityBackends([]interfaces.SecurityBackend{s.secBackend}) | ||
| + s.restore = func() { |
zyga
Sep 28, 2017
Contributor
I wonder if we should either use (en mass) testutil.BaseTest or remove it, we seem to have a swarm of restore me later functions in all the suites.
| @@ -165,6 +166,9 @@ type Info struct { | ||
| Plugs map[string]*PlugInfo | ||
| Slots map[string]*SlotInfo | ||
| + // Plugs or slots with issues (they are not included in Plugs or Slots) | ||
| + BadInterfaces map[string]string // slot or plug => message |
zyga
Sep 28, 2017
Contributor
Shall we store the real error objects here? This way we could generate i18n specific message later.
stolowski
Oct 3, 2017
Contributor
I'm not thrilled about storing error objects here, because these problems with interfaces are not actually treated as errors. Also, nothing is stopping us from populating this map with i18n, is it?
zyga
Oct 3, 2017
Contributor
The errors can be stored and transformed to per-request i18n message later. We cannot translate them at this time.
niemeyer
Oct 9, 2017
Contributor
We probably don't need to store those errors. Instead, if the interface definition is completely broken, we can reject the snap loading with the actual error.
stolowski
Oct 9, 2017
Contributor
@niemeyer We have two possible problems to deal with: an unknown interface or interface not passing sanitization. AFAIR we didn't want to reject snaps on unknown interfaces.
Shall we only reject snaps that fail sanitization?
niemeyer
Oct 18, 2017
Contributor
It's okay, let's keep it this way for now. Strings are fine for the time being (it is already like that in master). Easy to change when we want to.
stolowski
added some commits
Oct 3, 2017
All checks have passed
zyga
approved these changes
Oct 3, 2017
I think this looks good now. The i18n issues can be solved once we do per-request i18n
stolowski
requested a review
from
niemeyer
Oct 3, 2017
All checks have passed
This was referenced Oct 5, 2017
| + | ||
| +func (r *Repository) SetSanitizePlugSlots() { | ||
| + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) { | ||
| + r.SanitizePlugsSlots(snapInfo) |
niemeyer
Oct 9, 2017
Contributor
This doesn't look right. This is changing global state by a method on a repository instance, setting to its own method. The idea of enabling and disabling the global logic by fiddling with this function is also suspect.
Why do we need the repository for those validations? We know about every interface at runtime without the need to touch the repository. That function should even be changeable I think, but rather an actual function that should always be available.
stolowski
Oct 10, 2017
Contributor
Hmm. This function is called from snap.ReadInfo and needs to be a settable callback to avoid import cycle; in snapd it needs to actually sanitize plugs and slots against all interfaces, but in snap and snap-exec I set it to no-op func not to drag all the interfaces in (plus, it doesn't make sense to sanitize at this point).
To summarize, I'm not sure how to change this aspect to satisfy both cases - other than by having two ReadInfo functions (with- and without- sanitize), which I think is a bad idea.
NB, I got rid of the of the SetSanitizePlugSlots setter and do not depend on the repository anymore.
| @@ -51,6 +51,8 @@ func Manager(s *state.State, hookManager *hookstate.HookManager, extraInterfaces | ||
| runner: runner, | ||
| repo: interfaces.NewRepository(), | ||
| } | ||
| + m.repo.SetSanitizePlugSlots() |
niemeyer
Oct 9, 2017
Contributor
Same idea.. quite unexpected to have that initialized at a random point like this.
| @@ -165,6 +166,9 @@ type Info struct { | ||
| Plugs map[string]*PlugInfo | ||
| Slots map[string]*SlotInfo | ||
| + // Plugs or slots with issues (they are not included in Plugs or Slots) | ||
| + BadInterfaces map[string]string // slot or plug => message |
zyga
Sep 28, 2017
Contributor
Shall we store the real error objects here? This way we could generate i18n specific message later.
stolowski
Oct 3, 2017
Contributor
I'm not thrilled about storing error objects here, because these problems with interfaces are not actually treated as errors. Also, nothing is stopping us from populating this map with i18n, is it?
zyga
Oct 3, 2017
Contributor
The errors can be stored and transformed to per-request i18n message later. We cannot translate them at this time.
niemeyer
Oct 9, 2017
Contributor
We probably don't need to store those errors. Instead, if the interface definition is completely broken, we can reject the snap loading with the actual error.
stolowski
Oct 9, 2017
Contributor
@niemeyer We have two possible problems to deal with: an unknown interface or interface not passing sanitization. AFAIR we didn't want to reject snaps on unknown interfaces.
Shall we only reject snaps that fail sanitization?
niemeyer
Oct 18, 2017
Contributor
It's okay, let's keep it this way for now. Strings are fine for the time being (it is already like that in master). Easy to change when we want to.
|
I think that we can use the |
Some checks were not successful
Some checks were not successful
niemeyer
changed the title from
repo: sanitize plugs and slots early in ReadInfo
to
interfaces: sanitize plugs and slots early in ReadInfo
Oct 18, 2017
| @@ -43,6 +43,9 @@ var opts struct { | ||
| } | ||
| func main() { | ||
| + // plug/slot sanitization not used nor possible from snap-exec, make it no-op | ||
| + snap.SanitizePlugsSlots = func(snapInfo *snap.Info) {} |
niemeyer
Oct 18, 2017
Contributor
If you move this into an init() function there's no need to mock it in tests.
In either case, both snap-exec and the snap command should either have it in main, or in init, so we don't need to keep that distinction in mind.
| + for plugName, plugInfo := range snapInfo.Plugs { | ||
| + iface, ok := allInterfaces[plugInfo.Interface] | ||
| + if !ok { | ||
| + snapInfo.BadInterfaces[plugName] = "unknown interface" |
| @@ -165,6 +166,9 @@ type Info struct { | ||
| Plugs map[string]*PlugInfo | ||
| Slots map[string]*SlotInfo | ||
| + // Plugs or slots with issues (they are not included in Plugs or Slots) | ||
| + BadInterfaces map[string]string // slot or plug => message |
zyga
Sep 28, 2017
Contributor
Shall we store the real error objects here? This way we could generate i18n specific message later.
stolowski
Oct 3, 2017
Contributor
I'm not thrilled about storing error objects here, because these problems with interfaces are not actually treated as errors. Also, nothing is stopping us from populating this map with i18n, is it?
zyga
Oct 3, 2017
Contributor
The errors can be stored and transformed to per-request i18n message later. We cannot translate them at this time.
niemeyer
Oct 9, 2017
Contributor
We probably don't need to store those errors. Instead, if the interface definition is completely broken, we can reject the snap loading with the actual error.
stolowski
Oct 9, 2017
Contributor
@niemeyer We have two possible problems to deal with: an unknown interface or interface not passing sanitization. AFAIR we didn't want to reject snaps on unknown interfaces.
Shall we only reject snaps that fail sanitization?
niemeyer
Oct 18, 2017
Contributor
It's okay, let's keep it this way for now. Strings are fine for the time being (it is already like that in master). Easy to change when we want to.
| @@ -323,6 +327,32 @@ func (s *Info) Services() []*AppInfo { | ||
| return svcs | ||
| } | ||
| +func (s *Info) BadInterfacesInfoString() string { |
niemeyer
Oct 18, 2017
Contributor
This would be nicer going into a function of its own. It seems too specialized and too custom-formatted to make sense in Info itself.
The function might look like this, at the package level:
func BadInterfacesSummary(snapInfo *snap.Info) string
I think we'll also want to change the formatting soon so it's more readable, but we can do that in a follow up.
stolowski
added some commits
Oct 19, 2017
stolowski commentedSep 27, 2017
Sanitize plugs and slots early in snap.ReadInfo so that every PlugInfo and SlotInfo is guaranteed to be sanitized when used. To preserve the current treatment of bad interfaces (which we skip but log in the task) I had to move away from BadInterfaceError (which is not really an error) and store bad interfaces in snap.Info, as they need to be reported at the point we have task at hand, rather than immediately on sanitize.
See the plan outlined here https://forum.snapcraft.io/t/preparing-the-interfaces-logic-for-connection-hooks/2184 - this is item no. 1 of the plan.