From 88ecf0a5728a39f430bb7f709f4c2b6aa59b583e Mon Sep 17 00:00:00 2001
From: Tomas Jelinek <tojeline@redhat.com>
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

