From 88ecf0a5728a39f430bb7f709f4c2b6aa59b583e Mon Sep 17 00:00:00 2001 From: Tomas Jelinek Date: Wed, 31 Jan 2024 10:39:12 +0100 Subject: [PATCH] fix stonith level validation --- CHANGELOG.md | 2 + pcs/lib/cib/fencing_topology.py | 26 ++++------- .../tier0/lib/cib/test_fencing_topology.py | 44 +++++++------------ pcs_test/tier1/legacy/test_stonith.py | 24 +++++----- 4 files changed, 39 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 62e7f230..208126ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,10 @@ - Do not put empty uid/gid corosync configuration options to an uidgid file when not specified in `pcs cluster uidgid add` command. Empty options cause corosync start failure. ([ghissue#772]) +- Do not allow fencing levels other than 1..9 ([RHEL-2977]) [ghissue#772]: https://github.com/ClusterLabs/pcs/issues/772 +[RHEL-2977]: https://issues.redhat.com/browse/RHEL-2977 ## [0.11.7] - 2024-01-11 diff --git a/pcs/lib/cib/fencing_topology.py b/pcs/lib/cib/fencing_topology.py index 6a9fc71e..2434723b 100644 --- a/pcs/lib/cib/fencing_topology.py +++ b/pcs/lib/cib/fencing_topology.py @@ -2,7 +2,6 @@ from typing import ( Optional, Sequence, Set, - Tuple, ) from lxml import etree @@ -23,6 +22,7 @@ from pcs.common.reports import codes as report_codes from pcs.common.reports import has_errors from pcs.common.reports.item import ReportItem from pcs.common.types import StringCollection +from pcs.common.validate import is_integer from pcs.lib.cib.resource.stonith import is_stonith_resource from pcs.lib.cib.tools import find_unique_id from pcs.lib.errors import LibraryError @@ -60,9 +60,8 @@ def add_level( bool force_node -- continue even if a node (target) does not exist """ # pylint: disable=too-many-arguments - report_list, valid_level = _validate_level(level) reporter.report_list( - report_list + _validate_level(level) + _validate_target( cluster_status_nodes, target_type, target_value, force_node ) @@ -78,7 +77,7 @@ def add_level( if reporter.has_errors: raise LibraryError() _append_level_element( - topology_el, valid_level, target_type, target_value, devices + topology_el, int(level), target_type, target_value, devices ) @@ -262,22 +261,15 @@ def verify( return report_list -def _validate_level(level) -> Tuple[ReportItemList, Optional[int]]: +def _validate_level(level) -> ReportItemList: report_list: ReportItemList = [] - try: - candidate = int(level) - if candidate > 0: - return report_list, candidate - except ValueError: - pass - report_list.append( - ReportItem.error( - reports.messages.InvalidOptionValue( - "level", level, "a positive integer" + if not is_integer(level, 1, 9): + report_list.append( + ReportItem.error( + reports.messages.InvalidOptionValue("level", level, "1..9") ) ) - ) - return report_list, None + return report_list def _validate_target( diff --git a/pcs_test/tier0/lib/cib/test_fencing_topology.py b/pcs_test/tier0/lib/cib/test_fencing_topology.py index f00d6f31..7c16e15d 100644 --- a/pcs_test/tier0/lib/cib/test_fencing_topology.py +++ b/pcs_test/tier0/lib/cib/test_fencing_topology.py @@ -105,7 +105,7 @@ class StatusNodesMixin: @patch_lib("_validate_level_target_devices_does_not_exist", return_value=[]) @patch_lib("_validate_devices", return_value=[]) @patch_lib("_validate_target", return_value=[]) -@patch_lib("_validate_level", return_value=([], "valid_level")) +@patch_lib("_validate_level", return_value=[]) class AddLevel(TestCase): # pylint: disable=too-many-instance-attributes def setUp(self): @@ -193,6 +193,7 @@ class AddLevel(TestCase): mock_val_dupl, mock_append, ): + self.level = 1 lib.add_level( self.reporter, self.topology_el, @@ -210,7 +211,7 @@ class AddLevel(TestCase): ) mock_append.assert_called_once_with( self.topology_el, - "valid_level", + self.level, self.target_type, self.target_value, self.devices, @@ -224,22 +225,17 @@ class AddLevel(TestCase): mock_val_dupl, mock_append, ): - mock_val_level.return_value = ( - [ - reports.item.ReportItem.error( - reports.messages.InvalidOptionValue( - "level", self.level, "a positive integer" - ) - ) - ], - None, - ) + mock_val_level.return_value = [ + reports.item.ReportItem.error( + reports.messages.InvalidOptionValue("level", self.level, "1..9") + ) + ] report_list = [ fixture.error( report_codes.INVALID_OPTION_VALUE, option_value=self.level, option_name="level", - allowed_values="a positive integer", + allowed_values="1..9", cannot_be_empty=False, forbidden_characters=None, ), @@ -746,25 +742,17 @@ class Verify(TestCase, CibMixin, StatusNodesMixin): class ValidateLevel(TestCase): def test_success(self): - level_list = [ - (1, 1), - ("1", 1), - (9, 9), - ("9", 9), - ("05", 5), - ] - for level_in, level_out in level_list: - with self.subTest(level=level_in): - report_list, valid_level = lib._validate_level(level_in) - self.assertEqual(level_out, valid_level) + level_list = [1, "1", 9, "9", "05"] + for level in level_list: + with self.subTest(level=level): + report_list = lib._validate_level(level) assert_report_item_list_equal(report_list, []) def test_invalid(self): - level_list = ["", 0, "0", -1, "-1", "1abc"] + level_list = ["", 0, "0", -1, "-1", "1abc", "10"] for level in level_list: with self.subTest(level=level): - report_list, valid_level = lib._validate_level(level) - self.assertEqual(None, valid_level) + report_list = lib._validate_level(level) assert_report_item_list_equal( report_list, [ @@ -772,7 +760,7 @@ class ValidateLevel(TestCase): report_codes.INVALID_OPTION_VALUE, option_value=level, option_name="level", - allowed_values="a positive integer", + allowed_values="1..9", cannot_be_empty=False, forbidden_characters=None, ), diff --git a/pcs_test/tier1/legacy/test_stonith.py b/pcs_test/tier1/legacy/test_stonith.py index 76dd11c3..b293f1a7 100644 --- a/pcs_test/tier1/legacy/test_stonith.py +++ b/pcs_test/tier1/legacy/test_stonith.py @@ -2227,36 +2227,36 @@ class LevelAdd(LevelTestsBase): self.assert_pcs_fail( "stonith level add NaN rh7-1 F1".split(), ( - "Error: 'NaN' is not a valid level value, use a positive " - "integer\n" + ERRORS_HAVE_OCCURRED + "Error: 'NaN' is not a valid level value, use 1..9\n" + + ERRORS_HAVE_OCCURRED ), ) self.assert_pcs_fail( "-- stonith level add -10 rh7-1 F1".split(), ( - "Error: '-10' is not a valid level value, use a positive " - "integer\n" + ERRORS_HAVE_OCCURRED + "Error: '-10' is not a valid level value, use 1..9\n" + + ERRORS_HAVE_OCCURRED ), ) self.assert_pcs_fail( "stonith level add 10abc rh7-1 F1".split(), ( - "Error: '10abc' is not a valid level value, use a positive " - "integer\n" + ERRORS_HAVE_OCCURRED + "Error: '10abc' is not a valid level value, use 1..9\n" + + ERRORS_HAVE_OCCURRED ), ) self.assert_pcs_fail( "stonith level add 0 rh7-1 F1".split(), ( - "Error: '0' is not a valid level value, use a positive " - "integer\n" + ERRORS_HAVE_OCCURRED + "Error: '0' is not a valid level value, use 1..9\n" + + ERRORS_HAVE_OCCURRED ), ) self.assert_pcs_fail( "stonith level add 000 rh7-1 F1".split(), ( - "Error: '000' is not a valid level value, use a positive " - "integer\n" + ERRORS_HAVE_OCCURRED + "Error: '000' is not a valid level value, use 1..9\n" + + ERRORS_HAVE_OCCURRED ), ) @@ -2273,7 +2273,7 @@ class LevelAdd(LevelTestsBase): self.assert_pcs_fail( "stonith level add x rh7-X F0 dev@ce".split(), ( - "Error: 'x' is not a valid level value, use a positive integer\n" + "Error: 'x' is not a valid level value, use 1..9\n" "Error: Node 'rh7-X' does not appear to exist in configuration, " "use --force to override\n" "Error: invalid device id 'dev@ce', '@' is not a valid character " @@ -2286,7 +2286,7 @@ class LevelAdd(LevelTestsBase): self.assert_pcs_fail( "stonith level add x rh7-X F0 dev@ce --force".split(), ( - "Error: 'x' is not a valid level value, use a positive integer\n" + "Error: 'x' is not a valid level value, use 1..9\n" "Warning: Node 'rh7-X' does not appear to exist in configuration\n" "Error: invalid device id 'dev@ce', '@' is not a valid character " "for a device id\n" -- 2.25.1