From c6dff581a9c253170d5e3f12fb83d16a8dec8257 Mon Sep 17 00:00:00 2001 From: Ryan Harper Date: Thu, 12 Apr 2018 15:32:25 -0400 Subject: Implement ntp client spec with auto support for distro selection Add a base NTP client configuration dictionary and allow Distro specific changes to be merged. Add a select client function which implements logic to preferr installed clients over clients which need to be installed. Also allow distributions to override the cloud-init defaults. LP: #1749722 --- cloudinit/config/cc_ntp.py | 485 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 407 insertions(+), 78 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py index cbd0237d..9e074bda 100644 --- a/cloudinit/config/cc_ntp.py +++ b/cloudinit/config/cc_ntp.py @@ -10,20 +10,95 @@ from cloudinit.config.schema import ( get_schema_doc, validate_cloudconfig_schema) from cloudinit import log as logging from cloudinit.settings import PER_INSTANCE +from cloudinit import temp_utils from cloudinit import templater from cloudinit import type_utils from cloudinit import util +import copy import os +import six from textwrap import dedent LOG = logging.getLogger(__name__) frequency = PER_INSTANCE NTP_CONF = '/etc/ntp.conf' -TIMESYNCD_CONF = '/etc/systemd/timesyncd.conf.d/cloud-init.conf' NR_POOL_SERVERS = 4 -distros = ['centos', 'debian', 'fedora', 'opensuse', 'sles', 'ubuntu'] +distros = ['centos', 'debian', 'fedora', 'opensuse', 'rhel', 'sles', 'ubuntu'] + +NTP_CLIENT_CONFIG = { + 'chrony': { + 'check_exe': 'chronyd', + 'confpath': '/etc/chrony.conf', + 'packages': ['chrony'], + 'service_name': 'chrony', + 'template_name': 'chrony.conf.{distro}', + 'template': None, + }, + 'ntp': { + 'check_exe': 'ntpd', + 'confpath': NTP_CONF, + 'packages': ['ntp'], + 'service_name': 'ntp', + 'template_name': 'ntp.conf.{distro}', + 'template': None, + }, + 'ntpdate': { + 'check_exe': 'ntpdate', + 'confpath': NTP_CONF, + 'packages': ['ntpdate'], + 'service_name': 'ntpdate', + 'template_name': 'ntp.conf.{distro}', + 'template': None, + }, + 'systemd-timesyncd': { + 'check_exe': '/lib/systemd/systemd-timesyncd', + 'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf', + 'packages': [], + 'service_name': 'systemd-timesyncd', + 'template_name': 'timesyncd.conf', + 'template': None, + }, +} + +# This is Distro-specific configuration overrides of the base config +DISTRO_CLIENT_CONFIG = { + 'debian': { + 'chrony': { + 'confpath': '/etc/chrony/chrony.conf', + }, + }, + 'opensuse': { + 'chrony': { + 'service_name': 'chronyd', + }, + 'ntp': { + 'confpath': '/etc/ntp.conf', + 'service_name': 'ntpd', + }, + 'systemd-timesyncd': { + 'check_exe': '/usr/lib/systemd/systemd-timesyncd', + }, + }, + 'sles': { + 'chrony': { + 'service_name': 'chronyd', + }, + 'ntp': { + 'confpath': '/etc/ntp.conf', + 'service_name': 'ntpd', + }, + 'systemd-timesyncd': { + 'check_exe': '/usr/lib/systemd/systemd-timesyncd', + }, + }, + 'ubuntu': { + 'chrony': { + 'confpath': '/etc/chrony/chrony.conf', + }, + }, +} # The schema definition for each cloud-config module is a strict contract for @@ -48,7 +123,34 @@ schema = { 'distros': distros, 'examples': [ dedent("""\ + # Override ntp with chrony configuration on Ubuntu + ntp: + enabled: true + ntp_client: chrony # Uses cloud-init default chrony configuration + """), + dedent("""\ + # Provide a custom ntp client configuration ntp: + enabled: true + ntp_client: myntpclient + config: + confpath: /etc/myntpclient/myntpclient.conf + check_exe: myntpclientd + packages: + - myntpclient + service_name: myntpclient + template: | + ## template:jinja + # My NTP Client config + {% if pools -%}# pools{% endif %} + {% for pool in pools -%} + pool {{pool}} iburst + {% endfor %} + {%- if servers %}# servers + {% endif %} + {% for server in servers -%} + server {{server}} iburst + {% endfor %} pools: [0.int.pool.ntp.org, 1.int.pool.ntp.org, ntp.myorg.org] servers: - ntp.server.local @@ -83,79 +185,159 @@ schema = { List of ntp servers. If both pools and servers are empty, 4 default pool servers will be provided with the format ``{0-3}.{distro}.pool.ntp.org``.""") - } + }, + 'ntp_client': { + 'type': 'string', + 'default': 'auto', + 'description': dedent("""\ + Name of an NTP client to use to configure system NTP. + When unprovided or 'auto' the default client preferred + by the distribution will be used. The following + built-in client names can be used to override existing + configuration defaults: chrony, ntp, ntpdate, + systemd-timesyncd."""), + }, + 'enabled': { + 'type': 'boolean', + 'default': True, + 'description': dedent("""\ + Attempt to enable ntp clients if set to True. If set + to False, ntp client will not be configured or + installed"""), + }, + 'config': { + 'description': dedent("""\ + Configuration settings or overrides for the + ``ntp_client`` specified."""), + 'type': ['object'], + 'properties': { + 'confpath': { + 'type': 'string', + 'description': dedent("""\ + The path to where the ``ntp_client`` + configuration is written."""), + }, + 'check_exe': { + 'type': 'string', + 'description': dedent("""\ + The executable name for the ``ntp_client``. + For example, ntp service ``check_exe`` is + 'ntpd' because it runs the ntpd binary."""), + }, + 'packages': { + 'type': 'array', + 'items': { + 'type': 'string', + }, + 'uniqueItems': True, + 'description': dedent("""\ + List of packages needed to be installed for the + selected ``ntp_client``."""), + }, + 'service_name': { + 'type': 'string', + 'description': dedent("""\ + The systemd or sysvinit service name used to + start and stop the ``ntp_client`` + service."""), + }, + 'template': { + 'type': 'string', + 'description': dedent("""\ + Inline template allowing users to define their + own ``ntp_client`` configuration template. + The value must start with '## template:jinja' + to enable use of templating support. + """), + }, + }, + # Don't use REQUIRED_NTP_CONFIG_KEYS to allow for override + # of builtin client values. + 'required': [], + 'minProperties': 1, # If we have config, define something + 'additionalProperties': False + }, }, 'required': [], 'additionalProperties': False } } } - -__doc__ = get_schema_doc(schema) # Supplement python help() +REQUIRED_NTP_CONFIG_KEYS = frozenset([ + 'check_exe', 'confpath', 'packages', 'service_name']) -def handle(name, cfg, cloud, log, _args): - """Enable and configure ntp.""" - if 'ntp' not in cfg: - LOG.debug( - "Skipping module named %s, not present or disabled by cfg", name) - return - ntp_cfg = cfg['ntp'] - if ntp_cfg is None: - ntp_cfg = {} # Allow empty config which will install the package +__doc__ = get_schema_doc(schema) # Supplement python help() - # TODO drop this when validate_cloudconfig_schema is strict=True - if not isinstance(ntp_cfg, (dict)): - raise RuntimeError( - "'ntp' key existed in config, but not a dictionary type," - " is a {_type} instead".format(_type=type_utils.obj_name(ntp_cfg))) - validate_cloudconfig_schema(cfg, schema) - if ntp_installable(): - service_name = 'ntp' - confpath = NTP_CONF - template_name = None - packages = ['ntp'] - check_exe = 'ntpd' - else: - service_name = 'systemd-timesyncd' - confpath = TIMESYNCD_CONF - template_name = 'timesyncd.conf' - packages = [] - check_exe = '/lib/systemd/systemd-timesyncd' - - rename_ntp_conf() - # ensure when ntp is installed it has a configuration file - # to use instead of starting up with packaged defaults - write_ntp_config_template(ntp_cfg, cloud, confpath, template=template_name) - install_ntp(cloud.distro.install_packages, packages=packages, - check_exe=check_exe) +def distro_ntp_client_configs(distro): + """Construct a distro-specific ntp client config dictionary by merging + distro specific changes into base config. - try: - reload_ntp(service_name, systemd=cloud.distro.uses_systemd()) - except util.ProcessExecutionError as e: - LOG.exception("Failed to reload/start ntp service: %s", e) - raise + @param distro: String providing the distro class name. + @returns: Dict of distro configurations for ntp clients. + """ + dcfg = DISTRO_CLIENT_CONFIG + cfg = copy.copy(NTP_CLIENT_CONFIG) + if distro in dcfg: + cfg = util.mergemanydict([cfg, dcfg[distro]], reverse=True) + return cfg -def ntp_installable(): - """Check if we can install ntp package +def select_ntp_client(ntp_client, distro): + """Determine which ntp client is to be used, consulting the distro + for its preference. - Ubuntu-Core systems do not have an ntp package available, so - we always return False. Other systems require package managers to install - the ntp package If we fail to find one of the package managers, then we - cannot install ntp. + @param ntp_client: String name of the ntp client to use. + @param distro: Distro class instance. + @returns: Dict of the selected ntp client or {} if none selected. """ - if util.system_is_snappy(): - return False - if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])): - return True + # construct distro-specific ntp_client_config dict + distro_cfg = distro_ntp_client_configs(distro.name) + + # user specified client, return its config + if ntp_client and ntp_client != 'auto': + LOG.debug('Selected NTP client "%s" via user-data configuration', + ntp_client) + return distro_cfg.get(ntp_client, {}) + + # default to auto if unset in distro + distro_ntp_client = distro.get_option('ntp_client', 'auto') + + clientcfg = {} + if distro_ntp_client == "auto": + for client in distro.preferred_ntp_clients: + cfg = distro_cfg.get(client) + if util.which(cfg.get('check_exe')): + LOG.debug('Selected NTP client "%s", already installed', + client) + clientcfg = cfg + break + + if not clientcfg: + client = distro.preferred_ntp_clients[0] + LOG.debug( + 'Selected distro preferred NTP client "%s", not yet installed', + client) + clientcfg = distro_cfg.get(client) + else: + LOG.debug('Selected NTP client "%s" via distro system config', + distro_ntp_client) + clientcfg = distro_cfg.get(distro_ntp_client, {}) + + return clientcfg - return False +def install_ntp_client(install_func, packages=None, check_exe="ntpd"): + """Install ntp client package if not already installed. -def install_ntp(install_func, packages=None, check_exe="ntpd"): + @param install_func: function. This parameter is invoked with the contents + of the packages parameter. + @param packages: list. This parameter defaults to ['ntp']. + @param check_exe: string. The name of a binary that indicates the package + the specified package is already installed. + """ if util.which(check_exe): return if packages is None: @@ -164,15 +346,23 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"): install_func(packages) -def rename_ntp_conf(config=None): - """Rename any existing ntp.conf file""" - if config is None: # For testing - config = NTP_CONF - if os.path.exists(config): - util.rename(config, config + ".dist") +def rename_ntp_conf(confpath=None): + """Rename any existing ntp client config file + + @param confpath: string. Specify a path to an existing ntp client + configuration file. + """ + if os.path.exists(confpath): + util.rename(confpath, confpath + ".dist") def generate_server_names(distro): + """Generate a list of server names to populate an ntp client configuration + file. + + @param distro: string. Specify the distro name + @returns: list: A list of strings representing ntp servers for this distro. + """ names = [] pool_distro = distro # For legal reasons x.pool.sles.ntp.org does not exist, @@ -185,34 +375,60 @@ def generate_server_names(distro): return names -def write_ntp_config_template(cfg, cloud, path, template=None): - servers = cfg.get('servers', []) - pools = cfg.get('pools', []) +def write_ntp_config_template(distro_name, servers=None, pools=None, + path=None, template_fn=None, template=None): + """Render a ntp client configuration for the specified client. + + @param distro_name: string. The distro class name. + @param servers: A list of strings specifying ntp servers. Defaults to empty + list. + @param pools: A list of strings specifying ntp pools. Defaults to empty + list. + @param path: A string to specify where to write the rendered template. + @param template_fn: A string to specify the template source file. + @param template: A string specifying the contents of the template. This + content will be written to a temporary file before being used to render + the configuration file. + + @raises: ValueError when path is None. + @raises: ValueError when template_fn is None and template is None. + """ + if not servers: + servers = [] + if not pools: + pools = [] if len(servers) == 0 and len(pools) == 0: - pools = generate_server_names(cloud.distro.name) + pools = generate_server_names(distro_name) LOG.debug( 'Adding distro default ntp pool servers: %s', ','.join(pools)) - params = { - 'servers': servers, - 'pools': pools, - } + if not path: + raise ValueError('Invalid value for path parameter') - if template is None: - template = 'ntp.conf.%s' % cloud.distro.name + if not template_fn and not template: + raise ValueError('Not template_fn or template provided') - template_fn = cloud.get_template_filename(template) - if not template_fn: - template_fn = cloud.get_template_filename('ntp.conf') - if not template_fn: - raise RuntimeError( - 'No template found, not rendering {path}'.format(path=path)) + params = {'servers': servers, 'pools': pools} + if template: + tfile = temp_utils.mkstemp(prefix='template_name-', suffix=".tmpl") + template_fn = tfile[1] # filepath is second item in tuple + util.write_file(template_fn, content=template) templater.render_to_file(template_fn, path, params) + # clean up temporary template + if template: + util.del_file(template_fn) def reload_ntp(service, systemd=False): + """Restart or reload an ntp system service. + + @param service: A string specifying the name of the service to be affected. + @param systemd: A boolean indicating if the distro uses systemd, defaults + to False. + @returns: A tuple of stdout, stderr results from executing the action. + """ if systemd: cmd = ['systemctl', 'reload-or-restart', service] else: @@ -220,4 +436,117 @@ def reload_ntp(service, systemd=False): util.subp(cmd, capture=True) +def supplemental_schema_validation(ntp_config): + """Validate user-provided ntp:config option values. + + This function supplements flexible jsonschema validation with specific + value checks to aid in triage of invalid user-provided configuration. + + @param ntp_config: Dictionary of configuration value under 'ntp'. + + @raises: ValueError describing invalid values provided. + """ + errors = [] + missing = REQUIRED_NTP_CONFIG_KEYS.difference(set(ntp_config.keys())) + if missing: + keys = ', '.join(sorted(missing)) + errors.append( + 'Missing required ntp:config keys: {keys}'.format(keys=keys)) + elif not any([ntp_config.get('template'), + ntp_config.get('template_name')]): + errors.append( + 'Either ntp:config:template or ntp:config:template_name values' + ' are required') + for key, value in sorted(ntp_config.items()): + keypath = 'ntp:config:' + key + if key == 'confpath': + if not all([value, isinstance(value, six.string_types)]): + errors.append( + 'Expected a config file path {keypath}.' + ' Found ({value})'.format(keypath=keypath, value=value)) + elif key == 'packages': + if not isinstance(value, list): + errors.append( + 'Expected a list of required package names for {keypath}.' + ' Found ({value})'.format(keypath=keypath, value=value)) + elif key in ('template', 'template_name'): + if value is None: # Either template or template_name can be none + continue + if not isinstance(value, six.string_types): + errors.append( + 'Expected a string type for {keypath}.' + ' Found ({value})'.format(keypath=keypath, value=value)) + elif not isinstance(value, six.string_types): + errors.append( + 'Expected a string type for {keypath}.' + ' Found ({value})'.format(keypath=keypath, value=value)) + + if errors: + raise ValueError(r'Invalid ntp configuration:\n{errors}'.format( + errors='\n'.join(errors))) + + +def handle(name, cfg, cloud, log, _args): + """Enable and configure ntp.""" + if 'ntp' not in cfg: + LOG.debug( + "Skipping module named %s, not present or disabled by cfg", name) + return + ntp_cfg = cfg['ntp'] + if ntp_cfg is None: + ntp_cfg = {} # Allow empty config which will install the package + + # TODO drop this when validate_cloudconfig_schema is strict=True + if not isinstance(ntp_cfg, (dict)): + raise RuntimeError( + "'ntp' key existed in config, but not a dictionary type," + " is a {_type} instead".format(_type=type_utils.obj_name(ntp_cfg))) + + validate_cloudconfig_schema(cfg, schema) + + # Allow users to explicitly enable/disable + enabled = ntp_cfg.get('enabled', True) + if util.is_false(enabled): + LOG.debug("Skipping module named %s, disabled by cfg", name) + return + + # Select which client is going to be used and get the configuration + ntp_client_config = select_ntp_client(ntp_cfg.get('ntp_client'), + cloud.distro) + + # Allow user ntp config to override distro configurations + ntp_client_config = util.mergemanydict( + [ntp_client_config, ntp_cfg.get('config', {})], reverse=True) + + supplemental_schema_validation(ntp_client_config) + rename_ntp_conf(confpath=ntp_client_config.get('confpath')) + + template_fn = None + if not ntp_client_config.get('template'): + template_name = ( + ntp_client_config.get('template_name').replace('{distro}', + cloud.distro.name)) + template_fn = cloud.get_template_filename(template_name) + if not template_fn: + msg = ('No template found, not rendering %s' % + ntp_client_config.get('template_name')) + raise RuntimeError(msg) + + write_ntp_config_template(cloud.distro.name, + servers=ntp_cfg.get('servers', []), + pools=ntp_cfg.get('pools', []), + path=ntp_client_config.get('confpath'), + template_fn=template_fn, + template=ntp_client_config.get('template')) + + install_ntp_client(cloud.distro.install_packages, + packages=ntp_client_config['packages'], + check_exe=ntp_client_config['check_exe']) + try: + reload_ntp(ntp_client_config['service_name'], + systemd=cloud.distro.uses_systemd()) + except util.ProcessExecutionError as e: + LOG.exception("Failed to reload/start ntp service: %s", e) + raise + # vi: ts=4 expandtab -- cgit v1.2.3 From acca826adf39ddfedde78cfbfc47e81a06c6f42a Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 18 Apr 2018 03:35:41 -0600 Subject: pycodestyle: Fix invalid escape sequences in string literals. Python has deprecated these invalid string literals now https://bugs.python.org/issue27364 and pycodestyle is identifying them with a W605 warning. https://github.com/PyCQA/pycodestyle/pull/676 So basically, any use of \ not followed by one of [\'"abfnrtv] or \ooo (octal) \xhh (hex) or a newline is invalid. This is most comomnly seen for us in regex. To solve, you either: a.) use a raw string r'...' b.) correctly escape the \ that was not intended to be interpreted. --- cloudinit/analyze/__main__.py | 2 +- cloudinit/config/cc_apt_configure.py | 2 +- cloudinit/config/cc_power_state_change.py | 2 +- cloudinit/config/cc_rsyslog.py | 4 ++-- cloudinit/distros/freebsd.py | 6 +++--- cloudinit/util.py | 6 +++--- tests/cloud_tests/testcases/base.py | 2 +- tests/unittests/test_util.py | 2 +- 8 files changed, 13 insertions(+), 13 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/analyze/__main__.py b/cloudinit/analyze/__main__.py index 3ba5903f..f8613656 100644 --- a/cloudinit/analyze/__main__.py +++ b/cloudinit/analyze/__main__.py @@ -69,7 +69,7 @@ def analyze_blame(name, args): """ (infh, outfh) = configure_io(args) blame_format = ' %ds (%n)' - r = re.compile('(^\s+\d+\.\d+)', re.MULTILINE) + r = re.compile(r'(^\s+\d+\.\d+)', re.MULTILINE) for idx, record in enumerate(show.show_events(_get_events(infh), blame_format)): srecs = sorted(filter(r.match, record), reverse=True) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 5b9cbca0..afaca464 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -121,7 +121,7 @@ and https protocols respectively. The ``proxy`` key also exists as an alias for All source entries in ``apt-sources`` that match regex in ``add_apt_repo_match`` will be added to the system using ``add-apt-repository``. If ``add_apt_repo_match`` is not specified, it defaults -to ``^[\w-]+:\w`` +to ``^[\\w-]+:\\w`` **Add source list entries:** diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py index 4da3a588..50b37470 100644 --- a/cloudinit/config/cc_power_state_change.py +++ b/cloudinit/config/cc_power_state_change.py @@ -74,7 +74,7 @@ def givecmdline(pid): if util.is_FreeBSD(): (output, _err) = util.subp(['procstat', '-c', str(pid)]) line = output.splitlines()[1] - m = re.search('\d+ (\w|\.|-)+\s+(/\w.+)', line) + m = re.search(r'\d+ (\w|\.|-)+\s+(/\w.+)', line) return m.group(2) else: return util.load_file("/proc/%s/cmdline" % pid) diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index af08788c..27d2366c 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -203,8 +203,8 @@ LOG = logging.getLogger(__name__) COMMENT_RE = re.compile(r'[ ]*[#]+[ ]*') HOST_PORT_RE = re.compile( r'^(?P[@]{0,2})' - '(([[](?P[^\]]*)[\]])|(?P[^:]*))' - '([:](?P[0-9]+))?$') + r'(([[](?P[^\]]*)[\]])|(?P[^:]*))' + r'([:](?P[0-9]+))?$') def reload_syslog(command=DEF_RELOAD, systemd=False): diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 754d3df6..099fac5c 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -110,7 +110,7 @@ class Distro(distros.Distro): if dev.startswith('lo'): return dev - n = re.search('\d+$', dev) + n = re.search(r'\d+$', dev) index = n.group(0) (out, err) = util.subp(['ifconfig', '-a']) @@ -118,7 +118,7 @@ class Distro(distros.Distro): if len(x.split()) > 0] bsddev = 'NOT_FOUND' for line in ifconfigoutput: - m = re.match('^\w+', line) + m = re.match(r'^\w+', line) if m: if m.group(0).startswith('lo'): continue @@ -128,7 +128,7 @@ class Distro(distros.Distro): break # Replace the index with the one we're after. - bsddev = re.sub('\d+$', index, bsddev) + bsddev = re.sub(r'\d+$', index, bsddev) LOG.debug("Using network interface %s", bsddev) return bsddev diff --git a/cloudinit/util.py b/cloudinit/util.py index acdc0d85..1717b529 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -1446,7 +1446,7 @@ def get_config_logfiles(cfg): for fmt in get_output_cfg(cfg, None): if not fmt: continue - match = re.match('(?P\||>+)\s*(?P.*)', fmt) + match = re.match(r'(?P\||>+)\s*(?P.*)', fmt) if not match: continue target = match.group('target') @@ -2275,8 +2275,8 @@ def parse_mount(path): # the regex is a bit complex. to better understand this regex see: # https://regex101.com/r/2F6c1k/1 # https://regex101.com/r/T2en7a/1 - regex = r'^(/dev/[\S]+|.*zroot\S*?) on (/[\S]*) ' + \ - '(?=(?:type)[\s]+([\S]+)|\(([^,]*))' + regex = (r'^(/dev/[\S]+|.*zroot\S*?) on (/[\S]*) ' + r'(?=(?:type)[\s]+([\S]+)|\(([^,]*))') for line in mount_locs: m = re.search(regex, line) if not m: diff --git a/tests/cloud_tests/testcases/base.py b/tests/cloud_tests/testcases/base.py index 7598d469..4fda8f91 100644 --- a/tests/cloud_tests/testcases/base.py +++ b/tests/cloud_tests/testcases/base.py @@ -235,7 +235,7 @@ class CloudTestCase(unittest.TestCase): 'found unexpected kvm availability-zone %s' % v1_data['availability-zone']) self.assertIsNotNone( - re.match('[\da-f]{8}(-[\da-f]{4}){3}-[\da-f]{12}', + re.match(r'[\da-f]{8}(-[\da-f]{4}){3}-[\da-f]{12}', v1_data['instance-id']), 'kvm instance-id is not a UUID: %s' % v1_data['instance-id']) self.assertIn('ubuntu', v1_data['local-hostname']) diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 50101906..7cbd5538 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -800,7 +800,7 @@ class TestSubp(helpers.CiTestCase): os.chmod(noshebang, os.stat(noshebang).st_mode | stat.S_IEXEC) self.assertRaisesRegex(util.ProcessExecutionError, - 'Missing #! in script\?', + r'Missing #! in script\?', util.subp, (noshebang,)) def test_returns_none_if_no_capture(self): -- cgit v1.2.3 From 6d48d265a0548a2dc23e587f2a335d4e38e8db90 Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Wed, 18 Apr 2018 15:22:42 -0600 Subject: net: Depend on iproute2's ip instead of net-tools ifconfig or route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The net-tools package is deprecated and will eventually be dropped. Use "ip route", "link" or "address" instead of "ifconfig" or "route" calls. Cloud-init can now run in an environment that no longer has net-tools. This affects the network and route printing emitted to cloud-config-output.log as well as the cc_disable_ec2_metadata module. Additional changes:  - separate readResource and resourceLocation into standalone test    functions  - Fix ipv4 address rows to report scopes represented by ip addr show  - Formatted route/address ouput now handles multiple ipv4 and ipv6    addresses on a single interface Co-authored-by: James Hogarth Co-authored-by: Robert Schweikert --- cloudinit/config/cc_disable_ec2_metadata.py | 14 +- .../config/tests/test_disable_ec2_metadata.py | 50 +++ cloudinit/net/network_state.py | 11 +- cloudinit/netinfo.py | 345 ++++++++++++++++----- cloudinit/tests/helpers.py | 40 +-- cloudinit/tests/test_netinfo.py | 186 ++++++----- tests/data/netinfo/netdev-formatted-output | 10 + tests/data/netinfo/new-ifconfig-output | 18 ++ tests/data/netinfo/old-ifconfig-output | 18 ++ tests/data/netinfo/route-formatted-output | 22 ++ tests/data/netinfo/sample-ipaddrshow-output | 13 + tests/data/netinfo/sample-iproute-output-v4 | 3 + tests/data/netinfo/sample-iproute-output-v6 | 11 + tests/data/netinfo/sample-route-output-v4 | 5 + tests/data/netinfo/sample-route-output-v6 | 13 + tests/unittests/test_filters/test_launch_index.py | 10 +- tests/unittests/test_merging.py | 2 +- tests/unittests/test_runs/test_merge_run.py | 2 +- tests/unittests/test_util.py | 14 +- 19 files changed, 583 insertions(+), 204 deletions(-) create mode 100644 cloudinit/config/tests/test_disable_ec2_metadata.py create mode 100644 tests/data/netinfo/netdev-formatted-output create mode 100644 tests/data/netinfo/new-ifconfig-output create mode 100644 tests/data/netinfo/old-ifconfig-output create mode 100644 tests/data/netinfo/route-formatted-output create mode 100644 tests/data/netinfo/sample-ipaddrshow-output create mode 100644 tests/data/netinfo/sample-iproute-output-v4 create mode 100644 tests/data/netinfo/sample-iproute-output-v6 create mode 100644 tests/data/netinfo/sample-route-output-v4 create mode 100644 tests/data/netinfo/sample-route-output-v6 (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_disable_ec2_metadata.py b/cloudinit/config/cc_disable_ec2_metadata.py index c56319b5..885b3138 100644 --- a/cloudinit/config/cc_disable_ec2_metadata.py +++ b/cloudinit/config/cc_disable_ec2_metadata.py @@ -32,13 +32,23 @@ from cloudinit.settings import PER_ALWAYS frequency = PER_ALWAYS -REJECT_CMD = ['route', 'add', '-host', '169.254.169.254', 'reject'] +REJECT_CMD_IF = ['route', 'add', '-host', '169.254.169.254', 'reject'] +REJECT_CMD_IP = ['ip', 'route', 'add', 'prohibit', '169.254.169.254'] def handle(name, cfg, _cloud, log, _args): disabled = util.get_cfg_option_bool(cfg, "disable_ec2_metadata", False) if disabled: - util.subp(REJECT_CMD, capture=False) + reject_cmd = None + if util.which('ip'): + reject_cmd = REJECT_CMD_IP + elif util.which('ifconfig'): + reject_cmd = REJECT_CMD_IF + else: + log.error(('Neither "route" nor "ip" command found, unable to ' + 'manipulate routing table')) + return + util.subp(reject_cmd, capture=False) else: log.debug(("Skipping module named %s," " disabling the ec2 route not enabled"), name) diff --git a/cloudinit/config/tests/test_disable_ec2_metadata.py b/cloudinit/config/tests/test_disable_ec2_metadata.py new file mode 100644 index 00000000..67646b03 --- /dev/null +++ b/cloudinit/config/tests/test_disable_ec2_metadata.py @@ -0,0 +1,50 @@ +# This file is part of cloud-init. See LICENSE file for license information. + +"""Tests cc_disable_ec2_metadata handler""" + +import cloudinit.config.cc_disable_ec2_metadata as ec2_meta + +from cloudinit.tests.helpers import CiTestCase, mock + +import logging + +LOG = logging.getLogger(__name__) + +DISABLE_CFG = {'disable_ec2_metadata': 'true'} + + +class TestEC2MetadataRoute(CiTestCase): + + with_logs = True + + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which') + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp') + def test_disable_ifconfig(self, m_subp, m_which): + """Set the route if ifconfig command is available""" + m_which.side_effect = lambda x: x if x == 'ifconfig' else None + ec2_meta.handle('foo', DISABLE_CFG, None, LOG, None) + m_subp.assert_called_with( + ['route', 'add', '-host', '169.254.169.254', 'reject'], + capture=False) + + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which') + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp') + def test_disable_ip(self, m_subp, m_which): + """Set the route if ip command is available""" + m_which.side_effect = lambda x: x if x == 'ip' else None + ec2_meta.handle('foo', DISABLE_CFG, None, LOG, None) + m_subp.assert_called_with( + ['ip', 'route', 'add', 'prohibit', '169.254.169.254'], + capture=False) + + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which') + @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp') + def test_disable_no_tool(self, m_subp, m_which): + """Log error when neither route nor ip commands are available""" + m_which.return_value = None # Find neither ifconfig nor ip + ec2_meta.handle('foo', DISABLE_CFG, None, LOG, None) + self.assertEqual( + [mock.call('ip'), mock.call('ifconfig')], m_which.call_args_list) + m_subp.assert_not_called() + +# vi: ts=4 expandtab diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py index 6d63e5c5..72c803eb 100644 --- a/cloudinit/net/network_state.py +++ b/cloudinit/net/network_state.py @@ -7,6 +7,8 @@ import copy import functools import logging +import socket +import struct import six @@ -886,12 +888,9 @@ def net_prefix_to_ipv4_mask(prefix): This is the inverse of ipv4_mask_to_net_prefix. 24 -> "255.255.255.0" Also supports input as a string.""" - - mask = [0, 0, 0, 0] - for i in list(range(0, int(prefix))): - idx = int(i / 8) - mask[idx] = mask[idx] + (1 << (7 - i % 8)) - return ".".join([str(x) for x in mask]) + mask = socket.inet_ntoa( + struct.pack(">I", (0xffffffff << (32 - int(prefix)) & 0xffffffff))) + return mask def ipv4_mask_to_net_prefix(mask): diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py index 993b26cf..f0906160 100644 --- a/cloudinit/netinfo.py +++ b/cloudinit/netinfo.py @@ -8,9 +8,11 @@ # # This file is part of cloud-init. See LICENSE file for license information. +from copy import copy, deepcopy import re from cloudinit import log as logging +from cloudinit.net.network_state import net_prefix_to_ipv4_mask from cloudinit import util from cloudinit.simpletable import SimpleTable @@ -18,18 +20,90 @@ from cloudinit.simpletable import SimpleTable LOG = logging.getLogger() -def netdev_info(empty=""): - fields = ("hwaddr", "addr", "bcast", "mask") - (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1]) +DEFAULT_NETDEV_INFO = { + "ipv4": [], + "ipv6": [], + "hwaddr": "", + "up": False +} + + +def _netdev_info_iproute(ipaddr_out): + """ + Get network device dicts from ip route and ip link info. + + @param ipaddr_out: Output string from 'ip addr show' command. + + @returns: A dict of device info keyed by network device name containing + device configuration values. + @raise: TypeError if ipaddr_out isn't a string. + """ devs = {} - for line in str(ifcfg_out).splitlines(): + dev_name = None + for num, line in enumerate(ipaddr_out.splitlines()): + m = re.match(r'^\d+:\s(?P[^:]+):\s+<(?P\S+)>\s+.*', line) + if m: + dev_name = m.group('dev').lower().split('@')[0] + flags = m.group('flags').split(',') + devs[dev_name] = { + 'ipv4': [], 'ipv6': [], 'hwaddr': '', + 'up': bool('UP' in flags and 'LOWER_UP' in flags), + } + elif 'inet6' in line: + m = re.match( + r'\s+inet6\s(?P\S+)\sscope\s(?P\S+).*', line) + if not m: + LOG.warning( + 'Could not parse ip addr show: (line:%d) %s', num, line) + continue + devs[dev_name]['ipv6'].append(m.groupdict()) + elif 'inet' in line: + m = re.match( + r'\s+inet\s(?P\S+)(\sbrd\s(?P\S+))?\sscope\s' + r'(?P\S+).*', line) + if not m: + LOG.warning( + 'Could not parse ip addr show: (line:%d) %s', num, line) + continue + match = m.groupdict() + cidr4 = match.pop('cidr4') + addr, _, prefix = cidr4.partition('/') + if not prefix: + prefix = '32' + devs[dev_name]['ipv4'].append({ + 'ip': addr, + 'bcast': match['bcast'] if match['bcast'] else '', + 'mask': net_prefix_to_ipv4_mask(prefix), + 'scope': match['scope']}) + elif 'link' in line: + m = re.match( + r'\s+link/(?P\S+)\s(?P\S+).*', line) + if not m: + LOG.warning( + 'Could not parse ip addr show: (line:%d) %s', num, line) + continue + if m.group('link_type') == 'ether': + devs[dev_name]['hwaddr'] = m.group('hwaddr') + else: + devs[dev_name]['hwaddr'] = '' + else: + continue + return devs + + +def _netdev_info_ifconfig(ifconfig_data): + # fields that need to be returned in devs for each dev + devs = {} + for line in ifconfig_data.splitlines(): if len(line) == 0: continue if line[0] not in ("\t", " "): curdev = line.split()[0] - devs[curdev] = {"up": False} - for field in fields: - devs[curdev][field] = "" + # current ifconfig pops a ':' on the end of the device + if curdev.endswith(':'): + curdev = curdev[:-1] + if curdev not in devs: + devs[curdev] = deepcopy(DEFAULT_NETDEV_INFO) toks = line.lower().strip().split() if toks[0] == "up": devs[curdev]['up'] = True @@ -39,41 +113,50 @@ def netdev_info(empty=""): if re.search(r"flags=\d+", toks[i + 1]) + if res: + devs[curdev]['ipv6'][-1]['scope6'] = res.group(1) + return devs + + +def netdev_info(empty=""): + devs = {} + if util.which('ip'): + # Try iproute first of all + (ipaddr_out, _err) = util.subp(["ip", "addr", "show"]) + devs = _netdev_info_iproute(ipaddr_out) + elif util.which('ifconfig'): + # Fall back to net-tools if iproute2 is not present + (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1]) + devs = _netdev_info_ifconfig(ifcfg_out) + else: + LOG.warning( + "Could not print networks: missing 'ip' and 'ifconfig' commands") if empty != "": for (_devname, dev) in devs.items(): @@ -84,14 +167,94 @@ def netdev_info(empty=""): return devs -def route_info(): - (route_out, _err) = util.subp(["netstat", "-rn"], rcs=[0, 1]) +def _netdev_route_info_iproute(iproute_data): + """ + Get network route dicts from ip route info. + + @param iproute_data: Output string from ip route command. + + @returns: A dict containing ipv4 and ipv6 route entries as lists. Each + item in the list is a route dictionary representing destination, + gateway, flags, genmask and interface information. + """ + + routes = {} + routes['ipv4'] = [] + routes['ipv6'] = [] + entries = iproute_data.splitlines() + default_route_entry = { + 'destination': '', 'flags': '', 'gateway': '', 'genmask': '', + 'iface': '', 'metric': ''} + for line in entries: + entry = copy(default_route_entry) + if not line: + continue + toks = line.split() + flags = ['U'] + if toks[0] == "default": + entry['destination'] = "0.0.0.0" + entry['genmask'] = "0.0.0.0" + else: + if '/' in toks[0]: + (addr, cidr) = toks[0].split("/") + else: + addr = toks[0] + cidr = '32' + flags.append("H") + entry['genmask'] = net_prefix_to_ipv4_mask(cidr) + entry['destination'] = addr + entry['genmask'] = net_prefix_to_ipv4_mask(cidr) + entry['gateway'] = "0.0.0.0" + for i in range(len(toks)): + if toks[i] == "via": + entry['gateway'] = toks[i + 1] + flags.insert(1, "G") + if toks[i] == "dev": + entry["iface"] = toks[i + 1] + if toks[i] == "metric": + entry['metric'] = toks[i + 1] + entry['flags'] = ''.join(flags) + routes['ipv4'].append(entry) + try: + (iproute_data6, _err6) = util.subp( + ["ip", "--oneline", "-6", "route", "list", "table", "all"], + rcs=[0, 1]) + except util.ProcessExecutionError: + pass + else: + entries6 = iproute_data6.splitlines() + for line in entries6: + entry = {} + if not line: + continue + toks = line.split() + if toks[0] == "default": + entry['destination'] = "::/0" + entry['flags'] = "UG" + else: + entry['destination'] = toks[0] + entry['gateway'] = "::" + entry['flags'] = "U" + for i in range(len(toks)): + if toks[i] == "via": + entry['gateway'] = toks[i + 1] + entry['flags'] = "UG" + if toks[i] == "dev": + entry["iface"] = toks[i + 1] + if toks[i] == "metric": + entry['metric'] = toks[i + 1] + if toks[i] == "expires": + entry['flags'] = entry['flags'] + 'e' + routes['ipv6'].append(entry) + return routes + +def _netdev_route_info_netstat(route_data): routes = {} routes['ipv4'] = [] routes['ipv6'] = [] - entries = route_out.splitlines()[1:] + entries = route_data.splitlines() for line in entries: if not line: continue @@ -101,8 +264,8 @@ def route_info(): # default 10.65.0.1 UGS 0 34920 vtnet0 # # Linux netstat shows 2 more: - # Destination Gateway Genmask Flags MSS Window irtt Iface - # 0.0.0.0 10.65.0.1 0.0.0.0 UG 0 0 0 eth0 + # Destination Gateway Genmask Flags Metric Ref Use Iface + # 0.0.0.0 10.65.0.1 0.0.0.0 UG 0 0 0 eth0 if (len(toks) < 6 or toks[0] == "Kernel" or toks[0] == "Destination" or toks[0] == "Internet" or toks[0] == "Internet6" or toks[0] == "Routing"): @@ -125,31 +288,57 @@ def route_info(): routes['ipv4'].append(entry) try: - (route_out6, _err6) = util.subp(["netstat", "-A", "inet6", "-n"], - rcs=[0, 1]) + (route_data6, _err6) = util.subp( + ["netstat", "-A", "inet6", "--route", "--numeric"], rcs=[0, 1]) except util.ProcessExecutionError: pass else: - entries6 = route_out6.splitlines()[1:] + entries6 = route_data6.splitlines() for line in entries6: if not line: continue toks = line.split() - if (len(toks) < 6 or toks[0] == "Kernel" or + if (len(toks) < 7 or toks[0] == "Kernel" or + toks[0] == "Destination" or toks[0] == "Internet" or toks[0] == "Proto" or toks[0] == "Active"): continue entry = { - 'proto': toks[0], - 'recv-q': toks[1], - 'send-q': toks[2], - 'local address': toks[3], - 'foreign address': toks[4], - 'state': toks[5], + 'destination': toks[0], + 'gateway': toks[1], + 'flags': toks[2], + 'metric': toks[3], + 'ref': toks[4], + 'use': toks[5], + 'iface': toks[6], } + # skip lo interface on ipv6 + if entry['iface'] == "lo": + continue + # strip /128 from address if it's included + if entry['destination'].endswith('/128'): + entry['destination'] = re.sub( + r'\/128$', '', entry['destination']) routes['ipv6'].append(entry) return routes +def route_info(): + routes = {} + if util.which('ip'): + # Try iproute first of all + (iproute_out, _err) = util.subp(["ip", "-o", "route", "list"]) + routes = _netdev_route_info_iproute(iproute_out) + elif util.which('netstat'): + # Fall back to net-tools if iproute2 is not present + (route_out, _err) = util.subp( + ["netstat", "--route", "--numeric", "--extend"], rcs=[0, 1]) + routes = _netdev_route_info_netstat(route_out) + else: + LOG.warning( + "Could not print routes: missing 'ip' and 'netstat' commands") + return routes + + def getgateway(): try: routes = route_info() @@ -166,21 +355,30 @@ def netdev_pformat(): lines = [] try: netdev = netdev_info(empty=".") - except Exception: - lines.append(util.center("Net device info failed", '!', 80)) + except Exception as e: + lines.append( + util.center( + "Net device info failed ({error})".format(error=str(e)), + '!', 80)) else: + if not netdev: + return '\n' fields = ['Device', 'Up', 'Address', 'Mask', 'Scope', 'Hw-Address'] tbl = SimpleTable(fields) - for (dev, d) in sorted(netdev.items()): - tbl.add_row([dev, d["up"], d["addr"], d["mask"], ".", d["hwaddr"]]) - if d.get('addr6'): - tbl.add_row([dev, d["up"], - d["addr6"], ".", d.get("scope6"), d["hwaddr"]]) + for (dev, data) in sorted(netdev.items()): + for addr in data.get('ipv4'): + tbl.add_row( + [dev, data["up"], addr["ip"], addr["mask"], + addr.get('scope', '.'), data["hwaddr"]]) + for addr in data.get('ipv6'): + tbl.add_row( + [dev, data["up"], addr["ip"], ".", addr["scope6"], + data["hwaddr"]]) netdev_s = tbl.get_string() max_len = len(max(netdev_s.splitlines(), key=len)) header = util.center("Net device info", "+", max_len) lines.extend([header, netdev_s]) - return "\n".join(lines) + return "\n".join(lines) + "\n" def route_pformat(): @@ -188,7 +386,10 @@ def route_pformat(): try: routes = route_info() except Exception as e: - lines.append(util.center('Route info failed', '!', 80)) + lines.append( + util.center( + 'Route info failed ({error})'.format(error=str(e)), + '!', 80)) util.logexc(LOG, "Route info failed: %s" % e) else: if routes.get('ipv4'): @@ -205,20 +406,20 @@ def route_pformat(): header = util.center("Route IPv4 info", "+", max_len) lines.extend([header, route_s]) if routes.get('ipv6'): - fields_v6 = ['Route', 'Proto', 'Recv-Q', 'Send-Q', - 'Local Address', 'Foreign Address', 'State'] + fields_v6 = ['Route', 'Destination', 'Gateway', 'Interface', + 'Flags'] tbl_v6 = SimpleTable(fields_v6) for (n, r) in enumerate(routes.get('ipv6')): route_id = str(n) - tbl_v6.add_row([route_id, r['proto'], - r['recv-q'], r['send-q'], - r['local address'], r['foreign address'], - r['state']]) + if r['iface'] == 'lo': + continue + tbl_v6.add_row([route_id, r['destination'], + r['gateway'], r['iface'], r['flags']]) route_s = tbl_v6.get_string() max_len = len(max(route_s.splitlines(), key=len)) header = util.center("Route IPv6 info", "+", max_len) lines.extend([header, route_s]) - return "\n".join(lines) + return "\n".join(lines) + "\n" def debug_info(prefix='ci-info: '): diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py index 999b1d7c..82fd347b 100644 --- a/cloudinit/tests/helpers.py +++ b/cloudinit/tests/helpers.py @@ -190,35 +190,11 @@ class ResourceUsingTestCase(CiTestCase): super(ResourceUsingTestCase, self).setUp() self.resource_path = None - def resourceLocation(self, subname=None): - if self.resource_path is None: - paths = [ - os.path.join('tests', 'data'), - os.path.join('data'), - os.path.join(os.pardir, 'tests', 'data'), - os.path.join(os.pardir, 'data'), - ] - for p in paths: - if os.path.isdir(p): - self.resource_path = p - break - self.assertTrue((self.resource_path and - os.path.isdir(self.resource_path)), - msg="Unable to locate test resource data path!") - if not subname: - return self.resource_path - return os.path.join(self.resource_path, subname) - - def readResource(self, name): - where = self.resourceLocation(name) - with open(where, 'r') as fh: - return fh.read() - def getCloudPaths(self, ds=None): tmpdir = tempfile.mkdtemp() self.addCleanup(shutil.rmtree, tmpdir) cp = ch.Paths({'cloud_dir': tmpdir, - 'templates_dir': self.resourceLocation()}, + 'templates_dir': resourceLocation()}, ds=ds) return cp @@ -234,7 +210,7 @@ class FilesystemMockingTestCase(ResourceUsingTestCase): ResourceUsingTestCase.tearDown(self) def replicateTestRoot(self, example_root, target_root): - real_root = self.resourceLocation() + real_root = resourceLocation() real_root = os.path.join(real_root, 'roots', example_root) for (dir_path, _dirnames, filenames) in os.walk(real_root): real_path = dir_path @@ -399,6 +375,18 @@ def wrap_and_call(prefix, mocks, func, *args, **kwargs): p.stop() +def resourceLocation(subname=None): + path = os.path.join('tests', 'data') + if not subname: + return path + return os.path.join(path, subname) + + +def readResource(name, mode='r'): + with open(resourceLocation(name), mode) as fh: + return fh.read() + + try: skipIf = unittest.skipIf except AttributeError: diff --git a/cloudinit/tests/test_netinfo.py b/cloudinit/tests/test_netinfo.py index 7dea2e41..2537c1c2 100644 --- a/cloudinit/tests/test_netinfo.py +++ b/cloudinit/tests/test_netinfo.py @@ -2,105 +2,121 @@ """Tests netinfo module functions and classes.""" +from copy import copy + from cloudinit.netinfo import netdev_pformat, route_pformat -from cloudinit.tests.helpers import CiTestCase, mock +from cloudinit.tests.helpers import CiTestCase, mock, readResource # Example ifconfig and route output -SAMPLE_IFCONFIG_OUT = """\ -enp0s25 Link encap:Ethernet HWaddr 50:7b:9d:2c:af:91 - inet addr:192.168.2.18 Bcast:192.168.2.255 Mask:255.255.255.0 - inet6 addr: fe80::8107:2b92:867e:f8a6/64 Scope:Link - UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1 - RX packets:8106427 errors:55 dropped:0 overruns:0 frame:37 - TX packets:9339739 errors:0 dropped:0 overruns:0 carrier:0 - collisions:0 txqueuelen:1000 - RX bytes:4953721719 (4.9 GB) TX bytes:7731890194 (7.7 GB) - Interrupt:20 Memory:e1200000-e1220000 - -lo Link encap:Local Loopback - inet addr:127.0.0.1 Mask:255.0.0.0 - inet6 addr: ::1/128 Scope:Host - UP LOOPBACK RUNNING MTU:65536 Metric:1 - RX packets:579230851 errors:0 dropped:0 overruns:0 frame:0 - TX packets:579230851 errors:0 dropped:0 overruns:0 carrier:0 - collisions:0 txqueuelen:1 -""" - -SAMPLE_ROUTE_OUT = '\n'.join([ - '0.0.0.0 192.168.2.1 0.0.0.0 UG 0 0 0' - ' enp0s25', - '0.0.0.0 192.168.2.1 0.0.0.0 UG 0 0 0' - ' wlp3s0', - '192.168.2.0 0.0.0.0 255.255.255.0 U 0 0 0' - ' enp0s25']) - - -NETDEV_FORMATTED_OUT = '\n'.join([ - '+++++++++++++++++++++++++++++++++++++++Net device info+++++++++++++++++++' - '++++++++++++++++++++', - '+---------+------+------------------------------+---------------+-------+' - '-------------------+', - '| Device | Up | Address | Mask | Scope |' - ' Hw-Address |', - '+---------+------+------------------------------+---------------+-------+' - '-------------------+', - '| enp0s25 | True | 192.168.2.18 | 255.255.255.0 | . |' - ' 50:7b:9d:2c:af:91 |', - '| enp0s25 | True | fe80::8107:2b92:867e:f8a6/64 | . | link |' - ' 50:7b:9d:2c:af:91 |', - '| lo | True | 127.0.0.1 | 255.0.0.0 | . |' - ' . |', - '| lo | True | ::1/128 | . | host |' - ' . |', - '+---------+------+------------------------------+---------------+-------+' - '-------------------+']) - -ROUTE_FORMATTED_OUT = '\n'.join([ - '+++++++++++++++++++++++++++++Route IPv4 info++++++++++++++++++++++++++' - '+++', - '+-------+-------------+-------------+---------------+-----------+-----' - '--+', - '| Route | Destination | Gateway | Genmask | Interface | Flags' - ' |', - '+-------+-------------+-------------+---------------+-----------+' - '-------+', - '| 0 | 0.0.0.0 | 192.168.2.1 | 0.0.0.0 | wlp3s0 |' - ' UG |', - '| 1 | 192.168.2.0 | 0.0.0.0 | 255.255.255.0 | enp0s25 |' - ' U |', - '+-------+-------------+-------------+---------------+-----------+' - '-------+', - '++++++++++++++++++++++++++++++++++++++++Route IPv6 info++++++++++' - '++++++++++++++++++++++++++++++', - '+-------+-------------+-------------+---------------+---------------+' - '-----------------+-------+', - '| Route | Proto | Recv-Q | Send-Q | Local Address |' - ' Foreign Address | State |', - '+-------+-------------+-------------+---------------+---------------+' - '-----------------+-------+', - '| 0 | 0.0.0.0 | 192.168.2.1 | 0.0.0.0 | UG |' - ' 0 | 0 |', - '| 1 | 192.168.2.0 | 0.0.0.0 | 255.255.255.0 | U |' - ' 0 | 0 |', - '+-------+-------------+-------------+---------------+---------------+' - '-----------------+-------+']) +SAMPLE_OLD_IFCONFIG_OUT = readResource("netinfo/old-ifconfig-output") +SAMPLE_NEW_IFCONFIG_OUT = readResource("netinfo/new-ifconfig-output") +SAMPLE_IPADDRSHOW_OUT = readResource("netinfo/sample-ipaddrshow-output") +SAMPLE_ROUTE_OUT_V4 = readResource("netinfo/sample-route-output-v4") +SAMPLE_ROUTE_OUT_V6 = readResource("netinfo/sample-route-output-v6") +SAMPLE_IPROUTE_OUT_V4 = readResource("netinfo/sample-iproute-output-v4") +SAMPLE_IPROUTE_OUT_V6 = readResource("netinfo/sample-iproute-output-v6") +NETDEV_FORMATTED_OUT = readResource("netinfo/netdev-formatted-output") +ROUTE_FORMATTED_OUT = readResource("netinfo/route-formatted-output") class TestNetInfo(CiTestCase): maxDiff = None + with_logs = True + + @mock.patch('cloudinit.netinfo.util.which') + @mock.patch('cloudinit.netinfo.util.subp') + def test_netdev_old_nettools_pformat(self, m_subp, m_which): + """netdev_pformat properly rendering old nettools info.""" + m_subp.return_value = (SAMPLE_OLD_IFCONFIG_OUT, '') + m_which.side_effect = lambda x: x if x == 'ifconfig' else None + content = netdev_pformat() + self.assertEqual(NETDEV_FORMATTED_OUT, content) + @mock.patch('cloudinit.netinfo.util.which') @mock.patch('cloudinit.netinfo.util.subp') - def test_netdev_pformat(self, m_subp): - """netdev_pformat properly rendering network device information.""" - m_subp.return_value = (SAMPLE_IFCONFIG_OUT, '') + def test_netdev_new_nettools_pformat(self, m_subp, m_which): + """netdev_pformat properly rendering netdev new nettools info.""" + m_subp.return_value = (SAMPLE_NEW_IFCONFIG_OUT, '') + m_which.side_effect = lambda x: x if x == 'ifconfig' else None content = netdev_pformat() self.assertEqual(NETDEV_FORMATTED_OUT, content) + @mock.patch('cloudinit.netinfo.util.which') + @mock.patch('cloudinit.netinfo.util.subp') + def test_netdev_iproute_pformat(self, m_subp, m_which): + """netdev_pformat properly rendering ip route info.""" + m_subp.return_value = (SAMPLE_IPADDRSHOW_OUT, '') + m_which.side_effect = lambda x: x if x == 'ip' else None + content = netdev_pformat() + new_output = copy(NETDEV_FORMATTED_OUT) + # ip route show describes global scopes on ipv4 addresses + # whereas ifconfig does not. Add proper global/host scope to output. + new_output = new_output.replace('| . | 50:7b', '| global | 50:7b') + new_output = new_output.replace( + '255.0.0.0 | . |', '255.0.0.0 | host |') + self.assertEqual(new_output, content) + + @mock.patch('cloudinit.netinfo.util.which') + @mock.patch('cloudinit.netinfo.util.subp') + def test_netdev_warn_on_missing_commands(self, m_subp, m_which): + """netdev_pformat warns when missing both ip and 'netstat'.""" + m_which.return_value = None # Niether ip nor netstat found + content = netdev_pformat() + self.assertEqual('\n', content) + self.assertEqual( + "WARNING: Could not print networks: missing 'ip' and 'ifconfig'" + " commands\n", + self.logs.getvalue()) + m_subp.assert_not_called() + + @mock.patch('cloudinit.netinfo.util.which') @mock.patch('cloudinit.netinfo.util.subp') - def test_route_pformat(self, m_subp): - """netdev_pformat properly rendering network device information.""" - m_subp.return_value = (SAMPLE_ROUTE_OUT, '') + def test_route_nettools_pformat(self, m_subp, m_which): + """route_pformat properly rendering nettools route info.""" + + def subp_netstat_route_selector(*args, **kwargs): + if args[0] == ['netstat', '--route', '--numeric', '--extend']: + return (SAMPLE_ROUTE_OUT_V4, '') + if args[0] == ['netstat', '-A', 'inet6', '--route', '--numeric']: + return (SAMPLE_ROUTE_OUT_V6, '') + raise Exception('Unexpected subp call %s' % args[0]) + + m_subp.side_effect = subp_netstat_route_selector + m_which.side_effect = lambda x: x if x == 'netstat' else None content = route_pformat() self.assertEqual(ROUTE_FORMATTED_OUT, content) + + @mock.patch('cloudinit.netinfo.util.which') + @mock.patch('cloudinit.netinfo.util.subp') + def test_route_iproute_pformat(self, m_subp, m_which): + """route_pformat properly rendering ip route info.""" + + def subp_iproute_selector(*args, **kwargs): + if ['ip', '-o', 'route', 'list'] == args[0]: + return (SAMPLE_IPROUTE_OUT_V4, '') + v6cmd = ['ip', '--oneline', '-6', 'route', 'list', 'table', 'all'] + if v6cmd == args[0]: + return (SAMPLE_IPROUTE_OUT_V6, '') + raise Exception('Unexpected subp call %s' % args[0]) + + m_subp.side_effect = subp_iproute_selector + m_which.side_effect = lambda x: x if x == 'ip' else None + content = route_pformat() + self.assertEqual(ROUTE_FORMATTED_OUT, content) + + @mock.patch('cloudinit.netinfo.util.which') + @mock.patch('cloudinit.netinfo.util.subp') + def test_route_warn_on_missing_commands(self, m_subp, m_which): + """route_pformat warns when missing both ip and 'netstat'.""" + m_which.return_value = None # Niether ip nor netstat found + content = route_pformat() + self.assertEqual('\n', content) + self.assertEqual( + "WARNING: Could not print routes: missing 'ip' and 'netstat'" + " commands\n", + self.logs.getvalue()) + m_subp.assert_not_called() + +# vi: ts=4 expandtab diff --git a/tests/data/netinfo/netdev-formatted-output b/tests/data/netinfo/netdev-formatted-output new file mode 100644 index 00000000..283ab4a4 --- /dev/null +++ b/tests/data/netinfo/netdev-formatted-output @@ -0,0 +1,10 @@ ++++++++++++++++++++++++++++++++++++++++Net device info++++++++++++++++++++++++++++++++++++++++ ++---------+------+------------------------------+---------------+--------+-------------------+ +| Device | Up | Address | Mask | Scope | Hw-Address | ++---------+------+------------------------------+---------------+--------+-------------------+ +| enp0s25 | True | 192.168.2.18 | 255.255.255.0 | . | 50:7b:9d:2c:af:91 | +| enp0s25 | True | fe80::7777:2222:1111:eeee/64 | . | global | 50:7b:9d:2c:af:91 | +| enp0s25 | True | fe80::8107:2b92:867e:f8a6/64 | . | link | 50:7b:9d:2c:af:91 | +| lo | True | 127.0.0.1 | 255.0.0.0 | . | . | +| lo | True | ::1/128 | . | host | . | ++---------+------+------------------------------+---------------+--------+-------------------+ diff --git a/tests/data/netinfo/new-ifconfig-output b/tests/data/netinfo/new-ifconfig-output new file mode 100644 index 00000000..83d4ad16 --- /dev/null +++ b/tests/data/netinfo/new-ifconfig-output @@ -0,0 +1,18 @@ +enp0s25: flags=4163 mtu 1500 + inet 192.168.2.18 netmask 255.255.255.0 broadcast 192.168.2.255 + inet6 fe80::7777:2222:1111:eeee prefixlen 64 scopeid 0x30 + inet6 fe80::8107:2b92:867e:f8a6 prefixlen 64 scopeid 0x20 + ether 50:7b:9d:2c:af:91 txqueuelen 1000 (Ethernet) + RX packets 3017 bytes 10601563 (10.1 MiB) + RX errors 0 dropped 39 overruns 0 frame 0 + TX packets 2627 bytes 196976 (192.3 KiB) + TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 + +lo: flags=73 mtu 65536 + inet 127.0.0.1 netmask 255.0.0.0 + inet6 ::1 prefixlen 128 scopeid 0x10 + loop txqueuelen 1 (Local Loopback) + RX packets 0 bytes 0 (0.0 B) + RX errors 0 dropped 0 overruns 0 frame 0 + TX packets 0 bytes 0 (0.0 B) + TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 diff --git a/tests/data/netinfo/old-ifconfig-output b/tests/data/netinfo/old-ifconfig-output new file mode 100644 index 00000000..e01f763e --- /dev/null +++ b/tests/data/netinfo/old-ifconfig-output @@ -0,0 +1,18 @@ +enp0s25 Link encap:Ethernet HWaddr 50:7b:9d:2c:af:91 + inet addr:192.168.2.18 Bcast:192.168.2.255 Mask:255.255.255.0 + inet6 addr: fe80::7777:2222:1111:eeee/64 Scope:Global + inet6 addr: fe80::8107:2b92:867e:f8a6/64 Scope:Link + UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1 + RX packets:8106427 errors:55 dropped:0 overruns:0 frame:37 + TX packets:9339739 errors:0 dropped:0 overruns:0 carrier:0 + collisions:0 txqueuelen:1000 + RX bytes:4953721719 (4.9 GB) TX bytes:7731890194 (7.7 GB) + Interrupt:20 Memory:e1200000-e1220000 + +lo Link encap:Local Loopback + inet addr:127.0.0.1 Mask:255.0.0.0 + inet6 addr: ::1/128 Scope:Host + UP LOOPBACK RUNNING MTU:65536 Metric:1 + RX packets:579230851 errors:0 dropped:0 overruns:0 frame:0 + TX packets:579230851 errors:0 dropped:0 overruns:0 carrier:0 + collisions:0 txqueuelen:1 diff --git a/tests/data/netinfo/route-formatted-output b/tests/data/netinfo/route-formatted-output new file mode 100644 index 00000000..9d2c5dd3 --- /dev/null +++ b/tests/data/netinfo/route-formatted-output @@ -0,0 +1,22 @@ ++++++++++++++++++++++++++++++Route IPv4 info+++++++++++++++++++++++++++++ ++-------+-------------+-------------+---------------+-----------+-------+ +| Route | Destination | Gateway | Genmask | Interface | Flags | ++-------+-------------+-------------+---------------+-----------+-------+ +| 0 | 0.0.0.0 | 192.168.2.1 | 0.0.0.0 | enp0s25 | UG | +| 1 | 0.0.0.0 | 192.168.2.1 | 0.0.0.0 | wlp3s0 | UG | +| 2 | 192.168.2.0 | 0.0.0.0 | 255.255.255.0 | enp0s25 | U | ++-------+-------------+-------------+---------------+-----------+-------+ ++++++++++++++++++++++++++++++++++++Route IPv6 info+++++++++++++++++++++++++++++++++++ ++-------+---------------------------+---------------------------+-----------+-------+ +| Route | Destination | Gateway | Interface | Flags | ++-------+---------------------------+---------------------------+-----------+-------+ +| 0 | 2a00:abcd:82ae:cd33::657 | :: | enp0s25 | Ue | +| 1 | 2a00:abcd:82ae:cd33::/64 | :: | enp0s25 | U | +| 2 | 2a00:abcd:82ae:cd33::/56 | fe80::32ee:54de:cd43:b4e1 | enp0s25 | UG | +| 3 | fd81:123f:654::657 | :: | enp0s25 | U | +| 4 | fd81:123f:654::/64 | :: | enp0s25 | U | +| 5 | fd81:123f:654::/48 | fe80::32ee:54de:cd43:b4e1 | enp0s25 | UG | +| 6 | fe80::abcd:ef12:bc34:da21 | :: | enp0s25 | U | +| 7 | fe80::/64 | :: | enp0s25 | U | +| 8 | ::/0 | fe80::32ee:54de:cd43:b4e1 | enp0s25 | UG | ++-------+---------------------------+---------------------------+-----------+-------+ diff --git a/tests/data/netinfo/sample-ipaddrshow-output b/tests/data/netinfo/sample-ipaddrshow-output new file mode 100644 index 00000000..b2fa2672 --- /dev/null +++ b/tests/data/netinfo/sample-ipaddrshow-output @@ -0,0 +1,13 @@ +1: lo: mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000 + link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 + inet 127.0.0.1/8 scope host lo\ valid_lft forever preferred_lft forever + inet6 ::1/128 scope host \ valid_lft forever preferred_lft forever +2: enp0s25: mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 + link/ether 50:7b:9d:2c:af:91 brd ff:ff:ff:ff:ff:ff + inet 192.168.2.18/24 brd 192.168.2.255 scope global dynamic enp0s25 + valid_lft 84174sec preferred_lft 84174sec + inet6 fe80::7777:2222:1111:eeee/64 scope global + valid_lft forever preferred_lft forever + inet6 fe80::8107:2b92:867e:f8a6/64 scope link + valid_lft forever preferred_lft forever + diff --git a/tests/data/netinfo/sample-iproute-output-v4 b/tests/data/netinfo/sample-iproute-output-v4 new file mode 100644 index 00000000..904cb034 --- /dev/null +++ b/tests/data/netinfo/sample-iproute-output-v4 @@ -0,0 +1,3 @@ +default via 192.168.2.1 dev enp0s25 proto static metric 100 +default via 192.168.2.1 dev wlp3s0 proto static metric 150 +192.168.2.0/24 dev enp0s25 proto kernel scope link src 192.168.2.18 metric 100 diff --git a/tests/data/netinfo/sample-iproute-output-v6 b/tests/data/netinfo/sample-iproute-output-v6 new file mode 100644 index 00000000..12bb1c12 --- /dev/null +++ b/tests/data/netinfo/sample-iproute-output-v6 @@ -0,0 +1,11 @@ +2a00:abcd:82ae:cd33::657 dev enp0s25 proto kernel metric 256 expires 2334sec pref medium +2a00:abcd:82ae:cd33::/64 dev enp0s25 proto ra metric 100 pref medium +2a00:abcd:82ae:cd33::/56 via fe80::32ee:54de:cd43:b4e1 dev enp0s25 proto ra metric 100 pref medium +fd81:123f:654::657 dev enp0s25 proto kernel metric 256 pref medium +fd81:123f:654::/64 dev enp0s25 proto ra metric 100 pref medium +fd81:123f:654::/48 via fe80::32ee:54de:cd43:b4e1 dev enp0s25 proto ra metric 100 pref medium +fe80::abcd:ef12:bc34:da21 dev enp0s25 proto static metric 100 pref medium +fe80::/64 dev enp0s25 proto kernel metric 256 pref medium +default via fe80::32ee:54de:cd43:b4e1 dev enp0s25 proto static metric 100 pref medium +local ::1 dev lo table local proto none metric 0 pref medium +local 2600:1f16:b80:ad00:90a:c915:bca6:5ff2 dev lo table local proto none metric 0 pref medium diff --git a/tests/data/netinfo/sample-route-output-v4 b/tests/data/netinfo/sample-route-output-v4 new file mode 100644 index 00000000..ecc31d96 --- /dev/null +++ b/tests/data/netinfo/sample-route-output-v4 @@ -0,0 +1,5 @@ +Kernel IP routing table +Destination Gateway Genmask Flags Metric Ref Use Iface +0.0.0.0 192.168.2.1 0.0.0.0 UG 100 0 0 enp0s25 +0.0.0.0 192.168.2.1 0.0.0.0 UG 150 0 0 wlp3s0 +192.168.2.0 0.0.0.0 255.255.255.0 U 100 0 0 enp0s25 diff --git a/tests/data/netinfo/sample-route-output-v6 b/tests/data/netinfo/sample-route-output-v6 new file mode 100644 index 00000000..4712b73c --- /dev/null +++ b/tests/data/netinfo/sample-route-output-v6 @@ -0,0 +1,13 @@ +Kernel IPv6 routing table +Destination Next Hop Flag Met Re Use If +2a00:abcd:82ae:cd33::657/128 :: Ue 256 1 0 enp0s25 +2a00:abcd:82ae:cd33::/64 :: U 100 1 0 enp0s25 +2a00:abcd:82ae:cd33::/56 fe80::32ee:54de:cd43:b4e1 UG 100 1 0 enp0s25 +fd81:123f:654::657/128 :: U 256 1 0 enp0s25 +fd81:123f:654::/64 :: U 100 1 0 enp0s25 +fd81:123f:654::/48 fe80::32ee:54de:cd43:b4e1 UG 100 1 0 enp0s25 +fe80::abcd:ef12:bc34:da21/128 :: U 100 1 2 enp0s25 +fe80::/64 :: U 256 1 16880 enp0s25 +::/0 fe80::32ee:54de:cd43:b4e1 UG 100 1 0 enp0s25 +::/0 :: !n -1 1424956 lo +::1/128 :: Un 0 4 26289 lo diff --git a/tests/unittests/test_filters/test_launch_index.py b/tests/unittests/test_filters/test_launch_index.py index 6364d38e..e1a5d2c8 100644 --- a/tests/unittests/test_filters/test_launch_index.py +++ b/tests/unittests/test_filters/test_launch_index.py @@ -55,7 +55,7 @@ class TestLaunchFilter(helpers.ResourceUsingTestCase): return True def testMultiEmailIndex(self): - test_data = self.readResource('filter_cloud_multipart_2.email') + test_data = helpers.readResource('filter_cloud_multipart_2.email') ud_proc = ud.UserDataProcessor(self.getCloudPaths()) message = ud_proc.process(test_data) self.assertTrue(count_messages(message) > 0) @@ -70,7 +70,7 @@ class TestLaunchFilter(helpers.ResourceUsingTestCase): self.assertCounts(message, expected_counts) def testHeaderEmailIndex(self): - test_data = self.readResource('filter_cloud_multipart_header.email') + test_data = helpers.readResource('filter_cloud_multipart_header.email') ud_proc = ud.UserDataProcessor(self.getCloudPaths()) message = ud_proc.process(test_data) self.assertTrue(count_messages(message) > 0) @@ -85,7 +85,7 @@ class TestLaunchFilter(helpers.ResourceUsingTestCase): self.assertCounts(message, expected_counts) def testConfigEmailIndex(self): - test_data = self.readResource('filter_cloud_multipart_1.email') + test_data = helpers.readResource('filter_cloud_multipart_1.email') ud_proc = ud.UserDataProcessor(self.getCloudPaths()) message = ud_proc.process(test_data) self.assertTrue(count_messages(message) > 0) @@ -99,7 +99,7 @@ class TestLaunchFilter(helpers.ResourceUsingTestCase): self.assertCounts(message, expected_counts) def testNoneIndex(self): - test_data = self.readResource('filter_cloud_multipart.yaml') + test_data = helpers.readResource('filter_cloud_multipart.yaml') ud_proc = ud.UserDataProcessor(self.getCloudPaths()) message = ud_proc.process(test_data) start_count = count_messages(message) @@ -108,7 +108,7 @@ class TestLaunchFilter(helpers.ResourceUsingTestCase): self.assertTrue(self.equivalentMessage(message, filtered_message)) def testIndexes(self): - test_data = self.readResource('filter_cloud_multipart.yaml') + test_data = helpers.readResource('filter_cloud_multipart.yaml') ud_proc = ud.UserDataProcessor(self.getCloudPaths()) message = ud_proc.process(test_data) start_count = count_messages(message) diff --git a/tests/unittests/test_merging.py b/tests/unittests/test_merging.py index f51358da..3a5072c7 100644 --- a/tests/unittests/test_merging.py +++ b/tests/unittests/test_merging.py @@ -100,7 +100,7 @@ def make_dict(max_depth, seed=None): class TestSimpleRun(helpers.ResourceUsingTestCase): def _load_merge_files(self): - merge_root = self.resourceLocation('merge_sources') + merge_root = helpers.resourceLocation('merge_sources') tests = [] source_ids = collections.defaultdict(list) expected_files = {} diff --git a/tests/unittests/test_runs/test_merge_run.py b/tests/unittests/test_runs/test_merge_run.py index 5d3f1ca3..d1ac4942 100644 --- a/tests/unittests/test_runs/test_merge_run.py +++ b/tests/unittests/test_runs/test_merge_run.py @@ -25,7 +25,7 @@ class TestMergeRun(helpers.FilesystemMockingTestCase): 'cloud_init_modules': ['write-files'], 'system_info': {'paths': {'run_dir': new_root}} } - ud = self.readResource('user_data.1.txt') + ud = helpers.readResource('user_data.1.txt') cloud_cfg = util.yaml_dumps(cfg) util.ensure_dir(os.path.join(new_root, 'etc', 'cloud')) util.write_file(os.path.join(new_root, 'etc', diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 7cbd5538..e04ea031 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -325,7 +325,7 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): def test_precise_ext4_root(self): - lines = self.readResource('mountinfo_precise_ext4.txt').splitlines() + lines = helpers.readResource('mountinfo_precise_ext4.txt').splitlines() expected = ('/dev/mapper/vg0-root', 'ext4', '/') self.assertEqual(expected, util.parse_mount_info('/', lines)) @@ -347,7 +347,7 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): self.assertEqual(expected, util.parse_mount_info('/run/lock', lines)) def test_raring_btrfs_root(self): - lines = self.readResource('mountinfo_raring_btrfs.txt').splitlines() + lines = helpers.readResource('mountinfo_raring_btrfs.txt').splitlines() expected = ('/dev/vda1', 'btrfs', '/') self.assertEqual(expected, util.parse_mount_info('/', lines)) @@ -373,7 +373,7 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): m_os.path.exists.return_value = True # mock subp command from util.get_mount_info_fs_on_zpool zpool_output.return_value = ( - self.readResource('zpool_status_simple.txt'), '' + helpers.readResource('zpool_status_simple.txt'), '' ) # save function return values and do asserts ret = util.get_device_info_from_zpool('vmzroot') @@ -406,7 +406,7 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): m_os.path.exists.return_value = True # mock subp command from util.get_mount_info_fs_on_zpool zpool_output.return_value = ( - self.readResource('zpool_status_simple.txt'), 'error' + helpers.readResource('zpool_status_simple.txt'), 'error' ) # save function return values and do asserts ret = util.get_device_info_from_zpool('vmzroot') @@ -414,7 +414,8 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): @mock.patch('cloudinit.util.subp') def test_parse_mount_with_ext(self, mount_out): - mount_out.return_value = (self.readResource('mount_parse_ext.txt'), '') + mount_out.return_value = ( + helpers.readResource('mount_parse_ext.txt'), '') # this one is valid and exists in mount_parse_ext.txt ret = util.parse_mount('/var') self.assertEqual(('/dev/mapper/vg00-lv_var', 'ext4', '/var'), ret) @@ -430,7 +431,8 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase): @mock.patch('cloudinit.util.subp') def test_parse_mount_with_zfs(self, mount_out): - mount_out.return_value = (self.readResource('mount_parse_zfs.txt'), '') + mount_out.return_value = ( + helpers.readResource('mount_parse_zfs.txt'), '') # this one is valid and exists in mount_parse_zfs.txt ret = util.parse_mount('/var') self.assertEqual(('vmzroot/ROOT/freebsd/var', 'zfs', '/var'), ret) -- cgit v1.2.3 From 6811926fdb991ad02ad9c0134c1d4bbe82ef87e1 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 18 Apr 2018 20:37:07 -0600 Subject: Schema: do not warn on duplicate items in commands. runcmd, bootcmd, snap/commands, ubuntu-advantage/commands would log warning (and fail if strict) on duplicate values in the commands. But those should be allowed. Example, it is perfectly valid to do: runcmd: ['sleep 1', 'sleep 1'] LP: #1764264 --- cloudinit/config/cc_bootcmd.py | 1 - cloudinit/config/cc_runcmd.py | 1 - cloudinit/config/cc_snap.py | 1 - cloudinit/config/cc_ubuntu_advantage.py | 1 - cloudinit/config/tests/test_snap.py | 36 ++++++++++++++++++++++ .../unittests/test_handler/test_handler_bootcmd.py | 24 ++++++++++++++- .../unittests/test_handler/test_handler_runcmd.py | 25 +++++++++++++-- 7 files changed, 82 insertions(+), 7 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_bootcmd.py b/cloudinit/config/cc_bootcmd.py index 233da1ef..db64f0a6 100644 --- a/cloudinit/config/cc_bootcmd.py +++ b/cloudinit/config/cc_bootcmd.py @@ -63,7 +63,6 @@ schema = { 'additionalProperties': False, 'minItems': 1, 'required': [], - 'uniqueItems': True } } } diff --git a/cloudinit/config/cc_runcmd.py b/cloudinit/config/cc_runcmd.py index 539cbd5d..b6f6c807 100644 --- a/cloudinit/config/cc_runcmd.py +++ b/cloudinit/config/cc_runcmd.py @@ -66,7 +66,6 @@ schema = { 'additionalProperties': False, 'minItems': 1, 'required': [], - 'uniqueItems': True } } } diff --git a/cloudinit/config/cc_snap.py b/cloudinit/config/cc_snap.py index 34a53fd4..a7a03214 100644 --- a/cloudinit/config/cc_snap.py +++ b/cloudinit/config/cc_snap.py @@ -110,7 +110,6 @@ schema = { 'additionalItems': False, # Reject non-string & non-list 'minItems': 1, 'minProperties': 1, - 'uniqueItems': True }, 'squashfuse_in_container': { 'type': 'boolean' diff --git a/cloudinit/config/cc_ubuntu_advantage.py b/cloudinit/config/cc_ubuntu_advantage.py index 16b1868b..29d18c96 100644 --- a/cloudinit/config/cc_ubuntu_advantage.py +++ b/cloudinit/config/cc_ubuntu_advantage.py @@ -87,7 +87,6 @@ schema = { 'additionalItems': False, # Reject non-string & non-list 'minItems': 1, 'minProperties': 1, - 'uniqueItems': True } }, 'additionalProperties': False, # Reject keys not in schema diff --git a/cloudinit/config/tests/test_snap.py b/cloudinit/config/tests/test_snap.py index c5b4a9de..492d2d46 100644 --- a/cloudinit/config/tests/test_snap.py +++ b/cloudinit/config/tests/test_snap.py @@ -340,6 +340,42 @@ class TestSchema(CiTestCase): {'snap': {'assertions': {'01': 'also valid'}}}, schema) self.assertEqual('', self.logs.getvalue()) + def test_duplicates_are_fine_array_array(self): + """Duplicated commands array/array entries are allowed.""" + byebye = ["echo", "bye"] + try: + cfg = {'snap': {'commands': [byebye, byebye]}} + validate_cloudconfig_schema(cfg, schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("command entries can be duplicate.") + + def test_duplicates_are_fine_array_string(self): + """Duplicated commands array/string entries are allowed.""" + byebye = "echo bye" + try: + cfg = {'snap': {'commands': [byebye, byebye]}} + validate_cloudconfig_schema(cfg, schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("command entries can be duplicate.") + + def test_duplicates_are_fine_dict_array(self): + """Duplicated commands dict/array entries are allowed.""" + byebye = ["echo", "bye"] + try: + cfg = {'snap': {'commands': {'00': byebye, '01': byebye}}} + validate_cloudconfig_schema(cfg, schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("command entries can be duplicate.") + + def test_duplicates_are_fine_dict_string(self): + """Duplicated commands dict/string entries are allowed.""" + byebye = "echo bye" + try: + cfg = {'snap': {'commands': {'00': byebye, '01': byebye}}} + validate_cloudconfig_schema(cfg, schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("command entries can be duplicate.") + class TestHandle(CiTestCase): diff --git a/tests/unittests/test_handler/test_handler_bootcmd.py b/tests/unittests/test_handler/test_handler_bootcmd.py index 29fc25e4..c3abedde 100644 --- a/tests/unittests/test_handler/test_handler_bootcmd.py +++ b/tests/unittests/test_handler/test_handler_bootcmd.py @@ -1,6 +1,6 @@ # This file is part of cloud-init. See LICENSE file for license information. -from cloudinit.config import cc_bootcmd +from cloudinit.config import cc_bootcmd, schema from cloudinit.sources import DataSourceNone from cloudinit import (distros, helpers, cloud, util) from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJsonSchema @@ -138,4 +138,26 @@ class TestBootcmd(CiTestCase): self.logs.getvalue()) +class TestSchema(CiTestCase): + """Directly test schema rather than through handle.""" + + def test_duplicates_are_fine_array_array(self): + """Duplicated array entries are allowed.""" + try: + byebye = ["echo", "bye"] + schema.validate_cloudconfig_schema( + {'bootcmd': [byebye, byebye]}, cc_bootcmd.schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("runcmd entries as list are allowed to be duplicates.") + + def test_duplicates_are_fine_array_string(self): + """Duplicated array entries entries are allowed in values of array.""" + try: + byebye = "echo bye" + schema.validate_cloudconfig_schema( + {'bootcmd': [byebye, byebye]}, cc_bootcmd.schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("runcmd entries are allowed to be duplicates.") + + # vi: ts=4 expandtab diff --git a/tests/unittests/test_handler/test_handler_runcmd.py b/tests/unittests/test_handler/test_handler_runcmd.py index dbbb2717..ee1981d1 100644 --- a/tests/unittests/test_handler/test_handler_runcmd.py +++ b/tests/unittests/test_handler/test_handler_runcmd.py @@ -1,10 +1,10 @@ # This file is part of cloud-init. See LICENSE file for license information. -from cloudinit.config import cc_runcmd +from cloudinit.config import cc_runcmd, schema from cloudinit.sources import DataSourceNone from cloudinit import (distros, helpers, cloud, util) from cloudinit.tests.helpers import ( - FilesystemMockingTestCase, skipUnlessJsonSchema) + CiTestCase, FilesystemMockingTestCase, skipUnlessJsonSchema) import logging import os @@ -99,4 +99,25 @@ class TestRuncmd(FilesystemMockingTestCase): self.assertEqual(0o700, stat.S_IMODE(file_stat.st_mode)) +class TestSchema(CiTestCase): + """Directly test schema rather than through handle.""" + + def test_duplicates_are_fine_array(self): + """Duplicated array entries are allowed.""" + try: + byebye = ["echo", "bye"] + schema.validate_cloudconfig_schema( + {'runcmd': [byebye, byebye]}, cc_runcmd.schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("runcmd entries as list are allowed to be duplicates.") + + def test_duplicates_are_fine_string(self): + """Duplicated string entries are allowed.""" + try: + byebye = "echo bye" + schema.validate_cloudconfig_schema( + {'runcmd': [byebye, byebye]}, cc_runcmd.schema, strict=True) + except schema.SchemaValidationError as e: + self.fail("runcmd entries are allowed to be duplicates.") + # vi: ts=4 expandtab -- cgit v1.2.3 From 1081962eacf2814fea6f4fa3255c530de14e4a24 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 19 Apr 2018 21:30:08 -0600 Subject: pylint: pay attention to unused variable warnings. This enables warnings produced by pylint for unused variables (W0612), and fixes the existing errors. --- .pylintrc | 2 +- cloudinit/analyze/dump.py | 2 +- cloudinit/cmd/tests/test_main.py | 6 ++-- cloudinit/config/cc_apt_configure.py | 2 +- cloudinit/config/cc_emit_upstart.py | 2 +- cloudinit/config/cc_resizefs.py | 8 ++---- cloudinit/config/cc_rh_subscription.py | 18 ++++++------ cloudinit/config/cc_snap.py | 4 +-- cloudinit/config/cc_snappy.py | 4 +-- cloudinit/config/cc_ubuntu_advantage.py | 4 +-- cloudinit/config/schema.py | 4 +-- cloudinit/distros/freebsd.py | 2 +- cloudinit/distros/ubuntu.py | 2 +- cloudinit/net/__init__.py | 2 +- cloudinit/net/cmdline.py | 2 +- cloudinit/net/dhcp.py | 2 +- cloudinit/net/sysconfig.py | 2 +- cloudinit/reporting/events.py | 2 +- cloudinit/sources/DataSourceAliYun.py | 2 +- cloudinit/sources/DataSourceAzure.py | 33 +++++++++------------- cloudinit/sources/DataSourceMAAS.py | 2 +- cloudinit/sources/DataSourceOVF.py | 2 +- cloudinit/sources/DataSourceOpenStack.py | 4 +-- cloudinit/sources/helpers/digitalocean.py | 7 ++--- cloudinit/sources/helpers/openstack.py | 2 +- cloudinit/sources/helpers/vmware/imc/config_nic.py | 2 +- .../sources/helpers/vmware/imc/config_passwd.py | 4 +-- .../sources/helpers/vmware/imc/guestcust_util.py | 4 +-- cloudinit/sources/tests/test_init.py | 2 +- cloudinit/templater.py | 2 +- cloudinit/tests/helpers.py | 2 +- cloudinit/tests/test_util.py | 2 +- cloudinit/url_helper.py | 2 +- cloudinit/util.py | 2 +- tests/cloud_tests/bddeb.py | 2 +- tests/cloud_tests/collect.py | 3 +- tests/cloud_tests/platforms/instances.py | 2 +- tests/cloud_tests/platforms/lxd/instance.py | 10 +++---- tests/cloud_tests/setup_image.py | 11 ++++---- tests/cloud_tests/testcases/base.py | 2 +- .../testcases/examples/including_user_groups.py | 2 +- tests/cloud_tests/testcases/modules/user_groups.py | 2 +- tests/cloud_tests/util.py | 2 +- tests/unittests/test__init__.py | 2 +- tests/unittests/test_datasource/test_azure.py | 4 +-- tests/unittests/test_datasource/test_maas.py | 4 +-- tests/unittests/test_datasource/test_nocloud.py | 3 -- .../test_handler/test_handler_apt_source_v3.py | 2 +- tests/unittests/test_handler/test_handler_ntp.py | 2 +- tests/unittests/test_templating.py | 4 +-- tests/unittests/test_util.py | 6 ++-- 51 files changed, 95 insertions(+), 112 deletions(-) (limited to 'cloudinit/config') diff --git a/.pylintrc b/.pylintrc index 0bdfa59d..3bfa0c81 100644 --- a/.pylintrc +++ b/.pylintrc @@ -28,7 +28,7 @@ jobs=4 # W0703(broad-except) # W1401(anomalous-backslash-in-string) -disable=C, F, I, R, W0105, W0107, W0201, W0212, W0221, W0222, W0223, W0231, W0311, W0511, W0602, W0603, W0611, W0612, W0613, W0621, W0622, W0631, W0703, W1401 +disable=C, F, I, R, W0105, W0107, W0201, W0212, W0221, W0222, W0223, W0231, W0311, W0511, W0602, W0603, W0611, W0613, W0621, W0622, W0631, W0703, W1401 [REPORTS] diff --git a/cloudinit/analyze/dump.py b/cloudinit/analyze/dump.py index b071aa19..1f3060d0 100644 --- a/cloudinit/analyze/dump.py +++ b/cloudinit/analyze/dump.py @@ -112,7 +112,7 @@ def parse_ci_logline(line): return None event_description = stage_to_description[event_name] else: - (pymodloglvl, event_type, event_name) = eventstr.split()[0:3] + (_pymodloglvl, event_type, event_name) = eventstr.split()[0:3] event_description = eventstr.split(event_name)[1].strip() event = { diff --git a/cloudinit/cmd/tests/test_main.py b/cloudinit/cmd/tests/test_main.py index dbe421c0..e2c54ae8 100644 --- a/cloudinit/cmd/tests/test_main.py +++ b/cloudinit/cmd/tests/test_main.py @@ -56,7 +56,7 @@ class TestMain(FilesystemMockingTestCase): cmdargs = myargs( debug=False, files=None, force=False, local=False, reporter=None, subcommand='init') - (item1, item2) = wrap_and_call( + (_item1, item2) = wrap_and_call( 'cloudinit.cmd.main', {'util.close_stdin': True, 'netinfo.debug_info': 'my net debug info', @@ -85,7 +85,7 @@ class TestMain(FilesystemMockingTestCase): cmdargs = myargs( debug=False, files=None, force=False, local=False, reporter=None, subcommand='init') - (item1, item2) = wrap_and_call( + (_item1, item2) = wrap_and_call( 'cloudinit.cmd.main', {'util.close_stdin': True, 'netinfo.debug_info': 'my net debug info', @@ -133,7 +133,7 @@ class TestMain(FilesystemMockingTestCase): self.assertEqual(main.LOG, log) self.assertIsNone(args) - (item1, item2) = wrap_and_call( + (_item1, item2) = wrap_and_call( 'cloudinit.cmd.main', {'util.close_stdin': True, 'netinfo.debug_info': 'my net debug info', diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index afaca464..e18944ec 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -378,7 +378,7 @@ def apply_debconf_selections(cfg, target=None): # get a complete list of packages listed in input pkgs_cfgd = set() - for key, content in selsets.items(): + for _key, content in selsets.items(): for line in content.splitlines(): if line.startswith("#"): continue diff --git a/cloudinit/config/cc_emit_upstart.py b/cloudinit/config/cc_emit_upstart.py index 69dc2d5e..eb9fbe66 100644 --- a/cloudinit/config/cc_emit_upstart.py +++ b/cloudinit/config/cc_emit_upstart.py @@ -43,7 +43,7 @@ def is_upstart_system(): del myenv['UPSTART_SESSION'] check_cmd = ['initctl', 'version'] try: - (out, err) = util.subp(check_cmd, env=myenv) + (out, _err) = util.subp(check_cmd, env=myenv) return 'upstart' in out except util.ProcessExecutionError as e: LOG.debug("'%s' returned '%s', not using upstart", diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py index 013e69b5..82f29e10 100644 --- a/cloudinit/config/cc_resizefs.py +++ b/cloudinit/config/cc_resizefs.py @@ -89,13 +89,11 @@ def _resize_zfs(mount_point, devpth): def _get_dumpfs_output(mount_point): - dumpfs_res, err = util.subp(['dumpfs', '-m', mount_point]) - return dumpfs_res + return util.subp(['dumpfs', '-m', mount_point])[0] def _get_gpart_output(part): - gpart_res, err = util.subp(['gpart', 'show', part]) - return gpart_res + return util.subp(['gpart', 'show', part])[0] def _can_skip_resize_ufs(mount_point, devpth): @@ -113,7 +111,7 @@ def _can_skip_resize_ufs(mount_point, devpth): if not line.startswith('#'): newfs_cmd = shlex.split(line) opt_value = 'O:Ua:s:b:d:e:f:g:h:i:jk:m:o:' - optlist, args = getopt.getopt(newfs_cmd[1:], opt_value) + optlist, _args = getopt.getopt(newfs_cmd[1:], opt_value) for o, a in optlist: if o == "-s": cur_fs_sz = int(a) diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py index 530808ce..1c679430 100644 --- a/cloudinit/config/cc_rh_subscription.py +++ b/cloudinit/config/cc_rh_subscription.py @@ -209,8 +209,7 @@ class SubscriptionManager(object): cmd.append("--serverurl={0}".format(self.server_hostname)) try: - return_out, return_err = self._sub_man_cli(cmd, - logstring_val=True) + return_out = self._sub_man_cli(cmd, logstring_val=True)[0] except util.ProcessExecutionError as e: if e.stdout == "": self.log_warn("Registration failed due " @@ -233,8 +232,7 @@ class SubscriptionManager(object): # Attempting to register the system only try: - return_out, return_err = self._sub_man_cli(cmd, - logstring_val=True) + return_out = self._sub_man_cli(cmd, logstring_val=True)[0] except util.ProcessExecutionError as e: if e.stdout == "": self.log_warn("Registration failed due " @@ -257,7 +255,7 @@ class SubscriptionManager(object): .format(self.servicelevel)] try: - return_out, return_err = self._sub_man_cli(cmd) + return_out = self._sub_man_cli(cmd)[0] except util.ProcessExecutionError as e: if e.stdout.rstrip() != '': for line in e.stdout.split("\n"): @@ -275,7 +273,7 @@ class SubscriptionManager(object): def _set_auto_attach(self): cmd = ['attach', '--auto'] try: - return_out, return_err = self._sub_man_cli(cmd) + return_out = self._sub_man_cli(cmd)[0] except util.ProcessExecutionError as e: self.log_warn("Auto-attach failed with: {0}".format(e)) return False @@ -294,12 +292,12 @@ class SubscriptionManager(object): # Get all available pools cmd = ['list', '--available', '--pool-only'] - results, errors = self._sub_man_cli(cmd) + results = self._sub_man_cli(cmd)[0] available = (results.rstrip()).split("\n") # Get all consumed pools cmd = ['list', '--consumed', '--pool-only'] - results, errors = self._sub_man_cli(cmd) + results = self._sub_man_cli(cmd)[0] consumed = (results.rstrip()).split("\n") return available, consumed @@ -311,14 +309,14 @@ class SubscriptionManager(object): ''' cmd = ['repos', '--list-enabled'] - return_out, return_err = self._sub_man_cli(cmd) + return_out = self._sub_man_cli(cmd)[0] active_repos = [] for repo in return_out.split("\n"): if "Repo ID:" in repo: active_repos.append((repo.split(':')[1]).strip()) cmd = ['repos', '--list-disabled'] - return_out, return_err = self._sub_man_cli(cmd) + return_out = self._sub_man_cli(cmd)[0] inactive_repos = [] for repo in return_out.split("\n"): diff --git a/cloudinit/config/cc_snap.py b/cloudinit/config/cc_snap.py index a7a03214..90724b81 100644 --- a/cloudinit/config/cc_snap.py +++ b/cloudinit/config/cc_snap.py @@ -203,12 +203,12 @@ def maybe_install_squashfuse(cloud): return try: cloud.distro.update_package_sources() - except Exception as e: + except Exception: util.logexc(LOG, "Package update failed") raise try: cloud.distro.install_packages(['squashfuse']) - except Exception as e: + except Exception: util.logexc(LOG, "Failed to install squashfuse") raise diff --git a/cloudinit/config/cc_snappy.py b/cloudinit/config/cc_snappy.py index bab80bbe..15bee2d3 100644 --- a/cloudinit/config/cc_snappy.py +++ b/cloudinit/config/cc_snappy.py @@ -213,7 +213,7 @@ def render_snap_op(op, name, path=None, cfgfile=None, config=None): def read_installed_packages(): ret = [] - for (name, date, version, dev) in read_pkg_data(): + for (name, _date, _version, dev) in read_pkg_data(): if dev: ret.append(NAMESPACE_DELIM.join([name, dev])) else: @@ -222,7 +222,7 @@ def read_installed_packages(): def read_pkg_data(): - out, err = util.subp([SNAPPY_CMD, "list"]) + out, _err = util.subp([SNAPPY_CMD, "list"]) pkg_data = [] for line in out.splitlines()[1:]: toks = line.split(sep=None, maxsplit=3) diff --git a/cloudinit/config/cc_ubuntu_advantage.py b/cloudinit/config/cc_ubuntu_advantage.py index 29d18c96..5e082bd6 100644 --- a/cloudinit/config/cc_ubuntu_advantage.py +++ b/cloudinit/config/cc_ubuntu_advantage.py @@ -148,12 +148,12 @@ def maybe_install_ua_tools(cloud): return try: cloud.distro.update_package_sources() - except Exception as e: + except Exception: util.logexc(LOG, "Package update failed") raise try: cloud.distro.install_packages(['ubuntu-advantage-tools']) - except Exception as e: + except Exception: util.logexc(LOG, "Failed to install ubuntu-advantage-tools") raise diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index ca7d0d5b..76826e05 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -297,8 +297,8 @@ def get_schema(): configs_dir = os.path.dirname(os.path.abspath(__file__)) potential_handlers = find_modules(configs_dir) - for (fname, mod_name) in potential_handlers.items(): - mod_locs, looked_locs = importer.find_module( + for (_fname, mod_name) in potential_handlers.items(): + mod_locs, _looked_locs = importer.find_module( mod_name, ['cloudinit.config'], ['schema']) if mod_locs: mod = importer.import_module(mod_locs[0]) diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 099fac5c..5b1718a4 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -113,7 +113,7 @@ class Distro(distros.Distro): n = re.search(r'\d+$', dev) index = n.group(0) - (out, err) = util.subp(['ifconfig', '-a']) + (out, _err) = util.subp(['ifconfig', '-a']) ifconfigoutput = [x for x in (out.strip()).splitlines() if len(x.split()) > 0] bsddev = 'NOT_FOUND' diff --git a/cloudinit/distros/ubuntu.py b/cloudinit/distros/ubuntu.py index fdc1f622..68154104 100644 --- a/cloudinit/distros/ubuntu.py +++ b/cloudinit/distros/ubuntu.py @@ -25,7 +25,7 @@ class Distro(debian.Distro): def preferred_ntp_clients(self): """The preferred ntp client is dependent on the version.""" if not self._preferred_ntp_clients: - (name, version, codename) = util.system_info()['dist'] + (_name, _version, codename) = util.system_info()['dist'] # Xenial cloud-init only installed ntp, UbuntuCore has timesyncd. if codename == "xenial" and not util.system_is_snappy(): self._preferred_ntp_clients = ['ntp'] diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index f69c0ef2..80054546 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -295,7 +295,7 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True): def _version_2(netcfg): renames = [] - for key, ent in netcfg.get('ethernets', {}).items(): + for ent in netcfg.get('ethernets', {}).values(): # only rename if configured to do so name = ent.get('set-name') if not name: diff --git a/cloudinit/net/cmdline.py b/cloudinit/net/cmdline.py index 9e9fe0fe..f89a0f73 100755 --- a/cloudinit/net/cmdline.py +++ b/cloudinit/net/cmdline.py @@ -65,7 +65,7 @@ def _klibc_to_config_entry(content, mac_addrs=None): iface['mac_address'] = mac_addrs[name] # Handle both IPv4 and IPv6 values - for v, pre in (('ipv4', 'IPV4'), ('ipv6', 'IPV6')): + for pre in ('IPV4', 'IPV6'): # if no IPV4ADDR or IPV6ADDR, then go on. if pre + "ADDR" not in data: continue diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 087c0c03..12cf5097 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -216,7 +216,7 @@ def networkd_get_option_from_leases(keyname, leases_d=None): if leases_d is None: leases_d = NETWORKD_LEASES_DIR leases = networkd_load_leases(leases_d=leases_d) - for ifindex, data in sorted(leases.items()): + for _ifindex, data in sorted(leases.items()): if data.get(keyname): return data[keyname] return None diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py index 39d89c46..7a7f5093 100644 --- a/cloudinit/net/sysconfig.py +++ b/cloudinit/net/sysconfig.py @@ -364,7 +364,7 @@ class Renderer(renderer.Renderer): @classmethod def _render_subnet_routes(cls, iface_cfg, route_cfg, subnets): - for i, subnet in enumerate(subnets, start=len(iface_cfg.children)): + for _, subnet in enumerate(subnets, start=len(iface_cfg.children)): for route in subnet.get('routes', []): is_ipv6 = subnet.get('ipv6') or is_ipv6_addr(route['gateway']) diff --git a/cloudinit/reporting/events.py b/cloudinit/reporting/events.py index 4f62d2f9..e5dfab33 100644 --- a/cloudinit/reporting/events.py +++ b/cloudinit/reporting/events.py @@ -192,7 +192,7 @@ class ReportEventStack(object): def _childrens_finish_info(self): for cand_result in (status.FAIL, status.WARN): - for name, (value, msg) in self.children.items(): + for _name, (value, _msg) in self.children.items(): if value == cand_result: return (value, self.message) return (self.result, self.message) diff --git a/cloudinit/sources/DataSourceAliYun.py b/cloudinit/sources/DataSourceAliYun.py index 22279d09..858e0827 100644 --- a/cloudinit/sources/DataSourceAliYun.py +++ b/cloudinit/sources/DataSourceAliYun.py @@ -45,7 +45,7 @@ def _is_aliyun(): def parse_public_keys(public_keys): keys = [] - for key_id, key_body in public_keys.items(): + for _key_id, key_body in public_keys.items(): if isinstance(key_body, str): keys.append(key_body.strip()) elif isinstance(key_body, list): diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 0ee622e2..a71197a6 100644 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -107,31 +107,24 @@ def find_dev_from_busdev(camcontrol_out, busdev): return None -def get_dev_storvsc_sysctl(): +def execute_or_debug(cmd, fail_ret=None): try: - sysctl_out, err = util.subp(['sysctl', 'dev.storvsc']) + return util.subp(cmd)[0] except util.ProcessExecutionError: - LOG.debug("Fail to execute sysctl dev.storvsc") - sysctl_out = "" - return sysctl_out + LOG.debug("Failed to execute: %s", ' '.join(cmd)) + return fail_ret + + +def get_dev_storvsc_sysctl(): + return execute_or_debug(["sysctl", "dev.storvsc"], fail_ret="") def get_camcontrol_dev_bus(): - try: - camcontrol_b_out, err = util.subp(['camcontrol', 'devlist', '-b']) - except util.ProcessExecutionError: - LOG.debug("Fail to execute camcontrol devlist -b") - return None - return camcontrol_b_out + return execute_or_debug(['camcontrol', 'devlist', '-b']) def get_camcontrol_dev(): - try: - camcontrol_out, err = util.subp(['camcontrol', 'devlist']) - except util.ProcessExecutionError: - LOG.debug("Fail to execute camcontrol devlist") - return None - return camcontrol_out + return execute_or_debug(['camcontrol', 'devlist']) def get_resource_disk_on_freebsd(port_id): @@ -474,7 +467,7 @@ class DataSourceAzure(sources.DataSource): before we go into our polling loop.""" try: get_metadata_from_fabric(None, lease['unknown-245']) - except Exception as exc: + except Exception: LOG.warning( "Error communicating with Azure fabric; You may experience." "connectivity issues.", exc_info=True) @@ -492,7 +485,7 @@ class DataSourceAzure(sources.DataSource): jump back into the polling loop in order to retrieve the ovf_env.""" if not ret: return False - (md, self.userdata_raw, cfg, files) = ret + (_md, self.userdata_raw, cfg, _files) = ret path = REPROVISION_MARKER_FILE if (cfg.get('PreprovisionedVm') is True or os.path.isfile(path)): @@ -528,7 +521,7 @@ class DataSourceAzure(sources.DataSource): self.ds_cfg['agent_command']) try: fabric_data = metadata_func() - except Exception as exc: + except Exception: LOG.warning( "Error communicating with Azure fabric; You may experience." "connectivity issues.", exc_info=True) diff --git a/cloudinit/sources/DataSourceMAAS.py b/cloudinit/sources/DataSourceMAAS.py index 6ac88635..aa56addb 100644 --- a/cloudinit/sources/DataSourceMAAS.py +++ b/cloudinit/sources/DataSourceMAAS.py @@ -204,7 +204,7 @@ def read_maas_seed_url(seed_url, read_file_or_url=None, timeout=None, seed_url = seed_url[:-1] md = {} - for path, dictname, binary, optional in DS_FIELDS: + for path, _dictname, binary, optional in DS_FIELDS: if version is None: url = "%s/%s" % (seed_url, path) else: diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py index dc914a72..178ccb0f 100644 --- a/cloudinit/sources/DataSourceOVF.py +++ b/cloudinit/sources/DataSourceOVF.py @@ -556,7 +556,7 @@ def search_file(dirpath, filename): if not dirpath or not filename: return None - for root, dirs, files in os.walk(dirpath): + for root, _dirs, files in os.walk(dirpath): if filename in files: return os.path.join(root, filename) diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py index e55a7638..fb166ae1 100644 --- a/cloudinit/sources/DataSourceOpenStack.py +++ b/cloudinit/sources/DataSourceOpenStack.py @@ -86,7 +86,7 @@ class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource): md_urls.append(md_url) url2base[md_url] = url - (max_wait, timeout, retries) = self._get_url_settings() + (max_wait, timeout, _retries) = self._get_url_settings() start_time = time.time() avail_url = url_helper.wait_for_url(urls=md_urls, max_wait=max_wait, timeout=timeout) @@ -106,7 +106,7 @@ class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource): except IOError: return False - (max_wait, timeout, retries) = self._get_url_settings() + (_max_wait, timeout, retries) = self._get_url_settings() try: results = util.log_time(LOG.debug, diff --git a/cloudinit/sources/helpers/digitalocean.py b/cloudinit/sources/helpers/digitalocean.py index 693f8d5c..0e7cccac 100644 --- a/cloudinit/sources/helpers/digitalocean.py +++ b/cloudinit/sources/helpers/digitalocean.py @@ -41,10 +41,9 @@ def assign_ipv4_link_local(nic=None): "address") try: - (result, _err) = util.subp(ip_addr_cmd) + util.subp(ip_addr_cmd) LOG.debug("assigned ip4LL address '%s' to '%s'", addr, nic) - - (result, _err) = util.subp(ip_link_cmd) + util.subp(ip_link_cmd) LOG.debug("brought device '%s' up", nic) except Exception: util.logexc(LOG, "ip4LL address assignment of '%s' to '%s' failed." @@ -75,7 +74,7 @@ def del_ipv4_link_local(nic=None): ip_addr_cmd = ['ip', 'addr', 'flush', 'dev', nic] try: - (result, _err) = util.subp(ip_addr_cmd) + util.subp(ip_addr_cmd) LOG.debug("removed ip4LL addresses from %s", nic) except Exception as e: diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 26f3168d..a4cf0667 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -638,7 +638,7 @@ def convert_net_json(network_json=None, known_macs=None): known_macs = net.get_interfaces_by_mac() # go through and fill out the link_id_info with names - for link_id, info in link_id_info.items(): + for _link_id, info in link_id_info.items(): if info.get('name'): continue if info.get('mac') in known_macs: diff --git a/cloudinit/sources/helpers/vmware/imc/config_nic.py b/cloudinit/sources/helpers/vmware/imc/config_nic.py index 2d8900e2..3ef8c624 100644 --- a/cloudinit/sources/helpers/vmware/imc/config_nic.py +++ b/cloudinit/sources/helpers/vmware/imc/config_nic.py @@ -73,7 +73,7 @@ class NicConfigurator(object): The mac address(es) are in the lower case """ cmd = ['ip', 'addr', 'show'] - (output, err) = util.subp(cmd) + output, _err = util.subp(cmd) sections = re.split(r'\n\d+: ', '\n' + output)[1:] macPat = r'link/ether (([0-9A-Fa-f]{2}[:]){5}([0-9A-Fa-f]{2}))' diff --git a/cloudinit/sources/helpers/vmware/imc/config_passwd.py b/cloudinit/sources/helpers/vmware/imc/config_passwd.py index 75cfbaaf..8c91fa41 100644 --- a/cloudinit/sources/helpers/vmware/imc/config_passwd.py +++ b/cloudinit/sources/helpers/vmware/imc/config_passwd.py @@ -56,10 +56,10 @@ class PasswordConfigurator(object): LOG.info('Expiring password.') for user in uidUserList: try: - out, err = util.subp(['passwd', '--expire', user]) + util.subp(['passwd', '--expire', user]) except util.ProcessExecutionError as e: if os.path.exists('/usr/bin/chage'): - out, e = util.subp(['chage', '-d', '0', user]) + util.subp(['chage', '-d', '0', user]) else: LOG.warning('Failed to expire password for %s with error: ' '%s', user, e) diff --git a/cloudinit/sources/helpers/vmware/imc/guestcust_util.py b/cloudinit/sources/helpers/vmware/imc/guestcust_util.py index 44075255..a590f323 100644 --- a/cloudinit/sources/helpers/vmware/imc/guestcust_util.py +++ b/cloudinit/sources/helpers/vmware/imc/guestcust_util.py @@ -91,7 +91,7 @@ def enable_nics(nics): for attempt in range(0, enableNicsWaitRetries): logger.debug("Trying to connect interfaces, attempt %d", attempt) - (out, err) = set_customization_status( + (out, _err) = set_customization_status( GuestCustStateEnum.GUESTCUST_STATE_RUNNING, GuestCustEventEnum.GUESTCUST_EVENT_ENABLE_NICS, nics) @@ -104,7 +104,7 @@ def enable_nics(nics): return for count in range(0, enableNicsWaitCount): - (out, err) = set_customization_status( + (out, _err) = set_customization_status( GuestCustStateEnum.GUESTCUST_STATE_RUNNING, GuestCustEventEnum.GUESTCUST_EVENT_QUERY_NICS, nics) diff --git a/cloudinit/sources/tests/test_init.py b/cloudinit/sources/tests/test_init.py index e7fda22a..452e9219 100644 --- a/cloudinit/sources/tests/test_init.py +++ b/cloudinit/sources/tests/test_init.py @@ -278,7 +278,7 @@ class TestDataSource(CiTestCase): base_args = get_args(DataSource.get_hostname) # pylint: disable=W1505 # Import all DataSource subclasses so we can inspect them. modules = util.find_modules(os.path.dirname(os.path.dirname(__file__))) - for loc, name in modules.items(): + for _loc, name in modules.items(): mod_locs, _ = importer.find_module(name, ['cloudinit.sources'], []) if mod_locs: importer.import_module(mod_locs[0]) diff --git a/cloudinit/templater.py b/cloudinit/templater.py index 9a087e1c..7e7acb86 100644 --- a/cloudinit/templater.py +++ b/cloudinit/templater.py @@ -147,7 +147,7 @@ def render_string(content, params): Warning: py2 str with non-ascii chars will cause UnicodeDecodeError.""" if not params: params = {} - template_type, renderer, content = detect_template(content) + _template_type, renderer, content = detect_template(content) return renderer(content, params) # vi: ts=4 expandtab diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py index 82fd347b..5aada6e7 100644 --- a/cloudinit/tests/helpers.py +++ b/cloudinit/tests/helpers.py @@ -334,7 +334,7 @@ def dir2dict(startdir, prefix=None): flist = {} if prefix is None: prefix = startdir - for root, dirs, files in os.walk(startdir): + for root, _dirs, files in os.walk(startdir): for fname in files: fpath = os.path.join(root, fname) key = fpath[len(prefix):] diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py index 3f37dbb6..76eed076 100644 --- a/cloudinit/tests/test_util.py +++ b/cloudinit/tests/test_util.py @@ -135,7 +135,7 @@ class TestGetHostnameFqdn(CiTestCase): def test_get_hostname_fqdn_from_passes_metadata_only_to_cloud(self): """Calls to cloud.get_hostname pass the metadata_only parameter.""" mycloud = FakeCloud('cloudhost', 'cloudhost.mycloud.com') - hostname, fqdn = util.get_hostname_fqdn( + _hn, _fqdn = util.get_hostname_fqdn( cfg={}, cloud=mycloud, metadata_only=True) self.assertEqual( [{'fqdn': True, 'metadata_only': True}, diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 03a573af..1de07b1c 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -519,7 +519,7 @@ def oauth_headers(url, consumer_key, token_key, token_secret, consumer_secret, resource_owner_secret=token_secret, signature_method=oauth1.SIGNATURE_PLAINTEXT, timestamp=timestamp) - uri, signed_headers, body = client.sign(url) + _uri, signed_headers, _body = client.sign(url) return signed_headers # vi: ts=4 expandtab diff --git a/cloudinit/util.py b/cloudinit/util.py index 1717b529..310758dd 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -2214,7 +2214,7 @@ def parse_mtab(path): def find_freebsd_part(label_part): if label_part.startswith("/dev/label/"): target_label = label_part[5:] - (label_part, err) = subp(['glabel', 'status', '-s']) + (label_part, _err) = subp(['glabel', 'status', '-s']) for labels in label_part.split("\n"): items = labels.split() if len(items) > 0 and items[0].startswith(target_label): diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py index b9cfcfa6..f04d0cd4 100644 --- a/tests/cloud_tests/bddeb.py +++ b/tests/cloud_tests/bddeb.py @@ -113,7 +113,7 @@ def bddeb(args): @return_value: fail count """ LOG.info('preparing to build cloud-init deb') - (res, failed) = run_stage('build deb', [partial(setup_build, args)]) + _res, failed = run_stage('build deb', [partial(setup_build, args)]) return failed # vi: ts=4 expandtab diff --git a/tests/cloud_tests/collect.py b/tests/cloud_tests/collect.py index d4f9135b..1ba72856 100644 --- a/tests/cloud_tests/collect.py +++ b/tests/cloud_tests/collect.py @@ -25,7 +25,8 @@ def collect_script(instance, base_dir, script, script_name): script.encode(), rcs=False, description='collect: {}'.format(script_name)) if err: - LOG.debug("collect script %s had stderr: %s", script_name, err) + LOG.debug("collect script %s exited '%s' and had stderr: %s", + script_name, err, exit) if not isinstance(out, bytes): raise util.PlatformError( "Collection of '%s' returned type %s, expected bytes: %s" % diff --git a/tests/cloud_tests/platforms/instances.py b/tests/cloud_tests/platforms/instances.py index 3bad021f..cc439d29 100644 --- a/tests/cloud_tests/platforms/instances.py +++ b/tests/cloud_tests/platforms/instances.py @@ -108,7 +108,7 @@ class Instance(TargetBase): return client except (ConnectionRefusedError, AuthenticationException, BadHostKeyException, ConnectionResetError, SSHException, - OSError) as e: + OSError): retries -= 1 time.sleep(10) diff --git a/tests/cloud_tests/platforms/lxd/instance.py b/tests/cloud_tests/platforms/lxd/instance.py index 0d957bca..1c17c781 100644 --- a/tests/cloud_tests/platforms/lxd/instance.py +++ b/tests/cloud_tests/platforms/lxd/instance.py @@ -152,9 +152,8 @@ class LXDInstance(Instance): return fp.read() try: - stdout, stderr = subp( - ['lxc', 'console', '--show-log', self.name], decode=False) - return stdout + return subp(['lxc', 'console', '--show-log', self.name], + decode=False)[0] except ProcessExecutionError as e: raise PlatformError( "console log", @@ -214,11 +213,10 @@ def _has_proper_console_support(): reason = "LXD Driver version not 3.x+ (%s)" % dver else: try: - stdout, stderr = subp(['lxc', 'console', '--help'], - decode=False) + stdout = subp(['lxc', 'console', '--help'], decode=False)[0] if not (b'console' in stdout and b'log' in stdout): reason = "no '--log' in lxc console --help" - except ProcessExecutionError as e: + except ProcessExecutionError: reason = "no 'console' command in lxc client" if reason: diff --git a/tests/cloud_tests/setup_image.py b/tests/cloud_tests/setup_image.py index 6d242115..4e195709 100644 --- a/tests/cloud_tests/setup_image.py +++ b/tests/cloud_tests/setup_image.py @@ -25,10 +25,9 @@ def installed_package_version(image, package, ensure_installed=True): else: raise NotImplementedError - msg = 'query version for package: {}'.format(package) - (out, err, exit) = image.execute( - cmd, description=msg, rcs=(0,) if ensure_installed else range(0, 256)) - return out.strip() + return image.execute( + cmd, description='query version for package: {}'.format(package), + rcs=(0,) if ensure_installed else range(0, 256))[0].strip() def install_deb(args, image): @@ -54,7 +53,7 @@ def install_deb(args, image): remote_path], description=msg) # check installed deb version matches package fmt = ['-W', "--showformat=${Version}"] - (out, err, exit) = image.execute(['dpkg-deb'] + fmt + [remote_path]) + out = image.execute(['dpkg-deb'] + fmt + [remote_path])[0] expected_version = out.strip() found_version = installed_package_version(image, 'cloud-init') if expected_version != found_version: @@ -85,7 +84,7 @@ def install_rpm(args, image): image.execute(['rpm', '-U', remote_path], description=msg) fmt = ['--queryformat', '"%{VERSION}"'] - (out, err, exit) = image.execute(['rpm', '-q'] + fmt + [remote_path]) + (out, _err, _exit) = image.execute(['rpm', '-q'] + fmt + [remote_path]) expected_version = out.strip() found_version = installed_package_version(image, 'cloud-init') if expected_version != found_version: diff --git a/tests/cloud_tests/testcases/base.py b/tests/cloud_tests/testcases/base.py index 4fda8f91..0d1916b4 100644 --- a/tests/cloud_tests/testcases/base.py +++ b/tests/cloud_tests/testcases/base.py @@ -159,7 +159,7 @@ class CloudTestCase(unittest.TestCase): expected_net_keys = [ 'public-ipv4s', 'ipv4-associations', 'local-hostname', 'public-hostname'] - for mac, mac_data in macs.items(): + for mac_data in macs.values(): for key in expected_net_keys: self.assertIn(key, mac_data) self.assertIsNotNone( diff --git a/tests/cloud_tests/testcases/examples/including_user_groups.py b/tests/cloud_tests/testcases/examples/including_user_groups.py index 93b7a82d..4067348d 100644 --- a/tests/cloud_tests/testcases/examples/including_user_groups.py +++ b/tests/cloud_tests/testcases/examples/including_user_groups.py @@ -42,7 +42,7 @@ class TestUserGroups(base.CloudTestCase): def test_user_root_in_secret(self): """Test root user is in 'secret' group.""" - user, _, groups = self.get_data_file('root_groups').partition(":") + _user, _, groups = self.get_data_file('root_groups').partition(":") self.assertIn("secret", groups.split(), msg="User root is not in group 'secret'") diff --git a/tests/cloud_tests/testcases/modules/user_groups.py b/tests/cloud_tests/testcases/modules/user_groups.py index 93b7a82d..4067348d 100644 --- a/tests/cloud_tests/testcases/modules/user_groups.py +++ b/tests/cloud_tests/testcases/modules/user_groups.py @@ -42,7 +42,7 @@ class TestUserGroups(base.CloudTestCase): def test_user_root_in_secret(self): """Test root user is in 'secret' group.""" - user, _, groups = self.get_data_file('root_groups').partition(":") + _user, _, groups = self.get_data_file('root_groups').partition(":") self.assertIn("secret", groups.split(), msg="User root is not in group 'secret'") diff --git a/tests/cloud_tests/util.py b/tests/cloud_tests/util.py index 3dd4996d..06f7d865 100644 --- a/tests/cloud_tests/util.py +++ b/tests/cloud_tests/util.py @@ -358,7 +358,7 @@ class TargetBase(object): # when sh is invoked with '-c', then the first argument is "$0" # which is commonly understood as the "program name". # 'read_data' is the program name, and 'remote_path' is '$1' - stdout, stderr, rc = self._execute( + stdout, _stderr, rc = self._execute( ["sh", "-c", 'exec cat "$1"', 'read_data', remote_path]) if rc != 0: raise RuntimeError("Failed to read file '%s'" % remote_path) diff --git a/tests/unittests/test__init__.py b/tests/unittests/test__init__.py index 25878d7a..f1ab02e9 100644 --- a/tests/unittests/test__init__.py +++ b/tests/unittests/test__init__.py @@ -214,7 +214,7 @@ class TestCmdlineUrl(CiTestCase): def test_no_key_found(self, m_read): cmdline = "ro mykey=http://example.com/foo root=foo" fpath = self.tmp_path("ccpath") - lvl, msg = main.attempt_cmdline_url( + lvl, _msg = main.attempt_cmdline_url( fpath, network=True, cmdline=cmdline) m_read.assert_not_called() diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py index 3e8b7913..88fe76c7 100644 --- a/tests/unittests/test_datasource/test_azure.py +++ b/tests/unittests/test_datasource/test_azure.py @@ -214,7 +214,7 @@ scbus-1 on xpt0 bus 0 self.assertIn(tag, x) def tags_equal(x, y): - for x_tag, x_val in x.items(): + for x_val in x.values(): y_val = y.get(x_val.tag) self.assertEqual(x_val.text, y_val.text) @@ -1216,7 +1216,7 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): fake_resp.return_value = mock.MagicMock(status_code=200, text=content, content=content) dsa = dsaz.DataSourceAzure({}, distro=None, paths=self.paths) - md, ud, cfg, d = dsa._reprovision() + md, _ud, cfg, _d = dsa._reprovision() self.assertEqual(md['local-hostname'], hostname) self.assertEqual(cfg['system_info']['default_user']['name'], username) self.assertEqual(fake_resp.call_args_list, diff --git a/tests/unittests/test_datasource/test_maas.py b/tests/unittests/test_datasource/test_maas.py index 6e4031cf..c84d067e 100644 --- a/tests/unittests/test_datasource/test_maas.py +++ b/tests/unittests/test_datasource/test_maas.py @@ -53,7 +53,7 @@ class TestMAASDataSource(CiTestCase): my_d = os.path.join(self.tmp, "valid_extra") populate_dir(my_d, data) - ud, md, vd = DataSourceMAAS.read_maas_seed_dir(my_d) + ud, md, _vd = DataSourceMAAS.read_maas_seed_dir(my_d) self.assertEqual(userdata, ud) for key in ('instance-id', 'local-hostname'): @@ -149,7 +149,7 @@ class TestMAASDataSource(CiTestCase): 'meta-data/local-hostname': 'test-hostname', 'meta-data/vendor-data': yaml.safe_dump(expected_vd).encode(), } - ud, md, vd = self.mock_read_maas_seed_url( + _ud, md, vd = self.mock_read_maas_seed_url( valid, "http://example.com/foo") self.assertEqual(valid['meta-data/instance-id'], md['instance-id']) diff --git a/tests/unittests/test_datasource/test_nocloud.py b/tests/unittests/test_datasource/test_nocloud.py index 70d50de4..cdbd1e1a 100644 --- a/tests/unittests/test_datasource/test_nocloud.py +++ b/tests/unittests/test_datasource/test_nocloud.py @@ -51,9 +51,6 @@ class TestNoCloudDataSource(CiTestCase): class PsuedoException(Exception): pass - def my_find_devs_with(*args, **kwargs): - raise PsuedoException - self.mocks.enter_context( mock.patch.object(util, 'find_devs_with', side_effect=PsuedoException)) diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index 7bb1b7c4..e486862d 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -528,7 +528,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): expected = sorted([npre + suff for opre, npre, suff in files]) # create files - for (opre, npre, suff) in files: + for (opre, _npre, suff) in files: fpath = os.path.join(apt_lists_d, opre + suff) util.write_file(fpath, content=fpath) diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py index 02676aa6..17c53559 100644 --- a/tests/unittests/test_handler/test_handler_ntp.py +++ b/tests/unittests/test_handler/test_handler_ntp.py @@ -76,7 +76,7 @@ class TestNtp(FilesystemMockingTestCase): template = TIMESYNCD_TEMPLATE else: template = NTP_TEMPLATE - (confpath, template_fn) = self._generate_template(template=template) + (confpath, _template_fn) = self._generate_template(template=template) ntpconfig = copy.deepcopy(dcfg[client]) ntpconfig['confpath'] = confpath ntpconfig['template_name'] = os.path.basename(confpath) diff --git a/tests/unittests/test_templating.py b/tests/unittests/test_templating.py index 1080e135..20c87efa 100644 --- a/tests/unittests/test_templating.py +++ b/tests/unittests/test_templating.py @@ -50,12 +50,12 @@ class TestTemplates(test_helpers.CiTestCase): def test_detection(self): blob = "## template:cheetah" - (template_type, renderer, contents) = templater.detect_template(blob) + (template_type, _renderer, contents) = templater.detect_template(blob) self.assertIn("cheetah", template_type) self.assertEqual("", contents.strip()) blob = "blahblah $blah" - (template_type, renderer, contents) = templater.detect_template(blob) + (template_type, _renderer, _contents) = templater.detect_template(blob) self.assertIn("cheetah", template_type) self.assertEqual(blob, contents) diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index e04ea031..84941c7d 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -774,11 +774,11 @@ class TestSubp(helpers.CiTestCase): def test_subp_reads_env(self): with mock.patch.dict("os.environ", values={'FOO': 'BAR'}): - out, err = util.subp(self.printenv + ['FOO'], capture=True) + out, _err = util.subp(self.printenv + ['FOO'], capture=True) self.assertEqual('FOO=BAR', out.splitlines()[0]) def test_subp_env_and_update_env(self): - out, err = util.subp( + out, _err = util.subp( self.printenv + ['FOO', 'HOME', 'K1', 'K2'], capture=True, env={'FOO': 'BAR'}, update_env={'HOME': '/myhome', 'K2': 'V2'}) @@ -788,7 +788,7 @@ class TestSubp(helpers.CiTestCase): def test_subp_update_env(self): extra = {'FOO': 'BAR', 'HOME': '/root', 'K1': 'V1'} with mock.patch.dict("os.environ", values=extra): - out, err = util.subp( + out, _err = util.subp( self.printenv + ['FOO', 'HOME', 'K1', 'K2'], capture=True, update_env={'HOME': '/myhome', 'K2': 'V2'}) -- cgit v1.2.3 From 4952a8545b61ceb2fe26a933d2f64020d0281703 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 20 Apr 2018 12:22:23 -0600 Subject: set_passwords: Add newline to end of sshd config, only restart if updated. This admittedly does a fairly extensive re-factor to simply add a newline to the end of sshd_config. It makes the ssh_config updating portion of set_passwords more testable and adds tests for that. The new function is in 'update_ssh_config_lines' which allows you to update a config with multiple changes even though only a single one is currently used. We also only restart the ssh daemon now if a change was made to the config file. Before it was always restarted if the user specified a value for ssh_pwauth other than 'unchanged'. Thanks to Lorens Kockum for initial diagnosis and patch. LP: #1677205 --- cloudinit/config/cc_set_passwords.py | 105 ++++++++++++--------------- cloudinit/config/tests/test_set_passwords.py | 71 ++++++++++++++++++ cloudinit/ssh_util.py | 70 ++++++++++++++++-- tests/unittests/test_sshutil.py | 97 ++++++++++++++++++++++++- 4 files changed, 273 insertions(+), 70 deletions(-) create mode 100644 cloudinit/config/tests/test_set_passwords.py (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py index bb24d57f..5ef97376 100755 --- a/cloudinit/config/cc_set_passwords.py +++ b/cloudinit/config/cc_set_passwords.py @@ -68,16 +68,57 @@ import re import sys from cloudinit.distros import ug_util -from cloudinit import ssh_util +from cloudinit import log as logging +from cloudinit.ssh_util import update_ssh_config from cloudinit import util from string import ascii_letters, digits +LOG = logging.getLogger(__name__) + # We are removing certain 'painful' letters/numbers PW_SET = (''.join([x for x in ascii_letters + digits if x not in 'loLOI01'])) +def handle_ssh_pwauth(pw_auth, service_cmd=None, service_name="ssh"): + """Apply sshd PasswordAuthentication changes. + + @param pw_auth: config setting from 'pw_auth'. + Best given as True, False, or "unchanged". + @param service_cmd: The service command list (['service']) + @param service_name: The name of the sshd service for the system. + + @return: None""" + cfg_name = "PasswordAuthentication" + if service_cmd is None: + service_cmd = ["service"] + + if util.is_true(pw_auth): + cfg_val = 'yes' + elif util.is_false(pw_auth): + cfg_val = 'no' + else: + bmsg = "Leaving ssh config '%s' unchanged." % cfg_name + if pw_auth is None or pw_auth.lower() == 'unchanged': + LOG.debug("%s ssh_pwauth=%s", bmsg, pw_auth) + else: + LOG.warning("%s Unrecognized value: ssh_pwauth=%s", bmsg, pw_auth) + return + + updated = update_ssh_config({cfg_name: cfg_val}) + if not updated: + LOG.debug("No need to restart ssh service, %s not updated.", cfg_name) + return + + if 'systemctl' in service_cmd: + cmd = list(service_cmd) + ["restart", service_name] + else: + cmd = list(service_cmd) + [service_name, "restart"] + util.subp(cmd) + LOG.debug("Restarted the ssh daemon.") + + def handle(_name, cfg, cloud, log, args): if len(args) != 0: # if run from command line, and give args, wipe the chpasswd['list'] @@ -170,65 +211,9 @@ def handle(_name, cfg, cloud, log, args): if expired_users: log.debug("Expired passwords for: %s users", expired_users) - change_pwauth = False - pw_auth = None - if 'ssh_pwauth' in cfg: - if util.is_true(cfg['ssh_pwauth']): - change_pwauth = True - pw_auth = 'yes' - elif util.is_false(cfg['ssh_pwauth']): - change_pwauth = True - pw_auth = 'no' - elif str(cfg['ssh_pwauth']).lower() == 'unchanged': - log.debug('Leaving auth line unchanged') - change_pwauth = False - elif not str(cfg['ssh_pwauth']).strip(): - log.debug('Leaving auth line unchanged') - change_pwauth = False - elif not cfg['ssh_pwauth']: - log.debug('Leaving auth line unchanged') - change_pwauth = False - else: - msg = 'Unrecognized value %s for ssh_pwauth' % cfg['ssh_pwauth'] - util.logexc(log, msg) - - if change_pwauth: - replaced_auth = False - - # See: man sshd_config - old_lines = ssh_util.parse_ssh_config(ssh_util.DEF_SSHD_CFG) - new_lines = [] - i = 0 - for (i, line) in enumerate(old_lines): - # Keywords are case-insensitive and arguments are case-sensitive - if line.key == 'passwordauthentication': - log.debug("Replacing auth line %s with %s", i + 1, pw_auth) - replaced_auth = True - line.value = pw_auth - new_lines.append(line) - - if not replaced_auth: - log.debug("Adding new auth line %s", i + 1) - replaced_auth = True - new_lines.append(ssh_util.SshdConfigLine('', - 'PasswordAuthentication', - pw_auth)) - - lines = [str(l) for l in new_lines] - util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines), - copy_mode=True) - - try: - cmd = cloud.distro.init_cmd # Default service - cmd.append(cloud.distro.get_option('ssh_svcname', 'ssh')) - cmd.append('restart') - if 'systemctl' in cmd: # Switch action ordering - cmd[1], cmd[2] = cmd[2], cmd[1] - cmd = filter(None, cmd) # Remove empty arguments - util.subp(cmd) - log.debug("Restarted the ssh daemon") - except Exception: - util.logexc(log, "Restarting of the ssh daemon failed") + handle_ssh_pwauth( + cfg.get('ssh_pwauth'), service_cmd=cloud.distro.init_cmd, + service_name=cloud.distro.get_option('ssh_svcname', 'ssh')) if len(errors): log.debug("%s errors occured, re-raising the last one", len(errors)) diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py new file mode 100644 index 00000000..b051ec82 --- /dev/null +++ b/cloudinit/config/tests/test_set_passwords.py @@ -0,0 +1,71 @@ +# This file is part of cloud-init. See LICENSE file for license information. + +import mock + +from cloudinit.config import cc_set_passwords as setpass +from cloudinit.tests.helpers import CiTestCase +from cloudinit import util + +MODPATH = "cloudinit.config.cc_set_passwords." + + +class TestHandleSshPwauth(CiTestCase): + """Test cc_set_passwords handling of ssh_pwauth in handle_ssh_pwauth.""" + + with_logs = True + + @mock.patch(MODPATH + "util.subp") + def test_unknown_value_logs_warning(self, m_subp): + setpass.handle_ssh_pwauth("floo") + self.assertIn("Unrecognized value: ssh_pwauth=floo", + self.logs.getvalue()) + m_subp.assert_not_called() + + @mock.patch(MODPATH + "update_ssh_config", return_value=True) + @mock.patch(MODPATH + "util.subp") + def test_systemctl_as_service_cmd(self, m_subp, m_update_ssh_config): + """If systemctl in service cmd: systemctl restart name.""" + setpass.handle_ssh_pwauth( + True, service_cmd=["systemctl"], service_name="myssh") + self.assertEqual(mock.call(["systemctl", "restart", "myssh"]), + m_subp.call_args) + + @mock.patch(MODPATH + "update_ssh_config", return_value=True) + @mock.patch(MODPATH + "util.subp") + def test_service_as_service_cmd(self, m_subp, m_update_ssh_config): + """If systemctl in service cmd: systemctl restart name.""" + setpass.handle_ssh_pwauth( + True, service_cmd=["service"], service_name="myssh") + self.assertEqual(mock.call(["service", "myssh", "restart"]), + m_subp.call_args) + + @mock.patch(MODPATH + "update_ssh_config", return_value=False) + @mock.patch(MODPATH + "util.subp") + def test_not_restarted_if_not_updated(self, m_subp, m_update_ssh_config): + """If config is not updated, then no system restart should be done.""" + setpass.handle_ssh_pwauth(True) + m_subp.assert_not_called() + self.assertIn("No need to restart ssh", self.logs.getvalue()) + + @mock.patch(MODPATH + "update_ssh_config", return_value=True) + @mock.patch(MODPATH + "util.subp") + def test_unchanged_does_nothing(self, m_subp, m_update_ssh_config): + """If 'unchanged', then no updates to config and no restart.""" + setpass.handle_ssh_pwauth( + "unchanged", service_cmd=["systemctl"], service_name="myssh") + m_update_ssh_config.assert_not_called() + m_subp.assert_not_called() + + @mock.patch(MODPATH + "util.subp") + def test_valid_change_values(self, m_subp): + """If value is a valid changen value, then update should be called.""" + upname = MODPATH + "update_ssh_config" + optname = "PasswordAuthentication" + for value in util.FALSE_STRINGS + util.TRUE_STRINGS: + optval = "yes" if value in util.TRUE_STRINGS else "no" + with mock.patch(upname, return_value=False) as m_update: + setpass.handle_ssh_pwauth(value) + m_update.assert_called_with({optname: optval}) + m_subp.assert_not_called() + +# vi: ts=4 expandtab diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 882517f5..73c31772 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -279,24 +279,28 @@ class SshdConfigLine(object): def parse_ssh_config(fname): + if not os.path.isfile(fname): + return [] + return parse_ssh_config_lines(util.load_file(fname).splitlines()) + + +def parse_ssh_config_lines(lines): # See: man sshd_config # The file contains keyword-argument pairs, one per line. # Lines starting with '#' and empty lines are interpreted as comments. # Note: key-words are case-insensitive and arguments are case-sensitive - lines = [] - if not os.path.isfile(fname): - return lines - for line in util.load_file(fname).splitlines(): + ret = [] + for line in lines: line = line.strip() if not line or line.startswith("#"): - lines.append(SshdConfigLine(line)) + ret.append(SshdConfigLine(line)) continue try: key, val = line.split(None, 1) except ValueError: key, val = line.split('=', 1) - lines.append(SshdConfigLine(line, key, val)) - return lines + ret.append(SshdConfigLine(line, key, val)) + return ret def parse_ssh_config_map(fname): @@ -310,4 +314,56 @@ def parse_ssh_config_map(fname): ret[line.key] = line.value return ret + +def update_ssh_config(updates, fname=DEF_SSHD_CFG): + """Read fname, and update if changes are necessary. + + @param updates: dictionary of desired values {Option: value} + @return: boolean indicating if an update was done.""" + lines = parse_ssh_config(fname) + changed = update_ssh_config_lines(lines=lines, updates=updates) + if changed: + util.write_file( + fname, "\n".join([str(l) for l in lines]) + "\n", copy_mode=True) + return len(changed) != 0 + + +def update_ssh_config_lines(lines, updates): + """Update the ssh config lines per updates. + + @param lines: array of SshdConfigLine. This array is updated in place. + @param updates: dictionary of desired values {Option: value} + @return: A list of keys in updates that were changed.""" + found = set() + changed = [] + + # Keywords are case-insensitive and arguments are case-sensitive + casemap = dict([(k.lower(), k) for k in updates.keys()]) + + for (i, line) in enumerate(lines, start=1): + if not line.key: + continue + if line.key in casemap: + key = casemap[line.key] + value = updates[key] + found.add(key) + if line.value == value: + LOG.debug("line %d: option %s already set to %s", + i, key, value) + else: + changed.append(key) + LOG.debug("line %d: option %s updated %s -> %s", i, + key, line.value, value) + line.value = value + + if len(found) != len(updates): + for key, value in updates.items(): + if key in found: + continue + changed.append(key) + lines.append(SshdConfigLine('', key, value)) + LOG.debug("line %d: option %s added with %s", + len(lines), key, value) + return changed + # vi: ts=4 expandtab diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py index 4c62c8be..73ae897f 100644 --- a/tests/unittests/test_sshutil.py +++ b/tests/unittests/test_sshutil.py @@ -4,6 +4,7 @@ from mock import patch from cloudinit import ssh_util from cloudinit.tests import helpers as test_helpers +from cloudinit import util VALID_CONTENT = { @@ -56,7 +57,7 @@ TEST_OPTIONS = ( 'user \"root\".\';echo;sleep 10"') -class TestAuthKeyLineParser(test_helpers.TestCase): +class TestAuthKeyLineParser(test_helpers.CiTestCase): def test_simple_parse(self): # test key line with common 3 fields (keytype, base64, comment) @@ -126,7 +127,7 @@ class TestAuthKeyLineParser(test_helpers.TestCase): self.assertFalse(key.valid()) -class TestUpdateAuthorizedKeys(test_helpers.TestCase): +class TestUpdateAuthorizedKeys(test_helpers.CiTestCase): def test_new_keys_replace(self): """new entries with the same base64 should replace old.""" @@ -168,7 +169,7 @@ class TestUpdateAuthorizedKeys(test_helpers.TestCase): self.assertEqual(expected, found) -class TestParseSSHConfig(test_helpers.TestCase): +class TestParseSSHConfig(test_helpers.CiTestCase): def setUp(self): self.load_file_patch = patch('cloudinit.ssh_util.util.load_file') @@ -235,4 +236,94 @@ class TestParseSSHConfig(test_helpers.TestCase): self.assertEqual('foo', ret[0].key) self.assertEqual('bar', ret[0].value) + +class TestUpdateSshConfigLines(test_helpers.CiTestCase): + """Test the update_ssh_config_lines method.""" + exlines = [ + "#PasswordAuthentication yes", + "UsePAM yes", + "# Comment line", + "AcceptEnv LANG LC_*", + "X11Forwarding no", + ] + pwauth = "PasswordAuthentication" + + def check_line(self, line, opt, val): + self.assertEqual(line.key, opt.lower()) + self.assertEqual(line.value, val) + self.assertIn(opt, str(line)) + self.assertIn(val, str(line)) + + def test_new_option_added(self): + """A single update of non-existing option.""" + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, {'MyKey': 'MyVal'}) + self.assertEqual(['MyKey'], result) + self.check_line(lines[-1], "MyKey", "MyVal") + + def test_commented_out_not_updated_but_appended(self): + """Implementation does not un-comment and update lines.""" + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, {self.pwauth: "no"}) + self.assertEqual([self.pwauth], result) + self.check_line(lines[-1], self.pwauth, "no") + + def test_single_option_updated(self): + """A single update should have change made and line updated.""" + opt, val = ("UsePAM", "no") + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, {opt: val}) + self.assertEqual([opt], result) + self.check_line(lines[1], opt, val) + + def test_multiple_updates_with_add(self): + """Verify multiple updates some added some changed, some not.""" + updates = {"UsePAM": "no", "X11Forwarding": "no", "NewOpt": "newval", + "AcceptEnv": "LANG ADD LC_*"} + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, updates) + self.assertEqual(set(["UsePAM", "NewOpt", "AcceptEnv"]), set(result)) + self.check_line(lines[3], "AcceptEnv", updates["AcceptEnv"]) + + def test_return_empty_if_no_changes(self): + """If there are no changes, then return should be empty list.""" + updates = {"UsePAM": "yes"} + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, updates) + self.assertEqual([], result) + self.assertEqual(self.exlines, [str(l) for l in lines]) + + def test_keycase_not_modified(self): + """Original case of key should not be changed on update. + This behavior is to keep original config as much intact as can be.""" + updates = {"usepam": "no"} + lines = ssh_util.parse_ssh_config_lines(list(self.exlines)) + result = ssh_util.update_ssh_config_lines(lines, updates) + self.assertEqual(["usepam"], result) + self.assertEqual("UsePAM no", str(lines[1])) + + +class TestUpdateSshConfig(test_helpers.CiTestCase): + cfgdata = '\n'.join(["#Option val", "MyKey ORIG_VAL", ""]) + + def test_modified(self): + mycfg = self.tmp_path("ssh_config_1") + util.write_file(mycfg, self.cfgdata) + ret = ssh_util.update_ssh_config({"MyKey": "NEW_VAL"}, mycfg) + self.assertTrue(ret) + found = util.load_file(mycfg) + self.assertEqual(self.cfgdata.replace("ORIG_VAL", "NEW_VAL"), found) + # assert there is a newline at end of file (LP: #1677205) + self.assertEqual('\n', found[-1]) + + def test_not_modified(self): + mycfg = self.tmp_path("ssh_config_2") + util.write_file(mycfg, self.cfgdata) + with patch("cloudinit.ssh_util.util.write_file") as m_write_file: + ret = ssh_util.update_ssh_config({"MyKey": "ORIG_VAL"}, mycfg) + self.assertFalse(ret) + self.assertEqual(self.cfgdata, util.load_file(mycfg)) + m_write_file.assert_not_called() + + # vi: ts=4 expandtab -- cgit v1.2.3 From 5037252ca5dc54c6d978b23dba063ac5fabc98fa Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 20 Apr 2018 20:25:48 -0400 Subject: schema: in validation, raise ImportError if strict but no jsonschema. validate_cloudconfig_schema with strict=True would not actually validate if there was no jsonschema available. That seems kind of strange. The change here is to make it raise an exception if strict was passed in. And then to fix the one test that needed a skipIfJsonSchema wrapper. --- cloudinit/config/tests/test_snap.py | 41 +++++++----------- cloudinit/config/tests/test_ubuntu_advantage.py | 30 +++++++++++++- cloudinit/tests/helpers.py | 19 +++++++++ .../unittests/test_handler/test_handler_bootcmd.py | 42 +++++++++---------- .../unittests/test_handler/test_handler_runcmd.py | 48 +++++++++++----------- 5 files changed, 104 insertions(+), 76 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/tests/test_snap.py b/cloudinit/config/tests/test_snap.py index 492d2d46..34c80f1e 100644 --- a/cloudinit/config/tests/test_snap.py +++ b/cloudinit/config/tests/test_snap.py @@ -9,7 +9,7 @@ from cloudinit.config.cc_snap import ( from cloudinit.config.schema import validate_cloudconfig_schema from cloudinit import util from cloudinit.tests.helpers import ( - CiTestCase, mock, wrap_and_call, skipUnlessJsonSchema) + CiTestCase, SchemaTestCaseMixin, mock, wrap_and_call, skipUnlessJsonSchema) SYSTEM_USER_ASSERTION = """\ @@ -245,9 +245,10 @@ class TestRunCommands(CiTestCase): @skipUnlessJsonSchema() -class TestSchema(CiTestCase): +class TestSchema(CiTestCase, SchemaTestCaseMixin): with_logs = True + schema = schema def test_schema_warns_on_snap_not_as_dict(self): """If the snap configuration is not a dict, emit a warning.""" @@ -342,39 +343,27 @@ class TestSchema(CiTestCase): def test_duplicates_are_fine_array_array(self): """Duplicated commands array/array entries are allowed.""" - byebye = ["echo", "bye"] - try: - cfg = {'snap': {'commands': [byebye, byebye]}} - validate_cloudconfig_schema(cfg, schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("command entries can be duplicate.") + self.assertSchemaValid( + {'commands': [["echo", "bye"], ["echo" "bye"]]}, + "command entries can be duplicate.") def test_duplicates_are_fine_array_string(self): """Duplicated commands array/string entries are allowed.""" - byebye = "echo bye" - try: - cfg = {'snap': {'commands': [byebye, byebye]}} - validate_cloudconfig_schema(cfg, schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("command entries can be duplicate.") + self.assertSchemaValid( + {'commands': ["echo bye", "echo bye"]}, + "command entries can be duplicate.") def test_duplicates_are_fine_dict_array(self): """Duplicated commands dict/array entries are allowed.""" - byebye = ["echo", "bye"] - try: - cfg = {'snap': {'commands': {'00': byebye, '01': byebye}}} - validate_cloudconfig_schema(cfg, schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("command entries can be duplicate.") + self.assertSchemaValid( + {'commands': {'00': ["echo", "bye"], '01': ["echo", "bye"]}}, + "command entries can be duplicate.") def test_duplicates_are_fine_dict_string(self): """Duplicated commands dict/string entries are allowed.""" - byebye = "echo bye" - try: - cfg = {'snap': {'commands': {'00': byebye, '01': byebye}}} - validate_cloudconfig_schema(cfg, schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("command entries can be duplicate.") + self.assertSchemaValid( + {'commands': {'00': "echo bye", '01': "echo bye"}}, + "command entries can be duplicate.") class TestHandle(CiTestCase): diff --git a/cloudinit/config/tests/test_ubuntu_advantage.py b/cloudinit/config/tests/test_ubuntu_advantage.py index f2a59faf..f1beeff8 100644 --- a/cloudinit/config/tests/test_ubuntu_advantage.py +++ b/cloudinit/config/tests/test_ubuntu_advantage.py @@ -7,7 +7,8 @@ from cloudinit.config.cc_ubuntu_advantage import ( handle, maybe_install_ua_tools, run_commands, schema) from cloudinit.config.schema import validate_cloudconfig_schema from cloudinit import util -from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJsonSchema +from cloudinit.tests.helpers import ( + CiTestCase, mock, SchemaTestCaseMixin, skipUnlessJsonSchema) # Module path used in mocks @@ -105,9 +106,10 @@ class TestRunCommands(CiTestCase): @skipUnlessJsonSchema() -class TestSchema(CiTestCase): +class TestSchema(CiTestCase, SchemaTestCaseMixin): with_logs = True + schema = schema def test_schema_warns_on_ubuntu_advantage_not_as_dict(self): """If ubuntu-advantage configuration is not a dict, emit a warning.""" @@ -169,6 +171,30 @@ class TestSchema(CiTestCase): {'ubuntu-advantage': {'commands': {'01': 'also valid'}}}, schema) self.assertEqual('', self.logs.getvalue()) + def test_duplicates_are_fine_array_array(self): + """Duplicated commands array/array entries are allowed.""" + self.assertSchemaValid( + {'commands': [["echo", "bye"], ["echo" "bye"]]}, + "command entries can be duplicate.") + + def test_duplicates_are_fine_array_string(self): + """Duplicated commands array/string entries are allowed.""" + self.assertSchemaValid( + {'commands': ["echo bye", "echo bye"]}, + "command entries can be duplicate.") + + def test_duplicates_are_fine_dict_array(self): + """Duplicated commands dict/array entries are allowed.""" + self.assertSchemaValid( + {'commands': {'00': ["echo", "bye"], '01': ["echo", "bye"]}}, + "command entries can be duplicate.") + + def test_duplicates_are_fine_dict_string(self): + """Duplicated commands dict/string entries are allowed.""" + self.assertSchemaValid( + {'commands': {'00': "echo bye", '01': "echo bye"}}, + "command entries can be duplicate.") + class TestHandle(CiTestCase): diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py index 5aada6e7..4999f1f6 100644 --- a/cloudinit/tests/helpers.py +++ b/cloudinit/tests/helpers.py @@ -24,6 +24,8 @@ try: except ImportError: from ConfigParser import ConfigParser +from cloudinit.config.schema import ( + SchemaValidationError, validate_cloudconfig_schema) from cloudinit import helpers as ch from cloudinit import util @@ -312,6 +314,23 @@ class HttprettyTestCase(CiTestCase): super(HttprettyTestCase, self).tearDown() +class SchemaTestCaseMixin(unittest2.TestCase): + + def assertSchemaValid(self, cfg, msg="Valid Schema failed validation."): + """Assert the config is valid per self.schema. + + If there is only one top level key in the schema properties, then + the cfg will be put under that key.""" + props = list(self.schema.get('properties')) + # put cfg under top level key if there is only one in the schema + if len(props) == 1: + cfg = {props[0]: cfg} + try: + validate_cloudconfig_schema(cfg, self.schema, strict=True) + except SchemaValidationError: + self.fail(msg) + + def populate_dir(path, files): if not os.path.exists(path): os.makedirs(path) diff --git a/tests/unittests/test_handler/test_handler_bootcmd.py b/tests/unittests/test_handler/test_handler_bootcmd.py index c3abedde..b1375269 100644 --- a/tests/unittests/test_handler/test_handler_bootcmd.py +++ b/tests/unittests/test_handler/test_handler_bootcmd.py @@ -1,9 +1,10 @@ # This file is part of cloud-init. See LICENSE file for license information. -from cloudinit.config import cc_bootcmd, schema +from cloudinit.config.cc_bootcmd import handle, schema from cloudinit.sources import DataSourceNone from cloudinit import (distros, helpers, cloud, util) -from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJsonSchema +from cloudinit.tests.helpers import ( + CiTestCase, mock, SchemaTestCaseMixin, skipUnlessJsonSchema) import logging import tempfile @@ -50,7 +51,7 @@ class TestBootcmd(CiTestCase): """When the provided config doesn't contain bootcmd, skip it.""" cfg = {} mycloud = self._get_cloud('ubuntu') - cc_bootcmd.handle('notimportant', cfg, mycloud, LOG, None) + handle('notimportant', cfg, mycloud, LOG, None) self.assertIn( "Skipping module named notimportant, no 'bootcmd' key", self.logs.getvalue()) @@ -60,7 +61,7 @@ class TestBootcmd(CiTestCase): invalid_config = {'bootcmd': 1} cc = self._get_cloud('ubuntu') with self.assertRaises(TypeError) as context_manager: - cc_bootcmd.handle('cc_bootcmd', invalid_config, cc, LOG, []) + handle('cc_bootcmd', invalid_config, cc, LOG, []) self.assertIn('Failed to shellify bootcmd', self.logs.getvalue()) self.assertEqual( "Input to shellify was type 'int'. Expected list or tuple.", @@ -76,7 +77,7 @@ class TestBootcmd(CiTestCase): invalid_config = {'bootcmd': 1} cc = self._get_cloud('ubuntu') with self.assertRaises(TypeError): - cc_bootcmd.handle('cc_bootcmd', invalid_config, cc, LOG, []) + handle('cc_bootcmd', invalid_config, cc, LOG, []) self.assertIn( 'Invalid config:\nbootcmd: 1 is not of type \'array\'', self.logs.getvalue()) @@ -93,7 +94,7 @@ class TestBootcmd(CiTestCase): 'bootcmd': ['ls /', 20, ['wget', 'http://stuff/blah'], {'a': 'n'}]} cc = self._get_cloud('ubuntu') with self.assertRaises(TypeError) as context_manager: - cc_bootcmd.handle('cc_bootcmd', invalid_config, cc, LOG, []) + handle('cc_bootcmd', invalid_config, cc, LOG, []) expected_warnings = [ 'bootcmd.1: 20 is not valid under any of the given schemas', 'bootcmd.3: {\'a\': \'n\'} is not valid under any of the given' @@ -117,7 +118,7 @@ class TestBootcmd(CiTestCase): 'echo {0} $INSTANCE_ID > {1}'.format(my_id, out_file)]} with mock.patch(self._etmpfile_path, FakeExtendedTempFile): - cc_bootcmd.handle('cc_bootcmd', valid_config, cc, LOG, []) + handle('cc_bootcmd', valid_config, cc, LOG, []) self.assertEqual(my_id + ' iid-datasource-none\n', util.load_file(out_file)) @@ -128,7 +129,7 @@ class TestBootcmd(CiTestCase): with mock.patch(self._etmpfile_path, FakeExtendedTempFile): with self.assertRaises(util.ProcessExecutionError) as ctxt_manager: - cc_bootcmd.handle('does-not-matter', valid_config, cc, LOG, []) + handle('does-not-matter', valid_config, cc, LOG, []) self.assertIn( 'Unexpected error while running command.\n' "Command: ['/bin/sh',", @@ -138,26 +139,21 @@ class TestBootcmd(CiTestCase): self.logs.getvalue()) -class TestSchema(CiTestCase): +@skipUnlessJsonSchema() +class TestSchema(CiTestCase, SchemaTestCaseMixin): """Directly test schema rather than through handle.""" + schema = schema + def test_duplicates_are_fine_array_array(self): - """Duplicated array entries are allowed.""" - try: - byebye = ["echo", "bye"] - schema.validate_cloudconfig_schema( - {'bootcmd': [byebye, byebye]}, cc_bootcmd.schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("runcmd entries as list are allowed to be duplicates.") + """Duplicated commands array/array entries are allowed.""" + self.assertSchemaValid( + ["byebye", "byebye"], 'command entries can be duplicate') def test_duplicates_are_fine_array_string(self): - """Duplicated array entries entries are allowed in values of array.""" - try: - byebye = "echo bye" - schema.validate_cloudconfig_schema( - {'bootcmd': [byebye, byebye]}, cc_bootcmd.schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("runcmd entries are allowed to be duplicates.") + """Duplicated commands array/string entries are allowed.""" + self.assertSchemaValid( + ["echo bye", "echo bye"], "command entries can be duplicate.") # vi: ts=4 expandtab diff --git a/tests/unittests/test_handler/test_handler_runcmd.py b/tests/unittests/test_handler/test_handler_runcmd.py index ee1981d1..9ce334ac 100644 --- a/tests/unittests/test_handler/test_handler_runcmd.py +++ b/tests/unittests/test_handler/test_handler_runcmd.py @@ -1,10 +1,11 @@ # This file is part of cloud-init. See LICENSE file for license information. -from cloudinit.config import cc_runcmd, schema +from cloudinit.config.cc_runcmd import handle, schema from cloudinit.sources import DataSourceNone from cloudinit import (distros, helpers, cloud, util) from cloudinit.tests.helpers import ( - CiTestCase, FilesystemMockingTestCase, skipUnlessJsonSchema) + CiTestCase, FilesystemMockingTestCase, SchemaTestCaseMixin, + skipUnlessJsonSchema) import logging import os @@ -35,7 +36,7 @@ class TestRuncmd(FilesystemMockingTestCase): """When the provided config doesn't contain runcmd, skip it.""" cfg = {} mycloud = self._get_cloud('ubuntu') - cc_runcmd.handle('notimportant', cfg, mycloud, LOG, None) + handle('notimportant', cfg, mycloud, LOG, None) self.assertIn( "Skipping module named notimportant, no 'runcmd' key", self.logs.getvalue()) @@ -44,7 +45,7 @@ class TestRuncmd(FilesystemMockingTestCase): """Commands which can't be converted to shell will raise errors.""" invalid_config = {'runcmd': 1} cc = self._get_cloud('ubuntu') - cc_runcmd.handle('cc_runcmd', invalid_config, cc, LOG, []) + handle('cc_runcmd', invalid_config, cc, LOG, []) self.assertIn( 'Failed to shellify 1 into file' ' /var/lib/cloud/instances/iid-datasource-none/scripts/runcmd', @@ -59,7 +60,7 @@ class TestRuncmd(FilesystemMockingTestCase): """ invalid_config = {'runcmd': 1} cc = self._get_cloud('ubuntu') - cc_runcmd.handle('cc_runcmd', invalid_config, cc, LOG, []) + handle('cc_runcmd', invalid_config, cc, LOG, []) self.assertIn( 'Invalid config:\nruncmd: 1 is not of type \'array\'', self.logs.getvalue()) @@ -75,7 +76,7 @@ class TestRuncmd(FilesystemMockingTestCase): invalid_config = { 'runcmd': ['ls /', 20, ['wget', 'http://stuff/blah'], {'a': 'n'}]} cc = self._get_cloud('ubuntu') - cc_runcmd.handle('cc_runcmd', invalid_config, cc, LOG, []) + handle('cc_runcmd', invalid_config, cc, LOG, []) expected_warnings = [ 'runcmd.1: 20 is not valid under any of the given schemas', 'runcmd.3: {\'a\': \'n\'} is not valid under any of the given' @@ -90,7 +91,7 @@ class TestRuncmd(FilesystemMockingTestCase): """Valid runcmd schema is written to a runcmd shell script.""" valid_config = {'runcmd': [['ls', '/']]} cc = self._get_cloud('ubuntu') - cc_runcmd.handle('cc_runcmd', valid_config, cc, LOG, []) + handle('cc_runcmd', valid_config, cc, LOG, []) runcmd_file = os.path.join( self.new_root, 'var/lib/cloud/instances/iid-datasource-none/scripts/runcmd') @@ -99,25 +100,22 @@ class TestRuncmd(FilesystemMockingTestCase): self.assertEqual(0o700, stat.S_IMODE(file_stat.st_mode)) -class TestSchema(CiTestCase): +@skipUnlessJsonSchema() +class TestSchema(CiTestCase, SchemaTestCaseMixin): """Directly test schema rather than through handle.""" - def test_duplicates_are_fine_array(self): - """Duplicated array entries are allowed.""" - try: - byebye = ["echo", "bye"] - schema.validate_cloudconfig_schema( - {'runcmd': [byebye, byebye]}, cc_runcmd.schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("runcmd entries as list are allowed to be duplicates.") - - def test_duplicates_are_fine_string(self): - """Duplicated string entries are allowed.""" - try: - byebye = "echo bye" - schema.validate_cloudconfig_schema( - {'runcmd': [byebye, byebye]}, cc_runcmd.schema, strict=True) - except schema.SchemaValidationError as e: - self.fail("runcmd entries are allowed to be duplicates.") + schema = schema + + def test_duplicates_are_fine_array_array(self): + """Duplicated commands array/array entries are allowed.""" + self.assertSchemaValid( + [["echo", "bye"], ["echo", "bye"]], + "command entries can be duplicate.") + + def test_duplicates_are_fine_array_string(self): + """Duplicated commands array/string entries are allowed.""" + self.assertSchemaValid( + ["echo bye", "echo bye"], + "command entries can be duplicate.") # vi: ts=4 expandtab -- cgit v1.2.3 From 4731c8da25ee9bfbcf0ade1d7ffec95814d8622a Mon Sep 17 00:00:00 2001 From: Ryan Harper Date: Thu, 26 Apr 2018 16:35:23 -0400 Subject: net: detect unstable network names and trigger a settle if needed The cloud-init-local.service expects that any network device name changes have already been completed by the kernel or udev daemon. In some situations we've found that the renaming of interfaces from kernel names (eth0, eth1, etc) to their persistent names (eno1, ens3, enp0s1, etc) may happen after cloud-init-local has started where it reads values from sysfs about what network devices are present, and which device to use as a fallback nic. Subsequently, cloud-init-local would write out network configuration for a kernel device name which would no longer be present by the time that networking services start to bring up the devices. The result is that the instance does not get networking configured. Prior to use of systemd-networkd, the Ubuntu 'networking.service' unit included a call to udevadm settle which is why this race is not seen on a Xenial system. This change adds the ability to detect if an interface has a stable name, if if we find one without stable names and stable names have not been disabled (net.ifnames=0 in /proc/cmdline), then cloud-init will invoke udevadm settle. LP: #1766287 --- cloudinit/config/cc_disk_setup.py | 12 ++---- cloudinit/net/__init__.py | 26 ++++++++++++ cloudinit/net/tests/test_init.py | 1 + cloudinit/sources/DataSourceAltCloud.py | 5 +-- cloudinit/tests/test_util.py | 49 ++++++++++++++++++++++ cloudinit/util.py | 15 +++++++ tests/unittests/test_net.py | 73 +++++++++++++++++++++++++++++++-- 7 files changed, 165 insertions(+), 16 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py index c3e8c484..943089e0 100644 --- a/cloudinit/config/cc_disk_setup.py +++ b/cloudinit/config/cc_disk_setup.py @@ -680,13 +680,13 @@ def read_parttbl(device): reliable way to probe the partition table. """ blkdev_cmd = [BLKDEV_CMD, '--rereadpt', device] - udevadm_settle() + util.udevadm_settle() try: util.subp(blkdev_cmd) except Exception as e: util.logexc(LOG, "Failed reading the partition table %s" % e) - udevadm_settle() + util.udevadm_settle() def exec_mkpart_mbr(device, layout): @@ -737,14 +737,10 @@ def exec_mkpart(table_type, device, layout): return get_dyn_func("exec_mkpart_%s", table_type, device, layout) -def udevadm_settle(): - util.subp(['udevadm', 'settle']) - - def assert_and_settle_device(device): """Assert that device exists and settle so it is fully recognized.""" if not os.path.exists(device): - udevadm_settle() + util.udevadm_settle() if not os.path.exists(device): raise RuntimeError("Device %s did not exist and was not created " "with a udevamd settle." % device) @@ -752,7 +748,7 @@ def assert_and_settle_device(device): # Whether or not the device existed above, it is possible that udev # events that would populate udev database (for reading by lsdname) have # not yet finished. So settle again. - udevadm_settle() + util.udevadm_settle() def mkpart(device, definition): diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index 80054546..43226bd0 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -107,6 +107,21 @@ def is_bond(devname): return os.path.exists(sys_dev_path(devname, "bonding")) +def is_renamed(devname): + """ + /* interface name assignment types (sysfs name_assign_type attribute) */ + #define NET_NAME_UNKNOWN 0 /* unknown origin (not exposed to user) */ + #define NET_NAME_ENUM 1 /* enumerated by kernel */ + #define NET_NAME_PREDICTABLE 2 /* predictably named by the kernel */ + #define NET_NAME_USER 3 /* provided by user-space */ + #define NET_NAME_RENAMED 4 /* renamed by user-space */ + """ + name_assign_type = read_sys_net_safe(devname, 'name_assign_type') + if name_assign_type and name_assign_type in ['3', '4']: + return True + return False + + def is_vlan(devname): uevent = str(read_sys_net_safe(devname, "uevent")) return 'DEVTYPE=vlan' in uevent.splitlines() @@ -180,6 +195,17 @@ def find_fallback_nic(blacklist_drivers=None): if not blacklist_drivers: blacklist_drivers = [] + if 'net.ifnames=0' in util.get_cmdline(): + LOG.debug('Stable ifnames disabled by net.ifnames=0 in /proc/cmdline') + else: + unstable = [device for device in get_devicelist() + if device != 'lo' and not is_renamed(device)] + if len(unstable): + LOG.debug('Found unstable nic names: %s; calling udevadm settle', + unstable) + msg = 'Waiting for udev events to settle' + util.log_time(LOG.debug, msg, func=util.udevadm_settle) + # get list of interfaces that could have connections invalid_interfaces = set(['lo']) potential_interfaces = set([device for device in get_devicelist() diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py index 276556ee..5c017d15 100644 --- a/cloudinit/net/tests/test_init.py +++ b/cloudinit/net/tests/test_init.py @@ -199,6 +199,7 @@ class TestGenerateFallbackConfig(CiTestCase): self.sysdir = self.tmp_dir() + '/' self.m_sys_path.return_value = self.sysdir self.addCleanup(sys_mock.stop) + self.add_patch('cloudinit.net.util.udevadm_settle', 'm_settle') def test_generate_fallback_finds_connected_eth_with_mac(self): """generate_fallback_config finds any connected device with a mac.""" diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py index e1d0055b..f6e86f34 100644 --- a/cloudinit/sources/DataSourceAltCloud.py +++ b/cloudinit/sources/DataSourceAltCloud.py @@ -29,7 +29,6 @@ CLOUD_INFO_FILE = '/etc/sysconfig/cloud-info' # Shell command lists CMD_PROBE_FLOPPY = ['modprobe', 'floppy'] -CMD_UDEVADM_SETTLE = ['udevadm', 'settle', '--timeout=5'] META_DATA_NOT_SUPPORTED = { 'block-device-mapping': {}, @@ -196,9 +195,7 @@ class DataSourceAltCloud(sources.DataSource): # udevadm settle for floppy device try: - cmd = CMD_UDEVADM_SETTLE - cmd.append('--exit-if-exists=' + floppy_dev) - (cmd_out, _err) = util.subp(cmd) + (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5) LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out) except ProcessExecutionError as _err: util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err) diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py index 76eed076..3c05a437 100644 --- a/cloudinit/tests/test_util.py +++ b/cloudinit/tests/test_util.py @@ -212,4 +212,53 @@ class TestBlkid(CiTestCase): capture=True, decode="replace") +@mock.patch('cloudinit.util.subp') +class TestUdevadmSettle(CiTestCase): + def test_with_no_params(self, m_subp): + """called with no parameters.""" + util.udevadm_settle() + m_subp.called_once_with(mock.call(['udevadm', 'settle'])) + + def test_with_exists_and_not_exists(self, m_subp): + """with exists=file where file does not exist should invoke subp.""" + mydev = self.tmp_path("mydev") + util.udevadm_settle(exists=mydev) + m_subp.called_once_with( + ['udevadm', 'settle', '--exit-if-exists=%s' % mydev]) + + def test_with_exists_and_file_exists(self, m_subp): + """with exists=file where file does exist should not invoke subp.""" + mydev = self.tmp_path("mydev") + util.write_file(mydev, "foo\n") + util.udevadm_settle(exists=mydev) + self.assertIsNone(m_subp.call_args) + + def test_with_timeout_int(self, m_subp): + """timeout can be an integer.""" + timeout = 9 + util.udevadm_settle(timeout=timeout) + m_subp.called_once_with( + ['udevadm', 'settle', '--timeout=%s' % timeout]) + + def test_with_timeout_string(self, m_subp): + """timeout can be a string.""" + timeout = "555" + util.udevadm_settle(timeout=timeout) + m_subp.assert_called_once_with( + ['udevadm', 'settle', '--timeout=%s' % timeout]) + + def test_with_exists_and_timeout(self, m_subp): + """test call with both exists and timeout.""" + mydev = self.tmp_path("mydev") + timeout = "3" + util.udevadm_settle(exists=mydev) + m_subp.called_once_with( + ['udevadm', 'settle', '--exit-if-exists=%s' % mydev, + '--timeout=%s' % timeout]) + + def test_subp_exception_raises_to_caller(self, m_subp): + m_subp.side_effect = util.ProcessExecutionError("BOOM") + self.assertRaises(util.ProcessExecutionError, util.udevadm_settle) + + # vi: ts=4 expandtab diff --git a/cloudinit/util.py b/cloudinit/util.py index 310758dd..2828ca38 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -2727,4 +2727,19 @@ def mount_is_read_write(mount_point): mount_opts = result[-1].split(',') return mount_opts[0] == 'rw' + +def udevadm_settle(exists=None, timeout=None): + """Invoke udevadm settle with optional exists and timeout parameters""" + settle_cmd = ["udevadm", "settle"] + if exists: + # skip the settle if the requested path already exists + if os.path.exists(exists): + return + settle_cmd.extend(['--exit-if-exists=%s' % exists]) + if timeout: + settle_cmd.extend(['--timeout=%s' % timeout]) + + return subp(settle_cmd) + + # vi: ts=4 expandtab diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py index 84839fd7..fac82678 100644 --- a/tests/unittests/test_net.py +++ b/tests/unittests/test_net.py @@ -1442,6 +1442,7 @@ DEFAULT_DEV_ATTRS = { "address": "07-1C-C6-75-A4-BE", "device/driver": None, "device/device": None, + "name_assign_type": "4", } } @@ -1489,11 +1490,14 @@ class TestGenerateFallbackConfig(CiTestCase): 'eth0': { 'bridge': False, 'carrier': False, 'dormant': False, 'operstate': 'down', 'address': '00:11:22:33:44:55', - 'device/driver': 'hv_netsvc', 'device/device': '0x3'}, + 'device/driver': 'hv_netsvc', 'device/device': '0x3', + 'name_assign_type': '4'}, 'eth1': { 'bridge': False, 'carrier': False, 'dormant': False, 'operstate': 'down', 'address': '00:11:22:33:44:55', - 'device/driver': 'mlx4_core', 'device/device': '0x7'}, + 'device/driver': 'mlx4_core', 'device/device': '0x7', + 'name_assign_type': '4'}, + } tmp_dir = self.tmp_dir() @@ -1549,11 +1553,13 @@ iface eth0 inet dhcp 'eth1': { 'bridge': False, 'carrier': False, 'dormant': False, 'operstate': 'down', 'address': '00:11:22:33:44:55', - 'device/driver': 'hv_netsvc', 'device/device': '0x3'}, + 'device/driver': 'hv_netsvc', 'device/device': '0x3', + 'name_assign_type': '4'}, 'eth0': { 'bridge': False, 'carrier': False, 'dormant': False, 'operstate': 'down', 'address': '00:11:22:33:44:55', - 'device/driver': 'mlx4_core', 'device/device': '0x7'}, + 'device/driver': 'mlx4_core', 'device/device': '0x7', + 'name_assign_type': '4'}, } tmp_dir = self.tmp_dir() @@ -1602,6 +1608,65 @@ iface eth1 inet dhcp ] self.assertEqual(", ".join(expected_rule) + '\n', contents.lstrip()) + @mock.patch("cloudinit.util.udevadm_settle") + @mock.patch("cloudinit.net.sys_dev_path") + @mock.patch("cloudinit.net.read_sys_net") + @mock.patch("cloudinit.net.get_devicelist") + def test_unstable_names(self, mock_get_devicelist, mock_read_sys_net, + mock_sys_dev_path, mock_settle): + """verify that udevadm settle is called when we find unstable names""" + devices = { + 'eth0': { + 'bridge': False, 'carrier': False, 'dormant': False, + 'operstate': 'down', 'address': '00:11:22:33:44:55', + 'device/driver': 'hv_netsvc', 'device/device': '0x3', + 'name_assign_type': False}, + 'ens4': { + 'bridge': False, 'carrier': False, 'dormant': False, + 'operstate': 'down', 'address': '00:11:22:33:44:55', + 'device/driver': 'mlx4_core', 'device/device': '0x7', + 'name_assign_type': '4'}, + + } + + tmp_dir = self.tmp_dir() + _setup_test(tmp_dir, mock_get_devicelist, + mock_read_sys_net, mock_sys_dev_path, + dev_attrs=devices) + net.generate_fallback_config(config_driver=True) + self.assertEqual(1, mock_settle.call_count) + + @mock.patch("cloudinit.util.get_cmdline") + @mock.patch("cloudinit.util.udevadm_settle") + @mock.patch("cloudinit.net.sys_dev_path") + @mock.patch("cloudinit.net.read_sys_net") + @mock.patch("cloudinit.net.get_devicelist") + def test_unstable_names_disabled(self, mock_get_devicelist, + mock_read_sys_net, mock_sys_dev_path, + mock_settle, m_get_cmdline): + """verify udevadm settle not called when cmdline has net.ifnames=0""" + devices = { + 'eth0': { + 'bridge': False, 'carrier': False, 'dormant': False, + 'operstate': 'down', 'address': '00:11:22:33:44:55', + 'device/driver': 'hv_netsvc', 'device/device': '0x3', + 'name_assign_type': False}, + 'ens4': { + 'bridge': False, 'carrier': False, 'dormant': False, + 'operstate': 'down', 'address': '00:11:22:33:44:55', + 'device/driver': 'mlx4_core', 'device/device': '0x7', + 'name_assign_type': '4'}, + + } + + m_get_cmdline.return_value = 'net.ifnames=0' + tmp_dir = self.tmp_dir() + _setup_test(tmp_dir, mock_get_devicelist, + mock_read_sys_net, mock_sys_dev_path, + dev_attrs=devices) + net.generate_fallback_config(config_driver=True) + self.assertEqual(0, mock_settle.call_count) + class TestSysConfigRendering(CiTestCase): -- cgit v1.2.3 From fed07fc6cddebcbc3e744926509c362d9e200aeb Mon Sep 17 00:00:00 2001 From: Harm Weites Date: Tue, 1 May 2018 15:38:51 -0600 Subject: FreeBSD: Invoke growfs on ufs filesystems such that it does not prompt. By default, FreeBSD's growfs runs interactively asking a question which can be mitigated using the '-y' command line option. The fix here is simply to pass -y to growfs to avoid the prompt. LP: #1404745 --- cloudinit/config/cc_resizefs.py | 2 +- tests/unittests/test_handler/test_handler_resizefs.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py index 82f29e10..2edddd0c 100644 --- a/cloudinit/config/cc_resizefs.py +++ b/cloudinit/config/cc_resizefs.py @@ -81,7 +81,7 @@ def _resize_xfs(mount_point, devpth): def _resize_ufs(mount_point, devpth): - return ('growfs', devpth) + return ('growfs', '-y', devpth) def _resize_zfs(mount_point, devpth): diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py index 7a7ba1ff..f92175fd 100644 --- a/tests/unittests/test_handler/test_handler_resizefs.py +++ b/tests/unittests/test_handler/test_handler_resizefs.py @@ -147,7 +147,7 @@ class TestResizefs(CiTestCase): def test_resize_ufs_cmd_return(self): mount_point = '/' devpth = '/dev/sda2' - self.assertEqual(('growfs', devpth), + self.assertEqual(('growfs', '-y', devpth), _resize_ufs(mount_point, devpth)) @mock.patch('cloudinit.util.get_mount_info') -- cgit v1.2.3 From c2494322282575360a6eef3b02a80a3e3e5061d3 Mon Sep 17 00:00:00 2001 From: Stephen Ford Date: Wed, 16 May 2018 09:48:49 -0600 Subject: typos: Fix spelling mistakes in cc_mounts.py log messages This also makes some of the messages more consistent. --- cloudinit/config/cc_mounts.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py index f14a4fc5..02bedaba 100644 --- a/cloudinit/config/cc_mounts.py +++ b/cloudinit/config/cc_mounts.py @@ -233,7 +233,7 @@ def setup_swapfile(fname, size=None, maxsize=None): try: memsize = util.read_meminfo()['total'] except IOError as e: - LOG.debug("Not creating swap. failed to read meminfo") + LOG.debug("Not creating swap: failed to read meminfo") return util.ensure_dir(tdir) @@ -280,17 +280,17 @@ def handle_swapcfg(swapcfg): if os.path.exists(fname): if not os.path.exists("/proc/swaps"): - LOG.debug("swap file %s existed. no /proc/swaps. Being safe.", + LOG.debug("swap file %s exists, but no /proc/swaps exists, being safe", fname) return fname try: for line in util.load_file("/proc/swaps").splitlines(): if line.startswith(fname + " "): - LOG.debug("swap file %s already in use.", fname) + LOG.debug("swap file %s already in use", fname) return fname - LOG.debug("swap file %s existed, but not in /proc/swaps", fname) + LOG.debug("swap file %s exists, but not in /proc/swaps", fname) except Exception: - LOG.warning("swap file %s existed. Error reading /proc/swaps", + LOG.warning("swap file %s exists. Error reading /proc/swaps", fname) return fname @@ -337,7 +337,7 @@ def handle(_name, cfg, cloud, log, _args): start = str(cfgmnt[i][0]) sanitized = sanitize_devname(start, cloud.device_name_to_device, log) if sanitized is None: - log.debug("Ignorming nonexistant named mount %s", start) + log.debug("Ignoring nonexistent named mount %s", start) continue if sanitized != start: @@ -374,7 +374,7 @@ def handle(_name, cfg, cloud, log, _args): start = defmnt[0] sanitized = sanitize_devname(start, cloud.device_name_to_device, log) if sanitized is None: - log.debug("Ignoring nonexistant default named mount %s", start) + log.debug("Ignoring nonexistent default named mount %s", start) continue if sanitized != start: log.debug("changed default device %s => %s" % (start, sanitized)) @@ -397,7 +397,7 @@ def handle(_name, cfg, cloud, log, _args): actlist = [] for x in cfgmnt: if x[1] is None: - log.debug("Skipping non-existent device named %s", x[0]) + log.debug("Skipping nonexistent device named %s", x[0]) else: actlist.append(x) @@ -406,7 +406,7 @@ def handle(_name, cfg, cloud, log, _args): actlist.append([swapret, "none", "swap", "sw", "0", "0"]) if len(actlist) == 0: - log.debug("No modifications to fstab needed.") + log.debug("No modifications to fstab needed") return comment = "comment=cloudconfig" -- cgit v1.2.3 From 13f7f4e802729d22e9906155fee435c3e8a8975b Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 16 May 2018 19:09:32 -0400 Subject: flake8: fix flake8 errors in previous commit. --- cloudinit/config/cc_mounts.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py index 02bedaba..b3b25c3b 100644 --- a/cloudinit/config/cc_mounts.py +++ b/cloudinit/config/cc_mounts.py @@ -280,8 +280,8 @@ def handle_swapcfg(swapcfg): if os.path.exists(fname): if not os.path.exists("/proc/swaps"): - LOG.debug("swap file %s exists, but no /proc/swaps exists, being safe", - fname) + LOG.debug("swap file %s exists, but no /proc/swaps exists, " + "being safe", fname) return fname try: for line in util.load_file("/proc/swaps").splitlines(): -- cgit v1.2.3 From 30e730f7ca111487d243ba9f40c66df6d7a49953 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 17 May 2018 14:59:54 -0600 Subject: read_file_or_url: move to url_helper, fix bug in its FileResponse. The result of a read_file_or_url on a file and on a url would differ in behavior. str(UrlResponse) would return UrlResponse.contents.decode('utf-8') while str(FileResponse) would return str(FileResponse.contents) The difference being "b'foo'" versus "foo". As part of the general goal of cleaning util, move read_file_or_url into url_helper. --- cloudinit/cmd/main.py | 2 +- cloudinit/config/cc_phone_home.py | 7 +-- cloudinit/config/schema.py | 4 +- cloudinit/ec2_utils.py | 14 +++--- cloudinit/sources/DataSourceMAAS.py | 2 +- cloudinit/sources/helpers/azure.py | 5 ++- cloudinit/tests/test_url_helper.py | 28 +++++++++++- cloudinit/url_helper.py | 29 +++++++++++- cloudinit/user_data.py | 6 +-- cloudinit/util.py | 37 ++-------------- tests/unittests/test__init__.py | 8 ++-- .../unittests/test_datasource/test_azure_helper.py | 2 +- .../test_handler/test_handler_apt_conf_v1.py | 16 +++---- .../test_handler_apt_configure_sources_list_v1.py | 7 --- .../test_handler/test_handler_apt_source_v1.py | 27 +++++------- .../test_handler/test_handler_apt_source_v3.py | 27 +++++------- tests/unittests/test_handler/test_handler_ntp.py | 51 +++++++++------------- 17 files changed, 131 insertions(+), 141 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py index 3f2dbb93..d6ba90f4 100644 --- a/cloudinit/cmd/main.py +++ b/cloudinit/cmd/main.py @@ -187,7 +187,7 @@ def attempt_cmdline_url(path, network=True, cmdline=None): data = None header = b'#cloud-config' try: - resp = util.read_file_or_url(**kwargs) + resp = url_helper.read_file_or_url(**kwargs) if resp.ok(): data = resp.contents if not resp.contents.startswith(header): diff --git a/cloudinit/config/cc_phone_home.py b/cloudinit/config/cc_phone_home.py index 878069b7..3be0d1c1 100644 --- a/cloudinit/config/cc_phone_home.py +++ b/cloudinit/config/cc_phone_home.py @@ -41,6 +41,7 @@ keys to post. Available keys are: """ from cloudinit import templater +from cloudinit import url_helper from cloudinit import util from cloudinit.settings import PER_INSTANCE @@ -136,9 +137,9 @@ def handle(name, cfg, cloud, log, args): } url = templater.render_string(url, url_params) try: - util.read_file_or_url(url, data=real_submit_keys, - retries=tries, sec_between=3, - ssl_details=util.fetch_ssl_details(cloud.paths)) + url_helper.read_file_or_url( + url, data=real_submit_keys, retries=tries, sec_between=3, + ssl_details=util.fetch_ssl_details(cloud.paths)) except Exception: util.logexc(log, "Failed to post phone home data to %s in %s tries", url, tries) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index 76826e05..3bad8e22 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -4,7 +4,7 @@ from __future__ import print_function from cloudinit import importer -from cloudinit.util import find_modules, read_file_or_url +from cloudinit.util import find_modules, load_file import argparse from collections import defaultdict @@ -139,7 +139,7 @@ def validate_cloudconfig_file(config_path, schema, annotate=False): """ if not os.path.exists(config_path): raise RuntimeError('Configfile {0} does not exist'.format(config_path)) - content = read_file_or_url('file://{0}'.format(config_path)).contents + content = load_file(config_path, decode=False) if not content.startswith(CLOUD_CONFIG_HEADER): errors = ( ('header', 'File {0} needs to begin with "{1}"'.format( diff --git a/cloudinit/ec2_utils.py b/cloudinit/ec2_utils.py index dc3f0fc3..3b7b17f1 100644 --- a/cloudinit/ec2_utils.py +++ b/cloudinit/ec2_utils.py @@ -150,11 +150,9 @@ def get_instance_userdata(api_version='latest', # NOT_FOUND occurs) and just in that case returning an empty string. exception_cb = functools.partial(_skip_retry_on_codes, SKIP_USERDATA_CODES) - response = util.read_file_or_url(ud_url, - ssl_details=ssl_details, - timeout=timeout, - retries=retries, - exception_cb=exception_cb) + response = url_helper.read_file_or_url( + ud_url, ssl_details=ssl_details, timeout=timeout, + retries=retries, exception_cb=exception_cb) user_data = response.contents except url_helper.UrlError as e: if e.code not in SKIP_USERDATA_CODES: @@ -169,9 +167,9 @@ def _get_instance_metadata(tree, api_version='latest', ssl_details=None, timeout=5, retries=5, leaf_decoder=None): md_url = url_helper.combine_url(metadata_address, api_version, tree) - caller = functools.partial(util.read_file_or_url, - ssl_details=ssl_details, timeout=timeout, - retries=retries) + caller = functools.partial( + url_helper.read_file_or_url, ssl_details=ssl_details, + timeout=timeout, retries=retries) def mcaller(url): return caller(url).contents diff --git a/cloudinit/sources/DataSourceMAAS.py b/cloudinit/sources/DataSourceMAAS.py index aa56addb..bcb38544 100644 --- a/cloudinit/sources/DataSourceMAAS.py +++ b/cloudinit/sources/DataSourceMAAS.py @@ -198,7 +198,7 @@ def read_maas_seed_url(seed_url, read_file_or_url=None, timeout=None, If version is None, then / will not be used. """ if read_file_or_url is None: - read_file_or_url = util.read_file_or_url + read_file_or_url = url_helper.read_file_or_url if seed_url.endswith("/"): seed_url = seed_url[:-1] diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py index 90c12df1..e5696b1f 100644 --- a/cloudinit/sources/helpers/azure.py +++ b/cloudinit/sources/helpers/azure.py @@ -14,6 +14,7 @@ from cloudinit import temp_utils from contextlib import contextmanager from xml.etree import ElementTree +from cloudinit import url_helper from cloudinit import util LOG = logging.getLogger(__name__) @@ -55,14 +56,14 @@ class AzureEndpointHttpClient(object): if secure: headers = self.headers.copy() headers.update(self.extra_secure_headers) - return util.read_file_or_url(url, headers=headers) + return url_helper.read_file_or_url(url, headers=headers) def post(self, url, data=None, extra_headers=None): headers = self.headers if extra_headers is not None: headers = self.headers.copy() headers.update(extra_headers) - return util.read_file_or_url(url, data=data, headers=headers) + return url_helper.read_file_or_url(url, data=data, headers=headers) class GoalState(object): diff --git a/cloudinit/tests/test_url_helper.py b/cloudinit/tests/test_url_helper.py index b778a3a7..113249d9 100644 --- a/cloudinit/tests/test_url_helper.py +++ b/cloudinit/tests/test_url_helper.py @@ -1,7 +1,10 @@ # This file is part of cloud-init. See LICENSE file for license information. -from cloudinit.url_helper import oauth_headers +from cloudinit.url_helper import oauth_headers, read_file_or_url from cloudinit.tests.helpers import CiTestCase, mock, skipIf +from cloudinit import util + +import httpretty try: @@ -38,3 +41,26 @@ class TestOAuthHeaders(CiTestCase): 'url', 'consumer_key', 'token_key', 'token_secret', 'consumer_secret') self.assertEqual('url', return_value) + + +class TestReadFileOrUrl(CiTestCase): + def test_read_file_or_url_str_from_file(self): + """Test that str(result.contents) on file is text version of contents. + It should not be "b'data'", but just "'data'" """ + tmpf = self.tmp_path("myfile1") + data = b'This is my file content\n' + util.write_file(tmpf, data, omode="wb") + result = read_file_or_url("file://%s" % tmpf) + self.assertEqual(result.contents, data) + self.assertEqual(str(result), data.decode('utf-8')) + + @httpretty.activate + def test_read_file_or_url_str_from_url(self): + """Test that str(result.contents) on url is text version of contents. + It should not be "b'data'", but just "'data'" """ + url = 'http://hostname/path' + data = b'This is my url content\n' + httpretty.register_uri(httpretty.GET, url, data) + result = read_file_or_url(url) + self.assertEqual(result.contents, data) + self.assertEqual(str(result), data.decode('utf-8')) diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 1de07b1c..8067979e 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -15,6 +15,7 @@ import six import time from email.utils import parsedate +from errno import ENOENT from functools import partial from itertools import count from requests import exceptions @@ -80,6 +81,32 @@ def combine_url(base, *add_ons): return url +def read_file_or_url(url, timeout=5, retries=10, + headers=None, data=None, sec_between=1, ssl_details=None, + headers_cb=None, exception_cb=None): + url = url.lstrip() + if url.startswith("/"): + url = "file://%s" % url + if url.lower().startswith("file://"): + if data: + LOG.warning("Unable to post data to file resource %s", url) + file_path = url[len("file://"):] + try: + with open(file_path, "rb") as fp: + contents = fp.read() + except IOError as e: + code = e.errno + if e.errno == ENOENT: + code = NOT_FOUND + raise UrlError(cause=e, code=code, headers=None, url=url) + return FileResponse(file_path, contents=contents) + else: + return readurl(url, timeout=timeout, retries=retries, headers=headers, + headers_cb=headers_cb, data=data, + sec_between=sec_between, ssl_details=ssl_details, + exception_cb=exception_cb) + + # Made to have same accessors as UrlResponse so that the # read_file_or_url can return this or that object and the # 'user' of those objects will not need to know the difference. @@ -96,7 +123,7 @@ class StringResponse(object): return True def __str__(self): - return self.contents + return self.contents.decode('utf-8') class FileResponse(StringResponse): diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py index cc55daf8..8f6aba1e 100644 --- a/cloudinit/user_data.py +++ b/cloudinit/user_data.py @@ -19,7 +19,7 @@ import six from cloudinit import handlers from cloudinit import log as logging -from cloudinit.url_helper import UrlError +from cloudinit.url_helper import read_file_or_url, UrlError from cloudinit import util LOG = logging.getLogger(__name__) @@ -224,8 +224,8 @@ class UserDataProcessor(object): content = util.load_file(include_once_fn) else: try: - resp = util.read_file_or_url(include_url, - ssl_details=self.ssl_details) + resp = read_file_or_url(include_url, + ssl_details=self.ssl_details) if include_once_on and resp.ok(): util.write_file(include_once_fn, resp.contents, mode=0o600) diff --git a/cloudinit/util.py b/cloudinit/util.py index fc30018c..edfedc7d 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -857,37 +857,6 @@ def fetch_ssl_details(paths=None): return ssl_details -def read_file_or_url(url, timeout=5, retries=10, - headers=None, data=None, sec_between=1, ssl_details=None, - headers_cb=None, exception_cb=None): - url = url.lstrip() - if url.startswith("/"): - url = "file://%s" % url - if url.lower().startswith("file://"): - if data: - LOG.warning("Unable to post data to file resource %s", url) - file_path = url[len("file://"):] - try: - contents = load_file(file_path, decode=False) - except IOError as e: - code = e.errno - if e.errno == ENOENT: - code = url_helper.NOT_FOUND - raise url_helper.UrlError(cause=e, code=code, headers=None, - url=url) - return url_helper.FileResponse(file_path, contents=contents) - else: - return url_helper.readurl(url, - timeout=timeout, - retries=retries, - headers=headers, - headers_cb=headers_cb, - data=data, - sec_between=sec_between, - ssl_details=ssl_details, - exception_cb=exception_cb) - - def load_yaml(blob, default=None, allowed=(dict,)): loaded = default blob = decode_binary(blob) @@ -925,12 +894,14 @@ def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0): ud_url = "%s%s%s" % (base, "user-data", ext) md_url = "%s%s%s" % (base, "meta-data", ext) - md_resp = read_file_or_url(md_url, timeout, retries, file_retries) + md_resp = url_helper.read_file_or_url(md_url, timeout, retries, + file_retries) md = None if md_resp.ok(): md = load_yaml(decode_binary(md_resp.contents), default={}) - ud_resp = read_file_or_url(ud_url, timeout, retries, file_retries) + ud_resp = url_helper.read_file_or_url(ud_url, timeout, retries, + file_retries) ud = None if ud_resp.ok(): ud = ud_resp.contents diff --git a/tests/unittests/test__init__.py b/tests/unittests/test__init__.py index f1ab02e9..739bbebf 100644 --- a/tests/unittests/test__init__.py +++ b/tests/unittests/test__init__.py @@ -182,7 +182,7 @@ class TestCmdlineUrl(CiTestCase): self.assertEqual( ('url', 'http://example.com'), main.parse_cmdline_url(cmdline)) - @mock.patch('cloudinit.cmd.main.util.read_file_or_url') + @mock.patch('cloudinit.cmd.main.url_helper.read_file_or_url') def test_invalid_content(self, m_read): key = "cloud-config-url" url = 'http://example.com/foo' @@ -196,7 +196,7 @@ class TestCmdlineUrl(CiTestCase): self.assertIn(url, msg) self.assertFalse(os.path.exists(fpath)) - @mock.patch('cloudinit.cmd.main.util.read_file_or_url') + @mock.patch('cloudinit.cmd.main.url_helper.read_file_or_url') def test_valid_content(self, m_read): url = "http://example.com/foo" payload = b"#cloud-config\nmydata: foo\nbar: wark\n" @@ -210,7 +210,7 @@ class TestCmdlineUrl(CiTestCase): self.assertEqual(logging.INFO, lvl) self.assertIn(url, msg) - @mock.patch('cloudinit.cmd.main.util.read_file_or_url') + @mock.patch('cloudinit.cmd.main.url_helper.read_file_or_url') def test_no_key_found(self, m_read): cmdline = "ro mykey=http://example.com/foo root=foo" fpath = self.tmp_path("ccpath") @@ -221,7 +221,7 @@ class TestCmdlineUrl(CiTestCase): self.assertFalse(os.path.exists(fpath)) self.assertEqual(logging.DEBUG, lvl) - @mock.patch('cloudinit.cmd.main.util.read_file_or_url') + @mock.patch('cloudinit.cmd.main.url_helper.read_file_or_url') def test_exception_warns(self, m_read): url = "http://example.com/foo" cmdline = "ro cloud-config-url=%s root=LABEL=bar" % url diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py index b42b073f..af9d3e1a 100644 --- a/tests/unittests/test_datasource/test_azure_helper.py +++ b/tests/unittests/test_datasource/test_azure_helper.py @@ -195,7 +195,7 @@ class TestAzureEndpointHttpClient(CiTestCase): self.addCleanup(patches.close) self.read_file_or_url = patches.enter_context( - mock.patch.object(azure_helper.util, 'read_file_or_url')) + mock.patch.object(azure_helper.url_helper, 'read_file_or_url')) def test_non_secure_get(self): client = azure_helper.AzureEndpointHttpClient(mock.MagicMock()) diff --git a/tests/unittests/test_handler/test_handler_apt_conf_v1.py b/tests/unittests/test_handler/test_handler_apt_conf_v1.py index 83f962a9..6a4b03ee 100644 --- a/tests/unittests/test_handler/test_handler_apt_conf_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_conf_v1.py @@ -12,10 +12,6 @@ import shutil import tempfile -def load_tfile_or_url(*args, **kwargs): - return(util.decode_binary(util.read_file_or_url(*args, **kwargs).contents)) - - class TestAptProxyConfig(TestCase): def setUp(self): super(TestAptProxyConfig, self).setUp() @@ -36,7 +32,7 @@ class TestAptProxyConfig(TestCase): self.assertTrue(os.path.isfile(self.pfile)) self.assertFalse(os.path.isfile(self.cfile)) - contents = load_tfile_or_url(self.pfile) + contents = util.load_file(self.pfile) self.assertTrue(self._search_apt_config(contents, "http", "myproxy")) def test_apt_http_proxy_written(self): @@ -46,7 +42,7 @@ class TestAptProxyConfig(TestCase): self.assertTrue(os.path.isfile(self.pfile)) self.assertFalse(os.path.isfile(self.cfile)) - contents = load_tfile_or_url(self.pfile) + contents = util.load_file(self.pfile) self.assertTrue(self._search_apt_config(contents, "http", "myproxy")) def test_apt_all_proxy_written(self): @@ -64,7 +60,7 @@ class TestAptProxyConfig(TestCase): self.assertTrue(os.path.isfile(self.pfile)) self.assertFalse(os.path.isfile(self.cfile)) - contents = load_tfile_or_url(self.pfile) + contents = util.load_file(self.pfile) for ptype, pval in values.items(): self.assertTrue(self._search_apt_config(contents, ptype, pval)) @@ -80,7 +76,7 @@ class TestAptProxyConfig(TestCase): cc_apt_configure.apply_apt_config({'proxy': "foo"}, self.pfile, self.cfile) self.assertTrue(os.path.isfile(self.pfile)) - contents = load_tfile_or_url(self.pfile) + contents = util.load_file(self.pfile) self.assertTrue(self._search_apt_config(contents, "http", "foo")) def test_config_written(self): @@ -92,14 +88,14 @@ class TestAptProxyConfig(TestCase): self.assertTrue(os.path.isfile(self.cfile)) self.assertFalse(os.path.isfile(self.pfile)) - self.assertEqual(load_tfile_or_url(self.cfile), payload) + self.assertEqual(util.load_file(self.cfile), payload) def test_config_replaced(self): util.write_file(self.pfile, "content doesnt matter") cc_apt_configure.apply_apt_config({'conf': "foo"}, self.pfile, self.cfile) self.assertTrue(os.path.isfile(self.cfile)) - self.assertEqual(load_tfile_or_url(self.cfile), "foo") + self.assertEqual(util.load_file(self.cfile), "foo") def test_config_deleted(self): # if no 'conf' is provided, delete any previously written file diff --git a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py index d2b96f0b..23bd6e10 100644 --- a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py @@ -64,13 +64,6 @@ deb-src http://archive.ubuntu.com/ubuntu/ fakerelease main restricted """) -def load_tfile_or_url(*args, **kwargs): - """load_tfile_or_url - load file and return content after decoding - """ - return util.decode_binary(util.read_file_or_url(*args, **kwargs).contents) - - class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase): """TestAptSourceConfigSourceList Main Class to test sources list rendering diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py index 46ca4ce4..a3132fbd 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py @@ -39,13 +39,6 @@ S0ORP6HXET3+jC8BMG4tBWCTK/XEZw== ADD_APT_REPO_MATCH = r"^[\w-]+:\w" -def load_tfile_or_url(*args, **kwargs): - """load_tfile_or_url - load file and return content after decoding - """ - return util.decode_binary(util.read_file_or_url(*args, **kwargs).contents) - - class FakeDistro(object): """Fake Distro helper object""" def update_package_sources(self): @@ -125,7 +118,7 @@ class TestAptSourceConfig(TestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile_or_url(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://archive.ubuntu.com/ubuntu", "karmic-backports", @@ -157,13 +150,13 @@ class TestAptSourceConfig(TestCase): self.apt_src_basic(self.aptlistfile, cfg) # extra verify on two extra files of this test - contents = load_tfile_or_url(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://archive.ubuntu.com/ubuntu", "precise-backports", "main universe multiverse restricted"), contents, flags=re.IGNORECASE)) - contents = load_tfile_or_url(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://archive.ubuntu.com/ubuntu", "lucid-backports", @@ -220,7 +213,7 @@ class TestAptSourceConfig(TestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile_or_url(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "multiverse"), @@ -241,12 +234,12 @@ class TestAptSourceConfig(TestCase): # extra verify on two extra files of this test params = self._get_default_params() - contents = load_tfile_or_url(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "main"), contents, flags=re.IGNORECASE)) - contents = load_tfile_or_url(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "universe"), @@ -296,7 +289,7 @@ class TestAptSourceConfig(TestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile_or_url(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' @@ -336,14 +329,14 @@ class TestAptSourceConfig(TestCase): 'filename': self.aptlistfile3} self.apt_src_keyid(self.aptlistfile, [cfg1, cfg2, cfg3], 3) - contents = load_tfile_or_url(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' 'cloud-init-test/ubuntu'), "xenial", "universe"), contents, flags=re.IGNORECASE)) - contents = load_tfile_or_url(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' @@ -375,7 +368,7 @@ class TestAptSourceConfig(TestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile_or_url(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index e486862d..7a64c230 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -49,13 +49,6 @@ ADD_APT_REPO_MATCH = r"^[\w-]+:\w" TARGET = None -def load_tfile(*args, **kwargs): - """load_tfile_or_url - load file and return content after decoding - """ - return util.decode_binary(util.read_file_or_url(*args, **kwargs).contents) - - class TestAptSourceConfig(t_help.FilesystemMockingTestCase): """TestAptSourceConfig Main Class to test apt configs @@ -119,7 +112,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://test.ubuntu.com/ubuntu", "karmic-backports", @@ -151,13 +144,13 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): self._apt_src_basic(self.aptlistfile, cfg) # extra verify on two extra files of this test - contents = load_tfile(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://test.ubuntu.com/ubuntu", "precise-backports", "main universe multiverse restricted"), contents, flags=re.IGNORECASE)) - contents = load_tfile(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", "http://test.ubuntu.com/ubuntu", "lucid-backports", @@ -174,7 +167,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "multiverse"), @@ -201,12 +194,12 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): # extra verify on two extra files of this test params = self._get_default_params() - contents = load_tfile(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "main"), contents, flags=re.IGNORECASE)) - contents = load_tfile(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", params['MIRROR'], params['RELEASE'], "universe"), @@ -240,7 +233,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): self.assertTrue(os.path.isfile(filename)) - contents = load_tfile(filename) + contents = util.load_file(filename) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' @@ -277,14 +270,14 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): 'keyid': "03683F77"}} self._apt_src_keyid(self.aptlistfile, cfg, 3) - contents = load_tfile(self.aptlistfile2) + contents = util.load_file(self.aptlistfile2) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' 'cloud-init-test/ubuntu'), "xenial", "universe"), contents, flags=re.IGNORECASE)) - contents = load_tfile(self.aptlistfile3) + contents = util.load_file(self.aptlistfile3) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' @@ -310,7 +303,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): self.assertTrue(os.path.isfile(self.aptlistfile)) - contents = load_tfile(self.aptlistfile) + contents = util.load_file(self.aptlistfile) self.assertTrue(re.search(r"%s %s %s %s\n" % ("deb", ('http://ppa.launchpad.net/smoser/' diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py index 6da4564e..6fe3659d 100644 --- a/tests/unittests/test_handler/test_handler_ntp.py +++ b/tests/unittests/test_handler/test_handler_ntp.py @@ -155,9 +155,9 @@ class TestNtp(FilesystemMockingTestCase): path=confpath, template_fn=template_fn, template=None) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( - "servers []\npools ['10.0.0.1', '10.0.0.2']\n", content.decode()) + "servers []\npools ['10.0.0.1', '10.0.0.2']\n", + util.load_file(confpath)) def test_write_ntp_config_template_defaults_pools_w_empty_lists(self): """write_ntp_config_template defaults pools servers upon empty config. @@ -176,10 +176,9 @@ class TestNtp(FilesystemMockingTestCase): path=confpath, template_fn=template_fn, template=None) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers []\npools {0}\n".format(pools), - content.decode()) + util.load_file(confpath)) def test_defaults_pools_empty_lists_sles(self): """write_ntp_config_template defaults opensuse pools upon empty config. @@ -196,11 +195,11 @@ class TestNtp(FilesystemMockingTestCase): path=confpath, template_fn=template_fn, template=None) - content = util.read_file_or_url('file://' + confpath).contents for pool in default_pools: self.assertIn('opensuse', pool) self.assertEqual( - "servers []\npools {0}\n".format(default_pools), content.decode()) + "servers []\npools {0}\n".format(default_pools), + util.load_file(confpath)) self.assertIn( "Adding distro default ntp pool servers: {0}".format( ",".join(default_pools)), @@ -217,10 +216,9 @@ class TestNtp(FilesystemMockingTestCase): path=confpath, template_fn=template_fn, template=None) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "[Time]\nNTP=%s %s \n" % (" ".join(servers), " ".join(pools)), - content.decode()) + util.load_file(confpath)) def test_distro_ntp_client_configs(self): """Test we have updated ntp client configs on different distros""" @@ -267,17 +265,17 @@ class TestNtp(FilesystemMockingTestCase): cc_ntp.write_ntp_config_template(distro, servers=servers, pools=pools, path=confpath, template_fn=template_fn) - content = util.read_file_or_url('file://' + confpath).contents + content = util.load_file(confpath) if client in ['ntp', 'chrony']: expected_servers = '\n'.join([ 'server {0} iburst'.format(srv) for srv in servers]) print('distro=%s client=%s' % (distro, client)) - self.assertIn(expected_servers, content.decode('utf-8'), + self.assertIn(expected_servers, content, ('failed to render {0} conf' ' for distro:{1}'.format(client, distro))) expected_pools = '\n'.join([ 'pool {0} iburst'.format(pool) for pool in pools]) - self.assertIn(expected_pools, content.decode('utf-8'), + self.assertIn(expected_pools, content, ('failed to render {0} conf' ' for distro:{1}'.format(client, distro))) elif client == 'systemd-timesyncd': @@ -286,7 +284,7 @@ class TestNtp(FilesystemMockingTestCase): "# See timesyncd.conf(5) for details.\n\n" + "[Time]\nNTP=%s %s \n" % (" ".join(servers), " ".join(pools))) - self.assertEqual(expected_content, content.decode()) + self.assertEqual(expected_content, content) def test_no_ntpcfg_does_nothing(self): """When no ntp section is defined handler logs a warning and noops.""" @@ -308,10 +306,10 @@ class TestNtp(FilesystemMockingTestCase): confpath = ntpconfig['confpath'] m_select.return_value = ntpconfig cc_ntp.handle('cc_ntp', valid_empty_config, mycloud, None, []) - content = util.read_file_or_url('file://' + confpath).contents pools = cc_ntp.generate_server_names(mycloud.distro.name) self.assertEqual( - "servers []\npools {0}\n".format(pools), content.decode()) + "servers []\npools {0}\n".format(pools), + util.load_file(confpath)) self.assertNotIn('Invalid config:', self.logs.getvalue()) @skipUnlessJsonSchema() @@ -333,9 +331,8 @@ class TestNtp(FilesystemMockingTestCase): "Invalid config:\nntp.pools.0: 123 is not of type 'string'\n" "ntp.servers.1: None is not of type 'string'", self.logs.getvalue()) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual("servers ['valid', None]\npools [123]\n", - content.decode()) + util.load_file(confpath)) @skipUnlessJsonSchema() @mock.patch('cloudinit.config.cc_ntp.select_ntp_client') @@ -357,9 +354,8 @@ class TestNtp(FilesystemMockingTestCase): "Invalid config:\nntp.pools: 123 is not of type 'array'\n" "ntp.servers: 'non-array' is not of type 'array'", self.logs.getvalue()) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual("servers non-array\npools 123\n", - content.decode()) + util.load_file(confpath)) @skipUnlessJsonSchema() @mock.patch('cloudinit.config.cc_ntp.select_ntp_client') @@ -381,10 +377,9 @@ class TestNtp(FilesystemMockingTestCase): "Invalid config:\nntp: Additional properties are not allowed " "('invalidkey' was unexpected)", self.logs.getvalue()) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers []\npools ['0.mycompany.pool.ntp.org']\n", - content.decode()) + util.load_file(confpath)) @skipUnlessJsonSchema() @mock.patch('cloudinit.config.cc_ntp.select_ntp_client') @@ -407,10 +402,10 @@ class TestNtp(FilesystemMockingTestCase): " has non-unique elements\nntp.servers: " "['10.0.0.1', '10.0.0.1'] has non-unique elements", self.logs.getvalue()) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers ['10.0.0.1', '10.0.0.1']\n" - "pools ['0.mypool.org', '0.mypool.org']\n", content.decode()) + "pools ['0.mypool.org', '0.mypool.org']\n", + util.load_file(confpath)) @mock.patch('cloudinit.config.cc_ntp.select_ntp_client') def test_ntp_handler_timesyncd(self, m_select): @@ -426,10 +421,9 @@ class TestNtp(FilesystemMockingTestCase): confpath = ntpconfig['confpath'] m_select.return_value = ntpconfig cc_ntp.handle('cc_ntp', cfg, mycloud, None, []) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "[Time]\nNTP=192.168.2.1 192.168.2.2 0.mypool.org \n", - content.decode()) + util.load_file(confpath)) @mock.patch('cloudinit.config.cc_ntp.select_ntp_client') def test_ntp_handler_enabled_false(self, m_select): @@ -466,10 +460,9 @@ class TestNtp(FilesystemMockingTestCase): m_util.subp.assert_called_with( ['systemctl', 'reload-or-restart', service_name], capture=True) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers []\npools {0}\n".format(pools), - content.decode()) + util.load_file(confpath)) def test_opensuse_picks_chrony(self): """Test opensuse picks chrony or ntp on certain distro versions""" @@ -638,10 +631,9 @@ class TestNtp(FilesystemMockingTestCase): mock_path = 'cloudinit.config.cc_ntp.temp_utils._TMPDIR' with mock.patch(mock_path, self.new_root): cc_ntp.handle('notimportant', cfg, mycloud, None, None) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers []\npools ['mypool.org']\n%s" % custom, - content.decode()) + util.load_file(confpath)) @mock.patch('cloudinit.config.cc_ntp.supplemental_schema_validation') @mock.patch('cloudinit.config.cc_ntp.reload_ntp') @@ -675,10 +667,9 @@ class TestNtp(FilesystemMockingTestCase): with mock.patch(mock_path, self.new_root): cc_ntp.handle('notimportant', {'ntp': cfg}, mycloud, None, None) - content = util.read_file_or_url('file://' + confpath).contents self.assertEqual( "servers []\npools ['mypool.org']\n%s" % custom, - content.decode()) + util.load_file(confpath)) m_schema.assert_called_with(expected_merged_cfg) -- cgit v1.2.3 From 529d48f69d3784b2314397f5eab9d750ab03cf6a Mon Sep 17 00:00:00 2001 From: Lars Kellogg-Stedman Date: Tue, 22 May 2018 10:55:04 -0400 Subject: cc_mounts: Do not add devices to fstab that are already present. Do not add new entries to /etc/fstab for devices that already have an existing fstab entry. Resolves: rhbz#1542578 --- cloudinit/config/cc_mounts.py | 53 +++++++---- cloudinit/tests/helpers.py | 4 +- .../unittests/test_handler/test_handler_mounts.py | 104 ++++++++++++++++++++- 3 files changed, 136 insertions(+), 25 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py index b3b25c3b..eca6ea3f 100644 --- a/cloudinit/config/cc_mounts.py +++ b/cloudinit/config/cc_mounts.py @@ -76,6 +76,7 @@ DEVICE_NAME_FILTER = r"^([x]{0,1}[shv]d[a-z][0-9]*|sr[0-9]+)$" DEVICE_NAME_RE = re.compile(DEVICE_NAME_FILTER) WS = re.compile("[%s]+" % (whitespace)) FSTAB_PATH = "/etc/fstab" +MNT_COMMENT = "comment=cloudconfig" LOG = logging.getLogger(__name__) @@ -327,6 +328,22 @@ def handle(_name, cfg, cloud, log, _args): LOG.debug("mounts configuration is %s", cfgmnt) + fstab_lines = [] + fstab_devs = {} + fstab_removed = [] + + for line in util.load_file(FSTAB_PATH).splitlines(): + if MNT_COMMENT in line: + fstab_removed.append(line) + continue + + try: + toks = WS.split(line) + except Exception: + pass + fstab_devs[toks[0]] = line + fstab_lines.append(line) + for i in range(len(cfgmnt)): # skip something that wasn't a list if not isinstance(cfgmnt[i], list): @@ -336,12 +353,17 @@ def handle(_name, cfg, cloud, log, _args): start = str(cfgmnt[i][0]) sanitized = sanitize_devname(start, cloud.device_name_to_device, log) + if sanitized != start: + log.debug("changed %s => %s" % (start, sanitized)) + if sanitized is None: log.debug("Ignoring nonexistent named mount %s", start) continue + elif sanitized in fstab_devs: + log.info("Device %s already defined in fstab: %s", + sanitized, fstab_devs[sanitized]) + continue - if sanitized != start: - log.debug("changed %s => %s" % (start, sanitized)) cfgmnt[i][0] = sanitized # in case the user did not quote a field (likely fs-freq, fs_passno) @@ -373,11 +395,17 @@ def handle(_name, cfg, cloud, log, _args): for defmnt in defmnts: start = defmnt[0] sanitized = sanitize_devname(start, cloud.device_name_to_device, log) + if sanitized != start: + log.debug("changed default device %s => %s" % (start, sanitized)) + if sanitized is None: log.debug("Ignoring nonexistent default named mount %s", start) continue - if sanitized != start: - log.debug("changed default device %s => %s" % (start, sanitized)) + elif sanitized in fstab_devs: + log.debug("Device %s already defined in fstab: %s", + sanitized, fstab_devs[sanitized]) + continue + defmnt[0] = sanitized cfgmnt_has = False @@ -409,31 +437,18 @@ def handle(_name, cfg, cloud, log, _args): log.debug("No modifications to fstab needed") return - comment = "comment=cloudconfig" cc_lines = [] needswap = False dirs = [] for line in actlist: # write 'comment' in the fs_mntops, entry, claiming this - line[3] = "%s,%s" % (line[3], comment) + line[3] = "%s,%s" % (line[3], MNT_COMMENT) if line[2] == "swap": needswap = True if line[1].startswith("/"): dirs.append(line[1]) cc_lines.append('\t'.join(line)) - fstab_lines = [] - removed = [] - for line in util.load_file(FSTAB_PATH).splitlines(): - try: - toks = WS.split(line) - if toks[3].find(comment) != -1: - removed.append(line) - continue - except Exception: - pass - fstab_lines.append(line) - for d in dirs: try: util.ensure_dir(d) @@ -441,7 +456,7 @@ def handle(_name, cfg, cloud, log, _args): util.logexc(log, "Failed to make '%s' config-mount", d) sadds = [WS.sub(" ", n) for n in cc_lines] - sdrops = [WS.sub(" ", n) for n in removed] + sdrops = [WS.sub(" ", n) for n in fstab_removed] sops = (["- " + drop for drop in sdrops if drop not in sadds] + ["+ " + add for add in sadds if add not in sdrops]) diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py index 117a9cfe..07059fd4 100644 --- a/cloudinit/tests/helpers.py +++ b/cloudinit/tests/helpers.py @@ -111,12 +111,12 @@ class TestCase(unittest2.TestCase): super(TestCase, self).setUp() self.reset_global_state() - def add_patch(self, target, attr, **kwargs): + def add_patch(self, target, attr, *args, **kwargs): """Patches specified target object and sets it as attr on test instance also schedules cleanup""" if 'autospec' not in kwargs: kwargs['autospec'] = True - m = mock.patch(target, **kwargs) + m = mock.patch(target, *args, **kwargs) p = m.start() self.addCleanup(m.stop) setattr(self, attr, p) diff --git a/tests/unittests/test_handler/test_handler_mounts.py b/tests/unittests/test_handler/test_handler_mounts.py index fe492d4b..8fea6c2a 100644 --- a/tests/unittests/test_handler/test_handler_mounts.py +++ b/tests/unittests/test_handler/test_handler_mounts.py @@ -1,8 +1,6 @@ # This file is part of cloud-init. See LICENSE file for license information. import os.path -import shutil -import tempfile from cloudinit.config import cc_mounts @@ -18,8 +16,7 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase): def setUp(self): super(TestSanitizeDevname, self).setUp() - self.new_root = tempfile.mkdtemp() - self.addCleanup(shutil.rmtree, self.new_root) + self.new_root = self.tmp_dir() self.patchOS(self.new_root) def _touch(self, path): @@ -134,4 +131,103 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase): cc_mounts.sanitize_devname( 'ephemeral0.1', lambda x: disk_path, mock.Mock())) + +class TestFstabHandling(test_helpers.FilesystemMockingTestCase): + + swap_path = '/dev/sdb1' + + def setUp(self): + super(TestFstabHandling, self).setUp() + self.new_root = self.tmp_dir() + self.patchOS(self.new_root) + + self.fstab_path = os.path.join(self.new_root, 'etc/fstab') + self._makedirs('/etc') + + self.add_patch('cloudinit.config.cc_mounts.FSTAB_PATH', + 'mock_fstab_path', + self.fstab_path, + autospec=False) + + self.add_patch('cloudinit.config.cc_mounts._is_block_device', + 'mock_is_block_device', + return_value=True) + + self.add_patch('cloudinit.config.cc_mounts.util.subp', + 'mock_util_subp') + + self.mock_cloud = mock.Mock() + self.mock_log = mock.Mock() + self.mock_cloud.device_name_to_device = self.device_name_to_device + + def _makedirs(self, directory): + directory = os.path.join(self.new_root, directory.lstrip('/')) + if not os.path.exists(directory): + os.makedirs(directory) + + def device_name_to_device(self, path): + if path == 'swap': + return self.swap_path + else: + dev = None + + return dev + + def test_fstab_no_swap_device(self): + '''Ensure that cloud-init adds a discovered swap partition + to /etc/fstab.''' + + fstab_original_content = '' + fstab_expected_content = ( + '%s\tnone\tswap\tsw,comment=cloudconfig\t' + '0\t0\n' % (self.swap_path,) + ) + + with open(cc_mounts.FSTAB_PATH, 'w') as fd: + fd.write(fstab_original_content) + + cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) + + with open(cc_mounts.FSTAB_PATH, 'r') as fd: + fstab_new_content = fd.read() + self.assertEqual(fstab_expected_content, fstab_new_content) + + def test_fstab_same_swap_device_already_configured(self): + '''Ensure that cloud-init will not add a swap device if the same + device already exists in /etc/fstab.''' + + fstab_original_content = '%s swap swap defaults 0 0\n' % ( + self.swap_path,) + fstab_expected_content = fstab_original_content + + with open(cc_mounts.FSTAB_PATH, 'w') as fd: + fd.write(fstab_original_content) + + cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) + + with open(cc_mounts.FSTAB_PATH, 'r') as fd: + fstab_new_content = fd.read() + self.assertEqual(fstab_expected_content, fstab_new_content) + + def test_fstab_alternate_swap_device_already_configured(self): + '''Ensure that cloud-init will add a discovered swap device to + /etc/fstab even when there exists a swap definition on another + device.''' + + fstab_original_content = '/dev/sdc1 swap swap defaults 0 0\n' + fstab_expected_content = ( + fstab_original_content + + '%s\tnone\tswap\tsw,comment=cloudconfig\t' + '0\t0\n' % (self.swap_path,) + ) + + with open(cc_mounts.FSTAB_PATH, 'w') as fd: + fd.write(fstab_original_content) + + cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) + + with open(cc_mounts.FSTAB_PATH, 'r') as fd: + fstab_new_content = fd.read() + self.assertEqual(fstab_expected_content, fstab_new_content) + # vi: ts=4 expandtab -- cgit v1.2.3 From 3b28bdc616f3e7f4d6b419629dc7b9efc3ae8d1e Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Wed, 23 May 2018 15:56:16 -0400 Subject: yaml_load/schema: Add invalid line and column nums to error message Yaml tracebacks are generally hard to read for average users. Add a bit of logic to util.yaml_load and schema validation to look for YAMLError.context_marker or problem_marker line and column counts. No longer log the full exceeption traceback from the yaml_load error, instead just LOG.warning for the specific error and point to the offending line and column where the problem exists. --- cloudinit/config/schema.py | 60 +++++++++++++++++++++-------- cloudinit/util.py | 16 +++++++- tests/unittests/test_handler/test_schema.py | 39 ++++++++++++++++--- tests/unittests/test_util.py | 30 +++++++++++++-- 4 files changed, 118 insertions(+), 27 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index 3bad8e22..080a6d06 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -93,20 +93,33 @@ def validate_cloudconfig_schema(config, schema, strict=False): def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): """Return contents of the cloud-config file annotated with schema errors. - @param cloudconfig: YAML-loaded object from the original_content. + @param cloudconfig: YAML-loaded dict from the original_content or empty + dict if unparseable. @param original_content: The contents of a cloud-config file @param schema_errors: List of tuples from a JSONSchemaValidationError. The tuples consist of (schemapath, error_message). """ if not schema_errors: return original_content - schemapaths = _schemapath_for_cloudconfig(cloudconfig, original_content) + schemapaths = {} + if cloudconfig: + schemapaths = _schemapath_for_cloudconfig( + cloudconfig, original_content) errors_by_line = defaultdict(list) error_count = 1 error_footer = [] annotated_content = [] for path, msg in schema_errors: - errors_by_line[schemapaths[path]].append(msg) + match = re.match(r'format-l(?P\d+)\.c(?P\d+).*', path) + if match: + line, col = match.groups() + errors_by_line[int(line)].append(msg) + else: + col = None + errors_by_line[schemapaths[path]].append(msg) + if col is not None: + msg = 'Line {line} column {col}: {msg}'.format( + line=line, col=col, msg=msg) error_footer.append('# E{0}: {1}'.format(error_count, msg)) error_count += 1 lines = original_content.decode().split('\n') @@ -142,18 +155,31 @@ def validate_cloudconfig_file(config_path, schema, annotate=False): content = load_file(config_path, decode=False) if not content.startswith(CLOUD_CONFIG_HEADER): errors = ( - ('header', 'File {0} needs to begin with "{1}"'.format( + ('format-l1.c1', 'File {0} needs to begin with "{1}"'.format( config_path, CLOUD_CONFIG_HEADER.decode())),) - raise SchemaValidationError(errors) - + error = SchemaValidationError(errors) + if annotate: + print(annotated_cloudconfig_file({}, content, error.schema_errors)) + raise error try: cloudconfig = yaml.safe_load(content) - except yaml.parser.ParserError as e: - errors = ( - ('format', 'File {0} is not valid yaml. {1}'.format( - config_path, str(e))),) - raise SchemaValidationError(errors) - + except (yaml.YAMLError) as e: + line = column = 1 + mark = None + if hasattr(e, 'context_mark') and getattr(e, 'context_mark'): + mark = getattr(e, 'context_mark') + elif hasattr(e, 'problem_mark') and getattr(e, 'problem_mark'): + mark = getattr(e, 'problem_mark') + if mark: + line = mark.line + 1 + column = mark.column + 1 + errors = (('format-l{line}.c{col}'.format(line=line, col=column), + 'File {0} is not valid yaml. {1}'.format( + config_path, str(e))),) + error = SchemaValidationError(errors) + if annotate: + print(annotated_cloudconfig_file({}, content, error.schema_errors)) + raise error try: validate_cloudconfig_schema( cloudconfig, schema, strict=True) @@ -176,7 +202,7 @@ def _schemapath_for_cloudconfig(config, original_content): list_index = 0 RE_YAML_INDENT = r'^(\s*)' scopes = [] - for line_number, line in enumerate(content_lines): + for line_number, line in enumerate(content_lines, 1): indent_depth = len(re.match(RE_YAML_INDENT, line).groups()[0]) line = line.strip() if not line or line.startswith('#'): @@ -208,8 +234,8 @@ def _schemapath_for_cloudconfig(config, original_content): scopes.append((indent_depth + 2, key + '.0')) for inner_list_index in range(0, len(yaml.safe_load(value))): list_key = key + '.' + str(inner_list_index) - schema_line_numbers[list_key] = line_number + 1 - schema_line_numbers[key] = line_number + 1 + schema_line_numbers[list_key] = line_number + schema_line_numbers[key] = line_number return schema_line_numbers @@ -337,9 +363,11 @@ def handle_schema_args(name, args): try: validate_cloudconfig_file( args.config_file, full_schema, args.annotate) - except (SchemaValidationError, RuntimeError) as e: + except SchemaValidationError as e: if not args.annotate: error(str(e)) + except RuntimeError as e: + error(str(e)) else: print("Valid cloud-config file {0}".format(args.config_file)) if args.doc: diff --git a/cloudinit/util.py b/cloudinit/util.py index 653ed6ea..c0473b8c 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -874,8 +874,20 @@ def load_yaml(blob, default=None, allowed=(dict,)): " but got %s instead") % (allowed, type_utils.obj_name(converted))) loaded = converted - except (yaml.YAMLError, TypeError, ValueError): - logexc(LOG, "Failed loading yaml blob") + except (yaml.YAMLError, TypeError, ValueError) as e: + msg = 'Failed loading yaml blob' + mark = None + if hasattr(e, 'context_mark') and getattr(e, 'context_mark'): + mark = getattr(e, 'context_mark') + elif hasattr(e, 'problem_mark') and getattr(e, 'problem_mark'): + mark = getattr(e, 'problem_mark') + if mark: + msg += ( + '. Invalid format at line {line} column {col}: "{err}"'.format( + line=mark.line + 1, col=mark.column + 1, err=e)) + else: + msg += '. {err}'.format(err=e) + LOG.warning(msg) return loaded diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py index ac41f124..fb266faf 100644 --- a/tests/unittests/test_handler/test_schema.py +++ b/tests/unittests/test_handler/test_schema.py @@ -134,22 +134,35 @@ class ValidateCloudConfigFileTest(CiTestCase): with self.assertRaises(SchemaValidationError) as context_mgr: validate_cloudconfig_file(self.config_file, {}) self.assertEqual( - 'Cloud config schema errors: header: File {0} needs to begin with ' - '"{1}"'.format(self.config_file, CLOUD_CONFIG_HEADER.decode()), + 'Cloud config schema errors: format-l1.c1: File {0} needs to begin' + ' with "{1}"'.format( + self.config_file, CLOUD_CONFIG_HEADER.decode()), str(context_mgr.exception)) - def test_validateconfig_file_error_on_non_yaml_format(self): - """On non-yaml format, validate_cloudconfig_file errors.""" + def test_validateconfig_file_error_on_non_yaml_scanner_error(self): + """On non-yaml scan issues, validate_cloudconfig_file errors.""" + # Generate a scanner error by providing text on a single line with + # improper indent. + write_file(self.config_file, '#cloud-config\nasdf:\nasdf') + with self.assertRaises(SchemaValidationError) as context_mgr: + validate_cloudconfig_file(self.config_file, {}) + self.assertIn( + 'schema errors: format-l3.c1: File {0} is not valid yaml.'.format( + self.config_file), + str(context_mgr.exception)) + + def test_validateconfig_file_error_on_non_yaml_parser_error(self): + """On non-yaml parser issues, validate_cloudconfig_file errors.""" write_file(self.config_file, '#cloud-config\n{}}') with self.assertRaises(SchemaValidationError) as context_mgr: validate_cloudconfig_file(self.config_file, {}) self.assertIn( - 'schema errors: format: File {0} is not valid yaml.'.format( + 'schema errors: format-l2.c3: File {0} is not valid yaml.'.format( self.config_file), str(context_mgr.exception)) @skipUnlessJsonSchema() - def test_validateconfig_file_sctricty_validates_schema(self): + def test_validateconfig_file_sctrictly_validates_schema(self): """validate_cloudconfig_file raises errors on invalid schema.""" schema = { 'properties': {'p1': {'type': 'string', 'format': 'hostname'}}} @@ -342,6 +355,20 @@ class MainTest(CiTestCase): 'Expected either --config-file argument or --doc\n', m_stderr.getvalue()) + def test_main_absent_config_file(self): + """Main exits non-zero when config file is absent.""" + myargs = ['mycmd', '--annotate', '--config-file', 'NOT_A_FILE'] + with mock.patch('sys.exit', side_effect=self.sys_exit): + with mock.patch('sys.argv', myargs): + with mock.patch('sys.stderr', new_callable=StringIO) as \ + m_stderr: + with self.assertRaises(SystemExit) as context_manager: + main() + self.assertEqual(1, context_manager.exception.code) + self.assertEqual( + 'Configfile NOT_A_FILE does not exist\n', + m_stderr.getvalue()) + def test_main_prints_docs(self): """When --doc parameter is provided, main generates documentation.""" myargs = ['mycmd', '--doc'] diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 84941c7d..d774f3dc 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -4,6 +4,7 @@ from __future__ import print_function import logging import os +import re import shutil import stat import tempfile @@ -265,26 +266,49 @@ class TestGetCmdline(helpers.TestCase): self.assertEqual("abcd 123", ret) -class TestLoadYaml(helpers.TestCase): +class TestLoadYaml(helpers.CiTestCase): mydefault = "7b03a8ebace993d806255121073fed52" + with_logs = True def test_simple(self): mydata = {'1': "one", '2': "two"} self.assertEqual(util.load_yaml(yaml.dump(mydata)), mydata) def test_nonallowed_returns_default(self): + '''Any unallowed types result in returning default; log the issue.''' # for now, anything not in the allowed list just returns the default. myyaml = yaml.dump({'1': "one"}) self.assertEqual(util.load_yaml(blob=myyaml, default=self.mydefault, allowed=(str,)), self.mydefault) - - def test_bogus_returns_default(self): + regex = re.compile( + r'Yaml load allows \(<(class|type) \'str\'>,\) root types, but' + r' got dict') + self.assertTrue(regex.search(self.logs.getvalue()), + msg='Missing expected yaml load error') + + def test_bogus_scan_error_returns_default(self): + '''On Yaml scan error, load_yaml returns the default and logs issue.''' badyaml = "1\n 2:" self.assertEqual(util.load_yaml(blob=badyaml, default=self.mydefault), self.mydefault) + self.assertIn( + 'Failed loading yaml blob. Invalid format at line 2 column 3:' + ' "mapping values are not allowed here', + self.logs.getvalue()) + + def test_bogus_parse_error_returns_default(self): + '''On Yaml parse error, load_yaml returns default and logs issue.''' + badyaml = "{}}" + self.assertEqual(util.load_yaml(blob=badyaml, + default=self.mydefault), + self.mydefault) + self.assertIn( + 'Failed loading yaml blob. Invalid format at line 1 column 3:' + " \"expected \'\', but found \'}\'", + self.logs.getvalue()) def test_unsafe_types(self): # should not load complex types -- cgit v1.2.3 From 4ba4639b86edad7ec89a55a61b7d9075f92d2166 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Tue, 29 May 2018 13:19:27 -0400 Subject: pyflakes: fix unused variable references identified by pyflakes 2.0.0. A newer version of pyflakes (2.0.0) was released. It identifed some unused variables that version 1.6.0 did not identify. The change here merely fixes those unused variables. --- cloudinit/config/cc_mounts.py | 2 +- cloudinit/handlers/upstart_job.py | 2 +- cloudinit/sources/DataSourceAltCloud.py | 16 ++++++++-------- cloudinit/sources/DataSourceNoCloud.py | 4 ++-- cloudinit/sources/DataSourceOpenNebula.py | 2 +- cloudinit/util.py | 4 ++-- 6 files changed, 15 insertions(+), 15 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py index eca6ea3f..339baba9 100644 --- a/cloudinit/config/cc_mounts.py +++ b/cloudinit/config/cc_mounts.py @@ -233,7 +233,7 @@ def setup_swapfile(fname, size=None, maxsize=None): if str(size).lower() == "auto": try: memsize = util.read_meminfo()['total'] - except IOError as e: + except IOError: LOG.debug("Not creating swap: failed to read meminfo") return diff --git a/cloudinit/handlers/upstart_job.py b/cloudinit/handlers/upstart_job.py index 1ca92d4b..dc338769 100644 --- a/cloudinit/handlers/upstart_job.py +++ b/cloudinit/handlers/upstart_job.py @@ -97,7 +97,7 @@ def _has_suitable_upstart(): else: util.logexc(LOG, "dpkg --compare-versions failed [%s]", e.exit_code) - except Exception as e: + except Exception: util.logexc(LOG, "dpkg --compare-versions failed") return False else: diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py index f6e86f34..24fd65ff 100644 --- a/cloudinit/sources/DataSourceAltCloud.py +++ b/cloudinit/sources/DataSourceAltCloud.py @@ -184,11 +184,11 @@ class DataSourceAltCloud(sources.DataSource): cmd = CMD_PROBE_FLOPPY (cmd_out, _err) = util.subp(cmd) LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out) - except ProcessExecutionError as _err: - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err) + except ProcessExecutionError as e: + util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e) return False - except OSError as _err: - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err) + except OSError as e: + util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e) return False floppy_dev = '/dev/fd0' @@ -197,11 +197,11 @@ class DataSourceAltCloud(sources.DataSource): try: (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5) LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out) - except ProcessExecutionError as _err: - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err) + except ProcessExecutionError as e: + util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e) return False - except OSError as _err: - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err) + except OSError as e: + util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e) return False try: diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py index 5d3a8ddb..2daea59d 100644 --- a/cloudinit/sources/DataSourceNoCloud.py +++ b/cloudinit/sources/DataSourceNoCloud.py @@ -78,7 +78,7 @@ class DataSourceNoCloud(sources.DataSource): LOG.debug("Using seeded data from %s", path) mydata = _merge_new_seed(mydata, seeded) break - except ValueError as e: + except ValueError: pass # If the datasource config had a 'seedfrom' entry, then that takes @@ -117,7 +117,7 @@ class DataSourceNoCloud(sources.DataSource): try: seeded = util.mount_cb(dev, _pp2d_callback, pp2d_kwargs) - except ValueError as e: + except ValueError: if dev in label_list: LOG.warning("device %s with label=%s not a" "valid seed.", dev, label) diff --git a/cloudinit/sources/DataSourceOpenNebula.py b/cloudinit/sources/DataSourceOpenNebula.py index d4a41116..16c10785 100644 --- a/cloudinit/sources/DataSourceOpenNebula.py +++ b/cloudinit/sources/DataSourceOpenNebula.py @@ -378,7 +378,7 @@ def read_context_disk_dir(source_dir, asuser=None): if asuser is not None: try: pwd.getpwnam(asuser) - except KeyError as e: + except KeyError: raise BrokenContextDiskDir( "configured user '{user}' does not exist".format( user=asuser)) diff --git a/cloudinit/util.py b/cloudinit/util.py index c0473b8c..6ea6ca76 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -2531,8 +2531,8 @@ def _call_dmidecode(key, dmidecode_path): if result.replace(".", "") == "": return "" return result - except (IOError, OSError) as _err: - LOG.debug('failed dmidecode cmd: %s\n%s', cmd, _err) + except (IOError, OSError) as e: + LOG.debug('failed dmidecode cmd: %s\n%s', cmd, e) return None -- cgit v1.2.3 From 4ce6720104ec92d8d7c5aa993bf7ec405a2f53db Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 15 Jun 2018 20:00:15 -0600 Subject: lxd: Delete default network and detach device if lxd-init created them. Newer versions (3.0.1+) of lxd create the 'lxdbr0' network when 'lxd init --auto' is invoked. When cloud-init is given a network configuration to pass on to lxc and that config had no name specified or 'lxdbr0', then cloud-init would fail to create the network as it already exists. Similarly, we need to remove the device from the default profile so that the attach code can work. Also, add a _lxc method and use it to make sure we're getting the --force-local flag everywhere. LP: #1776958 --- cloudinit/config/cc_lxd.py | 64 ++++++++++++++++--- tests/unittests/test_handler/test_handler_lxd.py | 80 +++++++++++++++++++----- 2 files changed, 120 insertions(+), 24 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_lxd.py b/cloudinit/config/cc_lxd.py index 09374d2e..ac72ac4a 100644 --- a/cloudinit/config/cc_lxd.py +++ b/cloudinit/config/cc_lxd.py @@ -47,11 +47,16 @@ lxd-bridge will be configured accordingly. domain: """ +from cloudinit import log as logging from cloudinit import util import os distros = ['ubuntu'] +LOG = logging.getLogger(__name__) + +_DEFAULT_NETWORK_NAME = "lxdbr0" + def handle(name, cfg, cloud, log, args): # Get config @@ -109,6 +114,7 @@ def handle(name, cfg, cloud, log, args): # Set up lxd-bridge if bridge config is given dconf_comm = "debconf-communicate" if bridge_cfg: + net_name = bridge_cfg.get("name", _DEFAULT_NETWORK_NAME) if os.path.exists("/etc/default/lxd-bridge") \ and util.which(dconf_comm): # Bridge configured through packaging @@ -135,15 +141,18 @@ def handle(name, cfg, cloud, log, args): else: # Built-in LXD bridge support cmd_create, cmd_attach = bridge_to_cmd(bridge_cfg) + maybe_cleanup_default( + net_name=net_name, did_init=bool(init_cfg), + create=bool(cmd_create), attach=bool(cmd_attach)) if cmd_create: log.debug("Creating lxd bridge: %s" % " ".join(cmd_create)) - util.subp(cmd_create) + _lxc(cmd_create) if cmd_attach: log.debug("Setting up default lxd bridge: %s" % " ".join(cmd_create)) - util.subp(cmd_attach) + _lxc(cmd_attach) elif bridge_cfg: raise RuntimeError( @@ -204,10 +213,10 @@ def bridge_to_cmd(bridge_cfg): if bridge_cfg.get("mode") == "none": return None, None - bridge_name = bridge_cfg.get("name", "lxdbr0") + bridge_name = bridge_cfg.get("name", _DEFAULT_NETWORK_NAME) cmd_create = [] - cmd_attach = ["lxc", "network", "attach-profile", bridge_name, - "default", "eth0", "--force-local"] + cmd_attach = ["network", "attach-profile", bridge_name, + "default", "eth0"] if bridge_cfg.get("mode") == "existing": return None, cmd_attach @@ -215,7 +224,7 @@ def bridge_to_cmd(bridge_cfg): if bridge_cfg.get("mode") != "new": raise Exception("invalid bridge mode \"%s\"" % bridge_cfg.get("mode")) - cmd_create = ["lxc", "network", "create", bridge_name] + cmd_create = ["network", "create", bridge_name] if bridge_cfg.get("ipv4_address") and bridge_cfg.get("ipv4_netmask"): cmd_create.append("ipv4.address=%s/%s" % @@ -247,8 +256,47 @@ def bridge_to_cmd(bridge_cfg): if bridge_cfg.get("domain"): cmd_create.append("dns.domain=%s" % bridge_cfg.get("domain")) - cmd_create.append("--force-local") - return cmd_create, cmd_attach + +def _lxc(cmd): + env = {'LC_ALL': 'C'} + util.subp(['lxc'] + list(cmd) + ["--force-local"], update_env=env) + + +def maybe_cleanup_default(net_name, did_init, create, attach, + profile="default", nic_name="eth0"): + """Newer versions of lxc (3.0.1+) create a lxdbr0 network when + 'lxd init --auto' is run. Older versions did not. + + By removing ay that lxd-init created, we simply leave the add/attach + code in-tact. + + https://github.com/lxc/lxd/issues/4649""" + if net_name != _DEFAULT_NETWORK_NAME or not did_init: + return + + fail_assume_enoent = " failed. Assuming it did not exist." + succeeded = " succeeded." + if create: + msg = "Deletion of lxd network '%s'" % net_name + try: + _lxc(["network", "delete", net_name]) + LOG.debug(msg + succeeded) + except util.ProcessExecutionError as e: + if e.exit_code != 1: + raise e + LOG.debug(msg + fail_assume_enoent) + + if attach: + msg = "Removal of device '%s' from profile '%s'" % (nic_name, profile) + try: + _lxc(["profile", "device", "remove", profile, nic_name]) + LOG.debug(msg + succeeded) + except util.ProcessExecutionError as e: + if e.exit_code != 1: + raise e + LOG.debug(msg + fail_assume_enoent) + + # vi: ts=4 expandtab diff --git a/tests/unittests/test_handler/test_handler_lxd.py b/tests/unittests/test_handler/test_handler_lxd.py index a2054980..4dd7e09f 100644 --- a/tests/unittests/test_handler/test_handler_lxd.py +++ b/tests/unittests/test_handler/test_handler_lxd.py @@ -33,12 +33,16 @@ class TestLxd(t_help.CiTestCase): cc = cloud.Cloud(ds, paths, {}, d, None) return cc + @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default") @mock.patch("cloudinit.config.cc_lxd.util") - def test_lxd_init(self, mock_util): + def test_lxd_init(self, mock_util, m_maybe_clean): cc = self._get_cloud('ubuntu') mock_util.which.return_value = True + m_maybe_clean.return_value = None cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, []) self.assertTrue(mock_util.which.called) + # no bridge config, so maybe_cleanup should not be called. + self.assertFalse(m_maybe_clean.called) init_call = mock_util.subp.call_args_list[0][0][0] self.assertEqual(init_call, ['lxd', 'init', '--auto', @@ -46,32 +50,39 @@ class TestLxd(t_help.CiTestCase): '--storage-backend=zfs', '--storage-pool=poolname']) + @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default") @mock.patch("cloudinit.config.cc_lxd.util") - def test_lxd_install(self, mock_util): + def test_lxd_install(self, mock_util, m_maybe_clean): cc = self._get_cloud('ubuntu') cc.distro = mock.MagicMock() mock_util.which.return_value = None cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, []) self.assertNotIn('WARN', self.logs.getvalue()) self.assertTrue(cc.distro.install_packages.called) + cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, []) + self.assertFalse(m_maybe_clean.called) install_pkg = cc.distro.install_packages.call_args_list[0][0][0] self.assertEqual(sorted(install_pkg), ['lxd', 'zfs']) + @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default") @mock.patch("cloudinit.config.cc_lxd.util") - def test_no_init_does_nothing(self, mock_util): + def test_no_init_does_nothing(self, mock_util, m_maybe_clean): cc = self._get_cloud('ubuntu') cc.distro = mock.MagicMock() cc_lxd.handle('cc_lxd', {'lxd': {}}, cc, self.logger, []) self.assertFalse(cc.distro.install_packages.called) self.assertFalse(mock_util.subp.called) + self.assertFalse(m_maybe_clean.called) + @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default") @mock.patch("cloudinit.config.cc_lxd.util") - def test_no_lxd_does_nothing(self, mock_util): + def test_no_lxd_does_nothing(self, mock_util, m_maybe_clean): cc = self._get_cloud('ubuntu') cc.distro = mock.MagicMock() cc_lxd.handle('cc_lxd', {'package_update': True}, cc, self.logger, []) self.assertFalse(cc.distro.install_packages.called) self.assertFalse(mock_util.subp.called) + self.assertFalse(m_maybe_clean.called) def test_lxd_debconf_new_full(self): data = {"mode": "new", @@ -147,14 +158,13 @@ class TestLxd(t_help.CiTestCase): "domain": "lxd"} self.assertEqual( cc_lxd.bridge_to_cmd(data), - (["lxc", "network", "create", "testbr0", + (["network", "create", "testbr0", "ipv4.address=10.0.8.1/24", "ipv4.nat=true", "ipv4.dhcp.ranges=10.0.8.2-10.0.8.254", "ipv6.address=fd98:9e0:3744::1/64", - "ipv6.nat=true", "dns.domain=lxd", - "--force-local"], - ["lxc", "network", "attach-profile", - "testbr0", "default", "eth0", "--force-local"])) + "ipv6.nat=true", "dns.domain=lxd"], + ["network", "attach-profile", + "testbr0", "default", "eth0"])) def test_lxd_cmd_new_partial(self): data = {"mode": "new", @@ -163,19 +173,18 @@ class TestLxd(t_help.CiTestCase): "ipv6_nat": "true"} self.assertEqual( cc_lxd.bridge_to_cmd(data), - (["lxc", "network", "create", "lxdbr0", "ipv4.address=none", - "ipv6.address=fd98:9e0:3744::1/64", "ipv6.nat=true", - "--force-local"], - ["lxc", "network", "attach-profile", - "lxdbr0", "default", "eth0", "--force-local"])) + (["network", "create", "lxdbr0", "ipv4.address=none", + "ipv6.address=fd98:9e0:3744::1/64", "ipv6.nat=true"], + ["network", "attach-profile", + "lxdbr0", "default", "eth0"])) def test_lxd_cmd_existing(self): data = {"mode": "existing", "name": "testbr0"} self.assertEqual( cc_lxd.bridge_to_cmd(data), - (None, ["lxc", "network", "attach-profile", - "testbr0", "default", "eth0", "--force-local"])) + (None, ["network", "attach-profile", + "testbr0", "default", "eth0"])) def test_lxd_cmd_none(self): data = {"mode": "none"} @@ -183,4 +192,43 @@ class TestLxd(t_help.CiTestCase): cc_lxd.bridge_to_cmd(data), (None, None)) + +class TestLxdMaybeCleanupDefault(t_help.CiTestCase): + """Test the implementation of maybe_cleanup_default.""" + + defnet = cc_lxd._DEFAULT_NETWORK_NAME + + @mock.patch("cloudinit.config.cc_lxd._lxc") + def test_network_other_than_default_not_deleted(self, m_lxc): + """deletion or removal should only occur if bridge is default.""" + cc_lxd.maybe_cleanup_default( + net_name="lxdbr1", did_init=True, create=True, attach=True) + m_lxc.assert_not_called() + + @mock.patch("cloudinit.config.cc_lxd._lxc") + def test_did_init_false_does_not_delete(self, m_lxc): + """deletion or removal should only occur if did_init is True.""" + cc_lxd.maybe_cleanup_default( + net_name=self.defnet, did_init=False, create=True, attach=True) + m_lxc.assert_not_called() + + @mock.patch("cloudinit.config.cc_lxd._lxc") + def test_network_deleted_if_create_true(self, m_lxc): + """deletion of network should occur if create is True.""" + cc_lxd.maybe_cleanup_default( + net_name=self.defnet, did_init=True, create=True, attach=False) + m_lxc.assert_called_once_with(["network", "delete", self.defnet]) + + @mock.patch("cloudinit.config.cc_lxd._lxc") + def test_device_removed_if_attach_true(self, m_lxc): + """deletion of network should occur if create is True.""" + nic_name = "my_nic" + profile = "my_profile" + cc_lxd.maybe_cleanup_default( + net_name=self.defnet, did_init=True, create=False, attach=True, + profile=profile, nic_name=nic_name) + m_lxc.assert_called_once_with( + ["profile", "device", "remove", profile, nic_name]) + + # vi: ts=4 expandtab -- cgit v1.2.3 From a670eb816c8bd73262f29f3821b64b59ae09da94 Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Tue, 19 Jun 2018 16:47:58 -0600 Subject: docs: represent sudo:false in docs for user_groups config module --- cloudinit/config/cc_users_groups.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'cloudinit/config') diff --git a/cloudinit/config/cc_users_groups.py b/cloudinit/config/cc_users_groups.py index b215e95a..c95bdaad 100644 --- a/cloudinit/config/cc_users_groups.py +++ b/cloudinit/config/cc_users_groups.py @@ -54,8 +54,9 @@ config keys for an entry in ``users`` are as follows: - ``ssh_authorized_keys``: Optional. List of ssh keys to add to user's authkeys file. Default: none - ``ssh_import_id``: Optional. SSH id to import for user. Default: none - - ``sudo``: Optional. Sudo rule to use, or list of sudo rules to use. - Default: none. + - ``sudo``: Optional. Sudo rule to use, list of sudo rules to use or False. + Default: none. An absence of sudo key, or a value of none or false + will result in no sudo rules being written for the user. - ``system``: Optional. Create user as system user with no home directory. Default: false - ``uid``: Optional. The user's ID. Default: The next available value. @@ -82,6 +83,9 @@ config keys for an entry in ``users`` are as follows: users: - default + # User explicitly omitted from sudo permission; also default behavior. + - name: + sudo: false - name: expiredate: gecos: -- cgit v1.2.3