cloud-init-dev team mailing list archive

[Jon Grimm <jon.grimm@xxxxxxxxxxxxx>]

Thanks, it was enlightening and easy to read.  

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 <ryan.harper@xxxxxxxxxxxxx>
> +#
> +#    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:
> +        - |
> +        <assertion 1>
> +        - |
> +        <assertion 2>
> +        email: user@xxxxxxxx
> +        known: true
> +
> +"""
> +
> +from cloudinit import log as logging
> +from cloudinit.settings import PER_INSTANCE
> +from cloudinit import util
> +
> +LOG = logging.getLogger(__name__)

Ah, thanks for the explanation!  Maybe we should doc the cleanup somewhere?  Possibly a newbie/starter task.

> +
> +frequency = PER_INSTANCE
> +SNAPPY_CMD = "snap"
> +ASSERTIONS_FILE = "/var/lib/cloud/instance/snapd.assertions"
> +
> +distros = ['ubuntu']
> +
> +
> +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: foo@xxxxxx
> +  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:

1) WRT generic utility function so ok to be public (but just resisting moving for now).  +1  Seems fine to me too with that explanation. 
2) WRT leaving checks here.   I didn't mind having checks here, its just that you have similar checks in multiple places seemed a bit icky -- maybe the check at L149 is moved/removed. (again, totally 100% minor nit and certainly a judgement call even, just wanted to clarify my earlier comment, I realize I'm a bit weird that I like think about consistent error handling policies across a codebase).

> +        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'))

No worries, there just wasn't any comment so had to check.  Totally understand that there may be nothing we can really do.. but I didn't know if not being able to write the file would be useful to bubble to a user for debugability.  Or really what might be the reasons for 'snap ack' failing, but wondered about surfacing.  Again, not knowing enough about the underlying calls to say just something to think about on debugability/visibility.

> +    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.
> +    # 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/distros/__init__.py b/cloudinit/distros/__init__.py
> index b1192e8..101c7ec 100644
> --- a/cloudinit/distros/__init__.py
> +++ b/cloudinit/distros/__init__.py
> @@ -445,6 +448,32 @@ class Distro(object):
>              util.logexc(LOG, "Failed to create user %s", name)
>              raise e
>  
> +    def add_snap_user(self, name, **kwargs):
> +        """
> +        Add a snappy user to the system using snappy tools
> +        """
> +
> +        snapuser = kwargs.get('snapuser')
> +        known = kwargs.get('known', False)
> +        adduser_cmd = ["snap", "create-user", "--sudoer", "--json"]
> +        if known:
> +            adduser_cmd.append("--known")
> +        adduser_cmd.append(snapuser)
> +
> +        # Run the command
> +        LOG.debug("Adding snap user %s", name)
> +        try:
> +            (out, err) = util.subp(adduser_cmd, logstring=adduser_cmd,

Heh, ok, thought I was missing something/wanted to learn.. I can't see need for them.

> +                                   capture=True)
> +            LOG.debug("snap create-user returned: %s:%s", out, err)
> +            jobj = util.load_json(out)
> +            username = jobj.get('username', None)
> +        except Exception as e:
> +            util.logexc(LOG, "Failed to create snap user %s", name)
> +            raise e
> +
> +        return username
> +
>      def create_user(self, name, **kwargs):
>          """
>          Creates users for the system using the GNU passwd tools. This


-- 
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/304700
Your team cloud init development team is requested to review the proposed merge of ~raharper/cloud-init:snapuser-create into cloud-init:master.