From 65b30a04a234449cb4aa65606d47bf1d673592a4 Mon Sep 17 00:00:00 2001 From: Tomas Jelinek 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): """ + _fixture_agent_not_valid_xml = """ + + + + + + """ _fixture_fenced_xml = """ @@ -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("") + # pylint: disable=no-self-use + metadata = """ + + """ + 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( - """ - - 1.0 - - """ - ) + # pylint: disable=no-self-use + metadata = """ + + 1.0 + + """ + 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 = "" self.config.runner.pcmk.load_agent( agent_name="ocf:pacemaker:Dummy", - stdout="", + 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="") + metadata = "" + 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( - """ - - - - - - """ - ) - ) + self.assert_parse_result( + self.xml( + """ + + + + + + """ + ), + 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( - """ - - - - - - """ - ) - ) + self.assert_parse_result( + self.xml( + """ + + + + + + """ + ), + 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