From 08f92a9f0f318c73ff4528be255c7066d436a85e Mon Sep 17 00:00:00 2001 From: Adrian Likins Date: Sat, 20 Jan 2018 15:07:27 -0500 Subject: [PATCH] Fix fact deps when 'filter=ansible_fact' is used. (#33441) The accumulated collected_facts was being update with new facts _after_ filtering them. So only facts that pass the filter would ever be passed to other fact collectors. For 'filter=ansible_service_mgr', even though it requires the platform and distribution facts and even collects them, they would get filtered out and never passed to the other collectors that need them (service_mgr for ex). Fix is just to add the unfiltered facts to collected_facts. Adds unit tests for fact filter and collected_facts. Fixes #32286 --- .../module_utils/facts/ansible_collector.py | 8 +- .../facts/test_ansible_collector.py | 154 +++++++++++++++++- 2 files changed, 156 insertions(+), 6 deletions(-) diff --git a/lib/ansible/module_utils/facts/ansible_collector.py b/lib/ansible/module_utils/facts/ansible_collector.py index 360ac2f2f16..8ca0089efae 100644 --- a/lib/ansible/module_utils/facts/ansible_collector.py +++ b/lib/ansible/module_utils/facts/ansible_collector.py @@ -67,10 +67,6 @@ class AnsibleFactCollector(collector.BaseFactCollector): for collector_obj in self.collectors: info_dict = {} - # shallow copy of the accumulated collected facts to pass to each collector - # for reference. - collected_facts.update(facts_dict.copy()) - try: # Note: this collects with namespaces, so collected_facts also includes namespaces @@ -80,6 +76,10 @@ class AnsibleFactCollector(collector.BaseFactCollector): sys.stderr.write(repr(e)) sys.stderr.write('\n') + # shallow copy of the new facts to pass to each collector in collected_facts so facts + # can reference other facts they depend on. + collected_facts.update(info_dict.copy()) + # NOTE: If we want complicated fact dict merging, this is where it would hook in facts_dict.update(self._filter(info_dict, self.filter_spec)) diff --git a/test/units/module_utils/facts/test_ansible_collector.py b/test/units/module_utils/facts/test_ansible_collector.py index 27bb6da9fc7..3d61dce4bcb 100644 --- a/test/units/module_utils/facts/test_ansible_collector.py +++ b/test/units/module_utils/facts/test_ansible_collector.py @@ -75,13 +75,16 @@ ALL_COLLECTOR_CLASSES = \ FacterFactCollector] -def mock_module(gather_subset=None): +def mock_module(gather_subset=None, + filter=None): if gather_subset is None: gather_subset = ['all', '!facter', '!ohai'] + if filter is None: + filter = '*' mock_module = Mock() mock_module.params = {'gather_subset': gather_subset, 'gather_timeout': 5, - 'filter': '*'} + 'filter': filter} mock_module.get_bin_path = Mock(return_value=None) return mock_module @@ -258,6 +261,153 @@ class TestCollectedFacts(unittest.TestCase): self.assertNotIn(not_expected_fact, facts_keys) +class ProvidesOtherFactCollector(collector.BaseFactCollector): + name = 'provides_something' + _fact_ids = set(['needed_fact']) + + def collect(self, module=None, collected_facts=None): + return {'needed_fact': 'THE_NEEDED_FACT_VALUE'} + + +class RequiresOtherFactCollector(collector.BaseFactCollector): + name = 'requires_something' + + def collect(self, module=None, collected_facts=None): + collected_facts = collected_facts or {} + fact_dict = {} + fact_dict['needed_fact'] = collected_facts['needed_fact'] + fact_dict['compound_fact'] = "compound-%s" % collected_facts['needed_fact'] + return fact_dict + + +class ConCatFactCollector(collector.BaseFactCollector): + name = 'concat_collected' + + def collect(self, module=None, collected_facts=None): + collected_facts = collected_facts or {} + fact_dict = {} + con_cat_list = [] + for key, value in collected_facts.items(): + con_cat_list.append(value) + + fact_dict['concat_fact'] = '-'.join(con_cat_list) + return fact_dict + + +class TestCollectorDepsWithFilter(unittest.TestCase): + gather_subset = ['all', '!facter', '!ohai'] + + def _mock_module(self, gather_subset=None, filter=None): + return mock_module(gather_subset=self.gather_subset, + filter=filter) + + def setUp(self): + self.mock_module = self._mock_module() + self.collectors = self._collectors(mock_module) + + def _collectors(self, module, + all_collector_classes=None, + minimal_gather_subset=None): + return [ProvidesOtherFactCollector(), + RequiresOtherFactCollector()] + + def test_no_filter(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai']) + + facts_dict = self._collect(_mock_module) + + expected = {'needed_fact': 'THE_NEEDED_FACT_VALUE', + 'compound_fact': 'compound-THE_NEEDED_FACT_VALUE'} + + self.assertEquals(expected, facts_dict) + + def test_with_filter_on_compound_fact(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai'], + filter='compound_fact') + + facts_dict = self._collect(_mock_module) + + expected = {'compound_fact': 'compound-THE_NEEDED_FACT_VALUE'} + + self.assertEquals(expected, facts_dict) + + def test_with_filter_on_needed_fact(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai'], + filter='needed_fact') + + facts_dict = self._collect(_mock_module) + + expected = {'needed_fact': 'THE_NEEDED_FACT_VALUE'} + + self.assertEquals(expected, facts_dict) + + def test_with_filter_on_compound_gather_compound(self): + _mock_module = mock_module(gather_subset=['!all', '!any', 'compound_fact'], + filter='compound_fact') + + facts_dict = self._collect(_mock_module) + + expected = {'compound_fact': 'compound-THE_NEEDED_FACT_VALUE'} + + self.assertEquals(expected, facts_dict) + + def test_with_filter_no_match(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai'], + filter='ansible_this_doesnt_exist') + + facts_dict = self._collect(_mock_module) + + expected = {} + self.assertEquals(expected, facts_dict) + + def test_concat_collector(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai']) + + _collectors = self._collectors(_mock_module) + _collectors.append(ConCatFactCollector()) + + fact_collector = \ + ansible_collector.AnsibleFactCollector(collectors=_collectors, + namespace=ns, + filter_spec=_mock_module.params['filter']) + + collected_facts = {} + facts_dict = fact_collector.collect(module=_mock_module, + collected_facts=collected_facts) + self.assertIn('concat_fact', facts_dict) + self.assertTrue('THE_NEEDED_FACT_VALUE' in facts_dict['concat_fact']) + + def test_concat_collector_with_filter_on_concat(self): + _mock_module = mock_module(gather_subset=['all', '!facter', '!ohai'], + filter='concat_fact') + + _collectors = self._collectors(_mock_module) + _collectors.append(ConCatFactCollector()) + + fact_collector = \ + ansible_collector.AnsibleFactCollector(collectors=_collectors, + namespace=ns, + filter_spec=_mock_module.params['filter']) + + collected_facts = {} + facts_dict = fact_collector.collect(module=_mock_module, + collected_facts=collected_facts) + self.assertIn('concat_fact', facts_dict) + self.assertTrue('THE_NEEDED_FACT_VALUE' in facts_dict['concat_fact']) + self.assertTrue('compound' in facts_dict['concat_fact']) + + def _collect(self, _mock_module, collected_facts=None): + _collectors = self._collectors(_mock_module) + + fact_collector = \ + ansible_collector.AnsibleFactCollector(collectors=_collectors, + namespace=ns, + filter_spec=_mock_module.params['filter']) + facts_dict = fact_collector.collect(module=_mock_module, + collected_facts=collected_facts) + return facts_dict + + class ExceptionThrowingCollector(collector.BaseFactCollector): def collect(self, module=None, collected_facts=None): raise Exception('A collector failed')