From 72d1af86f37c886dad379d9b4f51238fdd6dd7ed Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Sun, 2 Jul 2023 22:01:53 +0200 Subject: [PATCH] [PR #6783/867704dd backport][stable-7] rhsm_repository: refactor handling of subscription-manager (#6830) rhsm_repository: refactor handling of subscription-manager (#6783) Create a small helper class Rhsm, so all the logic related to the interaction with subscription-manager is grouped there: - create the Rhsm object in main(), once the initial checks are done - search subscription-manager as required (so there is no need to manually check it), and store its path for reuse - store the common arguments for running subscription-manager - move run_subscription_manager() to Rhsm as run_repos() - get rid of the different list parameters: we list only all the repositories, so the other cases are not needed (and can be added easily, if needed) - move get_repository_list() to Rhsm as list_repositories() The execution of subscription-manager is improved as well: - pass the arguments to run_command() directly as list, rather than joining the arguments to string, which run_command() will need to split again - move the "repos" parameter directly in run_repos() - explicitly disable the shell, already off by default - disable the expansions of variables, as there are none Adapt the unit test to the different way run_command() is called. There should be no behaviour changes. (cherry picked from commit 867704dd75415d46b2d9cb6576d5e4cf7d5f18d9) Co-authored-by: Pino Toscano --- ...6783-rhsm_repository-internal-refactor.yml | 6 + plugins/modules/rhsm_repository.py | 163 +++++----- .../plugins/modules/test_rhsm_repository.py | 307 ++++++++++++------ 3 files changed, 305 insertions(+), 171 deletions(-) create mode 100644 changelogs/fragments/6783-rhsm_repository-internal-refactor.yml diff --git a/changelogs/fragments/6783-rhsm_repository-internal-refactor.yml b/changelogs/fragments/6783-rhsm_repository-internal-refactor.yml new file mode 100644 index 0000000000..7b76118d68 --- /dev/null +++ b/changelogs/fragments/6783-rhsm_repository-internal-refactor.yml @@ -0,0 +1,6 @@ +minor_changes: + - | + rhsm_repository - the interaction with ``subscription-manager`` was + refactored by grouping things together, removing unused bits, and hardening + the way it is run; no behaviour change is expected + (https://github.com/ansible-collections/community.general/pull/6783). diff --git a/plugins/modules/rhsm_repository.py b/plugins/modules/rhsm_repository.py index 151d0e8810..2a7d8acf2a 100644 --- a/plugins/modules/rhsm_repository.py +++ b/plugins/modules/rhsm_repository.py @@ -97,86 +97,91 @@ from copy import deepcopy from ansible.module_utils.basic import AnsibleModule -def run_subscription_manager(module, arguments): - # Execute subscription-manager with arguments and manage common errors - rhsm_bin = module.get_bin_path('subscription-manager') - if not rhsm_bin: - module.fail_json(msg='The executable file subscription-manager was not found in PATH') +class Rhsm(object): + def __init__(self, module): + self.module = module + self.rhsm_bin = self.module.get_bin_path('subscription-manager', required=True) + self.rhsm_kwargs = { + 'environ_update': dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'), + 'expand_user_and_vars': False, + 'use_unsafe_shell': False, + } - lang_env = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C') - rc, out, err = module.run_command("%s %s" % (rhsm_bin, " ".join(arguments)), environ_update=lang_env) + def run_repos(self, arguments): + """ + Execute `subscription-manager repos` with arguments and manage common errors + """ + rc, out, err = self.module.run_command( + [self.rhsm_bin, 'repos'] + arguments, + **self.rhsm_kwargs + ) - if rc == 0 and out == 'This system has no repositories available through subscriptions.\n': - module.fail_json(msg='This system has no repositories available through subscriptions') - elif rc == 1: - module.fail_json(msg='subscription-manager failed with the following error: %s' % err) - else: - return rc, out, err + if rc == 0 and out == 'This system has no repositories available through subscriptions.\n': + self.module.fail_json(msg='This system has no repositories available through subscriptions') + elif rc == 1: + self.module.fail_json(msg='subscription-manager failed with the following error: %s' % err) + else: + return rc, out, err + + def list_repositories(self): + """ + Generate RHSM repository list and return a list of dict + """ + rc, out, err = self.run_repos(['--list']) + + skip_lines = [ + '+----------------------------------------------------------+', + ' Available Repositories in /etc/yum.repos.d/redhat.repo' + ] + repo_id_re = re.compile(r'Repo ID:\s+(.*)') + repo_name_re = re.compile(r'Repo Name:\s+(.*)') + repo_url_re = re.compile(r'Repo URL:\s+(.*)') + repo_enabled_re = re.compile(r'Enabled:\s+(.*)') + + repo_id = '' + repo_name = '' + repo_url = '' + repo_enabled = '' + + repo_result = [] + for line in out.splitlines(): + if line == '' or line in skip_lines: + continue + + repo_id_match = repo_id_re.match(line) + if repo_id_match: + repo_id = repo_id_match.group(1) + continue + + repo_name_match = repo_name_re.match(line) + if repo_name_match: + repo_name = repo_name_match.group(1) + continue + + repo_url_match = repo_url_re.match(line) + if repo_url_match: + repo_url = repo_url_match.group(1) + continue + + repo_enabled_match = repo_enabled_re.match(line) + if repo_enabled_match: + repo_enabled = repo_enabled_match.group(1) + + repo = { + "id": repo_id, + "name": repo_name, + "url": repo_url, + "enabled": True if repo_enabled == '1' else False + } + + repo_result.append(repo) + + return repo_result -def get_repository_list(module, list_parameter): - # Generate RHSM repository list and return a list of dict - if list_parameter == 'list_enabled': - rhsm_arguments = ['repos', '--list-enabled'] - elif list_parameter == 'list_disabled': - rhsm_arguments = ['repos', '--list-disabled'] - elif list_parameter == 'list': - rhsm_arguments = ['repos', '--list'] - rc, out, err = run_subscription_manager(module, rhsm_arguments) - - skip_lines = [ - '+----------------------------------------------------------+', - ' Available Repositories in /etc/yum.repos.d/redhat.repo' - ] - repo_id_re = re.compile(r'Repo ID:\s+(.*)') - repo_name_re = re.compile(r'Repo Name:\s+(.*)') - repo_url_re = re.compile(r'Repo URL:\s+(.*)') - repo_enabled_re = re.compile(r'Enabled:\s+(.*)') - - repo_id = '' - repo_name = '' - repo_url = '' - repo_enabled = '' - - repo_result = [] - for line in out.splitlines(): - if line == '' or line in skip_lines: - continue - - repo_id_match = repo_id_re.match(line) - if repo_id_match: - repo_id = repo_id_match.group(1) - continue - - repo_name_match = repo_name_re.match(line) - if repo_name_match: - repo_name = repo_name_match.group(1) - continue - - repo_url_match = repo_url_re.match(line) - if repo_url_match: - repo_url = repo_url_match.group(1) - continue - - repo_enabled_match = repo_enabled_re.match(line) - if repo_enabled_match: - repo_enabled = repo_enabled_match.group(1) - - repo = { - "id": repo_id, - "name": repo_name, - "url": repo_url, - "enabled": True if repo_enabled == '1' else False - } - - repo_result.append(repo) - - return repo_result - - -def repository_modify(module, state, name, purge=False): +def repository_modify(module, rhsm, state, name, purge=False): name = set(name) - current_repo_list = get_repository_list(module, 'list') + current_repo_list = rhsm.list_repositories() updated_repo_list = deepcopy(current_repo_list) matched_existing_repo = {} for repoid in name: @@ -191,7 +196,7 @@ def repository_modify(module, state, name, purge=False): results = [] diff_before = "" diff_after = "" - rhsm_arguments = ['repos'] + rhsm_arguments = [] for repoid in matched_existing_repo: if len(matched_existing_repo[repoid]) == 0: @@ -236,7 +241,7 @@ def repository_modify(module, state, name, purge=False): 'after_header': "RHSM repositories"} if not module.check_mode and changed: - rc, out, err = run_subscription_manager(module, rhsm_arguments) + rc, out, err = rhsm.run_repos(rhsm_arguments) results = out.splitlines() module.exit_json(results=results, changed=changed, repositories=updated_repo_list, diff=diff) @@ -256,6 +261,8 @@ def main(): msg="Interacting with subscription-manager requires root permissions ('become: true')" ) + rhsm = Rhsm(module) + name = module.params['name'] state = module.params['state'] purge = module.params['purge'] @@ -268,7 +275,7 @@ def main(): collection_name='community.general', ) - repository_modify(module, state, name, purge) + repository_modify(module, rhsm, state, name, purge) if __name__ == '__main__': diff --git a/tests/unit/plugins/modules/test_rhsm_repository.py b/tests/unit/plugins/modules/test_rhsm_repository.py index d956148421..249d0495db 100644 --- a/tests/unit/plugins/modules/test_rhsm_repository.py +++ b/tests/unit/plugins/modules/test_rhsm_repository.py @@ -14,6 +14,7 @@ __metaclass__ = type import copy import fnmatch +import itertools import json from ansible.module_utils import basic @@ -143,6 +144,10 @@ Enabled: %s return self.repos +def flatten(iter_of_iters): + return list(itertools.chain.from_iterable(iter_of_iters)) + + # List with test repositories, directly from the Candlepin test data. REPOS_LIST = [ { @@ -239,9 +244,11 @@ REPOS = Repos(REPOS_LIST) # The mock string for the output of `subscription-manager repos --list`. REPOS_LIST_OUTPUT = REPOS.to_subman_list_output() -# MUST match what's in run_subscription_manager() in the module. +# MUST match what's in the Rhsm class in the module. SUBMAN_KWARGS = { 'environ_update': dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'), + 'expand_user_and_vars': False, + 'use_unsafe_shell': False, } @@ -255,15 +262,21 @@ TEST_CASES = [ 'id': 'test_enable_single', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-1000000000000023' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-1000000000000023', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -281,7 +294,11 @@ TEST_CASES = [ 'id': 'test_enable_already_enabled', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), @@ -299,16 +316,23 @@ TEST_CASES = [ 'id': 'test_enable_multiple', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-1000000000000023' - ' --enable content-label-no-gpg-32060' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-1000000000000023', + '--enable', + 'content-label-no-gpg-32060', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -326,16 +350,23 @@ TEST_CASES = [ 'id': 'test_enable_multiple_mixed', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-1000000000000023' - ' --enable fake-content-38072' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-1000000000000023', + '--enable', + 'fake-content-38072', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -354,15 +385,21 @@ TEST_CASES = [ 'id': 'test_purge_everything_but_one_disabled', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable never-enabled-content-801' - ) + ''.join([' --disable ' + i for i in REPOS.ids_enabled() if i != 'never-enabled-content-801']), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'never-enabled-content-801', + ] + flatten([['--disable', i] for i in REPOS.ids_enabled() if i != 'never-enabled-content-801']), SUBMAN_KWARGS, (0, '', '') ), @@ -381,18 +418,27 @@ TEST_CASES = [ 'id': 'test_purge_everything_but_one_enabled', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-99000' - ' --disable content-label-27060' - ' --disable awesomeos-x86_64-99000' - ' --disable fake-content-38072' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-99000', + '--disable', + 'content-label-27060', + '--disable', + 'awesomeos-x86_64-99000', + '--disable', + 'fake-content-38072', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -411,28 +457,47 @@ TEST_CASES = [ 'id': 'test_enable_everything_purge_everything_but_one_enabled', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS.copy().enable('*').to_subman_list_output(), '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable content-label-27060' - ' --disable never-enabled-content-801' - ' --disable never-enabled-content-100000000000060' - ' --disable awesomeos-x86_64-1000000000000023' - ' --disable awesomeos-ppc64-100000000000011' - ' --disable awesomeos-99000' - ' --disable content-label-no-gpg-32060' - ' --disable awesomeos-1000000000000023' - ' --disable awesomeos-x86-100000000000020' - ' --disable awesomeos-x86_64-99000' - ' --disable awesomeos-s390x-99000' - ' --disable awesomeos-modifier-37080' - ' --disable awesomeos-i686-99000' - ' --disable fake-content-38072' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'content-label-27060', + '--disable', + 'never-enabled-content-801', + '--disable', + 'never-enabled-content-100000000000060', + '--disable', + 'awesomeos-x86_64-1000000000000023', + '--disable', + 'awesomeos-ppc64-100000000000011', + '--disable', + 'awesomeos-99000', + '--disable', + 'content-label-no-gpg-32060', + '--disable', + 'awesomeos-1000000000000023', + '--disable', + 'awesomeos-x86-100000000000020', + '--disable', + 'awesomeos-x86_64-99000', + '--disable', + 'awesomeos-s390x-99000', + '--disable', + 'awesomeos-modifier-37080', + '--disable', + 'awesomeos-i686-99000', + '--disable', + 'fake-content-38072', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -450,23 +515,37 @@ TEST_CASES = [ 'id': 'test_enable_all_awesomeos_star', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-x86_64-1000000000000023' - ' --enable awesomeos-ppc64-100000000000011' - ' --enable awesomeos-99000' - ' --enable awesomeos-1000000000000023' - ' --enable awesomeos-x86-100000000000020' - ' --enable awesomeos-x86_64-99000' - ' --enable awesomeos-s390x-99000' - ' --enable awesomeos-modifier-37080' - ' --enable awesomeos-i686-99000' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-x86_64-1000000000000023', + '--enable', + 'awesomeos-ppc64-100000000000011', + '--enable', + 'awesomeos-99000', + '--enable', + 'awesomeos-1000000000000023', + '--enable', + 'awesomeos-x86-100000000000020', + '--enable', + 'awesomeos-x86_64-99000', + '--enable', + 'awesomeos-s390x-99000', + '--enable', + 'awesomeos-modifier-37080', + '--enable', + 'awesomeos-i686-99000', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -485,18 +564,27 @@ TEST_CASES = [ 'id': 'test_purge_everything_but_awesomeos_list', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --enable awesomeos-99000' - ' --enable awesomeos-x86_64-99000' - ' --disable content-label-27060' - ' --disable fake-content-38072' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--enable', + 'awesomeos-99000', + '--enable', + 'awesomeos-x86_64-99000', + '--disable', + 'content-label-27060', + '--disable', + 'fake-content-38072', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -514,7 +602,11 @@ TEST_CASES = [ 'id': 'test_enable_nonexisting', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), @@ -533,15 +625,21 @@ TEST_CASES = [ 'id': 'test_disable_single', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --disable awesomeos-99000' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--disable', + 'awesomeos-99000', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -560,15 +658,21 @@ TEST_CASES = [ 'id': 'test_disable_single_using_absent', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --disable awesomeos-99000' - ), + [ + '/testbin/subscription-manager', + 'repos', + '--disable', + 'awesomeos-99000', + ], SUBMAN_KWARGS, (0, '', '') ), @@ -587,7 +691,11 @@ TEST_CASES = [ 'id': 'test_disable_already_disabled', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), @@ -607,15 +715,19 @@ TEST_CASES = [ 'id': 'test_disable_already_disabled_and_purge', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ' --disable never-enabled-content-801' - ) + ''.join([' --disable ' + i for i in REPOS.ids_enabled()]), + [ + '/testbin/subscription-manager', + 'repos', + ] + flatten([['--disable', i] for i in REPOS.ids_enabled()]), SUBMAN_KWARGS, (0, '', '') ), @@ -635,14 +747,19 @@ TEST_CASES = [ 'id': 'test_disable_single_and_purge', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ), ( - ( - '/testbin/subscription-manager repos' - ) + ''.join([' --disable ' + i for i in REPOS.ids_enabled()]), + [ + '/testbin/subscription-manager', + 'repos', + ] + flatten([['--disable', i] for i in REPOS.ids_enabled()]), SUBMAN_KWARGS, (0, '', '') ), @@ -661,7 +778,11 @@ TEST_CASES = [ 'id': 'test_disable_nonexisting', 'run_command.calls': [ ( - '/testbin/subscription-manager repos --list', + [ + '/testbin/subscription-manager', + 'repos', + '--list', + ], SUBMAN_KWARGS, (0, REPOS_LIST_OUTPUT, '') ),