cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

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).


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).


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:
> +        - |

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.

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

logexc is really just for logging an exception. with a traceback to the log. agreed its a kind of weird function (taking a logger).

> +
> +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?

> +
> +
> +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:

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 = []

> +        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.

> +    # 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?

>      LOG.debug("snappy command is '%s'", SNAPPY_CMD)
>  
>  


-- 
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.