medium: parse: Detect and error on illegal ordering of op attributes …

…(bsc#1129210)

Unfortunately there is no way to allow mixing of instance_attributes
with op node attributes, as reconstructing the line-based syntax
from the XML is impossible.

This fix detects when the ordering is illegal and errors out.

Also remove accidentally duplicated code detected by code climate (!)

crmsh/parse.py

@@ -692,11 +692,19 @@ def match_op(self, out, pfx='op'):
         inst_attrs = None
         for nvp in all_attrs:
             if nvp.get('name') in valid_attrs:
+                if inst_attrs is not None:
+                    self.err("Attribute order error: {} must appear before any instance attribute".format(nvp.get('name')))
                 node.set(nvp.get('name'), nvp.get('value'))
             else:
                 if inst_attrs is None:
                     inst_attrs = xmlutil.child(node, 'instance_attributes')
                 inst_attrs.append(nvp)
+        if inst_attrs is not None:
+            node.append(inst_attrs)
+        for attr_list in self.match_attr_lists({'op_params': 'instance_attributes',
+                                                'op_meta': 'meta_attributes'},
+                                               implicit_initial='op_params'):
+            node.append(attr_list)
         out.append(node)
 
     def match_operations(self, out, match_id):
@@ -784,64 +792,6 @@ def match_ra_type(self, out):
         xmlutil.maybe_set(out, 'provider', cpt[1])
         xmlutil.maybe_set(out, 'type', cpt[2])
 
-    def match_op(self, out, pfx='op'):
-        """
-        op <optype> [<n>=<v> ...]
-
-        to:
-          <op name="monitor" timeout="30" interval="10" id="p_mysql-monitor-10">
-            <instance_attributes id="p_mysql-monitor-10-instance_attributes">
-              <nvpair name="depth" value="0" id="p_mysql-monitor-10-instance_attributes-depth"/>
-            </instance_attributes>
-          </op>
-        """
-        self.match('op')
-        op_type = self.match_identifier()
-        all_attrs = self.match_nvpairs(minpairs=0)
-        node = xmlutil.new('op', name=op_type)
-        if not any(nvp.get('name') == 'interval' for nvp in all_attrs):
-            all_attrs.append(xmlutil.nvpair('interval', '0'))
-        valid_attrs = validator.op_attributes()
-        inst_attrs = None
-        nvpairs_missed = 0
-        for nvp in all_attrs:
-            if nvp.get('name') in valid_attrs:
-                node.set(nvp.get('name'), nvp.get('value'))
-            else:
-                nvpairs_missed += 1
-        for i in range(nvpairs_missed):
-            self.rewind()
-        for attr_list in self.match_attr_lists({'op_params': 'instance_attributes',
-                                                'op_meta': 'meta_attributes'},
-                                               implicit_initial='op_params'):
-            node.append(attr_list)
-        out.append(node)
-
-    def match_operations(self, out, match_id):
-        from .cibconfig import cib_factory
-
-        def is_op():
-            return self.has_tokens() and self.current_token().lower() == 'op'
-        if match_id:
-            self.match('operations')
-        node = xmlutil.child(out, 'operations')
-        if match_id:
-            self.match_idspec()
-            match_id = self.matched(1)[1:].lower()
-            idval = self.matched(2)
-            if match_id == 'id-ref':
-                idval = cib_factory.resolve_id_ref('operations', idval)
-
-            node.set(match_id, idval)
-
-        # The ID assignment skips the operations node if possible,
-        # so we need to pass the prefix (id of the owner node)
-        # to match_op
-        pfx = out.get('id') or 'op'
-
-        while is_op():
-            self.match_op(node, pfx=pfx)
-
     def parse(self, cmd):
         return self.begin_dispatch(cmd, min_args=2)
 

test/unittests/test_parse.py

@@ -368,6 +368,10 @@ def test_op(self):
         self.assertEqual(out.get('role'), None)
         self.assertEqual(out.get('interval'), '60m')
 
+        out = self._parse('primitive rsc_dummy1 Dummy op monitor interval=10 OCF_CHECK_LEVEL=10 timeout=60')
+        # incorrect ordering of attributes
+        self.assertFalse(out)
+
     def test_acl(self):
         out = self._parse('role user-1 error')
         self.assertFalse(out)