Merge "API: lookup() ignore malformed MAC addresses"

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.