diff --git a/changelogs/fragments/10642-parted-cmd-list.yml b/changelogs/fragments/10642-parted-cmd-list.yml new file mode 100644 index 0000000000..29025512dd --- /dev/null +++ b/changelogs/fragments/10642-parted-cmd-list.yml @@ -0,0 +1,2 @@ +minor_changes: + - parted - using safer mechanism to run external command (https://github.com/ansible-collections/community.general/pull/10642). diff --git a/plugins/modules/parted.py b/plugins/modules/parted.py index 4bf0897afc..7af7b354f5 100644 --- a/plugins/modules/parted.py +++ b/plugins/modules/parted.py @@ -552,9 +552,9 @@ def parted(script, device, align): """ global module, parted_exec # pylint: disable=global-variable-not-assigned - align_option = '-a %s' % align + align_option = ['-a', align] if align == 'undefined': - align_option = '' + align_option = [] """ Use option --fix (-f) if available. Versions prior @@ -562,13 +562,12 @@ def parted(script, device, align): http://savannah.gnu.org/news/?id=10114 """ if parted_version() >= (3, 4, 64): - script_option = '-s -f' + script_option = ['-s', '-f'] else: - script_option = '-s' + script_option = ['-s'] if script and not module.check_mode: - # TODO: convert run_comand() argument to list! - command = "%s %s -m %s %s -- %s" % (parted_exec, script_option, align_option, device, script) + command = [parted_exec] + script_option + ['-m'] + align_option + [device, '--'] + script rc, out, err = module.run_command(command) if rc != 0: @@ -594,10 +593,7 @@ def part_exists(partitions, attribute, number): Looks if a partition that has a specific value for a specific attribute actually exists. """ - return any( - part[attribute] and - part[attribute] == number for part in partitions - ) + return any(part.get(attribute) == number for part in partitions) def check_size_format(size_str): @@ -612,8 +608,8 @@ def main(): global module, units_si, units_iec, parted_exec # pylint: disable=global-variable-not-assigned changed = False - output_script = "" - script = "" + output_script = [] + script = [] module = AnsibleModule( argument_spec=dict( device=dict(type='str', required=True), @@ -695,20 +691,19 @@ def main(): # Assign label if required mklabel_needed = current_device['generic'].get('table', None) != label if mklabel_needed: - script += "mklabel %s " % label + script += ["mklabel", label] # Create partition if required if part_type and (mklabel_needed or not part_exists(current_parts, 'num', number)): - script += "mkpart %s %s%s %s " % ( - part_type, - '%s ' % fs_type if fs_type is not None else '', - part_start, - part_end - ) + script += ["mkpart"] + script += [part_type] + if fs_type is not None: + script += [fs_type] + script += [part_start, part_end] # Set the unit of the run if unit and script: - script = "unit %s %s" % (unit, script) + script = ["unit", unit] + script # If partition exists, try to resize if resize and part_exists(current_parts, 'num', number): @@ -724,10 +719,7 @@ def main(): desired_part_end = convert_to_bytes(size, parsed_unit) if current_part_end != desired_part_end: - script += "resizepart %s %s " % ( - number, - part_end - ) + script += ["resizepart", str(number), part_end] # Execute the script and update the data structure. # This will create the partition for the next steps @@ -735,7 +727,7 @@ def main(): output_script += script parted(script, device, align) changed = True - script = "" + script = [] if not module.check_mode: current_parts = get_device_info(device, unit)['partitions'] @@ -748,10 +740,8 @@ def main(): # Assign name to the partition if name is not None and partition.get('name', None) != name: - # Wrap double quotes in single quotes so the shell doesn't strip - # the double quotes as those need to be included in the arg - # passed to parted - script += 'name %s \'"%s"\' ' % (number, name) + # The double quotes need to be included in the arg passed to parted + script += ['name', str(number), '"%s"' % name] # Manage flags if flags: @@ -765,14 +755,14 @@ def main(): flags_on = list(set(flags) - set(partition['flags'])) for f in flags_on: - script += "set %s %s on " % (number, f) + script += ["set", str(number), f, "on"] for f in flags_off: - script += "set %s %s off " % (number, f) + script += ["set", str(number), f, "off"] # Set the unit of the run if unit and script: - script = "unit %s %s" % (unit, script) + script = ["unit", unit] + script # Execute the script if script: @@ -783,21 +773,20 @@ def main(): elif state == 'absent': # Remove the partition if part_exists(current_parts, 'num', number) or module.check_mode: - script = "rm %s " % number + script = ["rm", str(number)] output_script += script changed = True parted(script, device, align) elif state == 'info': - output_script = "unit '%s' print " % unit - + output_script = ["unit", unit, "print"] # Final status of the device final_device_status = get_device_info(device, unit) module.exit_json( changed=changed, disk=final_device_status['generic'], partitions=final_device_status['partitions'], - script=output_script.strip() + script=output_script ) diff --git a/tests/unit/plugins/modules/test_parted.py b/tests/unit/plugins/modules/test_parted.py index 97c009d3e7..8fd6e35dc5 100644 --- a/tests/unit/plugins/modules/test_parted.py +++ b/tests/unit/plugins/modules/test_parted.py @@ -202,7 +202,7 @@ class TestParted(ModuleTestCase): 'state': 'present', }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='unit KiB mkpart primary 0% 100%') + self.execute_module(changed=True, script=['unit', 'KiB', 'mkpart', 'primary', '0%', '100%']) def test_create_new_partition_1G(self): with set_module_args({ @@ -212,7 +212,7 @@ class TestParted(ModuleTestCase): 'part_end': '1GiB', }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='unit KiB mkpart primary 0% 1GiB') + self.execute_module(changed=True, script=['unit', 'KiB', 'mkpart', 'primary', '0%', '1GiB']) def test_create_new_partition_minus_1G(self): with set_module_args({ @@ -223,7 +223,7 @@ class TestParted(ModuleTestCase): 'part_start': '-1GiB', }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='unit KiB mkpart primary ext2 -1GiB 100%') + self.execute_module(changed=True, script=['unit', 'KiB', 'mkpart', 'primary', 'ext2', '-1GiB', '100%']) def test_remove_partition_number_1(self): with set_module_args({ @@ -232,7 +232,7 @@ class TestParted(ModuleTestCase): 'state': 'absent', }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='rm 1') + self.execute_module(changed=True, script=['rm', '1']) def test_resize_partition(self): with set_module_args({ @@ -243,7 +243,7 @@ class TestParted(ModuleTestCase): 'resize': True }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='resizepart 3 100%') + self.execute_module(changed=True, script=['resizepart', '3', '100%']) def test_change_flag(self): # Flags are set in a second run of parted(). @@ -264,9 +264,9 @@ class TestParted(ModuleTestCase): # When using multiple flags: # order of execution is non deterministic, because set() operations are used in # the current implementation. - expected_calls_order1 = [call('unit KiB set 3 lvm on set 3 boot on ', + expected_calls_order1 = [call(['unit', 'KiB', 'set', '3', 'lvm', 'on', 'set', '3', 'boot', 'on'], '/dev/sdb', 'optimal')] - expected_calls_order2 = [call('unit KiB set 3 boot on set 3 lvm on ', + expected_calls_order2 = [call(['unit', 'KiB', 'set', '3', 'boot', 'on', 'set', '3', 'lvm', 'on'], '/dev/sdb', 'optimal')] self.assertTrue(self.parted.mock_calls == expected_calls_order1 or self.parted.mock_calls == expected_calls_order2) @@ -283,7 +283,8 @@ class TestParted(ModuleTestCase): '_ansible_check_mode': True, }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='unit KiB mkpart primary ext3 257GiB 100% unit KiB set 4 boot on') + self.execute_module(changed=True, + script=['unit', 'KiB', 'mkpart', 'primary', 'ext3', '257GiB', '100%', 'unit', 'KiB', 'set', '4', 'boot', 'on']) def test_create_label_gpt(self): # Like previous test, current implementation use parted to create the partition and @@ -299,7 +300,10 @@ class TestParted(ModuleTestCase): '_ansible_check_mode': True, }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict2): - self.execute_module(changed=True, script='unit KiB mklabel gpt mkpart primary 0% 100% unit KiB name 1 \'"lvmpartition"\' set 1 lvm on') + self.execute_module( + changed=True, + script=['unit', 'KiB', 'mklabel', 'gpt', 'mkpart', 'primary', '0%', '100%', + 'unit', 'KiB', 'name', '1', '"lvmpartition"', 'set', '1', 'lvm', 'on']) def test_change_label_gpt(self): # When partitions already exists and label is changed, mkpart should be called even when partition already exists, @@ -312,7 +316,7 @@ class TestParted(ModuleTestCase): '_ansible_check_mode': True, }): with patch('ansible_collections.community.general.plugins.modules.parted.get_device_info', return_value=parted_dict1): - self.execute_module(changed=True, script='unit KiB mklabel gpt mkpart primary 0% 100%') + self.execute_module(changed=True, script=['unit', 'KiB', 'mklabel', 'gpt', 'mkpart', 'primary', '0%', '100%']) def test_check_mode_unchanged(self): # Test that get_device_info result is checked in check mode too