[pcs-0.11] impove operations validation in pcs commands

CHANGELOG.md

@@ -7,9 +7,14 @@
 - `pcs constraint config` (and its variants for each constraint type) now list
   resources in sets in the order defined in the CIB, instead of sorting them
   alphabetically ([rhbz#2461143]) ([RHEL-176478])
+- Add validation for operations in `pcs resource update`, `pcs resource op add`,
+  `pcs stonith update`, and `pcs stonith op add` commands. Operation names are
+  now validated against agent metadata, and operation option values are also
+  verified ([RHEL-176268])
 
 
 [RHEL-149172]: https://redhat.atlassian.net/browse/RHEL-149172
+[RHEL-176268]: https://redhat.atlassian.net/browse/RHEL-176268
 [RHEL-176478]: https://redhat.atlassian.net/browse/RHEL-176478
 [rhbz#2461143]: https://bugzilla.redhat.com/show_bug.cgi?id=2461143
 

pcs/lib/cib/resource/operations.py

@@ -1,34 +1,18 @@
 from collections import defaultdict
 from dataclasses import replace as dt_replace
-from typing import (
-    Iterable,
-    List,
-    Optional,
-    Tuple,
-    cast,
-)
+from typing import Iterable, List, Optional, Tuple, cast
 
 from lxml import etree
 from lxml.etree import _Element
 
-from pcs.common import (
-    const,
-    pacemaker,
-    reports,
-)
+from pcs.common import const, pacemaker, reports
 from pcs.common.pacemaker.resource.operations import CibResourceOperationDto
-from pcs.common.reports import (
-    ReportItemList,
-    ReportProcessor,
-)
+from pcs.common.reports import ReportItemList, ReportProcessor
 from pcs.common.reports.item import ReportItem
 from pcs.common.tools import timeout_to_seconds
 from pcs.common.types import StringCollection
 from pcs.lib import validate
-from pcs.lib.cib import (
-    nvpair_multi,
-    rule,
-)
+from pcs.lib.cib import nvpair_multi, rule
 from pcs.lib.cib.nvpair import append_new_instance_attributes
 from pcs.lib.cib.resource.agent import (
     complete_operations_options,
@@ -109,16 +93,16 @@ def prepare(
     allowed_operation_name_list -- operation names defined by a resource agent
     allow_invalid -- flag for validation skipping
     """
-    operations_to_validate = _operations_to_normalized(raw_operation_list)
+    operations_to_validate = operations_to_normalized(raw_operation_list)
 
     report_list: ReportItemList = []
     report_list.extend(
-        _validate_operation_list(
+        validate_operation_list(
             operations_to_validate, allowed_operation_name_list, allow_invalid
         )
     )
 
-    operation_list = _normalized_to_operations(
+    operation_list = normalized_to_operations(
         operations_to_validate, new_role_names_supported
     )
 
@@ -146,15 +130,15 @@ def prepare(
     ]
 
 
-def _operations_to_normalized(
+def operations_to_normalized(
     raw_operation_list: Iterable[ResourceOperationFilteredIn],
 ) -> List[validate.TypeOptionNormalizedMap]:
     return [
         validate.values_to_pairs(op, _normalize) for op in raw_operation_list
     ]
 
 
-def _normalized_to_operations(
+def normalized_to_operations(
     normalized_pairs: Iterable[validate.TypeOptionNormalizedMap],
     new_role_names_supported: bool,
 ) -> List[ResourceOperationFilteredOut]:
@@ -170,21 +154,42 @@ def _replace_role(op_dict):
     ]
 
 
-def _validate_operation_list(
-    operation_list, allowed_operation_name_list, allow_invalid=False
-):
+def validate_operation_list(
+    operation_list: Iterable[validate.TypeOptionNormalizedMap],
+    allowed_operation_name_list: Optional[StringCollection],
+    allow_invalid: bool = False,
+) -> ReportItemList:
+    """
+    Validate given operation list and return a list of report items.
+
+    operation_list -- operations with normalized option names and values
+    allowed_operation_name_list -- operation names defined by a resource agent,
+        or None if agent metadata failed to load (in pcs resource update or
+        pcs resource op add). When None, operation name validation is skipped
+        for backward compatibility - the user should still be able to configure
+        operations even when the agent metadata is unavailable.
+        In pcs resource create, a list is always passed (empty for a void agent
+        when an agent loading error is forced).
+    allow_invalid -- if True, downgrade unknown operation name errors to
+        warnings (i.e. allow forcing)
+    """
     severity = reports.item.get_severity(reports.codes.FORCE, allow_invalid)
     option_type = "resource operation"
 
-    validators = [
+    validators: list[validate.ValidatorInterface] = [
         validate.NamesIn(ATTRIBUTES, option_type=option_type),
         validate.IsRequiredAll(["name"], option_type=option_type),
-        validate.ValueIn(
-            "name",
-            allowed_operation_name_list,
-            option_name_for_report="operation name",
-            severity=severity,
-        ),
+    ]
+    if allowed_operation_name_list is not None:
+        validators.append(
+            validate.ValueIn(
+                "name",
+                allowed_operation_name_list,
+                option_name_for_report="operation name",
+                severity=severity,
+            ),
+        )
+    validators += [
         validate.ValueIn("role", const.PCMK_ROLES),
         validate.ValueDeprecated(
             "role",

pcs/resource.py

@@ -57,12 +57,8 @@
 from pcs.common.pacemaker.resource.operations import (
     OCF_CHECK_LEVEL_INSTANCE_ATTRIBUTE_NAME,
 )
-from pcs.common.str_tools import (
-    format_list,
-    format_list_custom_last_separator,
-    format_plural,
-)
-from pcs.lib.cib.resource import guest_node, primitive
+from pcs.common.str_tools import format_list, format_plural
+from pcs.lib.cib.resource import guest_node, operations, primitive
 from pcs.lib.cib.tools import get_resources
 from pcs.lib.commands.resource import (
     _get_nodes_to_validate_against,
@@ -452,8 +448,37 @@ def resource_op_add(argv: Argv, modifiers: InputModifiers) -> None:
     if dom is None:
         dom = utils.get_cib_dom()
 
-    # add the requested operation
-    utils.replace_cib_configuration(resource_operation_add(dom, res_id, argv))
+    res_el = utils.dom_get_resource(dom, res_id)
+    if not res_el:
+        utils.err("Unable to find resource: %s" % res_id)
+
+    allowed_operation_name_list = None
+    agent_name = _get_resource_agent_name_from_rsc_el(res_el)
+    try:
+        agent_facade = _get_resource_agent_facade(agent_name)
+        allowed_operation_name_list = [
+            op.name for op in agent_facade.metadata.actions
+        ]
+    except lib_ra.ResourceAgentError as e:
+        # Do not fail with an error to keep backward compatibility.
+        # NOTE: Reconsider when moving operation commands to pcs.lib.
+        process_library_reports(
+            [
+                lib_ra.resource_agent_error_to_report_item(
+                    e,
+                    reports.ReportItemSeverity.warning(),
+                )
+            ]
+        )
+
+    utils.replace_cib_configuration(
+        resource_operation_add(
+            dom,
+            res_id,
+            argv,
+            allowed_operation_name_list=allowed_operation_name_list,
+        )
+    )
 
 
 def op_delete_cmd(lib: Any, argv: Argv, modifiers: InputModifiers) -> None:
@@ -1046,10 +1071,13 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None:  # noqa: PLR
 
     params = utils.convert_args_to_tuples(ra_values)
 
+    agent_name = _get_resource_agent_name_from_rsc_el(resource)
+    allowed_operation_name_list = None
     try:
-        agent_facade = _get_resource_agent_facade(
-            _get_resource_agent_name_from_rsc_el(resource)
-        )
+        agent_facade = _get_resource_agent_facade(agent_name)
+        allowed_operation_name_list = [
+            op.name for op in agent_facade.metadata.actions
+        ]
         report_list = primitive.validate_resource_instance_attributes_update(
             utils.cmd_runner(),
             agent_facade,
@@ -1101,12 +1129,12 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None:  # noqa: PLR
         resource, utils.convert_args_to_tuples(meta_values)
     )
 
-    operations = resource.getElementsByTagName("operations")
-    if not operations:
-        operations = dom.createElement("operations")
-        resource.appendChild(operations)
+    operations_el = resource.getElementsByTagName("operations")
+    if not operations_el:
+        operations_el = dom.createElement("operations")
+        resource.appendChild(operations_el)
     else:
-        operations = operations[0]
+        operations_el = operations_el[0]
 
     get_role = partial(
         pacemaker.role.get_value_for_cib,
@@ -1137,7 +1165,7 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None:  # noqa: PLR
 
         updating_op = None
         updating_op_before = None
-        for existing_op in operations.getElementsByTagName("op"):
+        for existing_op in operations_el.getElementsByTagName("op"):
             if updating_op:
                 updating_op_before = existing_op
                 break
@@ -1155,6 +1183,7 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None:  # noqa: PLR
             op_argv,
             validate_strict=False,
             before_op=updating_op_before,
+            allowed_operation_name_list=allowed_operation_name_list,
         )
 
     utils.replace_cib_configuration(dom)
@@ -1238,7 +1267,12 @@ def transform_master_to_clone(master_element):
 
 
 def resource_operation_add(  # noqa: PLR0912, PLR0915
-    dom, res_id, argv, validate_strict=True, before_op=None
+    dom,
+    res_id,
+    argv,
+    validate_strict=True,
+    before_op=None,
+    allowed_operation_name_list=None,
 ):
     """
     Commandline options:
@@ -1259,36 +1293,34 @@ def resource_operation_add(  # noqa: PLR0912, PLR0915
 
     if "=" in op_name:
         utils.err("%s does not appear to be a valid operation action" % op_name)
-    if "--force" not in utils.pcs_options:
-        valid_attrs = [
-            "id",
-            "name",
-            "interval",
-            "description",
-            "start-delay",
-            "interval-origin",
-            "timeout",
-            "enabled",
-            "record-pending",
-            "role",
-            "on-fail",
-            OCF_CHECK_LEVEL_INSTANCE_ATTRIBUTE_NAME,
-        ]
-        for key, value in op_properties:
-            if key not in valid_attrs:
-                utils.err(
-                    "%s is not a valid op option (use --force to override)"
-                    % key
-                )
-            if key == "role":
-                if value not in const.PCMK_ROLES:
-                    utils.err(
-                        "role must be: {} (use --force to override)".format(
-                            format_list_custom_last_separator(
-                                const.PCMK_ROLES, " or "
-                            )
-                        )
-                    )
+
+    op_dict = dict(op_properties)
+    if "name" in op_dict:
+        # deprecated since pcs-0.12.3
+        deprecation_warning(
+            "Specifying an operation name with 'name=<value>' syntax "
+            "is deprecated and might be removed in a future release. "
+            "Use the operation name as the first argument instead."
+        )
+    else:
+        op_dict["name"] = op_name
+
+    normalized = operations.operations_to_normalized([op_dict])
+    report_list = operations.validate_operation_list(
+        normalized,
+        allowed_operation_name_list,
+        allow_invalid="--force" in utils.pcs_options,
+    )
+    if report_list:
+        process_library_reports(report_list)
+
+    new_role_names_supported = utils.isCibVersionSatisfied(
+        dom, const.PCMK_NEW_ROLES_CIB_VERSION
+    )
+    op_normalized = operations.normalized_to_operations(
+        normalized, new_role_names_supported
+    )[0]
+    op_properties = sorted(op_normalized.items())
 
     interval = None
     for key, val in op_properties:
@@ -1299,9 +1331,6 @@ def resource_operation_add(  # noqa: PLR0912, PLR0915
         interval = "60s" if op_name == "monitor" else "0s"
         op_properties.append(("interval", interval))
 
-    op_properties.sort(key=lambda a: a[0])
-    op_properties.insert(0, ("name", op_name))
-
     generate_id = True
     for name, value in op_properties:
         if name == "id":
@@ -1335,26 +1364,16 @@ def resource_operation_add(  # noqa: PLR0912, PLR0915
                 "id", utils.find_unique_id(dom, "-".join((op_id, key, val)))
             )
             attrib_el.appendChild(nvpair_el)
-        elif key == "role" and "--force" not in utils.pcs_options:
-            op_el.setAttribute(
-                key,
-                pacemaker.role.get_value_for_cib(
-                    val,
-                    utils.isCibVersionSatisfied(
-                        dom, const.PCMK_NEW_ROLES_CIB_VERSION
-                    ),
-                ),
-            )
         else:
             op_el.setAttribute(key, val)
 
-    operations = res_el.getElementsByTagName("operations")
-    if not operations:
-        operations = dom.createElement("operations")
-        res_el.appendChild(operations)
+    operations_el = res_el.getElementsByTagName("operations")
+    if not operations_el:
+        operations_el = dom.createElement("operations")
+        res_el.appendChild(operations_el)
     else:
-        operations = operations[0]
-        duplicate_op_list = utils.operation_exists(operations, op_el)
+        operations_el = operations_el[0]
+        duplicate_op_list = utils.operation_exists(operations_el, op_el)
         if duplicate_op_list:
             utils.err(
                 "operation %s with interval %ss already specified for %s:\n%s"
@@ -1369,7 +1388,7 @@ def resource_operation_add(  # noqa: PLR0912, PLR0915
             )
         if validate_strict and "--force" not in utils.pcs_options:
             duplicate_op_list = utils.operation_exists_by_name(
-                operations, op_el
+                operations_el, op_el
             )
             if duplicate_op_list:
                 msg = (
@@ -1389,7 +1408,7 @@ def resource_operation_add(  # noqa: PLR0912, PLR0915
                     )
                 )
 
-    operations.insertBefore(op_el, before_op)
+    operations_el.insertBefore(op_el, before_op)
     return dom