From 65b30a04a234449cb4aa65606d47bf1d673592a4 Mon Sep 17 00:00:00 2001

From: Tomas Jelinek <tojeline@redhat.com>

Date: Wed, 9 Feb 2022 11:16:49 +0100

Subject: [PATCH 2/3] relax OCF 1.0 parser

---

 pcs/lib/resource_agent/facade.py              |  50 ++++--

 pcs/lib/resource_agent/ocf_transform.py       |  51 +++++-

 pcs/lib/resource_agent/xml.py                 |   8 +-

 .../tier0/lib/resource_agent/test_facade.py   |  44 +++++

 .../lib/resource_agent/test_ocf_transform.py  |  48 +++++-

 pcs_test/tier0/lib/resource_agent/test_xml.py | 155 ++++++++++--------

 6 files changed, 256 insertions(+), 100 deletions(-)

diff --git a/pcs/lib/resource_agent/facade.py b/pcs/lib/resource_agent/facade.py

index dea59a1a..8a65eb1c 100644

--- a/pcs/lib/resource_agent/facade.py

+++ b/pcs/lib/resource_agent/facade.py

@@ -2,12 +2,19 @@ from collections import defaultdict

 from dataclasses import replace as dc_replace

 from typing import Dict, Iterable, List, Optional, Set

+from lxml import etree

+

+from pcs import settings

 from pcs.common import reports

 from pcs.lib import validate

 from pcs.lib.external import CommandRunner

 from . import const

-from .error import ResourceAgentError, resource_agent_error_to_report_item

+from .error import (

+    ResourceAgentError,

+    resource_agent_error_to_report_item,

+    UnableToGetAgentMetadata,

+)

 from .name import name_to_void_metadata

 from .ocf_transform import ocf_version_to_ocf_unified

 from .pcs_transform import get_additional_trace_parameters, ocf_unified_to_pcs

@@ -195,24 +202,33 @@ class ResourceAgentFacadeFactory:

         name -- agent name to get a facade for

         """

-        metadata, raw_ocf_version = parse_metadata(

-            name,

-            load_metadata(self._runner, name),

-        )

-        if (

-            report_warnings

-            and raw_ocf_version not in const.SUPPORTED_OCF_VERSIONS

-        ):

-            self._report_processor.report(

-                reports.ReportItem.warning(

-                    reports.messages.AgentImplementsUnsupportedOcfVersionAssumedVersion(

-                        name.full_name,

-                        raw_ocf_version,

-                        sorted(const.SUPPORTED_OCF_VERSIONS),

-                        const.OCF_1_0,

+        dom_metadata = load_metadata(self._runner, name)

+        metadata, raw_ocf_version = parse_metadata(name, dom_metadata)

+        if report_warnings:

+            if raw_ocf_version not in const.SUPPORTED_OCF_VERSIONS:

+                self._report_processor.report(

+                    reports.ReportItem.warning(

+                        reports.messages.AgentImplementsUnsupportedOcfVersionAssumedVersion(

+                            name.full_name,

+                            raw_ocf_version,

+                            sorted(const.SUPPORTED_OCF_VERSIONS),

+                            const.OCF_1_0,

+                        )

                     )

                 )

-            )

+            if raw_ocf_version != const.OCF_1_1:

+                try:

+                    etree.RelaxNG(

+                        file=settings.path.ocf_1_0_schema

+                    ).assertValid(dom_metadata)

+                except etree.DocumentInvalid as e:

+                    self._report_processor.report(

+                        resource_agent_error_to_report_item(

+                            UnableToGetAgentMetadata(name.full_name, str(e)),

+                            severity=reports.ReportItemSeverity.warning(),

+                            is_stonith=name.is_stonith,

+                        )

+                    )

         return self._facade_from_metadata(ocf_version_to_ocf_unified(metadata))

     def void_facade_from_parsed_name(

diff --git a/pcs/lib/resource_agent/ocf_transform.py b/pcs/lib/resource_agent/ocf_transform.py

index e841b55e..7e6a14ad 100644

--- a/pcs/lib/resource_agent/ocf_transform.py

+++ b/pcs/lib/resource_agent/ocf_transform.py

@@ -67,20 +67,42 @@ def _ocf_1_1_to_ocf_unified(

         longdesc=metadata.longdesc,

         parameters=_ocf_1_1_parameter_list_to_ocf_unified(metadata.parameters),

         # OCF 1.1 actions are the same as in OCF 1.0

-        actions=_ocf_1_0_action_list_to_ocf_unified(metadata.actions),

+        actions=_ocf_1_1_action_list_to_ocf_unified(metadata.actions),

     )

 def _ocf_1_0_action_list_to_ocf_unified(

-    action_list: Iterable[

-        Union[ResourceAgentActionOcf1_0, ResourceAgentActionOcf1_1]

-    ],

+    action_list: Iterable[ResourceAgentActionOcf1_0],

 ) -> List[ResourceAgentAction]:

     """

     Transform OCF 1.0 actions to a universal format

     action_list -- actions according OCF 1.0

     """

+    return [

+        ResourceAgentAction(

+            name=action.name,

+            timeout=action.timeout,

+            interval=action.interval,

+            role=action.role,

+            start_delay=action.start_delay,

+            depth=action.depth,

+            automatic=_bool_value_legacy(action.automatic),

+            on_target=_bool_value_legacy(action.on_target),

+        )

+        for action in action_list

+        if action.name

+    ]

+

+

+def _ocf_1_1_action_list_to_ocf_unified(

+    action_list: Iterable[ResourceAgentActionOcf1_1],

+) -> List[ResourceAgentAction]:

+    """

+    Transform OCF 1.1 actions to a universal format

+

+    action_list -- actions according OCF 1.1

+    """

     return [

         ResourceAgentAction(

             name=action.name,

@@ -111,6 +133,8 @@ def _ocf_1_0_parameter_list_to_ocf_unified(

     result = []

     for parameter in parameter_list:

+        if not parameter.name:

+            continue

         result.append(

             ResourceAgentParameter(

                 name=parameter.name,

@@ -119,17 +143,17 @@ def _ocf_1_0_parameter_list_to_ocf_unified(

                 type=parameter.type,

                 default=parameter.default,

                 enum_values=parameter.enum_values,

-                required=_bool_value(parameter.required),

+                required=_bool_value_legacy(parameter.required),

                 advanced=False,

-                deprecated=_bool_value(parameter.deprecated),

+                deprecated=_bool_value_legacy(parameter.deprecated),

                 deprecated_by=sorted(deprecated_by_dict[parameter.name]),

                 deprecated_desc=None,

                 unique_group=(

                     f"{const.DEFAULT_UNIQUE_GROUP_PREFIX}{parameter.name}"

-                    if _bool_value(parameter.unique)

+                    if _bool_value_legacy(parameter.unique)

                     else None

                 ),

-                reloadable=_bool_value(parameter.unique),

+                reloadable=_bool_value_legacy(parameter.unique),

             )

         )

     return result

@@ -170,3 +194,14 @@ def _bool_value(value: Optional[str]) -> bool:

     value -- raw bool value

     """

     return value == "1"

+

+

+def _bool_value_legacy(value: Optional[str]) -> bool:

+    """

+    Transform raw bool value from metadata to bool type in backward compatible way

+

+    value -- raw bool value

+    """

+    return (

+        False if not value else value.lower() in {"true", "on", "yes", "y", "1"}

+    )

diff --git a/pcs/lib/resource_agent/xml.py b/pcs/lib/resource_agent/xml.py

index 1ba97216..0fc70527 100644

--- a/pcs/lib/resource_agent/xml.py

+++ b/pcs/lib/resource_agent/xml.py

@@ -94,9 +94,7 @@ def _metadata_xml_to_dom(metadata: str) -> _Element:

     """

     dom = xml_fromstring(metadata)

     ocf_version = _get_ocf_version(dom)

-    if ocf_version == const.OCF_1_0:

-        etree.RelaxNG(file=settings.path.ocf_1_0_schema).assertValid(dom)

-    elif ocf_version == const.OCF_1_1:

+    if ocf_version == const.OCF_1_1:

         etree.RelaxNG(file=settings.path.ocf_1_1_schema).assertValid(dom)

     return dom

@@ -230,7 +228,7 @@ def _parse_parameters_1_0(

         )

         result.append(

             ResourceAgentParameterOcf1_0(

-                name=str(parameter_el.attrib["name"]),

+                name=str(parameter_el.get("name", "")),

                 shortdesc=_get_shortdesc(parameter_el),

                 longdesc=_get_longdesc(parameter_el),

                 type=value_type,

@@ -286,7 +284,7 @@ def _parse_parameters_1_1(

 def _parse_actions_1_0(element: _Element) -> List[ResourceAgentActionOcf1_0]:

     return [

         ResourceAgentActionOcf1_0(

-            name=str(action.attrib["name"]),

+            name=str(action.get("name", "")),

             timeout=action.get("timeout"),

             interval=action.get("interval"),

             role=action.get("role"),

diff --git a/pcs_test/tier0/lib/resource_agent/test_facade.py b/pcs_test/tier0/lib/resource_agent/test_facade.py

index f6a9899c..313dfa2b 100644

--- a/pcs_test/tier0/lib/resource_agent/test_facade.py

+++ b/pcs_test/tier0/lib/resource_agent/test_facade.py

@@ -100,6 +100,13 @@ class ResourceAgentFacadeFactory(TestCase):

             </parameters>

         </resource-agent>

     """

+    _fixture_agent_not_valid_xml = """

+        <resource-agent name="agent">

+            <parameters>

+                <parameter label="something wrong"/>

+            </parameters>

+        </resource-agent>

+    """

     _fixture_fenced_xml = """

         <resource-agent name="pacemaker-fenced">

             <parameters>

@@ -172,6 +179,43 @@ class ResourceAgentFacadeFactory(TestCase):

         self.assertEqual(facade.metadata.name, name)

         self.assertTrue(facade.metadata.agent_exists)

+    def test_facade_ocf_1_0_not_valid(self):

+        name = ra.ResourceAgentName("service", None, "daemon")

+        self.config.runner.pcmk.load_agent(

+            agent_name="service:daemon",

+            stdout=self._fixture_agent_not_valid_xml,

+        )

+

+        env = self.env_assist.get_env()

+        facade = ra.ResourceAgentFacadeFactory(

+            env.cmd_runner(), env.report_processor

+        ).facade_from_parsed_name(name)

+        self.assertEqual(facade.metadata.name, name)

+        self.assertTrue(facade.metadata.agent_exists)

+        self.env_assist.assert_reports(

+            [

+                fixture.warn(

+                    reports.codes.UNABLE_TO_GET_AGENT_METADATA,

+                    agent=name.full_name,

+                    reason="Element parameter failed to validate attributes, line 3",

+                )

+            ]

+        )

+

+    def test_facade_ocf_1_0_not_valid_disabled_warning(self):

+        name = ra.ResourceAgentName("service", None, "daemon")

+        self.config.runner.pcmk.load_agent(

+            agent_name="service:daemon",

+            stdout=self._fixture_agent_not_valid_xml,

+        )

+

+        env = self.env_assist.get_env()

+        facade = ra.ResourceAgentFacadeFactory(

+            env.cmd_runner(), env.report_processor

+        ).facade_from_parsed_name(name, report_warnings=False)

+        self.assertEqual(facade.metadata.name, name)

+        self.assertTrue(facade.metadata.agent_exists)

+

     def test_facade_missing_agent(self):

         name = ra.ResourceAgentName("service", None, "daemon")

         self.config.runner.pcmk.load_agent(

diff --git a/pcs_test/tier0/lib/resource_agent/test_ocf_transform.py b/pcs_test/tier0/lib/resource_agent/test_ocf_transform.py

index 9e41b6af..d0de86e5 100644

--- a/pcs_test/tier0/lib/resource_agent/test_ocf_transform.py

+++ b/pcs_test/tier0/lib/resource_agent/test_ocf_transform.py

@@ -66,6 +66,18 @@ class OcfVersionToOcfUnified(TestCase):

                     obsoletes=None,

                     unique=None,

                 ),

+                ra.types.ResourceAgentParameterOcf1_0(

+                    name="",

+                    shortdesc="Parameters with no name are ignored",

+                    longdesc=None,

+                    type="string",

+                    default=None,

+                    enum_values=None,

+                    required=None,

+                    deprecated=None,

+                    obsoletes=None,

+                    unique=None,

+                ),

                 ra.types.ResourceAgentParameterOcf1_0(

                     name="param_2",

                     shortdesc="param_2 shortdesc",

@@ -109,10 +121,10 @@ class OcfVersionToOcfUnified(TestCase):

                     type="string",

                     default=None,

                     enum_values=None,

-                    required="1",

-                    deprecated="1",

+                    required="yeS",

+                    deprecated="True",

                     obsoletes="param_4",

-                    unique="1",

+                    unique="on",

                 ),

                 ra.types.ResourceAgentParameterOcf1_0(

                     name="param_6",

@@ -138,6 +150,16 @@ class OcfVersionToOcfUnified(TestCase):

                     automatic=None,

                     on_target=None,

                 ),

+                ra.types.ResourceAgentActionOcf1_0(

+                    name="",

+                    timeout=None,

+                    interval=None,

+                    role=None,

+                    start_delay=None,

+                    depth=None,

+                    automatic=None,

+                    on_target=None,

+                ),

                 ra.types.ResourceAgentActionOcf1_0(

                     name="action_2",

                     timeout="12",

@@ -158,6 +180,16 @@ class OcfVersionToOcfUnified(TestCase):

                     automatic="1",

                     on_target="0",

                 ),

+                ra.types.ResourceAgentActionOcf1_0(

+                    name="action_4",

+                    timeout=None,

+                    interval=None,

+                    role=None,

+                    start_delay=None,

+                    depth=None,

+                    automatic="yes",

+                    on_target="True",

+                ),

             ],

         )

         metadata_out = ra.ResourceAgentMetadata(

@@ -289,6 +321,16 @@ class OcfVersionToOcfUnified(TestCase):

                     automatic=True,

                     on_target=False,

                 ),

+                ra.ResourceAgentAction(

+                    name="action_4",

+                    timeout=None,

+                    interval=None,

+                    role=None,

+                    start_delay=None,

+                    depth=None,

+                    automatic=True,

+                    on_target=True,

+                ),

             ],

         )

         self.assertEqual(

diff --git a/pcs_test/tier0/lib/resource_agent/test_xml.py b/pcs_test/tier0/lib/resource_agent/test_xml.py

index 26bbbb7d..ea055ee2 100644

--- a/pcs_test/tier0/lib/resource_agent/test_xml.py

+++ b/pcs_test/tier0/lib/resource_agent/test_xml.py

@@ -164,8 +164,13 @@ class MetadataXmlToDom(TestCase):

             ra.xml._metadata_xml_to_dom("not an xml")

     def test_no_version_not_valid(self):

-        with self.assertRaises(etree.DocumentInvalid):

-            ra.xml._metadata_xml_to_dom("<resource-agent/>")

+        # pylint: disable=no-self-use

+        metadata = """

+            <resource-agent/>

+        """

+        assert_xml_equal(

+            metadata, etree_to_str(ra.xml._metadata_xml_to_dom(metadata))

+        )

     def test_no_version_valid(self):

         # pylint: disable=no-self-use

@@ -178,14 +183,15 @@ class MetadataXmlToDom(TestCase):

         )

     def test_ocf_1_0_not_valid(self):

-        with self.assertRaises(etree.DocumentInvalid):

-            ra.xml._metadata_xml_to_dom(

-                """

-                    <resource-agent>

-                        <version>1.0</version>

-                    </resource-agent>

-                """

-            )

+        # pylint: disable=no-self-use

+        metadata = """

+            <resource-agent>

+                <version>1.0</version>

+            </resource-agent>

+        """

+        assert_xml_equal(

+            metadata, etree_to_str(ra.xml._metadata_xml_to_dom(metadata))

+        )

     def test_ocf_1_0_valid(self):

         # pylint: disable=no-self-use

@@ -273,19 +279,16 @@ class LoadMetadata(TestCase):

     def test_not_valid_xml(self):

         agent_name = ra.ResourceAgentName("ocf", "pacemaker", "Dummy")

+        metadata = "<resource-agent/>"

         self.config.runner.pcmk.load_agent(

             agent_name="ocf:pacemaker:Dummy",

-            stdout="<resource-agent/>",

+            stdout=metadata,

         )

         env = self.env_assist.get_env()

-        with self.assertRaises(ra.UnableToGetAgentMetadata) as cm:

-            ra.xml.load_metadata(env.cmd_runner(), agent_name)

-        self.assertEqual(cm.exception.agent_name, "ocf:pacemaker:Dummy")

-        self.assertTrue(

-            cm.exception.message.startswith(

-                "Element resource-agent failed to validate"

-            )

+        assert_xml_equal(

+            metadata,

+            etree_to_str(ra.xml.load_metadata(env.cmd_runner(), agent_name)),

         )

@@ -335,16 +338,15 @@ class LoadFakeAgentMetadata(TestCase):

     def test_not_valid_xml(self):

         agent_name = ra.const.PACEMAKER_FENCED

-        self.config.runner.pcmk.load_fenced_metadata(stdout="<resource-agent/>")

+        metadata = "<resource-agent/>"

+        self.config.runner.pcmk.load_fenced_metadata(stdout=metadata)

         env = self.env_assist.get_env()

-        with self.assertRaises(ra.UnableToGetAgentMetadata) as cm:

-            ra.xml.load_fake_agent_metadata(env.cmd_runner(), agent_name)

-        self.assertEqual(cm.exception.agent_name, "pacemaker-fenced")

-        self.assertTrue(

-            cm.exception.message.startswith(

-                "Element resource-agent failed to validate"

-            )

+        assert_xml_equal(

+            metadata,

+            etree_to_str(

+                ra.xml.load_fake_agent_metadata(env.cmd_runner(), agent_name)

+            ),

         )

@@ -549,19 +551,37 @@ class ParseOcf10BaseMixin(ParseOcfToolsMixin):

         )

     def test_parameters_empty_parameter(self):

-        # parameters must have at least 'name' attribute

-        with self.assertRaises(ra.UnableToGetAgentMetadata):

-            self.parse(

-                self.xml(

-                    """

-                        <resource-agent>

-                            <parameters>

-                                <parameter/>

-                            </parameters>

-                        </resource-agent>

-                    """

-                )

-            )

+        self.assert_parse_result(

+            self.xml(

+                """

+                    <resource-agent>

+                        <parameters>

+                            <parameter/>

+                        </parameters>

+                    </resource-agent>

+                """

+            ),

+            ResourceAgentMetadataOcf1_0(

+                self.agent_name,

+                shortdesc=None,

+                longdesc=None,

+                parameters=[

+                    ResourceAgentParameterOcf1_0(

+                        name="",

+                        shortdesc=None,

+                        longdesc=None,

+                        type="string",

+                        default=None,

+                        enum_values=None,

+                        required=None,

+                        deprecated=None,

+                        obsoletes=None,

+                        unique=None,

+                    )

+                ],

+                actions=[],

+            ),

+        )

     def test_parameters_minimal(self):

         self.assert_parse_result(

@@ -708,19 +728,35 @@ class ParseOcf10BaseMixin(ParseOcfToolsMixin):

         )

     def test_actions_empty_action(self):

-        # actions must have at least 'name' attribute

-        with self.assertRaises(ra.UnableToGetAgentMetadata):

-            self.parse(

-                self.xml(

-                    """

-                        <resource-agent>

-                            <actions>

-                                <action/>

-                            </actions>

-                        </resource-agent>

-                    """

-                )

-            )

+        self.assert_parse_result(

+            self.xml(

+                """

+                    <resource-agent>

+                        <actions>

+                            <action/>

+                        </actions>

+                    </resource-agent>

+                """

+            ),

+            ResourceAgentMetadataOcf1_0(

+                self.agent_name,

+                shortdesc=None,

+                longdesc=None,

+                parameters=[],

+                actions=[

+                    ResourceAgentActionOcf1_0(

+                        name="",

+                        timeout=None,

+                        interval=None,

+                        role=None,

+                        start_delay=None,

+                        depth=None,

+                        automatic=None,

+                        on_target=None,

+                    ),

+                ],

+            ),

+        )

     def test_actions_multiple(self):

         self.assert_parse_result(

@@ -787,21 +823,6 @@ class ParseOcf10NoVersion(ParseOcf10BaseMixin, TestCase):

 class ParseOcf10UnsupportedVersion(ParseOcf10BaseMixin, TestCase):

     ocf_version = "0.1.2"

-    # These tests test that pcs raises an error if an agent doesn't conform to

-    # OCF schema. There is, however, no validation against OCF schema for

-    # agents with unsupported OCF version. That means no error message, pcs

-    # tries to process the agent and crashes. However bad that sounds, it's

-    # indended as that's how pcs behaved before OCF 1.1 was implemented.

-    # There's therefore no point in running these tests.

-

-    def test_parameters_empty_parameter(self):

-        # parameters must have at least 'name' attribute

-        pass

-

-    def test_actions_empty_action(self):

-        # actions must have at least 'name' attribute

-        pass

-

 class ParseOcf10ExplicitVersion(ParseOcf10BaseMixin, TestCase):

     ocf_version = "1.0"

-- 

2.34.1