diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd8fa378e..e950d7aee3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ Ansible Changes By Release * corrected dig lookup docs * fix type handling for sensu_silence so the module works * added fix for win_iis_webapppool to correctly handle array elements +* Fix bugs caused by lack of collector ordering like service_mgr being incorrect (https://github.com/ansible/ansible/issues/30753) diff --git a/lib/ansible/module_utils/facts/collector.py b/lib/ansible/module_utils/facts/collector.py index 4baf5bd338..232c506803 100644 --- a/lib/ansible/module_utils/facts/collector.py +++ b/lib/ansible/module_utils/facts/collector.py @@ -203,6 +203,28 @@ def build_fact_id_to_collector_map(collectors_for_platform): return fact_id_to_collector_map, aliases_map +def select_collector_classes(collector_names, all_fact_subsets, all_collector_classes): + # TODO: can be a set() + seen_collector_classes = [] + + selected_collector_classes = [] + + for candidate_collector_class in all_collector_classes: + candidate_collector_name = candidate_collector_class.name + + if candidate_collector_name not in collector_names: + continue + + collector_classes = all_fact_subsets.get(candidate_collector_name, []) + + for collector_class in collector_classes: + if collector_class not in seen_collector_classes: + selected_collector_classes.append(collector_class) + seen_collector_classes.append(collector_class) + + return selected_collector_classes + + def collector_classes_from_gather_subset(all_collector_classes=None, valid_subsets=None, minimal_gather_subset=None, @@ -248,19 +270,8 @@ def collector_classes_from_gather_subset(all_collector_classes=None, aliases_map=aliases_map, platform_info=platform_info) - # TODO: can be a set() - seen_collector_classes = [] - - selected_collector_classes = [] - - for collector_name in collector_names: - collector_classes = all_fact_subsets.get(collector_name, []) - - # TODO? log/warn if we dont find an implementation of a fact_id? - - for collector_class in collector_classes: - if collector_class not in seen_collector_classes: - selected_collector_classes.append(collector_class) - seen_collector_classes.append(collector_class) + selected_collector_classes = select_collector_classes(collector_names, + all_fact_subsets, + all_collector_classes) return selected_collector_classes diff --git a/lib/ansible/module_utils/facts/default_collectors.py b/lib/ansible/module_utils/facts/default_collectors.py index b5969f14bb..a302393c94 100644 --- a/lib/ansible/module_utils/facts/default_collectors.py +++ b/lib/ansible/module_utils/facts/default_collectors.py @@ -73,59 +73,81 @@ from ansible.module_utils.facts.virtual.netbsd import NetBSDVirtualCollector from ansible.module_utils.facts.virtual.openbsd import OpenBSDVirtualCollector from ansible.module_utils.facts.virtual.sunos import SunOSVirtualCollector +# these should always be first due to most other facts depending on them +_base = [ + PlatformFactCollector, + DistributionFactCollector, + LSBFactCollector +] + +# These restrict what is possible in others +_restrictive = [ + SelinuxFactCollector, + ApparmorFactCollector, + FipsFactCollector +] + +# general info, not required but probably useful for other facts +_general = [ + PythonFactCollector, + SystemCapabilitiesFactCollector, + PkgMgrFactCollector, + OpenBSDPkgMgrFactCollector, + ServiceMgrFactCollector, + CmdLineFactCollector, + DateTimeFactCollector, + EnvFactCollector, + SshPubKeyFactCollector, + UserFactCollector +] + +# virtual, this might also limit hardware/networking +_virtual = [ + VirtualCollector, + DragonFlyVirtualCollector, + FreeBSDVirtualCollector, + LinuxVirtualCollector, + OpenBSDVirtualCollector, + NetBSDVirtualCollector, + SunOSVirtualCollector, + HPUXVirtualCollector +] + +_hardware = [ + HardwareCollector, + AIXHardwareCollector, + DarwinHardwareCollector, + DragonFlyHardwareCollector, + FreeBSDHardwareCollector, + HPUXHardwareCollector, + HurdHardwareCollector, + LinuxHardwareCollector, + NetBSDHardwareCollector, + OpenBSDHardwareCollector, + SunOSHardwareCollector +] + +_network = [ + DnsFactCollector, + NetworkCollector, + AIXNetworkCollector, + DarwinNetworkCollector, + DragonFlyNetworkCollector, + FreeBSDNetworkCollector, + HPUXNetworkCollector, + HurdNetworkCollector, + LinuxNetworkCollector, + NetBSDNetworkCollector, + OpenBSDNetworkCollector, + SunOSNetworkCollector +] + +# other fact sources +_extra_facts = [ + LocalFactCollector, + FacterFactCollector, + OhaiFactCollector +] + # TODO: make config driven -collectors = [ApparmorFactCollector, - CmdLineFactCollector, - DateTimeFactCollector, - DistributionFactCollector, - DnsFactCollector, - EnvFactCollector, - FipsFactCollector, - - HardwareCollector, - AIXHardwareCollector, - DarwinHardwareCollector, - DragonFlyHardwareCollector, - FreeBSDHardwareCollector, - HPUXHardwareCollector, - HurdHardwareCollector, - LinuxHardwareCollector, - NetBSDHardwareCollector, - OpenBSDHardwareCollector, - SunOSHardwareCollector, - LocalFactCollector, - LSBFactCollector, - - NetworkCollector, - AIXNetworkCollector, - DarwinNetworkCollector, - DragonFlyNetworkCollector, - FreeBSDNetworkCollector, - HPUXNetworkCollector, - HurdNetworkCollector, - LinuxNetworkCollector, - NetBSDNetworkCollector, - OpenBSDNetworkCollector, - SunOSNetworkCollector, - - PkgMgrFactCollector, - OpenBSDPkgMgrFactCollector, - PlatformFactCollector, - PythonFactCollector, - SelinuxFactCollector, - ServiceMgrFactCollector, - SshPubKeyFactCollector, - SystemCapabilitiesFactCollector, - UserFactCollector, - - VirtualCollector, - DragonFlyVirtualCollector, - FreeBSDVirtualCollector, - LinuxVirtualCollector, - OpenBSDVirtualCollector, - NetBSDVirtualCollector, - SunOSVirtualCollector, - HPUXVirtualCollector, - - FacterFactCollector, - OhaiFactCollector] +collectors = _base + _restrictive + _general + _virtual + _hardware + _network + _extra_facts diff --git a/test/integration/inventory b/test/integration/inventory index 3534b8cb8a..697a6a8ba0 100644 --- a/test/integration/inventory +++ b/test/integration/inventory @@ -5,7 +5,7 @@ testhost2 ansible_ssh_host=127.0.0.1 ansible_connection=local testhost3 ansible_ssh_host=127.0.0.3 testhost4 ansible_ssh_host=127.0.0.4 # For testing fact gathering -facthost[0:20] ansible_host=127.0.0.1 ansible_connection=local +facthost[0:25] ansible_host=127.0.0.1 ansible_connection=local [binary_modules] testhost_binary_modules ansible_host=127.0.0.1 ansible_connection=local diff --git a/test/integration/targets/gathering_facts/test_gathering_facts.yml b/test/integration/targets/gathering_facts/test_gathering_facts.yml index 271d44266c..6769bf9ce7 100644 --- a/test/integration/targets/gathering_facts/test_gathering_facts.yml +++ b/test/integration/targets/gathering_facts/test_gathering_facts.yml @@ -11,6 +11,82 @@ - "!hardware" register: not_hardware_facts +- name: min and network test for platform added + hosts: facthost21 + tags: [ 'fact_network' ] + connection: local + gather_subset: "!all,network" + gather_facts: yes + tasks: + - name: Test that retrieving network facts works and gets prereqs from platform and distribution + assert: + that: + - 'ansible_default_ipv4|default("UNDEF") != "UNDEF"' + - 'ansible_interfaces|default("UNDEF") != "UNDEF"' + # these are true for linux, but maybe not for other os + - 'ansible_system|default("UNDEF") != "UNDEF"' + - 'ansible_distribution|default("UNDEF") != "UNDEF"' + # we dont really require these but they are in the min set + # - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"' + # - 'ansible_user_id|default("UNDEF") == "UNDEF"' + # - 'ansible_env|default("UNDEF") == "UNDEF"' + # - 'ansible_selinux|default("UNDEF") == "UNDEF"' + # - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"' + +- name: min and hardware test for platform added + hosts: facthost22 + tags: [ 'fact_hardware' ] + connection: local + gather_subset: "hardware" + gather_facts: yes + tasks: + - name: debug stuff + debug: + var: hostvars['facthost22'] + # we should also collect platform, but not distribution + - name: Test that retrieving hardware facts works and gets prereqs from platform and distribution + when: ansible_system|default("UNDEF") == "Linux" + assert: + # LinuxHardwareCollector requires 'platform' facts + that: + - 'ansible_memory_mb|default("UNDEF") != "UNDEF"' + - 'ansible_default_ipv4|default("UNDEF") == "UNDEF"' + - 'ansible_interfaces|default("UNDEF") == "UNDEF"' + # these are true for linux, but maybe not for other os + # hardware requires 'platform' + - 'ansible_system|default("UNDEF") != "UNDEF"' + - 'ansible_machine|default("UNDEF") != "UNDEF"' + # hardware does not require 'distribution' but it is min set + # - 'ansible_distribution|default("UNDEF") == "UNDEF"' + # we dont really require these but they are in the min set + # - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"' + # - 'ansible_user_id|default("UNDEF") == "UNDEF"' + # - 'ansible_env|default("UNDEF") == "UNDEF"' + # - 'ansible_selinux|default("UNDEF") == "UNDEF"' + # - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"' + +- name: min and service_mgr test for platform added + hosts: facthost23 + tags: [ 'fact_service_mgr' ] + connection: local + gather_subset: "!all,service_mgr" + gather_facts: yes + tasks: + - name: Test that retrieving service_mgr facts works and gets prereqs from platform and distribution + assert: + that: + - 'ansible_service_mgr|default("UNDEF") != "UNDEF"' + - 'ansible_default_ipv4|default("UNDEF") == "UNDEF"' + - 'ansible_interfaces|default("UNDEF") == "UNDEF"' + # these are true for linux, but maybe not for other os + - 'ansible_system|default("UNDEF") != "UNDEF"' + - 'ansible_distribution|default("UNDEF") != "UNDEF"' + # we dont really require these but they are in the min set + # - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"' + # - 'ansible_user_id|default("UNDEF") == "UNDEF"' + # - 'ansible_env|default("UNDEF") == "UNDEF"' + # - 'ansible_selinux|default("UNDEF") == "UNDEF"' + # - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"' - hosts: facthost0 tags: [ 'fact_min' ] @@ -18,8 +94,8 @@ gather_subset: "all" gather_facts: yes tasks: - - setup: - register: facts + #- setup: + # register: facts - name: Test that retrieving all facts works assert: that: @@ -36,7 +112,7 @@ tasks: - setup: filter: "*env*" - register: facts_results + register: fact_results - name: Test that retrieving all facts filtered to env works assert: @@ -53,7 +129,7 @@ tasks: - setup: filter: "ansible_user_id" - register: facts_results + register: fact_results - name: Test that retrieving all facts filtered to specific fact ansible_user_id works assert: @@ -72,7 +148,7 @@ tasks: - setup: filter: "*" - register: facts + register: fact_results - name: Test that retrieving all facts filtered to splat assert: @@ -89,7 +165,7 @@ tasks: - setup: filter: "" - register: facts + register: fact_results - name: Test that retrieving all facts filtered to empty filter_spec works assert: @@ -119,16 +195,16 @@ - hosts: facthost2 tags: [ 'fact_network' ] connection: local - gather_subset: "network" + gather_subset: "!all,!min,network" gather_facts: yes tasks: - name: Test that retrieving network facts work assert: that: - - 'ansible_user_id|default("UNDEF_MIN") != "UNDEF_MIN"' + - 'ansible_user_id|default("UNDEF") == "UNDEF"' - 'ansible_interfaces|default("UNDEF_NET") != "UNDEF_NET"' - - 'ansible_mounts|default("UNDEF_MOUNT") == "UNDEF_MOUNT"' - - 'ansible_virtualization_role|default("UNDEF_VIRT") == "UNDEF_VIRT"' + - 'ansible_mounts|default("UNDEF") == "UNDEF"' + - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"' - hosts: facthost3 tags: [ 'fact_hardware' ] diff --git a/test/units/module_utils/facts/test_collector.py b/test/units/module_utils/facts/test_collector.py index 362a6ffcfe..4e2b0cef50 100644 --- a/test/units/module_utils/facts/test_collector.py +++ b/test/units/module_utils/facts/test_collector.py @@ -20,6 +20,8 @@ from __future__ import (absolute_import, division) __metaclass__ = type +from collections import defaultdict + # for testing from ansible.compat.tests import unittest @@ -28,6 +30,91 @@ from ansible.module_utils.facts import collector from ansible.module_utils.facts import default_collectors +class TestFindCollectorsForPlatform(unittest.TestCase): + def test(self): + compat_platforms = [{'system': 'Generic'}] + res = collector.find_collectors_for_platform(default_collectors.collectors, + compat_platforms) + for coll_class in res: + self.assertIn(coll_class._platform, ('Generic')) + + def test_linux(self): + compat_platforms = [{'system': 'Linux'}] + res = collector.find_collectors_for_platform(default_collectors.collectors, + compat_platforms) + for coll_class in res: + self.assertIn(coll_class._platform, ('Linux')) + + def test_linux_or_generic(self): + compat_platforms = [{'system': 'Generic'}, {'system': 'Linux'}] + res = collector.find_collectors_for_platform(default_collectors.collectors, + compat_platforms) + for coll_class in res: + self.assertIn(coll_class._platform, ('Generic', 'Linux')) + + +class TestSelectCollectorNames(unittest.TestCase): + def test(self): + collector_names = set(['distribution', 'all_ipv4_addresses', + 'local', 'pkg_mgr']) + all_fact_subsets = self._all_fact_subsets() + all_collector_classes = self._all_collector_classes() + res = collector.select_collector_classes(collector_names, + all_fact_subsets, + all_collector_classes) + + expected = [default_collectors.DistributionFactCollector, + default_collectors.PkgMgrFactCollector] + + self.assertEqual(res, expected) + + def test_reverse(self): + collector_names = set(['distribution', 'all_ipv4_addresses', + 'local', 'pkg_mgr']) + all_fact_subsets = self._all_fact_subsets() + all_collector_classes = self._all_collector_classes() + all_collector_classes.reverse() + res = collector.select_collector_classes(collector_names, + all_fact_subsets, + all_collector_classes) + + expected = [default_collectors.PkgMgrFactCollector, + default_collectors.DistributionFactCollector] + + self.assertEqual(res, expected) + + def test_default_collectors(self): + platform_info = {'system': 'Generic'} + compat_platforms = [platform_info] + collectors_for_platform = collector.find_collectors_for_platform(default_collectors.collectors, + compat_platforms) + + all_fact_subsets, aliases_map = collector.build_fact_id_to_collector_map(collectors_for_platform) + + all_valid_subsets = frozenset(all_fact_subsets.keys()) + collector_names = collector.get_collector_names(valid_subsets=all_valid_subsets, + aliases_map=aliases_map, + platform_info=platform_info) + collector.select_collector_classes(collector_names, + all_fact_subsets, + default_collectors.collectors) + + def _all_collector_classes(self): + return [default_collectors.DistributionFactCollector, + default_collectors.PkgMgrFactCollector, + default_collectors.LinuxNetworkCollector] + + def _all_fact_subsets(self, data=None): + all_fact_subsets = defaultdict(list) + _data = {'pkg_mgr': [default_collectors.PkgMgrFactCollector], + 'distribution': [default_collectors.DistributionFactCollector], + 'network': [default_collectors.LinuxNetworkCollector]} + data = data or _data + for key, value in data.items(): + all_fact_subsets[key] = value + return all_fact_subsets + + class TestGetCollectorNames(unittest.TestCase): def test_none(self): res = collector.get_collector_names()