API: lookup() ignore malformed MAC addresses

This patch is adding code to ignore malformed MAC addresses which are
called as part of the lookup() mechanism. Prior to this patch, if a MAC
address wasn't in the expected format (six octets) the deployment of the
node would fail because the ramdisk wouldn't be able to lookup which
node it was in the Ironic database. One way to trigger this problem was
to deploy a node with an Infiniband Card which the MAC address (or GID)
contains 20 octets, that would result in a deployment failure even when
that NIC wasn't used by Ironic at all (not enrolled as a port).

The ListOfMacAdresses type was also deleted as part of this patch
because it wasn't used anywhere else.

Change-Id: I614fe63236985438d2f354d17a15d17649e72912
Closes-Bug: #1633585

ironic/api/controllers/v1/ramdisk.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 from oslo_config import cfg
+from oslo_log import log
 import pecan
 from pecan import rest
 from six.moves import http_client
@@ -24,12 +25,15 @@ from ironic.api.controllers.v1 import types
 from ironic.api.controllers.v1 import utils as api_utils
 from ironic.api import expose
 from ironic.common import exception
+from ironic.common.i18n import _LW
 from ironic.common import policy
 from ironic.common import states
+from ironic.common import utils
 from ironic import objects
 
 
 CONF = cfg.CONF
+LOG = log.getLogger(__name__)
 
 _LOOKUP_RETURN_FIELDS = ('uuid', 'properties', 'instance_info',
                          'driver_internal_info')
@@ -78,7 +82,7 @@ class LookupResult(base.APIBase):
 class LookupController(rest.RestController):
     """Controller handling node lookup for a deploy ramdisk."""
 
-    @expose.expose(LookupResult, types.list_of_macaddress, types.uuid)
+    @expose.expose(LookupResult, types.listtype, types.uuid)
     def get_all(self, addresses=None, node_uuid=None):
         """Look up a node by its MAC addresses and optionally UUID.
 
@@ -97,7 +101,29 @@ class LookupController(rest.RestController):
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:driver:ipa_lookup', cdict, cdict)
 
-        if not addresses and not node_uuid:
+        # Validate the list of MAC addresses
+        if addresses is None:
+            addresses = []
+
+        valid_addresses = []
+        invalid_addresses = []
+        for addr in addresses:
+            try:
+                mac = utils.validate_and_normalize_mac(addr)
+                valid_addresses.append(mac)
+            except exception.InvalidMAC:
+                invalid_addresses.append(addr)
+
+        if invalid_addresses:
+            node_log = ('' if not node_uuid
+                        else _LW('(Node UUID: %s)') % node_uuid)
+            LOG.warning(_LW('The following MAC addresses "%(addrs)s" are '
+                            'invalid and will be ignored by the lookup '
+                            'request %(node)s'),
+                        {'addrs': ', '.join(invalid_addresses),
+                         'node': node_log})
+
+        if not valid_addresses and not node_uuid:
             raise exception.IncompleteLookup()
 
         try:
@@ -106,7 +132,7 @@ class LookupController(rest.RestController):
                     pecan.request.context, node_uuid)
             else:
                 node = objects.Node.get_by_port_addresses(
-                    pecan.request.context, addresses)
+                    pecan.request.context, valid_addresses)
         except exception.NotFound:
             # NOTE(dtantsur): we are reraising the same exception to make sure
             # we don't disclose the difference between nodes that are not found

ironic/api/controllers/v1/types.py

@@ -176,26 +176,6 @@ class ListType(wtypes.UserType):
         return ListType.validate(value)
 
 
-class ListOfMacAddressesType(ListType):
-    """List of MAC addresses."""
-
-    @staticmethod
-    def validate(value):
-        """Validate and convert the input to a ListOfMacAddressesType.
-
-        :param value: A comma separated string of MAC addresses.
-        :returns: A list of unique MACs, whose order is not guaranteed.
-        """
-        items = ListType.validate(value)
-        return [MacAddressType.validate(item) for item in items]
-
-    @staticmethod
-    def frombasetype(value):
-        if value is None:
-            return None
-        return ListOfMacAddressesType.validate(value)
-
-
 macaddress = MacAddressType()
 uuid_or_name = UuidOrNameType()
 name = NameType()
@@ -204,7 +184,6 @@ boolean = BooleanType()
 listtype = ListType()
 # Can't call it 'json' because that's the name of the stdlib module
 jsontype = JsonType()
-list_of_macaddress = ListOfMacAddressesType()
 
 
 class JsonPatchType(wtypes.Base):

ironic/tests/unit/api/v1/test_ramdisk.py

@@ -100,6 +100,23 @@ class TestLookup(test_api_base.BaseApiTest):
                          set(data['node']))
         self._check_config(data)
 
+    @mock.patch.object(ramdisk.LOG, 'warning', autospec=True)
+    def test_ignore_malformed_address(self, mock_log):
+        obj_utils.create_test_port(self.context,
+                                   node_id=self.node.id,
+                                   address=self.addresses[1])
+
+        addresses = ('not-a-valid-address,80:00:02:48:fe:80:00:00:00:00:00:00'
+                     ':f4:52:14:03:00:54:06:c2,' + ','.join(self.addresses))
+        data = self.get_json(
+            '/lookup?addresses=%s' % addresses,
+            headers={api_base.Version.string: str(api_v1.MAX_VER)})
+        self.assertEqual(self.node.uuid, data['node']['uuid'])
+        self.assertEqual(set(ramdisk._LOOKUP_RETURN_FIELDS) | {'links'},
+                         set(data['node']))
+        self._check_config(data)
+        self.assertTrue(mock_log.called)
+
     def test_found_by_uuid(self):
         data = self.get_json(
             '/lookup?addresses=%s&node_uuid=%s' %

ironic/tests/unit/api/v1/test_types.py

@@ -41,27 +41,6 @@ class TestMacAddressType(base.TestCase):
                           types.MacAddressType.validate, 'invalid-mac')
 
 
-class TestListOfMacAddressesType(base.TestCase):
-
-    def test_valid_mac_addr(self):
-        test_mac = 'aa:bb:cc:11:22:33'
-        self.assertEqual([test_mac],
-                         types.ListOfMacAddressesType.validate(test_mac))
-
-    def test_valid_list(self):
-        test_mac = 'aa:bb:cc:11:22:33,11:22:33:44:55:66'
-        self.assertEqual(
-            sorted(test_mac.split(',')),
-            sorted(types.ListOfMacAddressesType.validate(test_mac)))
-
-    def test_invalid_mac_addr(self):
-        self.assertRaises(exception.InvalidMAC,
-                          types.ListOfMacAddressesType.validate, 'invalid-mac')
-        self.assertRaises(exception.InvalidMAC,
-                          types.ListOfMacAddressesType.validate,
-                          'aa:bb:cc:11:22:33,invalid-mac')
-
-
 class TestUuidType(base.TestCase):
 
     def test_valid_uuid(self):

releasenotes/notes/lookup-ignore-malformed-macs-09e7e909f3a134a3.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - Fixes a problem where the deployment of a node would fail to continue
+    if a malformed MAC address was passed to the lookup mechanism in the
+    Ironic API. For example, if a node contains an Infiniband card, the
+    lookup used to fail because the agent ramdisk passes a MAC address
+    (or GID) with 20 octets (instead of the expected 6 octets) as part
+    of the lookup request. Invalid addresses are now ignored.