validate sbd options

ClusterLabs · Jun 4, 2024 · be17a9b · be17a9b
1 parent af9510a
commit be17a9b
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@
 - Report an error when an invalid resource-discovery is specified ([RHEL-7701])
 - 'pcs booth destroy' now works for nodes without a cluster (such as
   arbitrators) ([RHEL-7737])
+- Validate SBD\_DELAY\_START and SBD\_STARTMODE options ([RHEL-17962])
 
 ### Deprecated
 - Pcs produces warnings about [features planned to be removed in pacemaker 3](https://projects.clusterlabs.org/w/projects/pacemaker/pacemaker_3.0_changes/pacemaker_3.0_configuration_changes/):
@@ -40,10 +41,11 @@
 [RHEL-7701]: https://issues.redhat.com/browse/RHEL-7701
 [RHEL-7737]: https://issues.redhat.com/browse/RHEL-7737
 [RHEL-16231]: https://issues.redhat.com/browse/RHEL-16231
+[RHEL-17962]: https://issues.redhat.com/browse/RHEL-17962
+[RHEL-21051]: https://issues.redhat.com/browse/RHEL-21051
+[RHEL-25854]: https://issues.redhat.com/browse/RHEL-25854
 [RHEL-27492]: https://issues.redhat.com/browse/RHEL-27492
 [RHEL-28749]: https://issues.redhat.com/browse/RHEL-28749
-[RHEL-25854]: https://issues.redhat.com/browse/RHEL-25854
-[RHEL-21051]: https://issues.redhat.com/browse/RHEL-21051
 [RHEL-36514]: https://issues.redhat.com/browse/RHEL-36514
 
 

diff --git a/pcs/lib/commands/sbd.py b/pcs/lib/commands/sbd.py
@@ -1,6 +1,9 @@
+from typing import Any
+
 from pcs import settings
 from pcs.common import reports
 from pcs.common.reports.item import ReportItem
+from pcs.common.validate import is_integer
 from pcs.lib import (
     sbd,
     validate,
@@ -22,13 +25,13 @@
 from pcs.lib.node import get_existing_nodes_names
 from pcs.lib.tools import environment_file_to_dict
 
-UNSUPPORTED_SBD_OPTION_LIST = [
+_UNSUPPORTED_SBD_OPTION_LIST = [
     "SBD_WATCHDOG_DEV",
     "SBD_OPTS",
     "SBD_PACEMAKER",
     "SBD_DEVICE",
 ]
-ALLOWED_SBD_OPTION_LIST = [
+_ALLOWED_SBD_OPTION_LIST = [
     "SBD_DELAY_START",
     "SBD_STARTMODE",
     "SBD_WATCHDOG_TIMEOUT",
@@ -38,13 +41,14 @@
     {"flush", "noflush"},
     {"reboot", "off", "crashdump"},
 )
+_STARTMODE_ALLOWED_VALUES = ["always", "clean"]
 
 
 def __tuple(set1, set2):
     return {f"{v1},{v2}" for v1 in set1 for v2 in set2}
 
 
-TIMEOUT_ACTION_ALLOWED_VALUE_LIST = sorted(
+_TIMEOUT_ACTION_ALLOWED_VALUE_LIST = sorted(
     _TIMEOUT_ACTION_ALLOWED_VALUES[0]
     | _TIMEOUT_ACTION_ALLOWED_VALUES[1]
     | __tuple(
@@ -56,6 +60,15 @@ def __tuple(set1, set2):
 )
 
 
+class _ValueSbdDelayStart(validate.ValuePredicateBase):
+    def _is_valid(self, value: validate.TypeOptionValue) -> bool:
+        # 1 means yes, so we don't allow it to prevent confusion
+        return value in ["yes", "no"] or is_integer(value, 2)
+
+    def _get_allowed_values(self) -> Any:
+        return "'yes', 'no' or an integer greater than 1"
+
+
 def _validate_sbd_options(
     sbd_config, allow_unknown_opts=False, allow_invalid_option_values=False
 ):
@@ -68,16 +81,29 @@ def _validate_sbd_options(
     """
     validators = [
         validate.NamesIn(
-            ALLOWED_SBD_OPTION_LIST,
-            banned_name_list=UNSUPPORTED_SBD_OPTION_LIST,
+            _ALLOWED_SBD_OPTION_LIST,
+            banned_name_list=_UNSUPPORTED_SBD_OPTION_LIST,
             severity=reports.item.get_severity(
                 reports.codes.FORCE, allow_unknown_opts
             ),
         ),
+        _ValueSbdDelayStart(
+            "SBD_DELAY_START",
+            severity=reports.item.get_severity(
+                reports.codes.FORCE, allow_invalid_option_values
+            ),
+        ),
+        validate.ValueIn(
+            "SBD_STARTMODE",
+            _STARTMODE_ALLOWED_VALUES,
+            severity=reports.item.get_severity(
+                reports.codes.FORCE, allow_invalid_option_values
+            ),
+        ),
         validate.ValueNonnegativeInteger("SBD_WATCHDOG_TIMEOUT"),
         validate.ValueIn(
             "SBD_TIMEOUT_ACTION",
-            TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
+            _TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
             severity=reports.item.get_severity(
                 reports.codes.FORCE, allow_invalid_option_values
             ),

diff --git a/pcs_test/tier0/lib/commands/sbd/test_enable_sbd.py b/pcs_test/tier0/lib/commands/sbd/test_enable_sbd.py
@@ -6,8 +6,8 @@
 from pcs.common import reports
 from pcs.common.reports import codes as report_codes
 from pcs.lib.commands.sbd import (
-    ALLOWED_SBD_OPTION_LIST,
-    TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
+    _ALLOWED_SBD_OPTION_LIST,
+    _TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
     enable_sbd,
 )
 from pcs.lib.corosync.config_parser import Parser
@@ -808,18 +808,56 @@ def test_invalid_opt_values(self):
                 default_watchdog="/dev/watchdog",
                 watchdog_dict={},
                 sbd_options={
+                    "SBD_DELAY_START": "bad_delay",
+                    "SBD_STARTMODE": "bad_startmode",
+                    "SBD_WATCHDOG_TIMEOUT": "bad_timeout",
                     "SBD_TIMEOUT_ACTION": "noflush,flush",
+                    "UNKNOWN_OPT1": 1,
                 },
             )
         )
         self.env_assist.assert_reports(
             [
+                fixture.error(
+                    report_codes.INVALID_OPTIONS,
+                    option_names=["UNKNOWN_OPT1"],
+                    option_type=None,
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
+                    allowed_patterns=[],
+                    force_code=report_codes.FORCE,
+                ),
+                fixture.error(
+                    report_codes.INVALID_OPTION_VALUE,
+                    force_code=report_codes.FORCE,
+                    option_name="SBD_DELAY_START",
+                    option_value="bad_delay",
+                    allowed_values="'yes', 'no' or an integer greater than 1",
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
+                fixture.error(
+                    report_codes.INVALID_OPTION_VALUE,
+                    force_code=report_codes.FORCE,
+                    option_name="SBD_STARTMODE",
+                    option_value="bad_startmode",
+                    allowed_values=["always", "clean"],
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
+                fixture.error(
+                    report_codes.INVALID_OPTION_VALUE,
+                    option_name="SBD_WATCHDOG_TIMEOUT",
+                    option_value="bad_timeout",
+                    allowed_values="a non-negative integer",
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
                 fixture.error(
                     report_codes.INVALID_OPTION_VALUE,
                     force_code=report_codes.FORCE,
                     option_name="SBD_TIMEOUT_ACTION",
                     option_value="noflush,flush",
-                    allowed_values=TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
+                    allowed_values=_TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
                     cannot_be_empty=False,
                     forbidden_characters=None,
                 ),
@@ -833,6 +871,9 @@ def test_invalid_opt_values_forced(self):
                 default_watchdog="/dev/watchdog",
                 watchdog_dict={},
                 sbd_options={
+                    "SBD_DELAY_START": "bad_delay",
+                    "SBD_STARTMODE": "bad_startmode",
+                    "SBD_WATCHDOG_TIMEOUT": "bad_timeout",
                     "SBD_TIMEOUT_ACTION": "noflush,flush",
                     "UNKNOWN_OPT1": 1,
                 },
@@ -841,22 +882,46 @@ def test_invalid_opt_values_forced(self):
         )
         self.env_assist.assert_reports(
             [
-                fixture.warn(
-                    report_codes.INVALID_OPTION_VALUE,
-                    option_name="SBD_TIMEOUT_ACTION",
-                    option_value="noflush,flush",
-                    allowed_values=TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
-                    cannot_be_empty=False,
-                    forbidden_characters=None,
-                ),
                 fixture.error(
                     report_codes.INVALID_OPTIONS,
                     option_names=["UNKNOWN_OPT1"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                     force_code=report_codes.FORCE,
                 ),
+                fixture.warn(
+                    report_codes.INVALID_OPTION_VALUE,
+                    option_name="SBD_DELAY_START",
+                    option_value="bad_delay",
+                    allowed_values="'yes', 'no' or an integer greater than 1",
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
+                fixture.warn(
+                    report_codes.INVALID_OPTION_VALUE,
+                    option_name="SBD_STARTMODE",
+                    option_value="bad_startmode",
+                    allowed_values=["always", "clean"],
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
+                fixture.error(
+                    report_codes.INVALID_OPTION_VALUE,
+                    option_name="SBD_WATCHDOG_TIMEOUT",
+                    option_value="bad_timeout",
+                    allowed_values="a non-negative integer",
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
+                fixture.warn(
+                    report_codes.INVALID_OPTION_VALUE,
+                    option_name="SBD_TIMEOUT_ACTION",
+                    option_value="noflush,flush",
+                    allowed_values=_TIMEOUT_ACTION_ALLOWED_VALUE_LIST,
+                    cannot_be_empty=False,
+                    forbidden_characters=None,
+                ),
             ]
         )
 
@@ -880,15 +945,15 @@ def test_unknown_sbd_opts(self):
                     report_codes.INVALID_OPTIONS,
                     option_names=["UNKNOWN_OPT1", "UNKNOWN_OPT2"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                     force_code=report_codes.FORCE,
                 ),
                 fixture.error(
                     report_codes.INVALID_OPTIONS,
                     option_names=["SBD_WATCHDOG_DEV"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                 ),
             ]
@@ -915,14 +980,14 @@ def test_unknown_sbd_opts_allowed(self):
                     report_codes.INVALID_OPTIONS,
                     option_names=["UNKNOWN_OPT1", "UNKNOWN_OPT2"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                 ),
                 fixture.error(
                     report_codes.INVALID_OPTIONS,
                     option_names=["SBD_WATCHDOG_DEV"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                 ),
             ]
@@ -1164,14 +1229,14 @@ def test_multiple_validation_failures(self):
                     report_codes.INVALID_OPTIONS,
                     option_names=["SBD_WATCHDOG_DEV"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                 ),
                 fixture.error(
                     report_codes.INVALID_OPTIONS,
                     option_names=["UNKNOWN_OPT1", "UNKNOWN_OPT2"],
                     option_type=None,
-                    allowed=sorted(ALLOWED_SBD_OPTION_LIST),
+                    allowed=sorted(_ALLOWED_SBD_OPTION_LIST),
                     allowed_patterns=[],
                     force_code=report_codes.FORCE,
                 ),