On Wed, Oct 19, 2016 at 8:04 AM, Scott Moser <email address hidden> wrote:

> over all, looks good.
> you dont have to clean up the handle, but if you see easy way to do that
> that'd be nice.
>
> is the snappy path now valid on non-snappy system ? (ubuntu server with
> 'snap' support).
>

It is, and util.is_system_snappy() I think passes where it works; I'll
confirm.


>
>
> lastly, please just review your commit message and make sure its up to
> date with all changes (it may well be, just think you made some changes
> since you wrote it).
>

ACK, it needs updating to account for the snappy: namespace changes.


>
>
> Diff comments:
>
> > diff --git a/cloudinit/config/cc_snap_config.py
> b/cloudinit/config/cc_snap_config.py
> > new file mode 100644
> > index 0000000..667b9c6
> > --- /dev/null
> > +++ b/cloudinit/config/cc_snap_config.py
> > @@ -0,0 +1,177 @@
> > +# vi: ts=4 expandtab
> > +#
> > +#    Copyright (C) 2016 Canonical Ltd.
> > +#
> > +#    Author: Ryan Harper <email address hidden>
> > +#
> > +#    This program is free software: you can redistribute it and/or
> modify
> > +#    it under the terms of the GNU General Public License version 3, as
> > +#    published by the Free Software Foundation.
> > +#
> > +#    This program is distributed in the hope that it will be useful,
> > +#    but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +#    GNU General Public License for more details.
> > +#
> > +#    You should have received a copy of the GNU General Public License
> > +#    along with this program.  If not, see <
> http://www.gnu.org/licenses/>.
> > +
> > +"""
> > +Snappy
> > +------
> > +**Summary:** snap_config modules allows configuration of snapd.
> > +
> > +This module uses the same ``snappy`` namespace for configuration but
> > +acts only only a subset of the configuration.
> > +
> > +If ``assertions`` is set and the user has included a list of assertions
> > +then cloud-init will collect the assertions into a single assertion file
> > +and invoke ``snap ack <path to file with assertions>`` which will
> attempt
> > +to load the provided assertions into the snapd assertion database.
> > +
> > +If ``email`` is set, this value is used to create an authorized user for
> > +contacting and installing snaps from the Ubuntu Store.  This is done by
> > +calling ``snap create-user`` command.
> > +
> > +If ``known`` is set to True, then it is expected the user also included
> > +an assertion of type ``system-user``.  When ``snap create-user`` is
> called
> > +cloud-init will append '--known' flag which instructs snapd to look for
> > +a system-user assertion with the details.  If ``known`` is not set, then
> > +``snap create-user`` will contact the Ubuntu SSO for validating and
> importing
> > +a system-user for the instance.
> > +
> > +.. note::
> > +    If the system is already managed, then cloud-init will not attempt
> to
> > +    create a system-user.
> > +
> > +**Internal name:** ``cc_snap_config``
> > +
> > +**Module frequency:** per instance
> > +
> > +**Supported distros:** ubuntu
> > +
> > +**Config keys**::
> > +
> > +    #cloud-config
> > +    snappy:
> > +        assertions:
> > +        - |
>
> should we support assertions as a dictionary ?
> it seemed that an assertion is in fact a dictionary, why not allow the
> user to provide it as one rather than a yaml rendered blob, and then we can
> render it.
>

It's not a dictionary, unfortunately; it's a stream of text separated by
newlines;

The format of an assertion is defined here, if you like:

https://github.com/snapcore/snapd/blob/master/asserts/asserts.go#L304

So, each entry in the list is blob of one or more assertions.  For example
you can do the following:

snap download hello
vi hello_*.snap.assertions

And see there are roughly 5 or so assertions in that single file.



>
> > +        <assertion 1>
> > +        - |
> > +        <assertion 2>
> > +        email: <email address hidden>
> > +        known: true
> > +
> > +"""
> > +
> > +from cloudinit import log as logging
> > +from cloudinit.settings import PER_INSTANCE
> > +from cloudinit import util
> > +
> > +LOG = logging.getLogger(__name__)
>
> logexc is really just for logging an exception. with a traceback to the
> log. agreed its a kind of weird function (taking a logger).
>

not following this feedback.  I want a logger so I can call LOG.NNN


>
> > +
> > +frequency = PER_INSTANCE
> > +SNAPPY_CMD = "snap"
> > +ASSERTIONS_FILE = "/var/lib/cloud/instance/snapd.assertions"
> > +
> > +distros = ['ubuntu']
>
> is this just ubuntu? or any distro with snap?
>

Could be other distros with snap.  I don't have an exhaustive list yet.


>
> > +
> > +
> > +def set_snappy_command():
> > +    global SNAPPY_CMD
> > +    if util.which("snappy-go"):
> > +        SNAPPY_CMD = "snappy-go"
> > +    elif util.which("snappy"):
> > +        SNAPPY_CMD = "snappy"
> > +    else:
> > +        SNAPPY_CMD = "snap"
> > +    LOG.debug("snappy command is '%s'", SNAPPY_CMD)
> > +
> > +
> > +"""
> > +snappy:
> > +  assertions:
> > +  - |
> > +  <snap assertion 1>
> > +  - |
> > +  <snap assertion 2>
> > +  email: <email address hidden>
> > +  known: true
> > +"""
> > +
> > +
> > +def add_assertions(assertions):
> > +    """ Import list of assertions by concatenating each
> > +        assertion into a string separated by a '\n'.
> > +        Write this string to a instance file and
> > +        then invoke `snap ack /path/to/file`
> > +        and check for errors.
> > +
> > +        If snap exits 0, then all assertions are imported.
> > +    """
> > +    if not assertions:
>
> the other thing that this sort of thing does (not necessarily here) is
> avoid a trap on python object initialization.
>
> def foo(mylist=[])
>    mylist.append("foo")
>    return mylist
>
> calling that multiple times like this:
>   mylist()
>   ret = mylist()
>
> will actually update the '[]' list.  so thats wy we often do the:
>    def foo(mylist=None)
>       if foo is None:
>           foo = []
>

ACK, will fix.


>
> > +        assertions = []
> > +
> > +    if not isinstance(assertions, list):
> > +        raise ValueError('assertion parameter was not a list: %s',
> assertions)
> > +
> > +    snap_cmd = [SNAPPY_CMD, 'ack']
> > +    combined = "\n".join(assertions)
> > +    if len(combined) == 0:
> > +        raise ValueError("Assertion list is empty")
> > +
> > +    for asrt in assertions:
> > +        LOG.debug('Acking: %s', asrt.split('\n')[0:2])
> > +
> > +    util.write_file(ASSERTIONS_FILE, combined.encode('utf-8'))
> > +    util.subp(snap_cmd + [ASSERTIONS_FILE], capture=True)
> > +
> > +
> > +def handle(name, cfg, cloud, log, args):
> > +    cfgin = cfg.get('snappy')
> > +    if not cfgin:
> > +        LOG.debug('No snappy config provided, skipping')
> > +        return
> > +
> > +    if not(util.system_is_snappy()):
> > +        LOG.debug("%s: system not snappy", name)
> > +        return
> > +
> > +    set_snappy_command()
> > +
> > +    assertions = cfgin.get('assertions', [])
> > +    if len(assertions) > 0:
> > +        LOG.debug('Importing user-provided snap assertions')
> > +        add_assertions(assertions)
> > +
> > +    # Create a snap user if requested.
>
> the less you do in a 'handle' the easier test is in myopinion.
> you can add a metdho that does this hunk of code below and then unit test
> it by itself and avoid the wierd signature of handle.
>

ACK, will move.


>
> > +    # Snap systems contact the store with a user's email
> > +    # and extract information needed to create a local user.
> > +    # A user may provide a 'system-user' assertion which includes
> > +    # the required information. Using such an assertion to create
> > +    # a local user requires specifying 'known: true' in the supplied
> > +    # user-data.
> > +    snapuser = cfgin.get('email', None)
> > +    if snapuser:
> > +        usercfg = {
> > +            'snapuser': snapuser,
> > +            'known': cfgin.get('known', False),
> > +        }
> > +
> > +        # query if we're already registered
> > +        out, _ = util.subp([SNAPPY_CMD, 'managed'], capture=True)
> > +        if out.strip() == "true":
> > +            LOG.warning('This device is already managed. '
> > +                        'Skipping system-user creation')
> > +            return
> > +
> > +        if usercfg.get('known'):
> > +            # Check that we imported a system-user assertion
> > +            out, _ = util.subp([SNAPPY_CMD, 'known', 'system-user'],
> > +                               capture=True)
> > +            if len(out) == 0:
> > +                LOG.error('Missing "system-user" assertion. '
> > +                          'Check "snappy" user-data assertions.')
> > +                return
> > +
> > +        cloud.distro.create_user(snapuser, **usercfg)
> > diff --git a/cloudinit/config/cc_snappy.py b/cloudinit/config/cc_snappy.
> py
> > index 36db9e6..e03ec48 100644
> > --- a/cloudinit/config/cc_snappy.py
> > +++ b/cloudinit/config/cc_snappy.py
> > @@ -257,24 +257,14 @@ def disable_enable_ssh(enabled):
> >          util.write_file(not_to_be_run, "cloud-init\n")
> >
> >
> > -def system_is_snappy():
> > -    # channel.ini is configparser loadable.
> > -    # snappy will move to using /etc/system-image/config.d/*.ini
> > -    # this is certainly not a perfect test, but good enough for now.
> > -    content = util.load_file("/etc/system-image/channel.ini",
> quiet=True)
> > -    if 'ubuntu-core' in content.lower():
> > -        return True
> > -    if os.path.isdir("/etc/system-image/config.d/"):
> > -        return True
> > -    return False
> > -
> > -
> >  def set_snappy_command():
> >      global SNAPPY_CMD
> >      if util.which("snappy-go"):
> >          SNAPPY_CMD = "snappy-go"
> > -    else:
> > +    elif util.which("snappy"):
> >          SNAPPY_CMD = "snappy"
> > +    else:
> > +        SNAPPY_CMD = "snap"
>
> we could probably drop legacy snappy . that path would only be taken on
> 15.04 , right?
>

Possibly; I just don't know if someone builds a new cloud-init and runs on
an older system.
I've never tested anything but the current snapd/all-snap systems.

It's certainly likely that a system that has 'snappy' won't have any of the
 commands I use:

create-user, managed, known, ack

I wonder if we should confirm we have those sub commands in snapd or just
let it fail
naturally?



>
> >      LOG.debug("snappy command is '%s'", SNAPPY_CMD)
> >
> >
>
>
> --
> https://code.launchpad.net/~raharper/cloud-init/+git/
> cloud-init/+merge/304700
> You are the owner of ~raharper/cloud-init:snapuser-create.
>