From 0f1a2cbe434cba243ce65ff43a88722c2bcf6f2c Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Thu, 11 Oct 2012 12:49:45 -0700 Subject: More adjustments/cleanups for the system configuration helper objects. 1. Add in a parser for the /etc/hostname file that can be shared 2. Adjust the sysconfig configobj parser to not always quote fields that it does not need to quote + add in tests around this to ensure that we don't go nuts with quoting again. --- tests/unittests/test_distros/test_sysconfig.py | 59 ++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 tests/unittests/test_distros/test_sysconfig.py (limited to 'tests/unittests/test_distros/test_sysconfig.py') diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py new file mode 100644 index 00000000..196d090d --- /dev/null +++ b/tests/unittests/test_distros/test_sysconfig.py @@ -0,0 +1,59 @@ +from mocker import MockerTestCase + +from cloudinit.distros.parsers.sys_conf import SysConf + + +# Lots of good examples @ +# http://content.hccfl.edu/pollock/AUnix1/SysconfigFilesDesc.txt + +class TestSysConfHelper(MockerTestCase): + def test_parse_no_change(self): + contents = '''# A comment +USESMBAUTH=no +KEYTABLE=/usr/lib/kbd/keytables/us.map +SHORTDATE=$(date +%y:%m:%d:%H:%M) +HOSTNAME=blahblah +NETMASK0=255.255.255.0 +# Inline comment +LIST=$LOGROOT/incremental-list +IPV6TO4_ROUTING="eth0-:0004::1/64 eth1-:0005::1/64" +ETHTOOL_OPTS="-K ${DEVICE} tso on; -G ${DEVICE} rx 256 tx 256" +USEMD5=no''' + conf = SysConf(contents.splitlines()) + self.assertEquals(conf['HOSTNAME'], 'blahblah') + self.assertEquals(conf['SHORTDATE'], '$(date +%y:%m:%d:%H:%M)') + # Should be unquoted + self.assertEquals(conf['ETHTOOL_OPTS'], ('-K ${DEVICE} tso on; ' + '-G ${DEVICE} rx 256 tx 256')) + self.assertEquals(contents, str(conf)) + + def test_parse_adjust(self): + contents = 'IPV6TO4_ROUTING="eth0-:0004::1/64 eth1-:0005::1/64"' + conf = SysConf(contents.splitlines()) + # Should be unquoted + self.assertEquals('eth0-:0004::1/64 eth1-:0005::1/64', + conf['IPV6TO4_ROUTING']) + conf['IPV6TO4_ROUTING'] = "blah \tblah" + contents2 = str(conf).strip() + # Should be requoted due to whitespace + self.assertEquals('IPV6TO4_ROUTING="blah \tblah"', contents2) + + def test_parse_no_adjust_shell(self): + conf = SysConf(''.splitlines()) + conf['B'] = ' $(time)' + contents = str(conf) + self.assertEquals('B= $(time)', contents) + + def test_parse_empty(self): + contents = '' + conf = SysConf(contents.splitlines()) + self.assertEquals('', str(conf).strip()) + + def test_parse_add_new(self): + contents = 'BLAH=b' + conf = SysConf(contents.splitlines()) + conf['Z'] = 'd' + contents = str(conf) + self.assertIn("Z=d", contents) + self.assertIn("BLAH=b", contents) + -- cgit v1.2.3 From 66f6d2d4fd1e8c1b397c9533ee0596d7b09e9824 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Thu, 11 Oct 2012 18:48:50 -0700 Subject: Update to use pipes.quote to ensure that variables adjusted in sysconfig files are properly quoted for there common use case, that being sourced into shell scripts. --- cloudinit/distros/parsers/sys_conf.py | 24 ++++++++++-------------- tests/unittests/test_distros/test_sysconfig.py | 14 +++++++++++--- 2 files changed, 21 insertions(+), 17 deletions(-) (limited to 'tests/unittests/test_distros/test_sysconfig.py') diff --git a/cloudinit/distros/parsers/sys_conf.py b/cloudinit/distros/parsers/sys_conf.py index 3d8802b8..7549c7a3 100644 --- a/cloudinit/distros/parsers/sys_conf.py +++ b/cloudinit/distros/parsers/sys_conf.py @@ -18,6 +18,7 @@ from StringIO import StringIO +import pipes import re # This library is used to parse/write @@ -30,6 +31,7 @@ import re import configobj + class SysConf(configobj.ConfigObj): def __init__(self, contents): configobj.ConfigObj.__init__(self, contents, @@ -50,24 +52,18 @@ class SysConf(configobj.ConfigObj): raise ValueError('Value "%s" is not a string' % (value)) if len(value) == 0: return '' - if re.search(r"[\n\r]", value): - raise ValueError('Value "%s" cannot be safely quoted.' % (value)) - quot = "%s" - if '#' in value: - quot = self._get_single_quote(value) - elif value[0] in ['"', "'"] and value[-1] in ['"', "'"]: - # Already quoted, leave it be - pass - elif "'" in value and '"' in value: - quot = self._get_triple_quote(value) + quot_func = (lambda x: str(x)) + if value[0] in ['"', "'"] and value[-1] in ['"', "'"]: + if len(value) == 1: + quot_func = self._get_single_quote else: - # Quote whitespace if it isn't the start+end of a shell command + # Quote whitespace if it isn't the start + end of a shell command white_space_ok = False if value.strip().startswith("$(") and value.strip().endswith(")"): white_space_ok = True - if re.search(r"[\t ]", value) and not white_space_ok: - quot = self._get_single_quote(value) - return quot % (value) + if re.search(r"[\t\r\n ]", value) and not white_space_ok: + quot_func = pipes.quote + return quot_func(value) def _write_line(self, indent_string, entry, this_entry, comment): # Ensure it is formatted fine for diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py index 196d090d..a07a251e 100644 --- a/tests/unittests/test_distros/test_sysconfig.py +++ b/tests/unittests/test_distros/test_sysconfig.py @@ -1,5 +1,7 @@ from mocker import MockerTestCase +import re + from cloudinit.distros.parsers.sys_conf import SysConf @@ -7,6 +9,11 @@ from cloudinit.distros.parsers.sys_conf import SysConf # http://content.hccfl.edu/pollock/AUnix1/SysconfigFilesDesc.txt class TestSysConfHelper(MockerTestCase): + def assertRegexpMatches(self, text, regexp): + regexp = re.compile(regexp) + self.assertTrue(regexp.search(text), + msg="%s must match %s!" % (text, regexp.pattern)) + def test_parse_no_change(self): contents = '''# A comment USESMBAUTH=no @@ -16,8 +23,8 @@ HOSTNAME=blahblah NETMASK0=255.255.255.0 # Inline comment LIST=$LOGROOT/incremental-list -IPV6TO4_ROUTING="eth0-:0004::1/64 eth1-:0005::1/64" -ETHTOOL_OPTS="-K ${DEVICE} tso on; -G ${DEVICE} rx 256 tx 256" +IPV6TO4_ROUTING='eth0-:0004::1/64 eth1-:0005::1/64' +ETHTOOL_OPTS='-K ${DEVICE} tso on; -G ${DEVICE} rx 256 tx 256' USEMD5=no''' conf = SysConf(contents.splitlines()) self.assertEquals(conf['HOSTNAME'], 'blahblah') @@ -25,6 +32,7 @@ USEMD5=no''' # Should be unquoted self.assertEquals(conf['ETHTOOL_OPTS'], ('-K ${DEVICE} tso on; ' '-G ${DEVICE} rx 256 tx 256')) + # This is harmless convertion self.assertEquals(contents, str(conf)) def test_parse_adjust(self): @@ -36,7 +44,7 @@ USEMD5=no''' conf['IPV6TO4_ROUTING'] = "blah \tblah" contents2 = str(conf).strip() # Should be requoted due to whitespace - self.assertEquals('IPV6TO4_ROUTING="blah \tblah"', contents2) + self.assertRegexpMatches(contents2, r'IPV6TO4_ROUTING=["\']blah \tblah["\']') def test_parse_no_adjust_shell(self): conf = SysConf(''.splitlines()) -- cgit v1.2.3 From 3cf8b618f9efcdb2e9cca695c2930a2bf14d42ec Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Thu, 11 Oct 2012 19:10:42 -0700 Subject: Handle the case where the value contains shell variables to be expanded which when using pipes.quote will now not be expanded, so add some checks to ensure that this case will still happen. --- cloudinit/distros/parsers/sys_conf.py | 17 ++++++++++++++++- tests/unittests/test_distros/test_sysconfig.py | 5 ++--- 2 files changed, 18 insertions(+), 4 deletions(-) (limited to 'tests/unittests/test_distros/test_sysconfig.py') diff --git a/cloudinit/distros/parsers/sys_conf.py b/cloudinit/distros/parsers/sys_conf.py index 7549c7a3..80c305fe 100644 --- a/cloudinit/distros/parsers/sys_conf.py +++ b/cloudinit/distros/parsers/sys_conf.py @@ -31,6 +31,12 @@ import re import configobj +def _contains_shell_variable(text): + if (re.search(r"\$\{.+\}", text) or + re.search(r"\$[a-zA-Z_]+[a-zA-Z0-9_]*", text)): + return True + return False + class SysConf(configobj.ConfigObj): def __init__(self, contents): @@ -62,7 +68,16 @@ class SysConf(configobj.ConfigObj): if value.strip().startswith("$(") and value.strip().endswith(")"): white_space_ok = True if re.search(r"[\t\r\n ]", value) and not white_space_ok: - quot_func = pipes.quote + if _contains_shell_variable(value): + # If it contains shell variables then we likely want to + # leave it alone since the pipes.quote function likes to + # use single quotes which won't get expanded... + if re.search(r"[\n\"']", value): + quot_func = (lambda x: self._get_triple_quote(x) % x) + else: + quot_func = (lambda x: self._get_single_quote(x) % x) + else: + quot_func = pipes.quote return quot_func(value) def _write_line(self, indent_string, entry, this_entry, comment): diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py index a07a251e..21e161ad 100644 --- a/tests/unittests/test_distros/test_sysconfig.py +++ b/tests/unittests/test_distros/test_sysconfig.py @@ -24,7 +24,7 @@ NETMASK0=255.255.255.0 # Inline comment LIST=$LOGROOT/incremental-list IPV6TO4_ROUTING='eth0-:0004::1/64 eth1-:0005::1/64' -ETHTOOL_OPTS='-K ${DEVICE} tso on; -G ${DEVICE} rx 256 tx 256' +ETHTOOL_OPTS="-K ${DEVICE} tso on; -G ${DEVICE} rx 256 tx 256" USEMD5=no''' conf = SysConf(contents.splitlines()) self.assertEquals(conf['HOSTNAME'], 'blahblah') @@ -32,7 +32,6 @@ USEMD5=no''' # Should be unquoted self.assertEquals(conf['ETHTOOL_OPTS'], ('-K ${DEVICE} tso on; ' '-G ${DEVICE} rx 256 tx 256')) - # This is harmless convertion self.assertEquals(contents, str(conf)) def test_parse_adjust(self): @@ -44,7 +43,7 @@ USEMD5=no''' conf['IPV6TO4_ROUTING'] = "blah \tblah" contents2 = str(conf).strip() # Should be requoted due to whitespace - self.assertRegexpMatches(contents2, r'IPV6TO4_ROUTING=["\']blah \tblah["\']') + self.assertRegexpMatches(contents2, r'IPV6TO4_ROUTING=[\']blah\s+blah[\']') def test_parse_no_adjust_shell(self): conf = SysConf(''.splitlines()) -- cgit v1.2.3 From bbe325c902ef3a3b8845cd3c1bb8bee0c3c74a89 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Thu, 11 Oct 2012 21:24:30 -0700 Subject: Add some more sysconfig tests + pylint cleanups. --- cloudinit/distros/__init__.py | 3 +- cloudinit/distros/parsers/sys_conf.py | 59 +++++++++++++++++--------- tests/unittests/test_distros/test_sysconfig.py | 21 ++++++++- 3 files changed, 59 insertions(+), 24 deletions(-) (limited to 'tests/unittests/test_distros/test_sysconfig.py') diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index bafa69d3..fa7cc1ca 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -132,7 +132,8 @@ class Distro(object): # temporarily (until reboot so it should # not be depended on). Use the write # hostname functions for 'permanent' adjustments. - LOG.debug("Non-persistently setting the system hostname to %s", hostname) + LOG.debug("Non-persistently setting the system hostname to %s", + hostname) try: util.subp(['hostname', hostname]) except util.ProcessExecutionError: diff --git a/cloudinit/distros/parsers/sys_conf.py b/cloudinit/distros/parsers/sys_conf.py index 1cefb8bc..5cd765fc 100644 --- a/cloudinit/distros/parsers/sys_conf.py +++ b/cloudinit/distros/parsers/sys_conf.py @@ -22,7 +22,7 @@ import pipes import re # This library is used to parse/write -# out the various sysconfig files edited +# out the various sysconfig files edited (best attempt effort) # # It has to be slightly modified though # to ensure that all values are quoted/unquoted correctly @@ -30,11 +30,26 @@ import re # bash scripts... import configobj +# See: http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap08.html +# or look at the 'param_expand()' function in the subst.c file in the bash +# source tarball... +SHELL_VAR_RULE = r'[a-zA-Z_]+[a-zA-Z0-9_]*' +SHELL_VAR_REGEXES = [ + # Basic variables + re.compile(r"\$" + SHELL_VAR_RULE), + # Things like $?, $0, $-, $@ + re.compile(r"\$[0-9#\?\-@\*]"), + # Things like ${blah:1} - but this one + # gets very complex so just try the + # simple path + re.compile(r"\$\{.+\}"), +] + def _contains_shell_variable(text): - if (re.search(r"\$\{.+\}", text) or - re.search(r"\$[a-zA-Z_]+[a-zA-Z0-9_]*", text)): - return True + for r in SHELL_VAR_REGEXES: + if r.search(text): + return True return False @@ -58,33 +73,36 @@ class SysConf(configobj.ConfigObj): raise ValueError('Value "%s" is not a string' % (value)) if len(value) == 0: return '' - quot_func = (lambda x: str(x)) + quot_func = None if value[0] in ['"', "'"] and value[-1] in ['"', "'"]: if len(value) == 1: - quot_func = (lambda x: self._get_single_quote(x) % x) + quot_func = (lambda x: + self._get_single_quote(x) % x) else: # Quote whitespace if it isn't the start + end of a shell command - white_space_ok = False if value.strip().startswith("$(") and value.strip().endswith(")"): - white_space_ok = True - if re.search(r"[\t\r\n ]", value) and not white_space_ok: - if _contains_shell_variable(value): - # If it contains shell variables then we likely want to - # leave it alone since the pipes.quote function likes to - # use single quotes which won't get expanded... - if re.search(r"[\n\"']", value): - quot_func = (lambda x: self._get_triple_quote(x) % x) + pass + else: + if re.search(r"[\t\r\n ]", value): + if _contains_shell_variable(value): + # If it contains shell variables then we likely want to + # leave it alone since the pipes.quote function likes + # to use single quotes which won't get expanded... + if re.search(r"[\n\"']", value): + quot_func = (lambda x: + self._get_triple_quote(x) % x) + else: + quot_func = (lambda x: + self._get_single_quote(x) % x) else: - quot_func = (lambda x: self._get_single_quote(x) % x) - else: - quot_func = pipes.quote + quot_func = pipes.quote + if not quot_func: + return value return quot_func(value) def _write_line(self, indent_string, entry, this_entry, comment): # Ensure it is formatted fine for # how these sysconfig scripts are used - if this_entry.startswith("'") or this_entry.startswith('"'): - val = this_entry val = self._decode_element(self._quote(this_entry)) key = self._decode_element(self._quote(entry)) cmnt = self._decode_element(comment) @@ -93,4 +111,3 @@ class SysConf(configobj.ConfigObj): self._a_to_u('='), val, cmnt) - diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py index 21e161ad..1e34909d 100644 --- a/tests/unittests/test_distros/test_sysconfig.py +++ b/tests/unittests/test_distros/test_sysconfig.py @@ -9,7 +9,8 @@ from cloudinit.distros.parsers.sys_conf import SysConf # http://content.hccfl.edu/pollock/AUnix1/SysconfigFilesDesc.txt class TestSysConfHelper(MockerTestCase): - def assertRegexpMatches(self, text, regexp): + # This function was added in 2.7, make it work for 2.6 + def assertRegMatches(self, text, regexp): regexp = re.compile(regexp) self.assertTrue(regexp.search(text), msg="%s must match %s!" % (text, regexp.pattern)) @@ -34,6 +35,21 @@ USEMD5=no''' '-G ${DEVICE} rx 256 tx 256')) self.assertEquals(contents, str(conf)) + def test_parse_shell_vars(self): + contents = 'USESMBAUTH=$XYZ' + conf = SysConf(contents.splitlines()) + self.assertEquals(contents, str(conf)) + conf = SysConf('') + conf['B'] = '${ZZ}d apples' + # Should be quoted + self.assertEquals('B="${ZZ}d apples"', str(conf)) + conf = SysConf('') + conf['B'] = '$? d apples' + self.assertEquals('B="$? d apples"', str(conf)) + contents = 'IPMI_WATCHDOG_OPTIONS="timeout=60"' + conf = SysConf(contents.splitlines()) + self.assertEquals('IPMI_WATCHDOG_OPTIONS=timeout=60', str(conf)) + def test_parse_adjust(self): contents = 'IPV6TO4_ROUTING="eth0-:0004::1/64 eth1-:0005::1/64"' conf = SysConf(contents.splitlines()) @@ -43,7 +59,8 @@ USEMD5=no''' conf['IPV6TO4_ROUTING'] = "blah \tblah" contents2 = str(conf).strip() # Should be requoted due to whitespace - self.assertRegexpMatches(contents2, r'IPV6TO4_ROUTING=[\']blah\s+blah[\']') + self.assertRegMatches(contents2, + r'IPV6TO4_ROUTING=[\']blah\s+blah[\']') def test_parse_no_adjust_shell(self): conf = SysConf(''.splitlines()) -- cgit v1.2.3 From c8c2ff830f917a7ddecab21eeecb99a20c2e9805 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Mon, 12 Nov 2012 22:14:31 -0800 Subject: Pylint and pep8 cleanups. --- cloudinit/distros/parsers/hosts.py | 1 - cloudinit/distros/rhel.py | 10 +++++----- tests/unittests/test_distros/test_sysconfig.py | 1 - 3 files changed, 5 insertions(+), 7 deletions(-) (limited to 'tests/unittests/test_distros/test_sysconfig.py') diff --git a/cloudinit/distros/parsers/hosts.py b/cloudinit/distros/parsers/hosts.py index 958a7c31..94c97051 100644 --- a/cloudinit/distros/parsers/hosts.py +++ b/cloudinit/distros/parsers/hosts.py @@ -90,4 +90,3 @@ class HostsConf(object): pieces = "\t".join(pieces) contents.write("%s%s\n" % (pieces, tail)) return contents.getvalue() - diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py index 7df01c62..bc0877d5 100644 --- a/cloudinit/distros/rhel.py +++ b/cloudinit/distros/rhel.py @@ -48,7 +48,7 @@ class Distro(distros.Distro): clock_conf_fn = "/etc/sysconfig/clock" locale_conf_fn = '/etc/sysconfig/i18n' network_conf_fn = "/etc/sysconfig/network" - hostname_conf_fn = "/etc/sysconfig/network" + hostname_conf_fn = "/etc/sysconfig/network" network_script_tpl = '/etc/sysconfig/network-scripts/ifcfg-%s' resolve_conf_fn = "/etc/resolv.conf" tz_local_fn = "/etc/localtime" @@ -65,11 +65,11 @@ class Distro(distros.Distro): self.package_command('install', pkglist) def _adjust_resolve(self, dns_servers, search_servers): - r_conf = ResolvConf(util.load_file(self.resolve_conf_fn)) try: + r_conf = ResolvConf(util.load_file(self.resolve_conf_fn)) r_conf.parse() except IOError: - util.logexc(LOG, + util.logexc(LOG, "Failed at parsing %s reverting to an empty instance", self.resolve_conf_fn) r_conf = ResolvConf('') @@ -178,10 +178,10 @@ class Distro(distros.Distro): def _read_conf(self, fn): exists = False - if os.path.isfile(fn): + try: contents = util.load_file(fn).splitlines() exists = True - else: + except IOError: contents = [] return (exists, SysConf(contents)) diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py index 1e34909d..0c651407 100644 --- a/tests/unittests/test_distros/test_sysconfig.py +++ b/tests/unittests/test_distros/test_sysconfig.py @@ -80,4 +80,3 @@ USEMD5=no''' contents = str(conf) self.assertIn("Z=d", contents) self.assertIn("BLAH=b", contents) - -- cgit v1.2.3