From e41893c9d085b4883366db65b9a74104f1949e1d Mon Sep 17 00:00:00 2001
From: Steve Baker <sbaker@redhat.com>
Date: Wed, 18 Nov 2020 21:18:48 +1300
Subject: [PATCH] JSON conversion followup change

This change addresses nit-level review comments from this task.

Story: 1651346
Task: 10551
Change-Id: I01608004ce90facadb73e252203900a1e62cbea1
---
 ironic/api/controllers/v1/allocation.py       |  8 ++++---
 ironic/api/controllers/v1/collection.py       |  3 ++-
 ironic/api/controllers/v1/deploy_template.py  |  3 +--
 ironic/api/controllers/v1/driver.py           |  4 +---
 ironic/api/controllers/v1/event.py            |  4 ++--
 ironic/api/controllers/v1/node.py             |  7 ------
 ironic/api/controllers/v1/utils.py            | 22 ++++++-------------
 ironic/api/method.py                          |  3 +++
 ironic/common/args.py                         | 11 ++++++----
 ironic/common/exception.py                    | 13 -----------
 .../unit/api/controllers/v1/test_driver.py    |  2 +-
 .../unit/api/controllers/v1/test_node.py      | 12 ----------
 .../unit/api/controllers/v1/test_utils.py     |  8 -------
 ironic/tests/unit/common/test_args.py         |  2 +-
 14 files changed, 30 insertions(+), 72 deletions(-)

diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py
index 1734bedf61..fb876c3811 100644
--- a/ironic/api/controllers/v1/allocation.py
+++ b/ironic/api/controllers/v1/allocation.py
@@ -77,11 +77,13 @@ def convert_with_links(rpc_allocation, fields=None, sanitize=True):
                 'owner'),
         list_fields=('candidate_nodes', 'traits')
     )
-    api_utils.populate_node_uuid(rpc_allocation, allocation,
-                                 raise_notfound=False)
+    try:
+        api_utils.populate_node_uuid(rpc_allocation, allocation)
+    except exception.NodeNotFound:
+        allocation['node_uuid'] = None
 
     if fields is not None:
-        api_utils.check_for_invalid_fields(fields, allocation.keys())
+        api_utils.check_for_invalid_fields(fields, set(allocation))
 
     if sanitize:
         allocation_sanitize(allocation, fields)
diff --git a/ironic/api/controllers/v1/collection.py b/ironic/api/controllers/v1/collection.py
index 8368069e88..db5b70764d 100644
--- a/ironic/api/controllers/v1/collection.py
+++ b/ironic/api/controllers/v1/collection.py
@@ -37,7 +37,8 @@ def list_convert_with_links(items, item_name, limit, url=None, fields=None,
     :param fields:
         Optional fields to use for sanitize function
     :param sanitize_func:
-        Optional sanitize function run on each item
+        Optional sanitize function run on each item, item changes will be
+        done in-place
     :param key_field:
         Key name for building next URL
     :param kwargs:
diff --git a/ironic/api/controllers/v1/deploy_template.py b/ironic/api/controllers/v1/deploy_template.py
index 6a56eec4a9..9e774f9488 100644
--- a/ironic/api/controllers/v1/deploy_template.py
+++ b/ironic/api/controllers/v1/deploy_template.py
@@ -40,8 +40,7 @@ METRICS = metrics_utils.get_metrics_logger(__name__)
 
 DEFAULT_RETURN_FIELDS = ['uuid', 'name']
 
-INTERFACE_NAMES = list(
-    conductor_steps.DEPLOYING_INTERFACE_PRIORITY.keys())
+INTERFACE_NAMES = list(conductor_steps.DEPLOYING_INTERFACE_PRIORITY)
 
 STEP_SCHEMA = {
     'type': 'object',
diff --git a/ironic/api/controllers/v1/driver.py b/ironic/api/controllers/v1/driver.py
index 4868fc4cf1..d3d920cc49 100644
--- a/ironic/api/controllers/v1/driver.py
+++ b/ironic/api/controllers/v1/driver.py
@@ -221,7 +221,7 @@ class DriverPassthruController(rest.RestController):
     @method.expose()
     @method.body('data')
     @args.validate(driver_name=args.string, method=args.string)
-    def _default(self, driver_name, method=None, data=None):
+    def _default(self, driver_name, method, data=None):
         """Call a driver API extension.
 
         :param driver_name: name of the driver to call.
@@ -229,8 +229,6 @@ class DriverPassthruController(rest.RestController):
                        implementation.
         :param data: body of data to supply to the specified method.
         """
-        if not method:
-            raise exception.MissingArgument('method')
 
         cdict = api.request.context.to_policy_values()
         policy.authorize('baremetal:driver:vendor_passthru', cdict, cdict)
diff --git a/ironic/api/controllers/v1/event.py b/ironic/api/controllers/v1/event.py
index f11650d4e6..8e17d3bfaa 100644
--- a/ironic/api/controllers/v1/event.py
+++ b/ironic/api/controllers/v1/event.py
@@ -67,7 +67,7 @@ EVENTS_SCHEMA = {
                 'type': 'object',
                 'properties': {
                     'event': {'type': 'string',
-                              'enum': list(EVENT_VALIDATORS.keys())},
+                              'enum': list(EVENT_VALIDATORS)},
                 },
                 'required': ['event'],
                 'additionalProperties': True,
@@ -80,7 +80,7 @@ EVENTS_SCHEMA = {
 
 
 def events_valid(name, value):
-    '''Validator for events'''
+    """Validator for events"""
 
     for event in value['events']:
         validator = EVENT_VALIDATORS[event['event']]
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index a436d29be2..8340152a4d 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -1277,13 +1277,6 @@ def node_sanitize(node, fields):
         node['driver_info'] = strutils.mask_dict_password(
             node['driver_info'], "******")
 
-        # NOTE(derekh): mask ssh keys for the ssh power driver.
-        # As this driver is deprecated masking here (opposed to strutils)
-        # is simpler, and easier to backport. This can be removed along
-        # with support for the ssh power driver.
-        if node['driver_info'].get('ssh_key_contents'):
-            node['driver_info']['ssh_key_contents'] = "******"
-
     if not show_instance_secrets and node.get('instance_info'):
         node['instance_info'] = strutils.mask_dict_password(
             node['instance_info'], "******")
diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py
index 2ad7cd7b45..474987f9a3 100644
--- a/ironic/api/controllers/v1/utils.py
+++ b/ironic/api/controllers/v1/utils.py
@@ -226,7 +226,7 @@ def object_to_dict(obj, created_at=True, updated_at=True, uuid=True,
     return to_dict
 
 
-def populate_node_uuid(obj, to_dict, raise_notfound=True):
+def populate_node_uuid(obj, to_dict):
     """Look up the node referenced in the object and populate a dict.
 
     The node is fetched with the object ``node_id`` attribute and the
@@ -236,23 +236,15 @@ def populate_node_uuid(obj, to_dict, raise_notfound=True):
         object to get the node_id attribute
     :param to_dict:
         dict to populate with a ``node_uuid`` value
-    :param raise_notfound:
-        If ``True`` raise a NodeNotFound exception if the node doesn't exist
-        otherwise set the dict ``node_uuid`` value to None.
     :raises:
-        exception.NodeNotFound if raise_notfound and the node is not found
+        exception.NodeNotFound if the node is not found
     """
     if not obj.node_id:
         to_dict['node_uuid'] = None
         return
-    try:
-        to_dict['node_uuid'] = objects.Node.get_by_id(
-            api.request.context,
-            obj.node_id).uuid
-    except exception.NodeNotFound:
-        if raise_notfound:
-            raise
-        to_dict['node_uuid'] = None
+    to_dict['node_uuid'] = objects.Node.get_by_id(
+        api.request.context,
+        obj.node_id).uuid
 
 
 def replace_node_uuid_with_id(to_dict):
@@ -343,7 +335,7 @@ def patched_validate_with_schema(patched_dict, schema, validator=None):
     :raises: exception.Invalid if validation fails
     """
     schema_fields = schema['properties']
-    for field in set(patched_dict.keys()):
+    for field in set(patched_dict):
         if field not in schema_fields:
             patched_dict.pop(field, None)
     if not validator:
@@ -385,7 +377,7 @@ def sanitize_dict(to_sanitize, fields):
     if fields is None:
         return
 
-    for key in set(to_sanitize.keys()):
+    for key in set(to_sanitize):
         if key not in fields and key != 'links':
             to_sanitize.pop(key, None)
 
diff --git a/ironic/api/method.py b/ironic/api/method.py
index 6afdb1622b..71ce6204a8 100644
--- a/ironic/api/method.py
+++ b/ironic/api/method.py
@@ -61,6 +61,9 @@ def expose(status_code=None):
                     pecan.response.status = 500
 
             def _empty():
+                # This is for a pecan workaround originally in WSME,
+                # but the original issue description is in an issue tracker
+                # that is now offline
                 pecan.request.pecan['content_type'] = None
                 pecan.response.content_type = None
 
diff --git a/ironic/common/args.py b/ironic/common/args.py
index 014fbd3186..94cfe8841b 100755
--- a/ironic/common/args.py
+++ b/ironic/common/args.py
@@ -100,7 +100,7 @@ def uuid_or_name(name, value):
     if value is None:
         return
     if (not utils.is_valid_logical_name(value)
-        and not uuidutils.is_uuid_like(value)):
+            and not uuidutils.is_uuid_like(value)):
         raise exception.InvalidParameterValue(
             _('Expected UUID or name for %s: %s') % (name, value))
     return value
@@ -271,7 +271,9 @@ def types(*types):
     :param: types one or more types to use for the isinstance test
     :returns: validator function which takes name and value arguments
     """
-    return functools.partial(_validate_types, types=tuple(types))
+    # Replace None with the None type
+    types = tuple((type(None) if tp is None else tp) for tp in types)
+    return functools.partial(_validate_types, types=types)
 
 
 def _apply_validator(name, value, val_functions):
@@ -352,7 +354,7 @@ def validate(*args, **kwargs):
                 elif param.default == inspect.Parameter.empty:
                     # no argument was provided, and there is no default
                     # in the parameter, so this is a mandatory argument
-                    raise exception.InvalidParameterValue(
+                    raise exception.MissingParameterValue(
                         _('Missing mandatory parameter: %s') % param.name)
 
             if param_positional:
@@ -388,7 +390,8 @@ patch = schema({
             'op': {'type': 'string', 'enum': ['add', 'replace', 'remove']},
             'value': {}
         },
-        'additionalProperties': False
+        'additionalProperties': False,
+        'required': ['op', 'path']
     }
 })
 """Validate a patch API operation"""
diff --git a/ironic/common/exception.py b/ironic/common/exception.py
index c2e5030e87..ba636e154e 100644
--- a/ironic/common/exception.py
+++ b/ironic/common/exception.py
@@ -774,19 +774,6 @@ class UnknownArgument(ClientSideError):
         }
 
 
-class MissingArgument(ClientSideError):
-    def __init__(self, argname, msg=''):
-        self.argname = argname
-        super(MissingArgument, self).__init__(msg)
-
-    @property
-    def faultstring(self):
-        return _('Missing argument: "%(argname)s"%(msg)s') % {
-            'argname': self.argname,
-            'msg': self.msg and ": " + self.msg or ""
-        }
-
-
 class UnknownAttribute(ClientSideError):
     def __init__(self, fieldname, attributes, msg=''):
         self.fieldname = fieldname
diff --git a/ironic/tests/unit/api/controllers/v1/test_driver.py b/ironic/tests/unit/api/controllers/v1/test_driver.py
index aa4b4e2737..7e8a0f4711 100644
--- a/ironic/tests/unit/api/controllers/v1/test_driver.py
+++ b/ironic/tests/unit/api/controllers/v1/test_driver.py
@@ -363,7 +363,7 @@ class TestListDrivers(base.BaseApiTest):
 
         self.assertEqual(http_client.BAD_REQUEST, response.status_int)
         error = json.loads(response.json['error_message'])
-        self.assertEqual('Missing argument: "method"',
+        self.assertEqual('Missing mandatory parameter: method',
                          error['faultstring'])
 
     @mock.patch.object(rpcapi.ConductorAPI,
diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py
index 14cb0d3c80..4f863a3a10 100644
--- a/ironic/tests/unit/api/controllers/v1/test_node.py
+++ b/ironic/tests/unit/api/controllers/v1/test_node.py
@@ -2198,18 +2198,6 @@ class TestListNodes(test_api_base.BaseApiTest):
         mock_vdi.assert_called_once_with(mock.ANY, mock.ANY,
                                          node.uuid, 'test-topic')
 
-    def test_ssh_creds_masked(self):
-        driver_info = {"ssh_password": "password", "ssh_key_contents": "key"}
-        node = obj_utils.create_test_node(self.context,
-                                          chassis_id=self.chassis.id,
-                                          driver_info=driver_info)
-        data = self.get_json(
-            '/nodes/%s' % node.uuid,
-            headers={api_base.Version.string: str(api_v1.max_version())})
-
-        self.assertEqual("******", data["driver_info"]["ssh_password"])
-        self.assertEqual("******", data["driver_info"]["ssh_key_contents"])
-
     @mock.patch.object(rpcapi.ConductorAPI, 'get_indicator_state')
     def test_get_indicator_state(self, mock_gis):
         node = obj_utils.create_test_node(self.context)
diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py
index df51e594e8..0d9b489fe9 100644
--- a/ironic/tests/unit/api/controllers/v1/test_utils.py
+++ b/ironic/tests/unit/api/controllers/v1/test_utils.py
@@ -858,14 +858,6 @@ class TestNodeIdent(base.TestCase):
             'node_uuid': '1be26c0b-03f2-4d2e-ae87-c02d7f33c123'
         }, d)
 
-        # not found, don't raise
-        mock_gbi.side_effect = exception.NodeNotFound(node=port.node_id)
-        d = {}
-        utils.populate_node_uuid(port, d, raise_notfound=False)
-        self.assertEqual({
-            'node_uuid': None
-        }, d)
-
         # not found, raise exception
         mock_gbi.side_effect = exception.NodeNotFound(node=port.node_id)
         d = {}
diff --git a/ironic/tests/unit/common/test_args.py b/ironic/tests/unit/common/test_args.py
index c4b4d2e889..5695bb8fc1 100644
--- a/ironic/tests/unit/common/test_args.py
+++ b/ironic/tests/unit/common/test_args.py
@@ -597,7 +597,7 @@ class ValidateTypesTest(BaseTest):
 
     def test_types(self):
 
-        @args.validate(foo=args.types(type(None), dict, str))
+        @args.validate(foo=args.types(None, dict, str))
         def doit(foo):
             return foo