On Wed, Oct 19, 2016 at 8:04 AM, Scott Moser <smo...@ubuntu.com> 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 <ryan.har...@canonical.com> > > +# > > +# 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: u...@user.org > > + 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: f...@foo.io > > + 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. > -- 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. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
