New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

overlord,daemon,cmd: re-map snap names around the edges of snapd #5491

Merged
merged 20 commits into from Jul 17, 2018

Conversation

Projects
None yet
4 participants
@zyga
Contributor

zyga commented Jul 10, 2018

This branch provides the support code for allowing snapd to re-map snap names in interface interactions around the edges of the system, namely the state and the API.

The branch contains three main segments:

  • the remapping support code in the interface manager (two patches)
  • re-mapping of connection state ({get,set}Conns) (one patch)
  • re-mapping of API requests and responses (one patch)
  • discarding commented-out stale test (one patch)

Updates over weekend:

  • reduce the code to just snap name re-mapping
  • unify with and replace the nickname system
  • enable the core-core-system mapper (system in the API, core in state and memory)

@zyga zyga added the ⛔ Blocked label Jul 10, 2018

@zyga zyga force-pushed the zyga:feature/plug-slot-remapping branch from 679b1de to 8056bc0 Jul 10, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jul 10, 2018

Codecov Report

Merging #5491 into master will increase coverage by 0.12%.
The diff coverage is 95.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5491      +/-   ##
==========================================
+ Coverage   78.95%   79.07%   +0.12%     
==========================================
  Files         514      514              
  Lines       38854    38918      +64     
==========================================
+ Hits        30678    30776      +98     
+ Misses       5710     5673      -37     
- Partials     2466     2469       +3
Impacted Files Coverage Δ
snap/info.go 83.47% <ø> (-0.38%) ⬇️
overlord/configstate/configstate.go 87.5% <100%> (+2.08%) ⬆️
daemon/api.go 77.33% <100%> (+2.49%) ⬆️
cmd/snap/cmd_interfaces.go 85.45% <100%> (ø) ⬆️
cmd/snap/cmd_interface.go 79.54% <75%> (ø) ⬆️
overlord/ifacestate/helpers.go 70.91% <93.65%> (+4.46%) ⬆️
overlord/ifacestate/handlers.go 64.88% <0%> (-0.4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 999c6bd...e969d39. Read the comment docs.

zyga added some commits Jul 9, 2018

overlord/ifacestate: add re-mapping function for plugs and slots
This patch adds re-mapping functions for plugs and slots. The idea with
re-mapping is that snapd can provide the appearance of a particular plug
and slot placement as observed in the on-disk state and over-the-wire
communication while having a different actual placement in memory. In
practice this is designed to allow us to place implicit slots on the
snapd snap.

Re-mapping is implemented by a golang interface. Currently only a no-op
mapper is used. The mapper is global but could be moved to the interface
manager after changing a large number of functions to become methods on
the interface manager. This can be done in a follow-up, if desired.

Apart from the golang interface, which provides the low-level
operations, a number of helpers for other data types are provided. This
includes connection references and plug and slot information objects.
Those helpers are used by the daemon API layer in one of the subsequent
patches.

Two real mappers are implemented: a no-op mapper and a core<=>snapd
mapper. For testing a upper<=>lower case mapper is used, for extra
clarity. Everything is accompanied by unit tests.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: drop re-mapping snap.{Plug,Slog}Info
The helpers for re-mapping the aforementioned types are no longer
necessary due to re-factoring and cleanup of the daemon code.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: re-map connection state on load/store from state
This patch changes getConns and setConns to re-map connection state just
after loading from the state and just before being saved to the state.
This is coupled with explicit tests for {get,set}Conns that we apparently
didn't have.

Note that at this stage the code is still dormant since re-mapping is
a no-op until the "snapd" snap becomes the host of the implicit slots.

This is the "back" side of the re-mapping. The "front" side applies to
re-mapping of the API layer. It is provided in the follow-up patch.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
daemon: re-map interface state in API requests/responses
This patch changes interface connection, disconnection and query APIs to
apply re-mapping on incoming requests and outgoing responses. This
allows code to issue explicit connect and disconnect API requests for
the "core" snap even though implicit slots are on the "snapd" snap.
Similarly the API for querying interface connections (both the legacy,
as used by "snap interfaces" and the per-interface as used by "snap
interface") are updated to take advantage of this feature.

Note that at this stage the code is still dormant since re-mapping is a
no-op until the "snapd" snap becomes the host of the implicit slots.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
daemon: drop unused, commented-out test
We don't need a test that doesn't work. I added tests for this in the
previous patch anyway.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga force-pushed the zyga:feature/plug-slot-remapping branch from 8056bc0 to 72f358b Jul 12, 2018

@zyga zyga added Core18 and removed ⛔ Blocked labels Jul 12, 2018

@pedronis

the code looks good, still have some high level questions/concerns

// Data coming from the state and from API requests is changed so that slots on "core"
// become slots on "snapd" (but only when "snapd" snap itself is being used). When
// data is about to hit the state again it is re-mapped back.
func RemapIncomingConnRef(cref *interfaces.ConnRef) (changed bool) {

This comment has been minimized.

@pedronis

pedronis Jul 13, 2018

Contributor

I think it would be safer if these Remap* functions didn't do changes in place but where functional (the internal interface can keep being in place)

This comment has been minimized.

@zyga

zyga Jul 13, 2018

Contributor

Done

for _, info := range infos {
// Convert interfaces.Info into interfaceJSON
plugs := make([]*plugJSON, 0, len(info.Plugs))
for _, plug := range info.Plugs {
// Re-map outgoing plug information.

This comment has been minimized.

@pedronis

pedronis Jul 13, 2018

Contributor

at this point in API if understand correctly we will possibly return "core" in some situations even if the snap doesn't exist, right?

wondering if we should go all the way and as we discussed skip this intermediate state and return "system" instead, it means that the mapping to state vs the mapping for the outgoing API world need to be different, or that we need another layer of mapping just here (core => system)

This comment has been minimized.

@zyga

zyga Jul 13, 2018

Contributor

Because the mapping is puggable we can easily return system here. It is all defined by the CoreSnapdMapper (which should be renamed). I can change that to return system for both snapd and core instead.

This comment has been minimized.

@zyga

zyga Jul 13, 2018

Contributor

I am looking at what it would take to return "system" to the API and "core" to to the state now.

return changedPlug || changedSlot
}
// RemapOutgoingPlugRef potentially re-maps the snap name of a plug reference.

This comment has been minimized.

@pedronis

pedronis Jul 13, 2018

Contributor

given that snapd will have only implicit slots afawk, remapping of plugs is only for completeness but is not stricly needed right?

This comment has been minimized.

@zyga

zyga Jul 13, 2018

Contributor

Exactly so

@pedronis pedronis requested a review from niemeyer Jul 13, 2018

zyga added some commits Jul 13, 2018

overlord,daemon: drop return values from interface mappers
The re-mapping helper functions were returning a boolean if a mapping
was actually applied (something changed) but this value is actually no
longer used anywhere. In the spirit of YAGNI let's remove it.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord,daemon: make re-mapping helpers functional
The remaping functions were modifying their arguments. This patch makes
them return a modified copy instead.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
daemon,api: separate mappings for state and API
This patch changes a number of things, the main concept is that
the mapping system can now offer separate replacements for state
handling, where we want to stick to the word "core" for compatibility,
and the API layer where we want to start using the "system" word
as a nickname to whatever is providing implicit slots.

As such the mapping functions are duplicated, once for state and once
for request/response. The intermediate helpers for working with
connection references are dropped because this just multiplies trivial
functions that don't need to be used.

The existing NilMapper is expanded to cover the new API. The existing
CoreSnapdMapper is split into two new mappers: CoreSnapdSystemMapper
which uses core in the state, snapd in memory and system in the API layer
and a new CoreCoreSystemMapper which uses core in the state and in memory
but system in the API layer.

That last new mapper is a candidate for transition of spread tests
that could start to talk about "system" natively (without the hacks in
the client) and would then work without change once the core-snapd-system
mapper can be used, which will happen once snapd starts to host implicit
slots.

The daemon and state are obviously updated to switch to the appopriate
mapping helper.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: move mapper closer to related functions
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: rename NilMapper to IdentityMapper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
cmd,overlord,daemon: merge snap nickname with iface mapper
While testing the interface mapper I realized the nickname system is
still in use and actually clashes with the interface mapper. Looking at
it close it is obvious that the nickname system must be folded into the
interface mapper for consistency and that the interface mapper must be
able to map snap names more than than plug or slot references.

Without this change we can never have implicit slots on the "snapd" snap
because implicit gadget connections would resolve to "core" (which can
be absent) and because the nickname handling is would clash with the
idea of using "system" in the API layer but "snapd" in the memory
(because nicknames would effectively change "system" to "core".

To fix this I extended the interface mapper to also be a snap name
mapper and removed the nickname system entirely. This also allows us to
drop the ever-growing list of special cases in the client (for interface
abbreviation logic).

More precisely this patch:
 - drops the {Use,Drop}Nick functions from the snap package
 - changes the default mapper to CoreCoreSystem mapper
 - adds a SnapMapper that has request/response state save/load functions
 - makes InterfaceMapper a superset of SnapMapper
 - refactors CoreCoreSystemMapper and CoreSnapdSystemMapper
 - makes the daemon use RemapSnapFromRequest instead of the nick
 - makes gadget resolver use RemapSnapFromRequest to resolve "system"
 - makes the client abbreviate just "system" slots

This comes without a full array of tests but without breaking any
existing test (woot). It also passes spread which is promising.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord,daemon: reduce InteraceMapper to SnapMapper
With the insight from the previous patch I have now simplified all of
the interface mapper to just a SnapMapper wich can be thought as
server-side nickname system.

This mostly cuts the amount of boilerplate code that doesn't do anything
we currently need by limiting the re-mapping to just snap names and to
not be able to differentiate plugs and slots anymore.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: add extra tests for better coverage
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: remove uneeded teardown method
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
daemon: avoid changing existing ConnRef
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
daemon: undo needless patch delta
This patch just restores original code where it doesn't make any
meaningful changes.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga changed the title from overlord,daemon: re-map interface plugs and slots around the edges of snapd to overlord,daemon,cmd: re-map snap names around the edges of snapd Jul 13, 2018

@mvo5

Looks nice! Thanks for working on this.

ifacestate.IdentityMapper // Embed the identity mapper to reuse empty state mapping functions.
}
func (m *inverseCaseMapper) RemapSnapFromRequest(snapName string) string {

This comment has been minimized.

@mvo5

mvo5 Jul 16, 2018

Collaborator

(nitpick^2) I wonder if we should have something like from-req-$snapName (and vice for the RemapSnapToResponse).

@@ -1647,7 +1647,7 @@ func splitQS(qs string) []string {
func getSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
vars := muxVars(r)
snapName := snap.DropNick(vars["name"])
snapName := ifacestate.RemapSnapFromRequest(vars["name"])

This comment has been minimized.

@mvo5

mvo5 Jul 16, 2018

Collaborator

For configuration we may need our own set of of remap helpers. Last time we discussed this the idea was that we map all configuration from "system" -> "core" even on a core18 system. If this has changed thats fine but we probably still need to tweak things a bit because e.g. "canConfigure()" allows setting "core" even if core is not installed. If we change where the "system" configuration is stored we probably need to update "canConfigure"

This comment has been minimized.

@pedronis

pedronis Jul 16, 2018

Contributor

it feels also strange to use a helper from ifacestate in configuration related code

This comment has been minimized.

@mvo5

mvo5 Jul 16, 2018

Collaborator

One simple fix would be to have similar remap helpers in configstate.

This comment has been minimized.

@zyga

zyga Jul 16, 2018

Contributor

This is incorrect application of IMO correct logic. This simple tweak ought to fix this:

snapName := ifacestate.RemapSnapToStore(ifacestate.RemapSnapFromRequest(vars["name"]))
@pedronis

using the helpers from ifacestate for the config bits feels confusing and seems also wrong, we need to map system to core in any case there or do some other changes

@@ -1647,7 +1647,7 @@ func splitQS(qs string) []string {
func getSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
vars := muxVars(r)
snapName := snap.DropNick(vars["name"])
snapName := ifacestate.RemapSnapFromRequest(vars["name"])

This comment has been minimized.

@pedronis

pedronis Jul 16, 2018

Contributor

it feels also strange to use a helper from ifacestate in configuration related code

// even if the request used "core".
func (m *CoreSnapdSystemMapper) RemapSnapFromRequest(snapName string) string {
if snapName == "system" || snapName == "core" {
return "snapd"

This comment has been minimized.

@pedronis

pedronis Jul 16, 2018

Contributor

this is wrong for the use we do for config!

return snapName
}
func (m *CoreSnapdSystemMapper) ConfigIfaceRemapSnapToResponse(snapName string) string {

This comment has been minimized.

@pedronis

pedronis Jul 16, 2018

Contributor

is ConfigIface a typo? is there no tests failing?

This comment has been minimized.

@mvo5

mvo5 Jul 16, 2018

Collaborator

Yeah, sorry for this.

*
*/
package snap

This comment has been minimized.

@pedronis

pedronis Jul 16, 2018

Contributor

I'm not sure snap is the best place for this, a mapper makes sense and can be chosen only inside snapd

This comment has been minimized.

@mvo5

mvo5 Jul 16, 2018

Collaborator

I pushed a simpler version that just adds a remap helper into configstate now.

@mvo5 mvo5 force-pushed the zyga:feature/plug-slot-remapping branch from ff338c7 to 292c38b Jul 16, 2018

@pedronis

+1, with some small comments

@@ -120,3 +120,19 @@ func Configure(st *state.State, snapName string, patch map[string]interface{}, f
task := hookstate.HookTask(st, summary, hooksup, contextData)
return state.NewTaskSet(task)
}
// RemapSnapFromRequest renames a snap as received from an API request
func RemapSnapFromRequest(snapName string) string {

This comment has been minimized.

@pedronis

pedronis Jul 17, 2018

Contributor

these need unit tests?

This comment has been minimized.

@zyga

zyga Jul 17, 2018

Contributor

Yes, I just wrote those.

// exactly the same as with the "snapd" mapper. This can be used to make any
// necessary adjustments the test suite.
type CoreCoreSystemMapper struct {
IdentityMapper // Embedding the nil mapper allows us to cut on boilerplate.

This comment has been minimized.

@pedronis

pedronis Jul 17, 2018

Contributor

nil mapper seems the wrong name now? same below

This comment has been minimized.

@zyga

zyga Jul 17, 2018

Contributor

Fixed here and below.

}
// RemapSnapToResponse renames a snap as about to be sent from an API response
func RemapSnapToResponse(snapName string) string {

This comment has been minimized.

@pedronis

pedronis Jul 17, 2018

Contributor

is this ever used?

This comment has been minimized.

@zyga

zyga Jul 17, 2018

Contributor

No it is not. I'm looking if it should be used now.

This comment has been minimized.

@zyga

zyga Jul 17, 2018

Contributor

Done now

zyga added some commits Jul 17, 2018

overlord/ifacestate: update comments to refer to identity mapper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/confistate: add tests for remappers
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@@ -1714,7 +1714,7 @@ func setSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
taskset, err := configstate.ConfigureInstalled(st, snapName, patchValues, 0)
if err != nil {
if _, ok := err.(*snap.NotInstalledError); ok {
return SnapNotFound(snapName, err)
return SnapNotFound(configstate.RemapSnapToResponse(snapName), err)

This comment has been minimized.

@pedronis

pedronis Jul 17, 2018

Contributor

we will not hit this case because core/system can be configured before anything is installed

This comment has been minimized.

@zyga

zyga Jul 17, 2018

Contributor

Removed now

@mvo5

mvo5 approved these changes Jul 17, 2018

@zyga zyga force-pushed the zyga:feature/plug-slot-remapping branch from e969d39 to 339a091 Jul 17, 2018

@mvo5 mvo5 merged commit 1544ced into snapcore:master Jul 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zyga zyga deleted the zyga:feature/plug-slot-remapping branch Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment