From 4bcc18d9d35393eb454f131eef434d9e67af1456 Mon Sep 17 00:00:00 2001 From: yan Date: Sat, 18 Apr 2015 10:20:19 -0700 Subject: [PATCH] Address @kuba's review comments --- .../client/plugins/nginx/configurator.py | 30 ++-- .../client/plugins/nginx/nginxparser.py | 24 +-- .../plugins/nginx/tests/nginxparser_test.py | 22 +-- .../client/plugins/nginx/tests/parser_test.py | 154 +++++++++--------- 4 files changed, 113 insertions(+), 117 deletions(-) diff --git a/letsencrypt/client/plugins/nginx/configurator.py b/letsencrypt/client/plugins/nginx/configurator.py index d799432f3..47a732070 100644 --- a/letsencrypt/client/plugins/nginx/configurator.py +++ b/letsencrypt/client/plugins/nginx/configurator.py @@ -90,11 +90,14 @@ class NginxConfigurator(object): # Entry point in main.py for installing cert def deploy_cert(self, domain, cert, key, cert_chain=None): # pylint: disable=unused-argument - """Deploys certificate to specified virtual host. Aborts if the - vhost is missing ssl_certificate or ssl_certificate_key. + """Deploys certificate to specified virtual host. - Nginx doesn't have a cert chain directive, so the last parameter is - always ignored. It expects the cert file to have the concatenated chain. + .. note:: Aborts if the vhost is missing ssl_certificate or + ssl_certificate_key. + + .. note:: Nginx doesn't have a cert chain directive, so the last + parameter is always ignored. It expects the cert file to have + the concatenated chain. .. note:: This doesn't save the config files! @@ -130,9 +133,11 @@ class NginxConfigurator(object): # Vhost parsing methods ####################### def choose_vhost(self, target_name): - """Chooses a virtual host based on the given domain name. NOTE: This - makes the vhost SSL-enabled if it isn't already. Follows Nginx's server - block selection rules but prefers blocks that are already SSL. + """Chooses a virtual host based on the given domain name. + + .. note:: This makes the vhost SSL-enabled if it isn't already. Follows + Nginx's server block selection rules preferring blocks that are + already SSL. .. todo:: This should maybe return list if no obvious answer is presented. @@ -149,10 +154,10 @@ class NginxConfigurator(object): vhost = None matches = self._get_ranked_matches(target_name) - if len(matches) == 0: + if not matches: # No matches at all :'( pass - elif matches[0]['rank'] in range(2, 6): + elif matches[0]['rank'] in xrange(2, 6): # Wildcard match - need to find the longest one rank = matches[0]['rank'] wildcards = [x for x in matches if x['rank'] == rank] @@ -167,8 +172,7 @@ class NginxConfigurator(object): return vhost def _get_ranked_matches(self, target_name): - """ - Returns a ranked list of vhosts that match target_name. + """Returns a ranked list of vhosts that match target_name. :param str target_name: The name to match :returns: list of dicts containing the vhost, the matching name, and @@ -374,10 +378,10 @@ class NginxConfigurator(object): sni_regex = re.compile(r"TLS SNI support enabled", re.IGNORECASE) sni_matches = sni_regex.findall(text) - if len(version_matches) == 0: + if not version_matches: raise errors.LetsEncryptConfiguratorError( "Unable to find Nginx version") - if len(sni_matches) == 0: + if not sni_matches: raise errors.LetsEncryptConfiguratorError( "Nginx build doesn't support SNI") diff --git a/letsencrypt/client/plugins/nginx/nginxparser.py b/letsencrypt/client/plugins/nginx/nginxparser.py index 947c05f2e..18ba8b0bd 100644 --- a/letsencrypt/client/plugins/nginx/nginxparser.py +++ b/letsencrypt/client/plugins/nginx/nginxparser.py @@ -8,9 +8,7 @@ from pyparsing import ( class RawNginxParser(object): # pylint: disable=expression-not-assigned - """ - A class that parses nginx configuration with pyparsing - """ + """A class that parses nginx configuration with pyparsing.""" # constants left_bracket = Literal("{").suppress() @@ -39,31 +37,23 @@ class RawNginxParser(object): self.source = source def parse(self): - """ - Returns the parsed tree. - """ + """Returns the parsed tree.""" return self.script.parseString(self.source) def as_list(self): - """ - Returns the list of tree. - """ + """Returns the parsed tree as a list.""" return self.parse().asList() class RawNginxDumper(object): # pylint: disable=too-few-public-methods - """ - A class that dumps nginx configuration from the provided tree. - """ + """A class that dumps nginx configuration from the provided tree.""" def __init__(self, blocks, indentation=4): self.blocks = blocks self.indentation = indentation def __iter__(self, blocks=None, current_indent=0, spacer=' '): - """ - Iterates the dumped nginx content. - """ + """Iterates the dumped nginx content.""" blocks = blocks or self.blocks for key, values in blocks: if current_indent: @@ -88,9 +78,7 @@ class RawNginxDumper(object): yield spacer * current_indent + key + spacer + values + ';' def as_string(self): - """ - Return the parsed block as a string. - """ + """Return the parsed block as a string.""" return '\n'.join(self) diff --git a/letsencrypt/client/plugins/nginx/tests/nginxparser_test.py b/letsencrypt/client/plugins/nginx/tests/nginxparser_test.py index b249b25cc..2e19e71d1 100644 --- a/letsencrypt/client/plugins/nginx/tests/nginxparser_test.py +++ b/letsencrypt/client/plugins/nginx/tests/nginxparser_test.py @@ -48,17 +48,17 @@ class TestRawNginxParser(unittest.TestCase): ]]]) self.assertEqual(dumped, - 'user www-data;\n' + - 'server {\n' + - ' listen 80;\n' + - ' server_name foo.com;\n' + - ' root /home/ubuntu/sites/foo/;\n \n' + - ' location /status {\n' + - ' check_status;\n \n' + - ' types {\n' + - ' image/jpeg jpg;\n' + - ' }\n' + - ' }\n' + + 'user www-data;\n' + 'server {\n' + ' listen 80;\n' + ' server_name foo.com;\n' + ' root /home/ubuntu/sites/foo/;\n \n' + ' location /status {\n' + ' check_status;\n \n' + ' types {\n' + ' image/jpeg jpg;\n' + ' }\n' + ' }\n' '}') def test_parse_from_file(self): diff --git a/letsencrypt/client/plugins/nginx/tests/parser_test.py b/letsencrypt/client/plugins/nginx/tests/parser_test.py index a76f2da25..21e96aa26 100644 --- a/letsencrypt/client/plugins/nginx/tests/parser_test.py +++ b/letsencrypt/client/plugins/nginx/tests/parser_test.py @@ -6,9 +6,9 @@ import shutil import unittest from letsencrypt.client.errors import LetsEncryptMisconfigurationError -from letsencrypt.client.plugins.nginx.nginxparser import dumps -from letsencrypt.client.plugins.nginx.obj import Addr, VirtualHost -from letsencrypt.client.plugins.nginx.parser import NginxParser, get_best_match +from letsencrypt.client.plugins.nginx import nginxparser +from letsencrypt.client.plugins.nginx import obj +from letsencrypt.client.plugins.nginx import parser from letsencrypt.client.plugins.nginx.tests import util @@ -26,52 +26,52 @@ class NginxParserTest(util.NginxTest): def test_root_normalized(self): path = os.path.join(self.temp_dir, "foo/////" "bar/../../testdata") - parser = NginxParser(path, None) - self.assertEqual(parser.root, self.config_path) + nparser = parser.NginxParser(path, None) + self.assertEqual(nparser.root, self.config_path) def test_root_absolute(self): - parser = NginxParser(os.path.relpath(self.config_path), None) - self.assertEqual(parser.root, self.config_path) + nparser = parser.NginxParser(os.path.relpath(self.config_path), None) + self.assertEqual(nparser.root, self.config_path) def test_root_no_trailing_slash(self): - parser = NginxParser(self.config_path + os.path.sep, None) - self.assertEqual(parser.root, self.config_path) + nparser = parser.NginxParser(self.config_path + os.path.sep, None) + self.assertEqual(nparser.root, self.config_path) def test_load(self): """Test recursive conf file parsing. """ - parser = NginxParser(self.config_path, self.ssl_options) - parser.load() - self.assertEqual(set([parser.abs_path(x) for x in + nparser = parser.NginxParser(self.config_path, self.ssl_options) + nparser.load() + self.assertEqual(set([nparser.abs_path(x) for x in ['foo.conf', 'nginx.conf', 'server.conf', 'sites-enabled/default', 'sites-enabled/example.com']]), - set(parser.parsed.keys())) + set(nparser.parsed.keys())) self.assertEqual([['server_name', 'somename alias another.alias']], - parser.parsed[parser.abs_path('server.conf')]) + nparser.parsed[nparser.abs_path('server.conf')]) self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*']]]], - parser.parsed[parser.abs_path( + nparser.parsed[nparser.abs_path( 'sites-enabled/example.com')]) def test_abs_path(self): - parser = NginxParser(self.config_path, self.ssl_options) - self.assertEqual('/etc/nginx/*', parser.abs_path('/etc/nginx/*')) + nparser = parser.NginxParser(self.config_path, self.ssl_options) + self.assertEqual('/etc/nginx/*', nparser.abs_path('/etc/nginx/*')) self.assertEqual(os.path.join(self.config_path, 'foo/bar/'), - parser.abs_path('foo/bar/')) + nparser.abs_path('foo/bar/')) def test_filedump(self): - parser = NginxParser(self.config_path, self.ssl_options) - parser.filedump('test') + nparser = parser.NginxParser(self.config_path, self.ssl_options) + nparser.filedump('test') # pylint: disable=protected-access - parsed = parser._parse_files(parser.abs_path( + parsed = nparser._parse_files(nparser.abs_path( 'sites-enabled/example.com.test')) - self.assertEqual(3, len(glob.glob(parser.abs_path('*.test')))) + self.assertEqual(3, len(glob.glob(nparser.abs_path('*.test')))) self.assertEqual(2, len( - glob.glob(parser.abs_path('sites-enabled/*.test')))) + glob.glob(nparser.abs_path('sites-enabled/*.test')))) self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], @@ -79,31 +79,34 @@ class NginxParserTest(util.NginxTest): parsed[0]) def test_get_vhosts(self): - parser = NginxParser(self.config_path, self.ssl_options) - vhosts = parser.get_vhosts() + nparser = parser.NginxParser(self.config_path, self.ssl_options) + vhosts = nparser.get_vhosts() - vhost1 = VirtualHost(parser.abs_path('nginx.conf'), - [Addr('', '8080', False, False)], - False, True, set(['localhost', - r'~^(www\.)?(example|bar)\.']), - []) - vhost2 = VirtualHost(parser.abs_path('nginx.conf'), - [Addr('somename', '8080', False, False), - Addr('', '8000', False, False)], - False, True, set(['somename', - 'another.alias', 'alias']), []) - vhost3 = VirtualHost(parser.abs_path('sites-enabled/example.com'), - [Addr('69.50.225.155', '9000', False, False), - Addr('127.0.0.1', '', False, False)], - False, True, set(['.example.com', 'example.*']), - []) - vhost4 = VirtualHost(parser.abs_path('sites-enabled/default'), - [Addr('myhost', '', False, True)], - False, True, set(['www.example.org']), []) - vhost5 = VirtualHost(parser.abs_path('foo.conf'), - [Addr('*', '80', True, True)], - True, True, set(['*.www.foo.com', - '*.www.example.com']), []) + vhost1 = obj.VirtualHost(nparser.abs_path('nginx.conf'), + [obj.Addr('', '8080', False, False)], + False, True, + set(['localhost', + r'~^(www\.)?(example|bar)\.']), + []) + vhost2 = obj.VirtualHost(nparser.abs_path('nginx.conf'), + [obj.Addr('somename', '8080', False, False), + obj.Addr('', '8000', False, False)], + False, True, + set(['somename', 'another.alias', 'alias']), + []) + vhost3 = obj.VirtualHost(nparser.abs_path('sites-enabled/example.com'), + [obj.Addr('69.50.225.155', '9000', + False, False), + obj.Addr('127.0.0.1', '', False, False)], + False, True, + set(['.example.com', 'example.*']), []) + vhost4 = obj.VirtualHost(nparser.abs_path('sites-enabled/default'), + [obj.Addr('myhost', '', False, True)], + False, True, set(['www.example.org']), []) + vhost5 = obj.VirtualHost(nparser.abs_path('foo.conf'), + [obj.Addr('*', '80', True, True)], + True, True, set(['*.www.foo.com', + '*.www.example.com']), []) self.assertEqual(5, len(vhosts)) example_com = [x for x in vhosts if 'example.com' in x.filep][0] @@ -118,39 +121,39 @@ class NginxParserTest(util.NginxTest): self.assertEquals(vhost2, somename) def test_add_server_directives(self): - parser = NginxParser(self.config_path, self.ssl_options) - parser.add_server_directives(parser.abs_path('nginx.conf'), - set(['localhost', - r'~^(www\.)?(example|bar)\.']), - [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert.pem']]) + nparser = parser.NginxParser(self.config_path, self.ssl_options) + nparser.add_server_directives(nparser.abs_path('nginx.conf'), + set(['localhost', + r'~^(www\.)?(example|bar)\.']), + [['foo', 'bar'], ['ssl_certificate', + '/etc/ssl/cert.pem']]) ssl_re = re.compile(r'foo bar;\n\s+ssl_certificate /etc/ssl/cert.pem') - self.assertEqual(1, len(re.findall(ssl_re, dumps(parser.parsed[ - parser.abs_path('nginx.conf')])))) - parser.add_server_directives(parser.abs_path('server.conf'), - set(['alias', 'another.alias', - 'somename']), - [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert2.pem']]) - self.assertEqual(parser.parsed[parser.abs_path('server.conf')], + self.assertEqual(1, len(re.findall(ssl_re, nginxparser.dumps( + nparser.parsed[nparser.abs_path('nginx.conf')])))) + nparser.add_server_directives(nparser.abs_path('server.conf'), + set(['alias', 'another.alias', + 'somename']), + [['foo', 'bar'], ['ssl_certificate', + '/etc/ssl/cert2.pem']]) + self.assertEqual(nparser.parsed[nparser.abs_path('server.conf')], [['server_name', 'somename alias another.alias'], ['foo', 'bar'], ['ssl_certificate', '/etc/ssl/cert2.pem']]) def test_replace_server_directives(self): - parser = NginxParser(self.config_path, self.ssl_options) + nparser = parser.NginxParser(self.config_path, self.ssl_options) target = set(['.example.com', 'example.*']) - filep = parser.abs_path('sites-enabled/example.com') - parser.add_server_directives( + filep = nparser.abs_path('sites-enabled/example.com') + nparser.add_server_directives( filep, target, [['server_name', 'foo bar']], True) self.assertEqual( - parser.parsed[filep], + nparser.parsed[filep], [[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', 'foo bar'], ['server_name', 'foo bar']]]]) self.assertRaises(LetsEncryptMisconfigurationError, - parser.add_server_directives, + nparser.add_server_directives, filep, set(['foo', 'bar']), [['ssl_certificate', 'cert.pem']], True) @@ -184,17 +187,18 @@ class NginxParserTest(util.NginxTest): (None, None)] for i, winner in enumerate(winners): - self.assertEqual(winner, get_best_match(target_name, names[i])) + self.assertEqual(winner, + parser.get_best_match(target_name, names[i])) def test_get_all_certs_keys(self): - parser = NginxParser(self.config_path, self.ssl_options) - filep = parser.abs_path('sites-enabled/example.com') - parser.add_server_directives(filep, - set(['.example.com', 'example.*']), - [['ssl_certificate', 'foo.pem'], - ['ssl_certificate_key', 'bar.key'], - ['listen', '443 ssl']]) - c_k = parser.get_all_certs_keys() + nparser = parser.NginxParser(self.config_path, self.ssl_options) + filep = nparser.abs_path('sites-enabled/example.com') + nparser.add_server_directives(filep, + set(['.example.com', 'example.*']), + [['ssl_certificate', 'foo.pem'], + ['ssl_certificate_key', 'bar.key'], + ['listen', '443 ssl']]) + c_k = nparser.get_all_certs_keys() self.assertEqual(set([('foo.pem', 'bar.key', filep)]), c_k)