From 933d92c879781266a599aa65b08db87ea2a8bc2e Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:25:14 +0200 Subject: [PATCH 01/12] salt/modules/acme.py: Fix function docstrings. Fix handling of "window" parameter in "needs_renewal". Pylint-inspired layout fixes. --- salt/modules/acme.py | 45 ++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index 74d48f8c2ca6..a23ed7ee16f8 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -42,6 +42,7 @@ # Import salt libs import salt.utils.path +from salt.exceptions import SaltInvocationError log = logging.getLogger(__name__) @@ -74,7 +75,7 @@ def _expires(name): cert_file = _cert_file(name, 'cert') # Use the salt module if available if 'tls.cert_info' in __salt__: - expiry = __salt__['tls.cert_info'](cert_file)['not_after'] + expiry = __salt__['tls.cert_info'](cert_file).get('not_after', 0) # Cobble it together using the openssl binary else: openssl_cmd = 'openssl x509 -in {0} -noout -enddate'.format(cert_file) @@ -90,8 +91,8 @@ def _renew_by(name, window=None): ''' Date before a certificate should be renewed - :param name: Common Name of the certificate (DNS name of certificate) - :param window: days before expiry date to renew + :param str name: Common Name of the certificate (DNS name of certificate) + :param int window: days before expiry date to renew :return datetime object of first renewal date ''' expiry = _expires(name) @@ -155,7 +156,8 @@ def cert(name, .. code-block:: bash - salt 'gitlab.example.com' acme.cert dev.example.com "[gitlab.example.com]" test_cert=True renew=14 webroot=/opt/gitlab/embedded/service/gitlab-rails/public + salt 'gitlab.example.com' acme.cert dev.example.com "[gitlab.example.com]" test_cert=True \ + renew=14 webroot=/opt/gitlab/embedded/service/gitlab-rails/public ''' cmd = [LEA, 'certonly', '--non-interactive', '--agree-tos'] @@ -224,9 +226,13 @@ def cert(name, cmd.append('--expand') res = __salt__['cmd.run_all'](' '.join(cmd)) if res['retcode'] != 0: - return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + return {'result': False, + 'comment': ('Certificate {0} renewal failed with:\n{1}' + ''.format(name, res['stderr']))} else: - return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + return {'result': False, + 'comment': ('Certificate {0} renewal failed with:\n{1}' + ''.format(name, res['stderr']))} if 'no action taken' in res['stdout']: comment = 'Certificate {0} unchanged'.format(cert_file) @@ -257,7 +263,7 @@ def certs(): salt 'vhost.example.com' acme.certs ''' - return __salt__['file.readdir'](LE_LIVE)[2:] + return [item for item in __salt__['file.readdir'](LE_LIVE)[2:] if os.path.isdir(item)] def info(name): @@ -267,7 +273,7 @@ def info(name): .. note:: Will output tls.cert_info if that's available, or OpenSSL text if not - :param name: CommonName of cert + :param str name: CommonName of certificate CLI example: @@ -275,6 +281,8 @@ def info(name): salt 'gitlab.example.com' acme.info dev.example.com ''' + if not has(name): + return None cert_file = _cert_file(name, 'cert') # Use the salt module if available if 'tls.cert_info' in __salt__: @@ -293,7 +301,7 @@ def expires(name): ''' The expiry date of a certificate in ISO format - :param name: CommonName of cert + :param str name: CommonName of certificate CLI example: @@ -308,7 +316,7 @@ def has(name): ''' Test if a certificate is in the Let's Encrypt Live directory - :param name: CommonName of cert + :param str name: CommonName of certificate Code example: @@ -324,8 +332,8 @@ def renew_by(name, window=None): ''' Date in ISO format when a certificate should first be renewed - :param name: CommonName of cert - :param window: number of days before expiry when renewal should take place + :param str name: CommonName of certificate + :param int window: number of days before expiry when renewal should take place ''' return _renew_by(name, window).isoformat() @@ -334,8 +342,8 @@ def needs_renewal(name, window=None): ''' Check if a certificate needs renewal - :param name: CommonName of cert - :param window: Window in days to renew earlier or True/force to just return True + :param str name: CommonName of certificate + :param bool/str/int window: Window in days to renew earlier or True/force to just return True Code example: @@ -349,4 +357,13 @@ def needs_renewal(name, window=None): if window is not None and window in ('force', 'Force', True): return True + if window: + if not (isinstance(window, int) or (hasattr(window, 'isdigit') and window.isdigit())): + raise SaltInvocationError( + 'The argument "window", if provided, must be one of the following : ' + 'True (boolean), "force" or "Force" (str) or a numerical value in days.' + ) + else: + window = int(window) + return _renew_by(name, window) <= datetime.datetime.today() From 40deb63e9128d7d7c5e0997a206e056cf99420c4 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:26:01 +0200 Subject: [PATCH 02/12] salt/states/acme.py: Add proper handling of testmode. Add proper detection if nothing/fetch/renew is needed to be done. --- salt/states/acme.py | 114 +++++++++++++++++++------------------------- 1 file changed, 48 insertions(+), 66 deletions(-) diff --git a/salt/states/acme.py b/salt/states/acme.py index 9ecc1fee284d..3ae7a508ba93 100644 --- a/salt/states/acme.py +++ b/salt/states/acme.py @@ -29,6 +29,10 @@ from __future__ import absolute_import, print_function, unicode_literals import logging +# Import salt libs +import salt.utils.dictdiffer +import salt.ext.six as six + log = logging.getLogger(__name__) @@ -88,74 +92,52 @@ def cert(name, :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare') :param dns_plugin_credentials: Path to the credentials file if required by the specified DNS plugin ''' + ret = {'name': name, 'result': 'changeme', 'comment': [], 'changes': {}} + action = None - if __opts__['test']: - ret = { - 'name': name, - 'changes': {}, - 'result': None - } - window = None - try: - window = int(renew) - except Exception: - pass - - comment = 'Certificate {0} '.format(name) - if not __salt__['acme.has'](name): - comment += 'would have been obtained' - elif __salt__['acme.needs_renewal'](name, window): - comment += 'would have been renewed' - else: - comment += 'would not have been touched' - ret['result'] = True - ret['comment'] = comment - return ret - + current_certificate = {} + new_certificate = {} if not __salt__['acme.has'](name): - old = None - else: - old = __salt__['acme.info'](name) - - res = __salt__['acme.cert']( - name, - aliases=aliases, - email=email, - webroot=webroot, - certname=certname, - test_cert=test_cert, - renew=renew, - keysize=keysize, - server=server, - owner=owner, - group=group, - mode=mode, - preferred_challenges=preferred_challenges, - tls_sni_01_port=tls_sni_01_port, - tls_sni_01_address=tls_sni_01_address, - http_01_port=http_01_port, - http_01_address=http_01_address, - dns_plugin=dns_plugin, - dns_plugin_credentials=dns_plugin_credentials, - ) - - ret = { - 'name': name, - 'result': res['result'] is not False, - 'comment': res['comment'] - } - - if res['result'] is None: - ret['changes'] = {} + action = 'obtain' + elif __salt__['acme.needs_renewal'](name, renew): + action = 'renew' + current_certificate = __salt__['acme.info'](name) else: - if not __salt__['acme.has'](name): - new = None + ret['result'] = True + ret['comment'].append('Certificate {} exists and does not need renewal.' + ''.format(name)) + + if action: + if __opts__['test']: + ret['result'] = None + ret['comment'].append('Certificate {} would have been {}ed.' + ''.format(name, action)) + ret['changes'] = {'old': 'current certificate', 'new': 'new certificate'} else: - new = __salt__['acme.info'](name) - - ret['changes'] = { - 'old': old, - 'new': new - } - + res = __salt__['acme.cert']( + name, + aliases=aliases, + email=email, + webroot=webroot, + certname=certname, + test_cert=test_cert, + renew=renew, + keysize=keysize, + server=server, + owner=owner, + group=group, + mode=mode, + preferred_challenges=preferred_challenges, + tls_sni_01_port=tls_sni_01_port, + tls_sni_01_address=tls_sni_01_address, + http_01_port=http_01_port, + http_01_address=http_01_address, + dns_plugin=dns_plugin, + dns_plugin_credentials=dns_plugin_credentials, + ) + ret['result'] = res['result'] + ret['comment'].append(res['comment']) + if ret['result']: + new_certificate = __salt__['acme.info'](name) + ret['changes'] = salt.utils.dictdiffer.deep_diff(current_certificate, new_certificate) return ret From f0726cf684b0234fe55c8e664cebc8c53062e743 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:26:41 +0200 Subject: [PATCH 03/12] Added unit tests for acme module and state. --- tests/unit/modules/test_acme.py | 221 ++++++++++++++++++++++++++++++++ tests/unit/states/test_acme.py | 128 ++++++++++++++++++ 2 files changed, 349 insertions(+) create mode 100644 tests/unit/modules/test_acme.py create mode 100644 tests/unit/states/test_acme.py diff --git a/tests/unit/modules/test_acme.py b/tests/unit/modules/test_acme.py new file mode 100644 index 000000000000..4d1d4b10c057 --- /dev/null +++ b/tests/unit/modules/test_acme.py @@ -0,0 +1,221 @@ +# -*- coding: utf-8 -*- +''' +:codeauthor: Herbert Buurman +''' + +# Import future libs +from __future__ import absolute_import, print_function, unicode_literals + +# Import Python libs +import textwrap +import datetime + +# Import Salt Testing libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.unit import skipIf, TestCase +from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch + +# Import Salt Module +import salt.modules.acme as acme +import salt.utils.dictupdate +from salt.exceptions import SaltInvocationError + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class AcmeTestCase(TestCase, LoaderModuleMockMixin): + ''' + Test cases for salt.modules.acme + ''' + + def setup_loader_modules(self): + return {acme: {}} + + def test_certs(self): + ''' + Test listing certs + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.readdir': MagicMock(return_value=['.', '..', 'README', 'test_expired', 'test_valid']) + }), \ + patch('os.path.isdir', side_effect=[False, True, True]): + self.assertEqual(acme.certs(), ['test_expired', 'test_valid']) + + def test_has(self): + ''' + Test checking if certificate (does not) exist. + ''' + with patch.dict(acme.__salt__, {'file.file_exists': MagicMock(return_value=True)}): # pylint: disable=no-member + self.assertTrue(acme.has('test_expired')) + with patch.dict(acme.__salt__, {'file.file_exists': MagicMock(return_value=False)}): # pylint: disable=no-member + self.assertFalse(acme.has('test_invalid')) + + def test_needs_renewal(self): + ''' + Test if expired certs do indeed need renewal. + ''' + expired = datetime.date.today() - datetime.timedelta(days=3) - datetime.date(1970, 1, 1) + valid = datetime.date.today() + datetime.timedelta(days=3) - datetime.date(1970, 1, 1) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': expired.total_seconds()}) + }): + self.assertTrue(acme.needs_renewal('test_expired')) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': valid.total_seconds()}) + }): + self.assertFalse(acme.needs_renewal('test_valid')) + # Test with integer window parameter + self.assertTrue(acme.needs_renewal('test_valid', window=5)) + # Test with string-like window parameter + self.assertTrue(acme.needs_renewal('test_valid', window='5')) + # Test with invalid window parameter + self.assertRaises(SaltInvocationError, acme.needs_renewal, 'test_valid', window='foo') + + def test_expires(self): + ''' + Test if expires function functions properly. + ''' + test_value = datetime.datetime.today() - datetime.timedelta(days=3) + test_stamp = test_value - datetime.datetime(1970, 1, 1) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': test_stamp.total_seconds()}) + }): + self.assertEqual( + acme.expires('test_expired'), + datetime.datetime.fromtimestamp(test_stamp.total_seconds()).isoformat() + ) + + def test_info(self): + ''' + Test certificate information retrieval. + ''' + certinfo_result = { + "not_after": 1559471377, + "signature_algorithm": "sha256WithRSAEncryption", + "extensions": {}, + "fingerprint": ("FB:A4:5F:71:D6:5D:6C:B6:1D:2C:FD:91:09:2C:1C:52:" + "3C:EC:B6:4D:1A:95:65:37:04:D0:E2:5E:C7:64:0C:9C"), + "serial_number": 6461481982668892235, + "issuer": {}, + "not_before": 1559557777, + "subject": {}, + } + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value=certinfo_result), + 'file.file_exists': MagicMock(return_value=True) + }): + self.assertEqual(acme.info('test'), certinfo_result) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run': MagicMock(return_value='foo'), + 'file.file_exists': MagicMock(return_value=True) + }): + self.assertEqual(acme.info('test'), 'foo') + + def test_cert(self): + ''' + Test certificate retrieval/renewal + ''' + valid_timestamp = (datetime.datetime.now() + datetime.timedelta(days=30) - + datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() + expired_timestamp = (datetime.datetime.now() - datetime.timedelta(days=3) - + datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() + cmd_new_cert = { + 'stdout': textwrap.dedent('''IMPORTANT NOTES: + - Congratulations! Your certificate and chain have been saved at: + /etc/letsencrypt/live/test/fullchain.pem + Your key file has been saved at: + /etc/letsencrypt/live/test/privkey.pem + Your cert will expire on 2019-08-07. To obtain a new or tweaked + version of this certificate in the future, simply run certbot + again. To non-interactively renew *all* of your certificates, run + "certbot renew" + - If you like Certbot, please consider supporting our work by: + + Donating to ISRG / Let's Encrypt: https://letsencrypt.org/donate + Donating to EFF: https://eff.org/donate-le + '''), + 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + Plugins selected: Authenticator standalone, Installer None + Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org + Obtaining a new certificate + Resetting dropped connection: acme-v02.api.letsencrypt.org + '''), + 'retcode': 0, + } + result_new_cert = { + "comment": "Certificate test obtained", + "not_after": datetime.datetime.fromtimestamp(valid_timestamp).isoformat(), + "changes": { + "mode": "0640" + }, + "result": True + } + + cmd_no_renew = { + 'stdout': textwrap.dedent(''' + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + Certificate not yet due for renewal; no action taken. + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + '''), + 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + Plugins selected: Authenticator standalone, Installer None + Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org + Cert not yet due for renewal + Keeping the existing certificate + '''), + 'retcode': 0 + } + result_no_renew = { + "comment": "Certificate /etc/letsencrypt/live/test/cert.pem unchanged", + "not_after": datetime.datetime.fromtimestamp(valid_timestamp).isoformat(), + "changes": {}, + "result": True + } + result_renew = { + "comment": "Certificate test renewed", + "not_after": datetime.datetime.fromtimestamp(expired_timestamp).isoformat(), + "changes": {}, + "result": True + } + + # Test fetching new certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_new_cert), + 'file.file_exists': MagicMock(return_value=False), + 'tls.cert_info': MagicMock(return_value={'not_after': valid_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'changes:mode', '0640'), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_new_cert) + # Test not renewing a valid certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_no_renew), + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value={'not_after': valid_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'result', True), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_no_renew) + # Test renewing an expired certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_new_cert), + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value={'not_after': expired_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'result', True), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_renew) diff --git a/tests/unit/states/test_acme.py b/tests/unit/states/test_acme.py new file mode 100644 index 000000000000..c5deaa063ace --- /dev/null +++ b/tests/unit/states/test_acme.py @@ -0,0 +1,128 @@ +# -*- coding: utf-8 -*- +''' +:codeauthor: Herbert Buurman +''' + +# Import future libs +from __future__ import absolute_import, print_function, unicode_literals + +# Import Salt Testing libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.unit import skipIf, TestCase +from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch + +# Import Salt Module +import salt.states.acme as acme + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class AcmeTestCase(TestCase, LoaderModuleMockMixin): + ''' + Test cases for salt.modules.acme + ''' + + def setup_loader_modules(self): + return {acme: {'__opts__': {'test': False}}} + + def test_cert_no_changes_t(self): + ''' + Test cert state with no needed changes. (test=True) + ''' + # With test=True + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=False), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Certificate test exists and does not need renewal.'], + 'changes': {}, + }) + + def test_cert_no_changes(self): + ''' + Test cert state with no needed changes. (test=False) + ''' + # With test=False + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=False), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Certificate test exists and does not need renewal.'], + 'changes': {}, + }) + + def test_cert_fresh_certificate_t(self): + ''' + Test cert state fetching a new certificate. (test=True) + ''' + # With test=True + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=False), + # 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + 'acme.info': MagicMock(return_value={'foo': 'bar'}), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': None, + 'comment': ['Certificate test would have been obtained.'], + 'changes': {'old': 'current certificate', 'new': 'new certificate'}, + }) + + def test_cert_fresh_certificate(self): + ''' + Test cert state fetching a new certificate. (test=False) + ''' + # With test=False + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=False), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + 'acme.info': MagicMock(return_value={'foo': 'bar'}), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Mockery'], + 'changes': {'new': {'foo': 'bar'}}, + }) + + def test_cert_renew_certificate_t(self): + ''' + Test cert state renewing a certificate. (test=True) + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=True), + 'acme.info': MagicMock(side_effect=[{'name': 'old cert'}, {'name': 'new cert'}]), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': None, + 'comment': ['Certificate test would have been renewed.'], + 'changes': {'old': 'current certificate', 'new': 'new certificate'}, + }) + + def test_cert_renew_certificate(self): + ''' + Test cert state renewing a certificate. (test=False) + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=True), + 'acme.info': MagicMock(side_effect=[{'name': 'old cert'}, {'name': 'new cert'}]), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Mockery'], + 'changes': {'old': {'name': 'old cert'}, 'new': {'name': 'new cert'}}, + }) From d5cf39029b5594002d955a3a6bbb53987bced773 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:41:09 +0200 Subject: [PATCH 04/12] salt/modules/acme.py: Changed "info" to always return a dict. Added x509.read_certificate as alternative method of getting certificate information if tls.cert_info is not available. --- salt/modules/acme.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index a23ed7ee16f8..efbb1d21f477 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -83,7 +83,6 @@ def _expires(name): strptime_sux_cmd = 'date --date="$({0} | cut -d= -f2)" +%s'.format(openssl_cmd) expiry = float(__salt__['cmd.shell'](strptime_sux_cmd, output_loglevel='quiet')) # expiry = datetime.datetime.strptime(expiry.split('=', 1)[-1], '%b %e %H:%M:%S %Y %Z') - return datetime.datetime.fromtimestamp(expiry) @@ -274,6 +273,7 @@ def info(name): Will output tls.cert_info if that's available, or OpenSSL text if not :param str name: CommonName of certificate + :return dict CLI example: @@ -282,19 +282,22 @@ def info(name): salt 'gitlab.example.com' acme.info dev.example.com ''' if not has(name): - return None + return {} cert_file = _cert_file(name, 'cert') - # Use the salt module if available + # Use the tls salt module if available if 'tls.cert_info' in __salt__: cert_info = __salt__['tls.cert_info'](cert_file) # Strip out the extensions object contents; # these trip over our poor state output # and they serve no real purpose here anyway cert_info['extensions'] = cert_info['extensions'].keys() - return cert_info - # Cobble it together using the openssl binary - openssl_cmd = 'openssl x509 -in {0} -noout -text'.format(cert_file) - return __salt__['cmd.run'](openssl_cmd, output_loglevel='quiet') + elif 'x509.read_certificate' in __salt__: + cert_info = __salt__['x509.read_certificate'](cert_file) + else: + # Cobble it together using the openssl binary + openssl_cmd = 'openssl x509 -in {0} -noout -text'.format(cert_file) + cert_info = {'text': __salt__['cmd.run'](openssl_cmd, output_loglevel='quiet')} + return cert_info def expires(name): From 55a2961a033b41995fe3a8a758a830ae43c595a6 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:41:23 +0200 Subject: [PATCH 05/12] salt/states/acme.py: Removed unused import --- salt/states/acme.py | 1 - 1 file changed, 1 deletion(-) diff --git a/salt/states/acme.py b/salt/states/acme.py index 3ae7a508ba93..b4c0101bd824 100644 --- a/salt/states/acme.py +++ b/salt/states/acme.py @@ -31,7 +31,6 @@ # Import salt libs import salt.utils.dictdiffer -import salt.ext.six as six log = logging.getLogger(__name__) From 45879dcd30e4f1a027fa628f607a1537663570e9 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:42:48 +0200 Subject: [PATCH 06/12] tests/unit/modules/test_acme.py: Added test for salt.modules.acme.info using x509.read_certificates. Fixed incorrect data placement in textwrap.dedent. Updated test for acme.info to expect dict when using openssl cli. --- tests/unit/modules/test_acme.py | 48 +++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/unit/modules/test_acme.py b/tests/unit/modules/test_acme.py index 4d1d4b10c057..f4d7743ef3e0 100644 --- a/tests/unit/modules/test_acme.py +++ b/tests/unit/modules/test_acme.py @@ -88,7 +88,7 @@ def test_info(self): ''' Test certificate information retrieval. ''' - certinfo_result = { + certinfo_tls_result = { "not_after": 1559471377, "signature_algorithm": "sha256WithRSAEncryption", "extensions": {}, @@ -99,16 +99,46 @@ def test_info(self): "not_before": 1559557777, "subject": {}, } + certinfo_x509_result = { + "Not After": "2019-06-02 10:29:37", + "Subject Hash": "54:3B:6C:A4", + "Serial Number": "59:AB:CB:A0:FB:90:E8:4B", + "SHA1 Finger Print": "F1:8D:F3:26:1B:D3:88:32:CD:B6:FA:3B:85:58:DA:C7:6F:62:BE:7E", + "SHA-256 Finger Print": ("FB:A4:5F:71:D6:5D:6C:B6:1D:2C:FD:91:09:2C:1C:52:" + "3C:EC:B6:4D:1A:95:65:37:04:D0:E2:5E:C7:64:0C:9C"), + "MD5 Finger Print": "95:B5:96:9B:42:A5:9E:20:78:FD:99:09:4B:21:1E:97", + "Version": 3, + "Key Size": 2048, + "Public Key": ("-----BEGIN PUBLIC KEY-----\n" + "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsVO2vwQPKU92PSBnuGid\n" + "k8t6KWVE2jEBM10u7CgqQmD/JCnYflEHAo1nOsD7wxdhBrxhf5Qs+pEX1HOsh8VA\n" + "HDTim0iE8nQVJ0Iuen2SrwaWMhwKmZTSJRYMgd46oCMi2RdlCvcgF2Hw6RTwF7FT\n" + "hnksc4HBT91XddnP32N558tOT3YejafQNvClz5WcR+E0JzqGrV/+wfe3o+j/q5eK\n" + "UowttWazeSMvuROtqj/fEk0rop4D14pgzZqWi30tjwhJNl6fSPFWBrLEHGNyDJ+O\n" + "zfov0B2MRLJibH7GMkOCwsP2g1lVOReqcml+ju6zAKW8nHBTRg0iXB18Ifxef57Y\n" + "AQIDAQAB\n" + "-----END PUBLIC KEY-----\n"), + "Issuer": {}, + "Issuer Hash": "54:3B:6C:A4", + "Not Before": "2019-06-03 10:29:37", + "Subject": {} + } + with patch.dict(acme.__salt__, { # pylint: disable=no-member - 'tls.cert_info': MagicMock(return_value=certinfo_result), - 'file.file_exists': MagicMock(return_value=True) + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value=certinfo_tls_result), }): - self.assertEqual(acme.info('test'), certinfo_result) + self.assertEqual(acme.info('test'), certinfo_tls_result) with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.file_exists': MagicMock(return_value=True), + 'x509.read_certificate': MagicMock(return_value=certinfo_x509_result), + }): + self.assertEqual(acme.info('test'), certinfo_x509_result) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.file_exists': MagicMock(return_value=True), 'cmd.run': MagicMock(return_value='foo'), - 'file.file_exists': MagicMock(return_value=True) }): - self.assertEqual(acme.info('test'), 'foo') + self.assertEqual(acme.info('test'), {'text': 'foo'}) def test_cert(self): ''' @@ -119,7 +149,8 @@ def test_cert(self): expired_timestamp = (datetime.datetime.now() - datetime.timedelta(days=3) - datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() cmd_new_cert = { - 'stdout': textwrap.dedent('''IMPORTANT NOTES: + 'stdout': textwrap.dedent(''' + IMPORTANT NOTES: - Congratulations! Your certificate and chain have been saved at: /etc/letsencrypt/live/test/fullchain.pem Your key file has been saved at: @@ -133,7 +164,8 @@ def test_cert(self): Donating to ISRG / Let's Encrypt: https://letsencrypt.org/donate Donating to EFF: https://eff.org/donate-le '''), - 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + 'stderr': textwrap.dedent(''' + Saving debug log to /var/log/letsencrypt/letsencrypt.log Plugins selected: Authenticator standalone, Installer None Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org Obtaining a new certificate From b53f14c0726e4224416511e0e62fc9543de88ef1 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Fri, 26 Jul 2019 10:43:21 +0200 Subject: [PATCH 07/12] Optimized convoluted window check in "needs_renewal". Improved/fixed function docstrings. --- salt/modules/acme.py | 62 ++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index efbb1d21f477..76552281f555 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -70,7 +70,8 @@ def _expires(name): ''' Return the expiry date of a cert - :return datetime object of expiry date + :rtype: datetime + :return: Expiry date ''' cert_file = _cert_file(name, 'cert') # Use the salt module if available @@ -92,7 +93,8 @@ def _renew_by(name, window=None): :param str name: Common Name of the certificate (DNS name of certificate) :param int window: days before expiry date to renew - :return datetime object of first renewal date + :rtype: datetime + :return: First renewal date ''' expiry = _expires(name) if window is not None: @@ -127,8 +129,10 @@ def cert(name, :param aliases: subjectAltNames (Additional DNS names on certificate) :param email: e-mail address for interaction with ACME provider :param webroot: True or a full path to use to use webroot. Otherwise use standalone mode - :param test_cert: Request a certificate from the Happy Hacker Fake CA (mutually exclusive with 'server') - :param renew: True/'force' to force a renewal, or a window of renewal before expiry in days + :param test_cert: Request a certificate from the Happy Hacker Fake CA (mutually + exclusive with 'server') + :param renew: True/'force' to force a renewal, or a window of renewal before + expiry in days :param keysize: RSA key bits :param server: API endpoint to talk to :param owner: owner of the private key file @@ -136,20 +140,26 @@ def cert(name, :param mode: mode of the private key file :param certname: Name of the certificate to save :param preferred_challenges: A sorted, comma delimited list of the preferred - challenge to use during authorization with the - most preferred challenge listed first. + challenge to use during authorization with the most preferred challenge + listed first. :param tls_sni_01_port: Port used during tls-sni-01 challenge. This only affects - the port Certbot listens on. A conforming ACME server - will still attempt to connect on port 443. + the port Certbot listens on. A conforming ACME server will still attempt + to connect on port 443. :param tls_sni_01_address: The address the server listens to during tls-sni-01 - challenge. + challenge. :param http_01_port: Port used in the http-01 challenge. This only affects - the port Certbot listens on. A conforming ACME server - will still attempt to connect on port 80. + the port Certbot listens on. A conforming ACME server will still attempt + to connect on port 80. :param https_01_address: The address the server listens to during http-01 challenge. - :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare') - :param dns_plugin_credentials: Path to the credentials file if required by the specified DNS plugin - :return: dict with 'result' True/False/None, 'comment' and certificate's expiry date ('not_after') + :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare' + or 'digitalocean') + :param dns_plugin_credentials: Path to the credentials file if required by + the specified DNS plugin + :param dns_plugin_propagate_seconds: Number of seconds to wait for DNS propogations + before asking ACME servers to verify the DNS record. (default 10) + :rtype: dict + :return: Dictionary with 'result' True/False/None, 'comment' and certificate's + expiry date ('not_after') CLI example: @@ -269,11 +279,12 @@ def info(name): ''' Return information about a certificate - .. note:: - Will output tls.cert_info if that's available, or OpenSSL text if not - :param str name: CommonName of certificate - :return dict + :rtype: dict + :return: Dictionary with information about the certificate. + If neither the ``tls`` nor the ``x509`` module can be used to determine + the certificate information, the information will be retrieved as one + big text block under the key ``text`` using the openssl cli. CLI example: @@ -305,6 +316,8 @@ def expires(name): The expiry date of a certificate in ISO format :param str name: CommonName of certificate + :rtype: str + :return: Expiry date in ISO format. CLI example: @@ -320,6 +333,7 @@ def has(name): Test if a certificate is in the Let's Encrypt Live directory :param str name: CommonName of certificate + :rtype: bool Code example: @@ -337,6 +351,8 @@ def renew_by(name, window=None): :param str name: CommonName of certificate :param int window: number of days before expiry when renewal should take place + :rtype: str + :return: Date of certificate renewal in ISO format. ''' return _renew_by(name, window).isoformat() @@ -347,6 +363,8 @@ def needs_renewal(name, window=None): :param str name: CommonName of certificate :param bool/str/int window: Window in days to renew earlier or True/force to just return True + :rtype: bool + :return: Whether or not the certificate needs to be renewed. Code example: @@ -357,16 +375,14 @@ def needs_renewal(name, window=None): else: log.info('Your certificate is still good') ''' - if window is not None and window in ('force', 'Force', True): - return True - if window: + if str(window).lower in ('force', 'true'): + return True if not (isinstance(window, int) or (hasattr(window, 'isdigit') and window.isdigit())): raise SaltInvocationError( 'The argument "window", if provided, must be one of the following : ' 'True (boolean), "force" or "Force" (str) or a numerical value in days.' ) - else: - window = int(window) + window = int(window) return _renew_by(name, window) <= datetime.datetime.today() From 298c15105c57251a27d7226a8c23cee83f5c572c Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Thu, 25 Apr 2019 10:56:57 -0600 Subject: [PATCH 08/12] Merge pull request #52455 from ogd-software/more_dictupdate_tools More dictupdate tools --- doc/topics/jinja/index.rst | 131 ++++++++++++++++ salt/utils/dictupdate.py | 228 ++++++++++++++++++++++++++-- tests/unit/utils/test_dictupdate.py | 135 +++++++++++++++- tests/unit/utils/test_jinja.py | 145 +++++++++++++++++- 4 files changed, 617 insertions(+), 22 deletions(-) diff --git a/doc/topics/jinja/index.rst b/doc/topics/jinja/index.rst index e3ac299fcd9f..505d2fe95936 100644 --- a/doc/topics/jinja/index.rst +++ b/doc/topics/jinja/index.rst @@ -1019,6 +1019,137 @@ Returns: d94a45acd81f8e3107d237dbc0d5d195f6a52a0d188bc0284c0763ece1eac9f9496fb6a531a296074c87b3540398dace1222b42e150e67c9301383fde3d66ae5 +.. jinja_ref:: set_dict_key_value + +``set_dict_key_value`` +---------------------- + +..versionadded:: Neon + +Allows you to set a value in a nested dictionary without having to worry if all the nested keys actually exist. +Missing keys will be automatically created if they do not exist. +The default delimiter for the keys is ':', however, with the `delimiter`-parameter, a different delimiter can be specified. + +Examples: + +.. code-block:: jinja + +Example 1: + {%- set foo = {} %} + {{ foo | set_dict_key_value('bar:baz', 42) }} + +Example 2: + {{ {} | set_dict_key_value('bar.baz.qux', 42, delimiter='.') }} + +Returns: + +.. code-block:: text + +Example 1: + {'bar': {'baz': 42}} + +Example 2: + {'bar': {'baz': {'qux': 42}}} + + +.. jinja_ref:: append_dict_key_value + +``append_dict_key_value`` +------------------------- + +..versionadded:: Neon + +Allows you to append to a list nested (deep) in a dictionary without having to worry if all the nested keys (or the list itself) actually exist. +Missing keys will automatically be created if they do not exist. +The default delimiter for the keys is ':', however, with the `delimiter`-parameter, a different delimiter can be specified. + +Examples: + +.. code-block:: jinja + +Example 1: + {%- set foo = {'bar': {'baz': [1, 2]}} %} + {{ foo | append_dict_key_value('bar:baz', 42) }} + +Example 2: + {%- set foo = {} %} + {{ foo | append_dict_key_value('bar:baz:qux', 42) }} + +Returns: + +.. code-block:: text + +Example 1: + {'bar': {'baz': [1, 2, 42]}} + +Example 2: + {'bar': {'baz': {'qux': [42]}}} + + +.. jinja_ref:: extend_dict_key_value + +``extend_dict_key_value`` +------------------------- + +..versionadded:: Neon + +Allows you to extend a list nested (deep) in a dictionary without having to worry if all the nested keys (or the list itself) actually exist. +Missing keys will automatically be created if they do not exist. +The default delimiter for the keys is ':', however, with the `delimiter`-parameter, a different delimiter can be specified. + +Examples: + +.. code-block:: jinja + +Example 1: + {%- set foo = {'bar': {'baz': [1, 2]}} %} + {{ foo | extend_dict_key_value('bar:baz', [42, 42]) }} + +Example 2: + {{ {} | extend_dict_key_value('bar:baz:qux', [42]) }} + +Returns: + +.. code-block:: text + +Example 1: + {'bar': {'baz': [1, 2, 42, 42]}} + +Example 2: + {'bar': {'baz': {'qux': [42]}}} + + +.. jinja_ref:: update_dict_key_value + +``update_dict_key_value`` +------------------------- + +..versionadded:: Neon + +Allows you to update a dictionary nested (deep) in another dictionary without having to worry if all the nested keys actually exist. +Missing keys will automatically be created if they do not exist. +The default delimiter for the keys is ':', however, with the `delimiter`-parameter, a different delimiter can be specified. + +Examples: + +.. code-block:: jinja + +Example 1: + {%- set foo = {'bar': {'baz': {'qux': 1}}} %} + {{ foo | update_dict_key_value('bar:baz', {'quux': 3}) }} + +Example 2: + {{ {} | update_dict_key_value('bar:baz:qux', {'quux': 3}) }} + +.. code-block:: text + +Example 1: + {'bar': {'baz': {'qux': 1, 'quux': 3}}} + +Example 2: + {'bar': {'baz': {'qux': {'quux': 3}}}} + + .. jinja_ref:: md5 ``md5`` diff --git a/salt/utils/dictupdate.py b/salt/utils/dictupdate.py index 70b7a9c4b895..c71cf168dbd8 100644 --- a/salt/utils/dictupdate.py +++ b/salt/utils/dictupdate.py @@ -15,7 +15,14 @@ # Import 3rd-party libs import copy import logging + +# Import salt libs import salt.ext.six as six +from salt.defaults import DEFAULT_TARGET_DELIM +import salt.utils.data +from salt.exceptions import SaltInvocationError +from salt.utils.odict import OrderedDict +from salt.utils.decorators.jinja import jinja_filter log = logging.getLogger(__name__) @@ -55,8 +62,7 @@ def update(dest, upd, recursive_update=True, merge_lists=False): and isinstance(val, Mapping): ret = update(dest_subkey, val, merge_lists=merge_lists) dest[key] = ret - elif isinstance(dest_subkey, list) \ - and isinstance(val, list): + elif isinstance(dest_subkey, list) and isinstance(val, list): if merge_lists: merged = copy.deepcopy(dest_subkey) merged.extend([x for x in val if x not in merged]) @@ -66,15 +72,14 @@ def update(dest, upd, recursive_update=True, merge_lists=False): else: dest[key] = upd[key] return dest - else: - try: - for k in upd: - dest[k] = upd[k] - except AttributeError: - # this mapping is not a dict - for k in upd: - dest[k] = upd[k] - return dest + try: + for k in upd: + dest[k] = upd[k] + except AttributeError: + # this mapping is not a dict + for k in upd: + dest[k] = upd[k] + return dest def merge_list(obj_a, obj_b): @@ -132,3 +137,204 @@ def merge(obj_a, obj_b, strategy='smart', renderer='yaml', merge_lists=False): merged = merge_recurse(obj_a, obj_b) return merged + + +def ensure_dict_key( + in_dict, + keys, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Ensures that in_dict contains the series of recursive keys defined in keys. + + :param dict in_dict: The dict to work with. + :param str keys: The delimited string with one or more keys. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + ''' + if delimiter in keys: + a_keys = keys.split(delimiter) + else: + a_keys = [keys] + dict_pointer = in_dict + while a_keys: + current_key = a_keys.pop(0) + if current_key not in dict_pointer or not isinstance(dict_pointer[current_key], dict): + dict_pointer[current_key] = OrderedDict() if ordered_dict else {} + dict_pointer = dict_pointer[current_key] + return in_dict + + +def _dict_rpartition( + in_dict, + keys, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Helper function to: + - Ensure all but the last key in `keys` exist recursively in `in_dict`. + - Return the dict at the one-to-last key, and the last key + + :param dict in_dict: The dict to work with. + :param str keys: The delimited string with one or more keys. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + + :return tuple(dict, str) + ''' + if delimiter in keys: + all_but_last_keys, _, last_key = keys.rpartition(delimiter) + ensure_dict_key(in_dict, + all_but_last_keys, + delimiter=delimiter, + ordered_dict=ordered_dict) + dict_pointer = salt.utils.data.traverse_dict(in_dict, + all_but_last_keys, + default=None, + delimiter=delimiter) + else: + dict_pointer = in_dict + last_key = keys + return dict_pointer, last_key + + +@jinja_filter('set_dict_key_value') +def set_dict_key_value( + in_dict, + keys, + value, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Ensures that in_dict contains the series of recursive keys defined in keys. + Also sets whatever is at the end of `in_dict` traversed with `keys` to `value`. + + :param dict in_dict: The dictionary to work with + :param str keys: The delimited string with one or more keys. + :param any value: The value to assign to the nested dict-key. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + + :return dict: Though it updates in_dict in-place. + ''' + dict_pointer, last_key = _dict_rpartition(in_dict, + keys, + delimiter=delimiter, + ordered_dict=ordered_dict) + dict_pointer[last_key] = value + return in_dict + + +@jinja_filter('update_dict_key_value') +def update_dict_key_value( + in_dict, + keys, + value, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Ensures that in_dict contains the series of recursive keys defined in keys. + Also updates the dict, that is at the end of `in_dict` traversed with `keys`, + with `value`. + + :param dict in_dict: The dictionary to work with + :param str keys: The delimited string with one or more keys. + :param any value: The value to update the nested dict-key with. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + + :return dict: Though it updates in_dict in-place. + ''' + dict_pointer, last_key = _dict_rpartition(in_dict, + keys, + delimiter=delimiter, + ordered_dict=ordered_dict) + if last_key not in dict_pointer or dict_pointer[last_key] is None: + dict_pointer[last_key] = OrderedDict() if ordered_dict else {} + try: + dict_pointer[last_key].update(value) + except AttributeError: + raise SaltInvocationError('The last key contains a {}, which cannot update.' + ''.format(type(dict_pointer[last_key]))) + except (ValueError, TypeError): + raise SaltInvocationError('Cannot update {} with a {}.' + ''.format(type(dict_pointer[last_key]), type(value))) + return in_dict + + +@jinja_filter('append_dict_key_value') +def append_dict_key_value( + in_dict, + keys, + value, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Ensures that in_dict contains the series of recursive keys defined in keys. + Also appends `value` to the list that is at the end of `in_dict` traversed + with `keys`. + + :param dict in_dict: The dictionary to work with + :param str keys: The delimited string with one or more keys. + :param any value: The value to append to the nested dict-key. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + + :return dict: Though it updates in_dict in-place. + ''' + dict_pointer, last_key = _dict_rpartition(in_dict, + keys, + delimiter=delimiter, + ordered_dict=ordered_dict) + if last_key not in dict_pointer or dict_pointer[last_key] is None: + dict_pointer[last_key] = [] + try: + dict_pointer[last_key].append(value) + except AttributeError: + raise SaltInvocationError('The last key contains a {}, which cannot append.' + ''.format(type(dict_pointer[last_key]))) + return in_dict + + +@jinja_filter('extend_dict_key_value') +def extend_dict_key_value( + in_dict, + keys, + value, + delimiter=DEFAULT_TARGET_DELIM, + ordered_dict=False): + ''' + Ensures that in_dict contains the series of recursive keys defined in keys. + Also extends the list, that is at the end of `in_dict` traversed with `keys`, + with `value`. + + :param dict in_dict: The dictionary to work with + :param str keys: The delimited string with one or more keys. + :param any value: The value to extend the nested dict-key with. + :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. + :param bool ordered_dict: Create OrderedDicts if keys are missing. + Default: create regular dicts. + + :return dict: Though it updates in_dict in-place. + ''' + dict_pointer, last_key = _dict_rpartition( + in_dict, + keys, + delimiter=delimiter, + ordered_dict=ordered_dict) + if last_key not in dict_pointer or dict_pointer[last_key] is None: + dict_pointer[last_key] = [] + try: + dict_pointer[last_key].extend(value) + except AttributeError: + raise SaltInvocationError('The last key contains a {}, which cannot extend.' + ''.format(type(dict_pointer[last_key]))) + except TypeError: + raise SaltInvocationError('Cannot extend {} with a {}.' + ''.format(type(dict_pointer[last_key]), type(value))) + return in_dict diff --git a/tests/unit/utils/test_dictupdate.py b/tests/unit/utils/test_dictupdate.py index 4e8a7a540846..222f8fe321fa 100644 --- a/tests/unit/utils/test_dictupdate.py +++ b/tests/unit/utils/test_dictupdate.py @@ -9,6 +9,8 @@ # Import Salt libs import salt.utils.dictupdate as dictupdate +from salt.utils.odict import OrderedDict +from salt.exceptions import SaltInvocationError class UtilDictupdateTestCase(TestCase): @@ -91,7 +93,8 @@ def test_update(self): mdict = copy.deepcopy(self.dict1) mdict['C']['F']['G'] = ['a', 'b'] res = dictupdate.update(copy.deepcopy(mdict), - {'C': {'F': {'G': ['c', 'd']}}}, merge_lists=False) + {'C': {'F': {'G': ['c', 'd']}}}, + merge_lists=False) mdict['C']['F']['G'] = ['c', 'd'] self.assertEqual(res, mdict) @@ -99,7 +102,8 @@ def test_update(self): mdict = copy.deepcopy(self.dict1) mdict['C']['F']['G'] = ['a', 'b'] res = dictupdate.update(copy.deepcopy(mdict), - {'C': {'F': {'G': ['c', 'd']}}}, merge_lists=True) + {'C': {'F': {'G': ['c', 'd']}}}, + merge_lists=True) mdict['C']['F']['G'] = ['a', 'b', 'c', 'd'] self.assertEqual(res, mdict) @@ -107,7 +111,8 @@ def test_update(self): mdict = copy.deepcopy(self.dict1) mdict['C']['F']['G'] = ['a', 'b'] res = dictupdate.update(copy.deepcopy(mdict), - {'C': {'F': {'G': ['d', 'c', 'b', 'a']}}}, merge_lists=True) + {'C': {'F': {'G': ['d', 'c', 'b', 'a']}}}, + merge_lists=True) mdict['C']['F']['G'] = ['a', 'b', 'd', 'c'] self.assertEqual(res, mdict) @@ -195,3 +200,127 @@ def test_merge_list_append(self): mdict1['A'] = ['B'] ret = dictupdate.merge_list(mdict1, {'A': ['b', 'c']}) self.assertEqual({'A': [['B'], ['b', 'c']], 'C': {'D': 'E', 'F': {'I': 'J', 'G': 'H'}}}, ret) + + +class UtilDeepDictUpdateTestCase(TestCase): + + dict1 = {'A': 'B', 'C': {'D': 'E', 'F': {'G': 'H', 'I': 'J'}}} + + def test_deep_set_overwrite(self): + ''' + Test overwriting an existing value. + ''' + mdict = copy.deepcopy(self.dict1) + res = dictupdate.set_dict_key_value(mdict, 'C:F', 'foo') + self.assertEqual({'A': 'B', 'C': {'D': 'E', 'F': 'foo'}}, res) + # Verify modify-in-place + self.assertEqual({'A': 'B', 'C': {'D': 'E', 'F': 'foo'}}, mdict) + + # Test using alternative delimiter + res = dictupdate.set_dict_key_value(mdict, 'C/F', {'G': 'H', 'I': 'J'}, delimiter='/') + self.assertEqual(self.dict1, res) + + # Test without using a delimiter in the keys + res = dictupdate.set_dict_key_value(mdict, 'C', None) + self.assertEqual({'A': 'B', 'C': None}, res) + + def test_deep_set_create(self): + ''' + Test creating new nested keys. + ''' + mdict = copy.deepcopy(self.dict1) + res = dictupdate.set_dict_key_value(mdict, 'K:L:M', 'Q') + self.assertEqual({'A': 'B', 'C': {'D': 'E', 'F': {'G': 'H', 'I': 'J'}}, 'K': {'L': {'M': 'Q'}}}, res) + + def test_deep_set_ordered_dicts(self): + ''' + Test creating new nested ordereddicts. + ''' + res = dictupdate.set_dict_key_value({}, 'A:B', 'foo', ordered_dict=True) + self.assertEqual({'A': OrderedDict([('B', 'foo')])}, res) + + def test_deep_append(self): + ''' + Test appending to a list. + ''' + sdict = {'bar': {'baz': [1, 2]}} + res = dictupdate.append_dict_key_value(sdict, 'bar:baz', 42) + self.assertEqual({'bar': {'baz': [1, 2, 42]}}, res) + # Append with alternate delimiter + res = dictupdate.append_dict_key_value(sdict, 'bar~baz', 43, delimiter='~') + self.assertEqual({'bar': {'baz': [1, 2, 42, 43]}}, res) + # Append to a not-yet existing list + res = dictupdate.append_dict_key_value({}, 'foo:bar:baz', 42) + self.assertEqual({'foo': {'bar': {'baz': [42]}}}, res) + + def test_deep_extend(self): + ''' + Test extending a list. + Note that the provided value (to extend with) will be coerced to a list + if this is not already a list. This can cause unexpected behaviour. + ''' + sdict = {'bar': {'baz': [1, 2]}} + res = dictupdate.extend_dict_key_value(sdict, 'bar:baz', [42, 42]) + self.assertEqual({'bar': {'baz': [1, 2, 42, 42]}}, res) + + # Extend a not-yet existing list + res = dictupdate.extend_dict_key_value({}, 'bar:baz:qux', [42]) + self.assertEqual({'bar': {'baz': {'qux': [42]}}}, res) + + # Extend with a dict (remember, foo has been updated in the first test) + res = dictupdate.extend_dict_key_value(sdict, 'bar:baz', {'qux': 'quux'}) + self.assertEqual({'bar': {'baz': [1, 2, 42, 42, 'qux']}}, res) + + def test_deep_extend_illegal_addition(self): + ''' + Test errorhandling extending lists with illegal types. + ''' + # Extend with an illegal type + for extend_with in [42, None]: + with self.assertRaisesRegex(SaltInvocationError, + r"Cannot extend {} with a {}." + "".format(type([]), type(extend_with))): + dictupdate.extend_dict_key_value({}, 'foo', extend_with) + + def test_deep_extend_illegal_source(self): + ''' + Test errorhandling extending things that are not a list. + ''' + # Extend an illegal type + for extend_this in [{}, 42, 'bar']: + with self.assertRaisesRegex(SaltInvocationError, + r"The last key contains a {}, which cannot extend." + "".format(type(extend_this))): + dictupdate.extend_dict_key_value({'foo': extend_this}, 'foo', [42]) + + def test_deep_update(self): + ''' + Test updating a (sub)dict. + ''' + mdict = copy.deepcopy(self.dict1) + res = dictupdate.update_dict_key_value(mdict, 'C:F', {'foo': 'bar', 'qux': 'quux'}) + self.assertEqual({'A': 'B', 'C': {'D': 'E', 'F': {'G': 'H', 'I': 'J', 'foo': 'bar', 'qux': 'quux'}}}, res) + + # Test updating a non-existing subkey + res = dictupdate.update_dict_key_value({}, 'foo:bar:baz', {'qux': 'quux'}) + self.assertEqual({'foo': {'bar': {'baz': {'qux': 'quux'}}}}, res) + # Test updating a non-existing subkey, with a different delimiter + res = dictupdate.update_dict_key_value({}, 'foo bar baz', {'qux': 'quux'}, delimiter=' ') + self.assertEqual({'foo': {'bar': {'baz': {'qux': 'quux'}}}}, res) + + def test_deep_update_illegal_update(self): + ''' + Test errorhandling updating a (sub)dict with illegal types. + ''' + # Update with an illegal type + for update_with in [42, None, [42], 'bar']: + with self.assertRaisesRegex(SaltInvocationError, + r"Cannot update {} with a {}." + "".format(type({}), type(update_with))): + dictupdate.update_dict_key_value({}, 'foo', update_with) + # Again, but now using OrderedDicts + for update_with in [42, None, [42], 'bar']: + with self.assertRaisesRegex(SaltInvocationError, + r"Cannot update {} with a {}." + "".format(type(OrderedDict()), type(update_with))): + dictupdate.update_dict_key_value({}, 'foo', update_with, ordered_dict=True) diff --git a/tests/unit/utils/test_jinja.py b/tests/unit/utils/test_jinja.py index 8e008da8661c..ca72a8280bc8 100644 --- a/tests/unit/utils/test_jinja.py +++ b/tests/unit/utils/test_jinja.py @@ -52,6 +52,13 @@ except ImportError: HAS_TIMELIB = False +try: + import jmespath # pylint: disable=W0611 + HAS_JMESPATH = True +except ImportError: + HAS_JMESPATH = False + +CACHEDIR = os.path.join(TMP, 'jinja-template-cache') BLINESEP = salt.utils.stringutils.to_bytes(os.linesep) @@ -63,7 +70,9 @@ def test_tojson(self): ''' data = {'Non-ascii words': ['süß', 'спам', 'яйца']} result = tojson(data) - expected = '{"Non-ascii words": ["s\\u00fc\\u00df", "\\u0441\\u043f\\u0430\\u043c", "\\u044f\\u0439\\u0446\\u0430"]}' + expected = ('{"Non-ascii words": ["s\\u00fc\\u00df", ' + '"\\u0441\\u043f\\u0430\\u043c", ' + '"\\u044f\\u0439\\u0446\\u0430"]}') assert result == expected, result @@ -803,20 +812,20 @@ def test_load_yaml(self): with self.assertRaises((TypeError, exceptions.TemplateRuntimeError)): env.from_string('{% set document = document|load_yaml %}' - '{{ document.foo }}').render(document={"foo": "it works"}) + '{{ document.foo }}').render(document={"foo": "it works"}) def test_load_tag(self): env = Environment(extensions=[SerializerExtension]) source = '{{ bar }}, ' + \ '{% load_yaml as docu %}{foo: it works, {{ bar }}: baz}{% endload %}' + \ - '{{ docu.foo }}' + '{{ docu.foo }}' rendered = env.from_string(source).render(bar="barred") self.assertEqual(rendered, "barred, it works") source = '{{ bar }}, {% load_json as docu %}{"foo": "it works", "{{ bar }}": "baz"}{% endload %}' + \ - '{{ docu.foo }}' + '{{ docu.foo }}' rendered = env.from_string(source).render(bar="barred") self.assertEqual(rendered, "barred, it works") @@ -931,15 +940,14 @@ def test_nested_structures(self): ('foo', OrderedDict([ ('bar', 'baz'), ('qux', 42) - ]) - ) + ])) ]) rendered = env.from_string('{{ data }}').render(data=data) self.assertEqual( rendered, "{u'foo': {u'bar': u'baz', u'qux': 42}}" if six.PY2 - else "{'foo': {'bar': 'baz', 'qux': 42}}" + else "{'foo': {'bar': 'baz', 'qux': 42}}" ) rendered = env.from_string('{{ data }}').render(data=[ @@ -953,7 +961,117 @@ def test_nested_structures(self): self.assertEqual( rendered, "[{'foo': u'bar'}, {'baz': 42}]" if six.PY2 - else "[{'foo': 'bar'}, {'baz': 42}]" + else "[{'foo': 'bar'}, {'baz': 42}]" + ) + + def test_set_dict_key_value(self): + ''' + Test the `set_dict_key_value` Jinja filter. + ''' + rendered = render_jinja_tmpl("{{ {} | set_dict_key_value('foo:bar:baz', 42) }}", + dict(opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': 42}}}") + + rendered = render_jinja_tmpl("{{ {} | set_dict_key_value('foo.bar.baz', 42, delimiter='.') }}", + dict(opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': 42}}}") + + def test_update_dict_key_value(self): + ''' + Test the `update_dict_key_value` Jinja filter. + ''' + # Use OrderedDicts to avoid random key-order-switches in the rendered string. + expected = OrderedDict([('bar', OrderedDict([('baz', OrderedDict([('qux', 1), ('quux', 3)]))]))]) + dataset = OrderedDict([('bar', OrderedDict([('baz', OrderedDict([('qux', 1)]))]))]) + dataset_exp = OrderedDict([('quux', 3)]) + rendered = render_jinja_tmpl("{{ foo | update_dict_key_value('bar:baz', exp) }}", + dict(foo=dataset, + exp=dataset_exp, + opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual( + rendered, + "{u'bar': {u'baz': {u'qux': 1, u'quux': 3}}}" if six.PY2 + else "{'bar': {'baz': {'qux': 1, 'quux': 3}}}") + + # Test incorrect usage + for update_with in [42, 'foo', [42]]: + template = "{{ {} | update_dict_key_value('bar:baz', update_with) }}" + expected = r"Cannot update {} with a {}.".format(type({}), type(update_with)) + self.assertRaisesRegex( + SaltRenderError, + expected, + render_jinja_tmpl, + template, + dict(update_with=update_with, + opts=self.local_opts, + saltenv='test', + salt=self.local_salt) + ) + + def test_append_dict_key_value(self): + ''' + Test the `append_dict_key_value` Jinja filter. + ''' + rendered = render_jinja_tmpl("{{ {} | append_dict_key_value('foo:bar:baz', 42) }}", + dict(opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': [42]}}}") + + rendered = render_jinja_tmpl("{{ foo | append_dict_key_value('bar:baz', 42) }}", + dict(foo={'bar': {'baz': [1, 2]}}, + opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual( + rendered, + "{u'bar': {u'baz': [1, 2, 42]}}" if six.PY2 + else "{'bar': {'baz': [1, 2, 42]}}" + ) + + def test_extend_dict_key_value(self): + ''' + Test the `extend_dict_key_value` Jinja filter. + ''' + rendered = render_jinja_tmpl("{{ {} | extend_dict_key_value('foo:bar:baz', [42]) }}", + dict(opts=self.local_opts, saltenv='test', salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': [42]}}}") + + rendered = render_jinja_tmpl("{{ foo | extend_dict_key_value('bar:baz', [42, 43]) }}", + dict(foo={'bar': {'baz': [1, 2]}}, + opts=self.local_opts, + saltenv='test', + salt=self.local_salt)) + self.assertEqual( + rendered, + "{u'bar': {u'baz': [1, 2, 42, 43]}}" if six.PY2 + else "{'bar': {'baz': [1, 2, 42, 43]}}" + ) + # Edge cases + rendered = render_jinja_tmpl("{{ {} | extend_dict_key_value('foo:bar:baz', 'quux') }}", + dict(opts=self.local_opts, saltenv='test', salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': ['q', 'u', 'u', 'x']}}}") + # Beware! When supplying a dict, the list gets extended with the dict coerced to a list, + # which will only contain the keys of the dict. + rendered = render_jinja_tmpl("{{ {} | extend_dict_key_value('foo:bar:baz', {'foo': 'bar'}) }}", + dict(opts=self.local_opts, saltenv='test', salt=self.local_salt)) + self.assertEqual(rendered, "{'foo': {'bar': {'baz': ['foo']}}}") + + # Test incorrect usage + template = "{{ {} | extend_dict_key_value('bar:baz', 42) }}" + expected = r"Cannot extend {} with a {}.".format(type([]), type(42)) + self.assertRaisesRegex( + SaltRenderError, + expected, + render_jinja_tmpl, + template, + dict(opts=self.local_opts, saltenv='test', salt=self.local_salt) ) def test_sequence(self): @@ -1321,6 +1439,17 @@ def test_base64_decode(self): dict(opts=self.local_opts, saltenv='test', salt=self.local_salt)) self.assertEqual(rendered, 'random') + @skipIf(HAS_JMESPATH is False, 'The `jmespath` library is not installed.') + def test_json_query(self): + ''' + Test the `json_query` Jinja filter. + ''' + rendered = render_jinja_tmpl( + "{{ [1, 2, 3] | json_query('[1]')}}", + dict(opts=self.local_opts, saltenv='test', salt=self.local_salt) + ) + self.assertEqual(rendered, '2') + # def test_print(self): # env = Environment(extensions=[SerializerExtension]) # source = '{% import_yaml "toto.foo" as docu %}' From f48deb6c3a6b3d681abfec3d8ccbbe4995a036b4 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Fri, 26 Jul 2019 14:14:49 +0200 Subject: [PATCH 09/12] Removed test for json_query as that is not implemented in this branch. --- tests/unit/utils/test_jinja.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/tests/unit/utils/test_jinja.py b/tests/unit/utils/test_jinja.py index ca72a8280bc8..856865856630 100644 --- a/tests/unit/utils/test_jinja.py +++ b/tests/unit/utils/test_jinja.py @@ -52,12 +52,6 @@ except ImportError: HAS_TIMELIB = False -try: - import jmespath # pylint: disable=W0611 - HAS_JMESPATH = True -except ImportError: - HAS_JMESPATH = False - CACHEDIR = os.path.join(TMP, 'jinja-template-cache') BLINESEP = salt.utils.stringutils.to_bytes(os.linesep) @@ -1439,17 +1433,6 @@ def test_base64_decode(self): dict(opts=self.local_opts, saltenv='test', salt=self.local_salt)) self.assertEqual(rendered, 'random') - @skipIf(HAS_JMESPATH is False, 'The `jmespath` library is not installed.') - def test_json_query(self): - ''' - Test the `json_query` Jinja filter. - ''' - rendered = render_jinja_tmpl( - "{{ [1, 2, 3] | json_query('[1]')}}", - dict(opts=self.local_opts, saltenv='test', salt=self.local_salt) - ) - self.assertEqual(rendered, '2') - # def test_print(self): # env = Environment(extensions=[SerializerExtension]) # source = '{% import_yaml "toto.foo" as docu %}' From 6ebc71863fd54e7566764d9c719c8a75d16d74e9 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Mon, 9 Dec 2019 16:40:09 +0100 Subject: [PATCH 10/12] Remove CACHEDIR again This was removed in https://github.com/saltstack/salt/commit/ac0db895abf144832e792f9f83c9562cdc306acb --- tests/unit/utils/test_jinja.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/utils/test_jinja.py b/tests/unit/utils/test_jinja.py index 856865856630..6769b0b6f589 100644 --- a/tests/unit/utils/test_jinja.py +++ b/tests/unit/utils/test_jinja.py @@ -52,7 +52,6 @@ except ImportError: HAS_TIMELIB = False -CACHEDIR = os.path.join(TMP, 'jinja-template-cache') BLINESEP = salt.utils.stringutils.to_bytes(os.linesep) From 39c8148d4aeb3c646b0fc07acc6ef28914f2e170 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 10 Dec 2019 16:30:18 +0100 Subject: [PATCH 11/12] Update docstrings to return type and return comment to restructuredtext format --- salt/utils/dictupdate.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/salt/utils/dictupdate.py b/salt/utils/dictupdate.py index c71cf168dbd8..456e71337303 100644 --- a/salt/utils/dictupdate.py +++ b/salt/utils/dictupdate.py @@ -152,6 +152,8 @@ def ensure_dict_key( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. + :rtype: dict + :return: Returns the modified in-place `in_dict`. ''' if delimiter in keys: a_keys = keys.split(delimiter) @@ -181,8 +183,8 @@ def _dict_rpartition( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. - - :return tuple(dict, str) + :rtype: tuple(dict, str) + :return: (The dict at the one-to-last key, the last key) ''' if delimiter in keys: all_but_last_keys, _, last_key = keys.rpartition(delimiter) @@ -217,8 +219,8 @@ def set_dict_key_value( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. - - :return dict: Though it updates in_dict in-place. + :rtype: dict + :return: Returns the modified in-place `in_dict`. ''' dict_pointer, last_key = _dict_rpartition(in_dict, keys, @@ -246,8 +248,8 @@ def update_dict_key_value( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. - - :return dict: Though it updates in_dict in-place. + :rtype: dict + :return: Returns the modified in-place `in_dict`. ''' dict_pointer, last_key = _dict_rpartition(in_dict, keys, @@ -284,8 +286,8 @@ def append_dict_key_value( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. - - :return dict: Though it updates in_dict in-place. + :rtype: dict + :return: Returns the modified in-place `in_dict`. ''' dict_pointer, last_key = _dict_rpartition(in_dict, keys, @@ -319,8 +321,8 @@ def extend_dict_key_value( :param str delimiter: The delimiter to use in `keys`. Defaults to ':'. :param bool ordered_dict: Create OrderedDicts if keys are missing. Default: create regular dicts. - - :return dict: Though it updates in_dict in-place. + :rtype: dict + :return: Returns the modified in-place `in_dict`. ''' dict_pointer, last_key = _dict_rpartition( in_dict, From 4beb9d3d9ccefcdfd9c78f47a4a5aa11e6273e5b Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Thu, 12 Dec 2019 15:46:13 +0100 Subject: [PATCH 12/12] Use os.path.join instead of hardcoded fullpath to fix test error on Windows --- tests/unit/modules/test_acme.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/modules/test_acme.py b/tests/unit/modules/test_acme.py index f4d7743ef3e0..3400c734389c 100644 --- a/tests/unit/modules/test_acme.py +++ b/tests/unit/modules/test_acme.py @@ -9,6 +9,7 @@ # Import Python libs import textwrap import datetime +import os # Import Salt Testing libs from tests.support.mixins import LoaderModuleMockMixin @@ -197,7 +198,7 @@ def test_cert(self): 'retcode': 0 } result_no_renew = { - "comment": "Certificate /etc/letsencrypt/live/test/cert.pem unchanged", + "comment": "Certificate " + os.path.join('/etc/letsencrypt/live/test', 'cert.pem') + " unchanged", "not_after": datetime.datetime.fromtimestamp(valid_timestamp).isoformat(), "changes": {}, "result": True