interfaces/repo: validate slot/plug names #2932

Merged
merged 2 commits into from Apr 6, 2017

Conversation

Projects
None yet
7 participants
Contributor

stolowski commented Feb 24, 2017

It's seems that by an oversight or an error in refactoring we don't validate slot and plug names coming from snap yaml on installation anymore. This allows names such as ttyS5 go through, which then causes an error on interface hooks ("error: cannot perform the following tasks:- Prepare connection of slot caracalla:ttyS4 (internal error: no registered handlers for hook "prepare-slot-ttyS4"), because they are strict on names.

This PR adds validation (back?) to AddSnap. Note that it may break (according to morhpis, it actually will break) some existing snaps, so I'm open to discussion what we can do to workaround/mitigate, other than relaxing this validation.

I think this code is good and should land but we need a way to migrate systems that used wrong plugs / slot names to alternate names.

One idea is to ship a core snap that can rename interfaces according to gadget hints. Following that release a gadget that assumes: snapdXX which contains such definitions.

mvo5 approved these changes Mar 3, 2017

Code looks fine, mitigation for existing user is something we need to discuss, ideally with @niemeyer

@niemeyer niemeyer changed the title from snap yaml: validate slot/plug names to interfaces/repo: validate slot/plug names Mar 10, 2017

@pedronis pedronis added the Blocked label Mar 11, 2017

Contributor

pedronis commented Mar 11, 2017

marked as Blocked until we discuss the path forward for preexisting snaps/revisions with the issue

Contributor

stolowski commented Mar 15, 2017

Regarding existing snaps affected by this change... I think they are broken already anyway - if they use capital letters in plugs (slots), they will fail on hooks (even if they don't have hooks) - that would need to be checked with @morphis who originally experienced this issue.

Contributor

morphis commented Mar 15, 2017

@stolowski We mainly have those plugs on gadget snaps but we can work around this problem when we coordinate a release of the core snap and the gadget snap to happen with the same OTA.

Contributor

niemeyer commented Mar 16, 2017

@morphis Thanks for the flexibility there, it's appreciated. I suggest going ahead right now and replacing the plugs/slot names so that they are lowercase. This was never intended to be accepted, and there are likely areas in the code already that will blow up if they see such names.

I've got a list of snap.yaml files from Celso today. It's not complete, but it comprises 663 snaps that are classic or strict and released in stable. Missing from the set from stable are only very old snaps released before the store started collecting snap.yaml into the database.

The result is that none of them have uppercase slot or plug names. If you're curious, this is the list: http://paste.ubuntu.com/24186301/

We can try to gather more data, but I consider this and @morphis willingness to address the issue with existing gadgets as good evidence that we can go ahead and fix the bug.

@niemeyer niemeyer removed the Blocked label Mar 16, 2017

Contributor

morphis commented Mar 16, 2017

Contributor

morphis commented Mar 16, 2017

@niemeyer Btw. your list doesn't cover branded stores as I miss specific product ones which are definitely there. Can you cross-check those too?

@zyga zyga added the Blocked label Apr 6, 2017

Contributor

zyga commented Apr 6, 2017

We cannot land this as core has duplicated plug/slot name and it may have unexpected consequences.

Contributor

niemeyer commented Apr 6, 2017

This looks completely unrelated to that issue. Let's please discuss in:

https://forum.snapcraft.io/t/duplicate-plug-slot-names-inside-the-core-snap/184

@niemeyer niemeyer removed the Blocked label Apr 6, 2017

@niemeyer niemeyer added this to the 2.24 milestone Apr 6, 2017

Contributor

niemeyer commented Apr 6, 2017

I'm tentatively merging this targeting 2.24.

@mvo5 @morphis Let's keep an eye on potential side effects here.

@niemeyer niemeyer merged commit 5438518 into snapcore:master Apr 6, 2017

5 of 6 checks passed

xenial-ppc64el autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment