From 9bd70045c9628d78493dba0eac71571223ce8dda Mon Sep 17 00:00:00 2001 From: Harm Geerts Date: Fri, 30 Apr 2021 21:28:28 +0200 Subject: [PATCH] Fix BaseFileCacheModule#keys to respect prefix (#72789) * Fix BaseFileCacheModule#keys to respect prefix Change: - Previously BaseFileCacheModule#keys would return keys with the cache prefix. These keys are impossible to retrieve from the cache without removing the prefix or using the cache without a prefix. Now it removes the prefix from the key and only returns keys that share the same prefix as the cache. Test Plan: - New unit tests * Add explicit BaseFileCacheModule#keys test Test that files that do not match the cache prefix are ignored. Test that the prefix is removed from the cache key. --- .../fragments/ansible-test-cache-plugin.yml | 3 + lib/ansible/plugins/cache/__init__.py | 12 ++- test/units/plugins/cache/test_cache.py | 85 +++++++++++++++++-- 3 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 changelogs/fragments/ansible-test-cache-plugin.yml diff --git a/changelogs/fragments/ansible-test-cache-plugin.yml b/changelogs/fragments/ansible-test-cache-plugin.yml new file mode 100644 index 00000000000..2c2ebe1c31f --- /dev/null +++ b/changelogs/fragments/ansible-test-cache-plugin.yml @@ -0,0 +1,3 @@ +bugfixes: + - Fix ``keys()`` implementation of ``BaseFileCacheModule`` to strip the prefix + from the key and only return keys that share the same prefix as the cache. diff --git a/lib/ansible/plugins/cache/__init__.py b/lib/ansible/plugins/cache/__init__.py index df139777646..f164acee7b2 100644 --- a/lib/ansible/plugins/cache/__init__.py +++ b/lib/ansible/plugins/cache/__init__.py @@ -190,10 +190,20 @@ class BaseFileCacheModule(BaseCacheModule): return True def keys(self): + # When using a prefix we must remove it from the key name before + # checking the expiry and returning it to the caller. Keys that do not + # share the same prefix cannot be fetched from the cache. + prefix = self.get_option('_prefix') + prefix_length = len(prefix) keys = [] for k in os.listdir(self._cache_dir): - if not (k.startswith('.') or self.has_expired(k)): + if k.startswith('.') or not k.startswith(prefix): + continue + + k = k[prefix_length:] + if not self.has_expired(k): keys.append(k) + return keys def contains(self, key): diff --git a/test/units/plugins/cache/test_cache.py b/test/units/plugins/cache/test_cache.py index a0c36c60264..c4e0079a482 100644 --- a/test/units/plugins/cache/test_cache.py +++ b/test/units/plugins/cache/test_cache.py @@ -19,6 +19,10 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import os +import shutil +import tempfile + from units.compat import unittest, mock from ansible.errors import AnsibleError from ansible.plugins.cache import CachePluginAdjudicator @@ -60,14 +64,14 @@ class TestCachePluginAdjudicator(unittest.TestCase): assert self.cache.get('foo') is None def test___getitem__(self): - with pytest.raises(KeyError) as err: + with pytest.raises(KeyError): self.cache['foo'] def test_pop_with_default(self): assert self.cache.pop('foo', 'bar') == 'bar' def test_pop_without_default(self): - with pytest.raises(KeyError) as err: + with pytest.raises(KeyError): assert self.cache.pop('foo') def test_pop(self): @@ -79,6 +83,15 @@ class TestCachePluginAdjudicator(unittest.TestCase): self.cache.update({'cache_key': {'key2': 'updatedvalue'}}) assert self.cache['cache_key']['key2'] == 'updatedvalue' + def test_update_cache_if_changed(self): + # Changes are stored in the CachePluginAdjudicator and will be + # persisted to the plugin when calling update_cache_if_changed() + # The exception is flush which flushes the plugin immediately. + assert len(self.cache.keys()) == 2 + assert len(self.cache._plugin.keys()) == 0 + self.cache.update_cache_if_changed() + assert len(self.cache._plugin.keys()) == 2 + def test_flush(self): # Fake that the cache already has some data in it but the adjudicator # hasn't loaded it in. @@ -87,18 +100,70 @@ class TestCachePluginAdjudicator(unittest.TestCase): self.cache._plugin.set('another wolf', 'another animal') # The adjudicator does't know about the new entries - assert len(self.cache) == 2 + assert len(self.cache.keys()) == 2 # But the cache itself does - assert len(self.cache._plugin._cache) == 3 + assert len(self.cache._plugin.keys()) == 3 # If we call flush, both the adjudicator and the cache should flush self.cache.flush() - assert len(self.cache) == 0 - assert len(self.cache._plugin._cache) == 0 + assert len(self.cache.keys()) == 0 + assert len(self.cache._plugin.keys()) == 0 + + +class TestJsonFileCache(TestCachePluginAdjudicator): + cache_prefix = '' + + def setUp(self): + self.cache_dir = tempfile.mkdtemp(prefix='ansible-plugins-cache-') + self.cache = CachePluginAdjudicator( + plugin_name='jsonfile', _uri=self.cache_dir, + _prefix=self.cache_prefix) + self.cache['cache_key'] = {'key1': 'value1', 'key2': 'value2'} + self.cache['cache_key_2'] = {'key': 'value'} + + def test_keys(self): + # A cache without a prefix will consider all files in the cache + # directory as valid cache entries. + self.cache._plugin._dump( + 'no prefix', os.path.join(self.cache_dir, 'no_prefix')) + self.cache._plugin._dump( + 'special cache', os.path.join(self.cache_dir, 'special_test')) + + # The plugin does not know the CachePluginAdjudicator entries. + assert sorted(self.cache._plugin.keys()) == [ + 'no_prefix', 'special_test'] + + assert 'no_prefix' in self.cache + assert 'special_test' in self.cache + assert 'test' not in self.cache + assert self.cache['no_prefix'] == 'no prefix' + assert self.cache['special_test'] == 'special cache' + + def tearDown(self): + shutil.rmtree(self.cache_dir) + + +class TestJsonFileCachePrefix(TestJsonFileCache): + cache_prefix = 'special_' + + def test_keys(self): + # For caches with a prefix only files that match the prefix are + # considered. The prefix is removed from the key name. + self.cache._plugin._dump( + 'no prefix', os.path.join(self.cache_dir, 'no_prefix')) + self.cache._plugin._dump( + 'special cache', os.path.join(self.cache_dir, 'special_test')) + + # The plugin does not know the CachePluginAdjudicator entries. + assert sorted(self.cache._plugin.keys()) == ['test'] + + assert 'no_prefix' not in self.cache + assert 'special_test' not in self.cache + assert 'test' in self.cache + assert self.cache['test'] == 'special cache' class TestFactCache(unittest.TestCase): - def setUp(self): with mock.patch('ansible.constants.CACHE_PLUGIN', 'memory'): self.cache = FactCache() @@ -110,6 +175,12 @@ class TestFactCache(unittest.TestCase): self.assertEqual(type(a_copy), dict) self.assertEqual(a_copy, dict(avocado='fruit', daisy='flower')) + def test_flush(self): + self.cache['motorcycle'] = 'vehicle' + self.cache['sock'] = 'clothing' + self.cache.flush() + assert len(self.cache.keys()) == 0 + def test_plugin_load_failure(self): # See https://github.com/ansible/ansible/issues/18751 # Note no fact_connection config set, so this will fail