From e7e5ceba8cf4328ad7523da7a2887775fb168e2b Mon Sep 17 00:00:00 2001 From: Guillaume Demonet Date: Mon, 28 Dec 2020 10:46:04 +0100 Subject: [PATCH] salt: Avoid duplicating static pod manifests When using `metalk8s.static_pod_managed`, we call `file.managed` behind the scenes. This state does a lot of magic, including creating a temporary file with the new contents before replacing the old file. This temp file gets created **in the same directory** as the managed file by default, so it gets picked up by `kubelet` as if it were another static Pod to manage. If the replacement occurs too late, `kubelet` may have already created another Pod for the temp file, and may not be able to "remember" the old Pod, hence not cleaning it up. This results in "rogue containers", which can create issues (e.g. preventing new containers from binding some ports on the host). This commit reimplements the 'file.managed' state in a minimal fashion, to ensure the temporary file used for making an "atomic replace" is ignored by kubelet. Note that it requires us to also reimplement the 'file.manage_file' execution function, since it always relies on the existing "atomic copy" operation from `salt.utils.files.copyfile`. Fixes: #2840 --- salt/_modules/metalk8s.py | 164 ++++++++++++++++++ salt/_states/metalk8s.py | 111 +++++++++--- .../unit/modules/files/test_metalk8s.yaml | 94 ++++++++++ salt/tests/unit/modules/test_metalk8s.py | 151 +++++++++++++++- 4 files changed, 492 insertions(+), 28 deletions(-) diff --git a/salt/_modules/metalk8s.py b/salt/_modules/metalk8s.py index 0eddc39028..d48ac2786b 100644 --- a/salt/_modules/metalk8s.py +++ b/salt/_modules/metalk8s.py @@ -3,17 +3,20 @@ Module for handling MetalK8s specific calls. ''' import functools +import itertools import logging import os.path import re import six import socket +import tempfile import time from salt.pillar import get_pillar from salt.exceptions import CommandExecutionError import salt.utils.args import salt.utils.files +from salt.utils.hashutils import get_hash log = logging.getLogger(__name__) @@ -355,3 +358,164 @@ def cmp_sorted(*args, **kwargs): kwargs['key'] = functools.cmp_to_key(kwargs.pop('cmp')) return sorted(*args, **kwargs) + + +def _error(ret, err_msg): + ret["result"] = False + ret["comment"] = err_msg + return ret + + +def _atomic_copy(source, dest, tmp_prefix="."): # pragma: no cover + """Minimalistic implementation of an atomic copy operation. + + First, we write the contents of the source file to a temporary file, which + is saved in the same directory as the target, with a custom prefix. + Finally, we link the temporary file contents to the destination filename. + """ + base_name = os.path.basename(dest) + dir_name = os.path.dirname(dest) + + with open(source, mode='rb') as src_file: + contents = src_file.read() + + with tempfile.NamedTemporaryFile( + prefix=tmp_prefix, dir=dir_name, delete=False, + ) as tmp_file: + tmp_file.write(contents) + + try: + os.replace(tmp_file.name, dest) + except OSError as exc: + os.remove(tmp_file.name) + raise + + +def manage_static_pod_manifest( + name, source_filename, source, source_sum, saltenv='base', +): + """Checks a manifest file and applies changes if necessary. + + Implementation derived from saltstack/salt salt.modules.file.manage_file. + + name: + Path to the static pod manifest. + + source_filename: + Path to the cached source file on the minion. The hash sum of this + file will be compared with the `source_sum` argument to determine + whether the source file should be fetched again using `cp.cache_file`. + + source: + Reference for the source file (from the master). + + source_sum: + Hash sum for the source file. + + CLI Example: + .. code-block:: bash + salt '*' metalk8s.manage_static_pod_manifest /etc/kubernetes/manifests/etcd.yaml '' salt://metalk8s/kubernetes/etcd/files/manifest.yaml '{hash_type: 'md5', 'hsum': }' saltenv=metalk8s-2.7.0 + """ + ret = {"name": name, "changes": {}, "comment": "", "result": True} + + def _clean_tmp(sfn): + if sfn.startswith(os.path.join( + tempfile.gettempdir(), salt.utils.files.TEMPFILE_PREFIX + )): + # Don't remove if it exists in file_roots (any saltenv) + all_roots = itertools.chain.from_iterable( + __opts__["file_roots"].values() + ) + in_roots = any(sfn.startswith(root) for root in all_roots) + # Only clean up files that exist + if os.path.exists(sfn) and not in_roots: + os.remove(sfn) + + target_dir = os.path.dirname(name) + target_exists = os.path.isfile(name) or os.path.islink(name) + hash_type = source_sum.get("hash_type", __opts__["hash_type"]) + + if not source: + return _error(ret, "Must provide a source") + if not os.path.isdir(target_dir): + return _error( + ret, "Target directory {} does not exist".format(target_dir) + ) + + if source_filename: + # File should be already cached, verify its checksum + cached_hash = get_hash(source_filename, form=hash_type) + if source_sum.get("hsum") != cached_hash: + log.debug( + "Cached source file {} does not match expected checksum, " + "will fetch it again".format(source_filename) + ) + source_filename = '' # Reset source filename to fetch it again + + if not source_filename: + # File is not present or outdated, cache it + source_filename = __salt__["cp.cache_file"](source, saltenv) + if not source_filename: + return _error( + ret, "Source file '{}' not found".format(source) + ) + + # Recalculate source sum now that file has been cached + source_sum = { + "hash_type": hash_type, + "hsum": get_hash(source_filename, form=hash_type) + } + + # Check changes if the target file exists + if target_exists: + if os.path.islink(name): + real_name = os.path.realpath(name) + else: + real_name = name + + target_hash = get_hash(real_name, hash_type) + source_hash = source_sum.get("hsum") + + # Check if file needs to be replaced + if source_hash != target_hash: + # Print a diff equivalent to diff -u old new + if __salt__["config.option"]("obfuscate_templates"): + ret["changes"]["diff"] = "" + else: + try: + ret["changes"]["diff"] = __salt__["file.get_diff"]( + real_name, source_filename, show_filenames=False + ) + except CommandExecutionError as exc: + ret["changes"]["diff"] = exc.strerror + + else: # target file does not exist + ret["changes"]["diff"] = "New file" + real_name = name + + if ret["changes"] and not __opts__["test"]: + # The file needs to be replaced + try: + _atomic_copy(source_filename, real_name) + except OSError as io_error: + _clean_tmp(source_filename) + return _error(ret, "Failed to commit change: {}".format(io_error)) + + # Always enforce perms, even if no changes to contents (this module is + # idempotent) + ret, _ = __salt__["file.check_perms"]( + name, ret, user="root", group="root", mode="0600", + ) + + if ret["changes"]: + if __opts__["test"]: + ret["comment"] = "File {} would be updated".format(name) + else: + ret["comment"] = "File {} updated".format(name) + + elif not ret["changes"] and ret["result"]: + ret["comment"] = "File {} is in the correct state".format(name) + + if source_filename: + _clean_tmp(source_filename) + return ret diff --git a/salt/_states/metalk8s.py b/salt/_states/metalk8s.py index 5e8f9dbf43..e68287779b 100644 --- a/salt/_states/metalk8s.py +++ b/salt/_states/metalk8s.py @@ -1,28 +1,39 @@ """Custom states for MetalK8s.""" +import logging +import tempfile import time +import traceback import re +from salt.ext import six + __virtualname__ = "metalk8s" +log = logging.getLogger(__name__) def __virtual__(): return __virtualname__ +def _error(ret, err_msg): + ret["result"] = False + ret["comment"] = err_msg + return ret + + def static_pod_managed(name, source, config_files=None, config_files_opt=None, - context=None, - **kwargs): + context=None): """Simple helper to edit a static Pod manifest if configuration changes. Expects the template to use: - `config_digest` variable and store it in the `metadata.annotations` section, with the key `metalk8s.scality.com/config-digest`. - - `metalk8s_version` variabble and store it in the `metadata.labels` + - `metalk8s_version` variable and store it in the `metadata.labels` section, with the key `metalk8s.scality.com/version`. name: @@ -41,21 +52,42 @@ def static_pod_managed(name, context: Context to use to render the source template. - - kwargs: - Any arguments supported by `file.managed` are supported. """ + ret = {"changes": {}, "comment": "", "name": name, "result": True} + + if not name: + return _error(ret, "Manifest file name is required") + + if not isinstance(source, six.text_type): + return _error(ret, "Source must be a single string") + if not config_files: config_files = [] for config_file in config_files_opt or []: if __salt__["file.file_exists"](config_file): config_files.append(config_file) + else: + log.debug( + "Ignoring optional config file %s: file does not exist", + config_file + ) + + config_file_digests = [] + for config_file in config_files: + try: + digest = __salt__["hashutil.digest_file"]( + config_file, checksum="sha256" + ) + except CommandExecutionError as exc: + return _error( + ret, + "Unable to compute digest of config file {}: {}".format( + config_file, exc + ) + ) + config_file_digests.append(digest) - config_file_digests = [ - __salt__["hashutil.digest_file"](config_file, checksum="sha256") - for config_file in config_files - ] config_digest = __salt__["hashutil.md5_digest"]( "-".join(config_file_digests) ) @@ -63,22 +95,53 @@ def static_pod_managed(name, match = re.search(r'metalk8s-(?P.+)$', __env__) metalk8s_version = match.group('version') if match else "unknown" - return __states__["file.managed"]( - name, - source, - template=kwargs.pop("template", "jinja"), - user=kwargs.pop("user", "root"), - group=kwargs.pop("group", "root"), - mode=kwargs.pop("mode", "0600"), - makedirs=kwargs.pop("makedirs", False), - backup=kwargs.pop("backup", False), - context=dict( - context or {}, - config_digest=config_digest, metalk8s_version=metalk8s_version - ), - **kwargs + context_ = dict( + context or {}, + config_digest=config_digest, metalk8s_version=metalk8s_version ) + if __opts__["test"]: + log.warning("Test functionality is not yet implemented.") + ret["comment"] = ( + "The manifest {} is in the correct state (supposedly)." + ).format(name) + return ret + + # Gather the source file from the server + try: + source_filename, source_sum, comment_ = __salt__["file.get_managed"]( + name, + template="jinja", + source=source, + source_hash="", + source_hash_name=None, + user="root", + group="root", + mode="0600", + attrs=None, + saltenv=__env__, + context=context_, + defaults=None, + ) + except Exception as exc: # pylint: disable=broad-except + log.debug(traceback.format_exc()) + return _error(ret, "Unable to get managed file: {}".format(exc)) + + if comment_: + return _error(ret, comment_) + else: + try: + return __salt__["metalk8s.manage_static_pod_manifest"]( + name, + source_filename, + source, + source_sum, + saltenv=__env__, + ) + except Exception as exc: # pylint: disable=broad-except + log.debug(traceback.format_exc()) + return _error(ret, "Unable to manage file: {}".format(exc)) + def module_run(name, attemps=1, sleep_time=10, **kwargs): """Classic module.run with a retry logic as it's buggy in salt version diff --git a/salt/tests/unit/modules/files/test_metalk8s.yaml b/salt/tests/unit/modules/files/test_metalk8s.yaml index c24dbde1c5..69dc268c0e 100644 --- a/salt/tests/unit/modules/files/test_metalk8s.yaml +++ b/salt/tests/unit/modules/files/test_metalk8s.yaml @@ -328,3 +328,97 @@ format_slots: my_mod.my_fun: null raises: True result: "Unable to compute slot '__slot__:salt:my_mod.my_fun\\(\\)': An error has occurred" + +manage_static_pod_manifest: + # Nominal: pre-cached source + - name: &manifest_name /etc/kubernetes/manifests/my-pod.yaml + source: &manifest_source salt://my/state/files/my-pod-manifest.yaml.j2 + pre_cached_source: True + result: &manifest_nominal_result_no_changes + name: *manifest_name + changes: {} + comment: >- + File /etc/kubernetes/manifests/my-pod.yaml is in the correct state + result: True + # Nominal: source not in cache + - name: *manifest_name + source: *manifest_source + result: *manifest_nominal_result_no_changes + # Nominal: cached hash mismatch + - name: *manifest_name + source: *manifest_source + pre_cached_source: True + cached_hash_mismatch: True + result: *manifest_nominal_result_no_changes + # Nominal: target is a link + - name: *manifest_name + source: *manifest_source + target_links_to: /some/other/path.yaml + result: *manifest_nominal_result_no_changes + # Nominal: target hash mismatch + - name: *manifest_name + source: *manifest_source + target_hash_mismatch: True + result: &manifest_nominal_result_updated + <<: *manifest_nominal_result_no_changes + changes: + diff: Some diff + comment: >- + File /etc/kubernetes/manifests/my-pod.yaml updated + # Nominal: new file + - name: *manifest_name + source: *manifest_source + target_exists: False + result: + <<: *manifest_nominal_result_updated + changes: + diff: New file + # Nominal: obfuscate templates + - name: *manifest_name + source: *manifest_source + obfuscate_templates: True + target_hash_mismatch: True + result: + <<: *manifest_nominal_result_updated + changes: + diff: + # Nominal: file.get_diff failure + - name: *manifest_name + source: *manifest_source + get_diff_error: Failed to compute diff + target_hash_mismatch: True + result: + <<: *manifest_nominal_result_updated + changes: + diff: Failed to compute diff + # Nominal: test mode + - name: *manifest_name + source: *manifest_source + target_hash_mismatch: True + opts: + test: True + result: + <<: *manifest_nominal_result_updated + comment: >- + File /etc/kubernetes/manifests/my-pod.yaml would be updated + # Error: missing source + - name: *manifest_name + error: Must provide a source + # Error: target directory does not exist + - name: *manifest_name + source: *manifest_source + target_dir_exists: False + error: Target directory /etc/kubernetes/manifests does not exist + # Error: source could not be cached + - name: *manifest_name + source: *manifest_source + cache_file_ret: False + error: >- + Source file 'salt://my/state/files/my-pod-manifest.yaml.j2' not found + # Error: copy error + - name: *manifest_name + source: *manifest_source + target_exists: False + atomic_copy_raises: Could not copy! + error: >- + Failed to commit change: Could not copy! diff --git a/salt/tests/unit/modules/test_metalk8s.py b/salt/tests/unit/modules/test_metalk8s.py index fc2fd4c400..8c4067e6f8 100644 --- a/salt/tests/unit/modules/test_metalk8s.py +++ b/salt/tests/unit/modules/test_metalk8s.py @@ -1,9 +1,11 @@ import os.path +import tempfile from unittest import TestCase from unittest.mock import MagicMock, mock_open, patch from parameterized import param, parameterized from salt.exceptions import CommandExecutionError +import salt.utils.files import yaml import metalk8s @@ -270,10 +272,7 @@ def test_check_pillar_keys(self, keys, result, raises=False, else: pillar_get_mock.assert_not_called() - @parameterized.expand( - param.explicit(kwargs=test_case) - for test_case in yaml.safe_load(open(YAML_TESTS_FILE))["format_slots"] - ) + @utils.parameterized_from_cases(YAML_TESTS_CASES["format_slots"]) def test_format_slots(self, data, result, slots_returns=None, raises=False): """ @@ -320,3 +319,147 @@ def test_cmp_sorted(self, obj, result, **kwargs): metalk8s.cmp_sorted(obj, **kwargs), result ) + + @utils.parameterized_from_cases( + YAML_TESTS_CASES["manage_static_pod_manifest"] + ) + def test_manage_static_pod_manifest( + self, + name, + result=None, + error=None, + pre_cached_source=False, + cached_hash_mismatch=False, + target_hash_mismatch=False, + target_exists=True, + target_dir_exists=True, + target_links_to=None, + obfuscate_templates=False, + get_diff_error=None, + opts=None, + cache_file_ret=None, + atomic_copy_raises=None, + **kwargs, + ): + """Test the behaviour of ``manage_static_pod_manifest` function.""" + + isdir_mock = MagicMock(return_value=target_dir_exists) + isfile_mock = MagicMock(return_value=target_exists) + islink_mock = MagicMock(return_value=(target_links_to is not None)) + exists_mock = MagicMock(return_value=True) # Only used for _clean_tmp + remove_mock = MagicMock() # Only used for _clean_tmp + + source_filename = "" + if pre_cached_source: + source_filename = os.path.join( + tempfile.gettempdir(), + '{}some-tmp-file'.format(salt.utils.files.TEMPFILE_PREFIX), + ) + + real_name = name + realpath_mock = MagicMock(side_effect=lambda x: x) + if target_links_to is not None: + real_name = target_links_to + realpath_mock.return_value = target_links_to + + if cache_file_ret is None: + cache_file_ret = os.path.join( + tempfile.gettempdir(), + '{}other-tmp-file'.format(salt.utils.files.TEMPFILE_PREFIX), + ) + cache_file_mock = MagicMock(return_value=cache_file_ret) + + def _atomic_copy(src, dst): + if atomic_copy_raises: + raise OSError(atomic_copy_raises) + + atomic_copy_mock = MagicMock(side_effect=_atomic_copy) + + def _get_hash(filename, form="md5"): + if cached_hash_mismatch and filename == source_filename: + return 'some-different-hash' + + if target_hash_mismatch and filename == real_name: + return 'some-outdated-hash' + + return 'some-hash' + + get_hash_mock = MagicMock(side_effect=_get_hash) + + def _option(key): + if key == "obfuscate_templates": + return obfuscate_templates + raise NotImplementedError( + "This 'config.option' mock only handles the " + "'obfuscate_templates' key" + ) + + option_mock = MagicMock(side_effect=_option) + + def _check_perms(name, ret, **kwargs): + return ret, None + + check_perms_mock = MagicMock(side_effect=_check_perms) + + def _get_diff(*_a, **_k): + if get_diff_error is not None: + raise CommandExecutionError(get_diff_error) + return "Some diff" + + get_diff_mock = MagicMock(side_effect=_get_diff) + + salt_dict = { + "config.option": option_mock, + "cp.cache_file": cache_file_mock, + "file.check_perms": check_perms_mock, + "file.get_diff": get_diff_mock, + } + opts_dict = dict( + { + "test": False, + "hash_type": "md5", + "file_roots": {"base": ["/srv/salt"]}, + }, + **(opts or {}) + ) + call_kwargs = dict( + { + "source": "", + "source_filename": source_filename, + "source_sum": {"hsum": "some-hash"}, + }, + **kwargs + ) + + with patch.dict(metalk8s.__opts__, opts_dict), \ + patch.dict(metalk8s.__salt__, salt_dict), \ + patch("os.remove", remove_mock), \ + patch("os.path.exists", exists_mock), \ + patch("os.path.isdir", isdir_mock), \ + patch("os.path.isfile", isfile_mock), \ + patch("os.path.islink", islink_mock), \ + patch("os.path.realpath", realpath_mock), \ + patch("metalk8s.get_hash", get_hash_mock), \ + patch("metalk8s._atomic_copy", atomic_copy_mock): + actual_result = metalk8s.manage_static_pod_manifest( + name, **call_kwargs + ) + + if error is not None: + self.assertIsNone( + result, + "Cannot provide both `result` and `error` in a test case" + ) + self.assertFalse(actual_result["result"]) + self.assertEqual(actual_result["comment"], error) + else: + self.assertEqual(actual_result, result) + + if cached_hash_mismatch: + self.assertEqual(cache_file_mock.call_count, 1) + + if error is not None and atomic_copy_raises is None: + # We should not have reached the tempfile cleanup + self.assertEqual(remove_mock.call_count, 0) + else: + self.assertEqual(remove_mock.call_count, 1)