many: rename two core plugs that clash with slot names #3154

Merged
merged 11 commits into from Apr 10, 2017

Conversation

Projects
None yet
5 participants
Contributor

zyga commented Apr 7, 2017

This branch takes a proactive approach towards ensuring we don't have clashing plug and slot names
on the core snap. The core ships with "network-bind" and "core-support" plugs that clash with
the corresponding slot names. Because of missing validation this did not surface earlier.
Validation is added in a separate branch.

This branch focuses on automatically renaming the two plugs so that no clash occurs.
This has two parts:

  • support for renaming plugs on snap.Info
  • rename the two plugs on when loading snap meta-data from disk

@zyga zyga added this to the 2.24 milestone Apr 7, 2017

snap/snaptest/snaptest.go
+ c.Assert(err, check.IsNil)
+ snapInfo.SideInfo = *sideInfo
+ err = snap.Validate(snapInfo)
+ c.Assert(err, check.Not(check.IsNil))
@chipaca

chipaca Apr 7, 2017

Member

You'll get a better error message if you use c.Assert(err, check.NotNil)

@zyga

zyga Apr 7, 2017

Contributor

Nice suggestion, done

zyga added some commits Apr 7, 2017

snap: add function for renaming plugs
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: rename clashing plugs on core snap
This patch makes the interface manager transparently rename two plugs
(network-bind and core-support) on the core snap so that they no longer
clash with the slots with the same name.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap/snaptest: add MockInvalidInfo
This patch allows to create invalid snap.Info objects for testing.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: extend the test for snap.RenamePlug
We now also check that slots are not affected

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap/snaptest: improve MockInvalidInfo
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: do extra check in plug rename tests
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap,overlord/ifacestate: rename clashing core plugs in snap/
This patch moves the rename of two clashing plugs that exist on the core
snap from overlord/ifacestate to snap/. In particular the two functions
ReadInfo and ReadInfoFromSnapFile now perform this operation. This will
ensure that nobody has to remember to call the fix function in new places.

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

zyga commented Apr 10, 2017

I removed the commit that renamed stored connection data so this no longer touches the state. The missing commit can be found in #3160

snap/info.go
@@ -538,6 +559,8 @@ func ReadInfo(name string, si *SideInfo) (*Info, error) {
return nil, err
}
+ info.renameClashingCorePlugs()
@mvo5

mvo5 Apr 10, 2017

Collaborator

Having this in two places makes me wonder if we should move it instead into InfoFromSnapYaml() ?

@zyga

zyga Apr 10, 2017

Contributor

I think we need to revisit this in general as some "fixup" is done in overlord/snapmgr and some here but I didn't want to do any wider changes. EDIT: Ideally all of broken.go would be private and would be applied internally.

@mvo5

mvo5 Apr 10, 2017

Collaborator

Yeah, my concern here is just that we have two places currently where we fixup things snap.go. I would really like to have only a single place.

@pedronis

pedronis Apr 10, 2017

Contributor

yes, a bit unclear why not doing it in InfoFromSnapYaml ?

@zyga

zyga Apr 10, 2017

Contributor

No real reason, I'll move it now. EDIT: done

zyga added some commits Apr 10, 2017

tests: adjust to look for network-bind-plug
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: move plug core rename to InfoFromSnapYaml
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5 approved these changes Apr 10, 2017

Thanks, looks good!

LGTM other than:

snap/info.go
@@ -275,6 +275,27 @@ func (s *Info) NeedsClassic() bool {
return s.Confinement == ClassicConfinement
}
+// RenamePlug renames the plug from oldName to newName, if present
+func (s *Info) RenamePlug(oldName, newName string) {
@niemeyer

niemeyer Apr 10, 2017

Contributor

This should be renamePlug inside broken.go rather than a public API. This is far from being a well supported or desired operation.

@zyga

zyga Apr 10, 2017

Contributor

Done, can you +1 this so that we can land it tomorrow morning?

snap: name Info.RenamePlug private
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga merged commit 4d17974 into snapcore:master Apr 10, 2017

6 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:rename-clashing-core-plugs branch Apr 10, 2017

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