Secure RBAC - Efficent node santiziation

An investigation of performance issues in Ironic revealed that the
policy checking was performing extra un-needed work which performed
excess computational overhead when parsing the result data.

In this specific case, the Secure RBAC work added some additional
policy checks around individual the fields.

Change-Id: I77b6e0e6c721f2ff1f8b9f511acde97fcdb21a39
Story: 2008885
Task: 42432

ironic/api/controllers/v1/node.py

@@ -1366,6 +1366,9 @@ def node_sanitize(node, fields):
     if lessee:
         target_dict['node.lessee'] = lessee
 
+    # Scrub the dictionary's contents down to what was requested.
+    api_utils.sanitize_dict(node, fields)
+
     # NOTE(tenbrae): the 'show_password' policy setting name exists for
     #             legacy purposes and can not be changed. Changing it will
     #             cause upgrade problems for any operators who have
@@ -1387,40 +1390,48 @@ def node_sanitize(node, fields):
     evaluate_additional_policies = not policy.check_policy(
         "baremetal:node:get:filter_threshold",
         target_dict, cdict)
+
+    node_keys = node.keys()
+
     if evaluate_additional_policies:
         # NOTE(TheJulia): The net effect of this is that by default,
         # at least matching common/policy.py defaults. is these should
         # be stripped out.
-        if not policy.check("baremetal:node:get:last_error",
+        if ('last_error' in node_keys
-                            target_dict, cdict):
+            and not policy.check("baremetal:node:get:last_error",
+                                 target_dict, cdict)):
             # Guard the last error from being visible as it can contain
             # hostnames revealing infrastucture internal details.
             node['last_error'] = ('** Value Redacted - Requires '
                                   'baremetal:node:get:last_error '
                                   'permission. **')
-        if not policy.check("baremetal:node:get:reservation",
+        if ('reservation' in node_keys
-                            target_dict, cdict):
+            and not policy.check("baremetal:node:get:reservation",
+                                 target_dict, cdict)):
             # Guard conductor names from being visible.
             node['reservation'] = ('** Redacted - requires baremetal:'
                                    'node:get:reservation permission. **')
-        if not policy.check("baremetal:node:get:driver_internal_info",
+        if ('driver_internal_info' in node_keys
-                            target_dict, cdict):
+            and not policy.check("baremetal:node:get:driver_internal_info",
+                                 target_dict, cdict)):
             # Guard conductor names from being visible.
             node['driver_internal_info'] = {
                 'content': '** Redacted - Requires baremetal:node:get:'
                            'driver_internal_info permission. **'}
-        if not policy.check("baremetal:node:get:driver_info",
+
-                            target_dict, cdict):
+    if 'driver_info' in node_keys:
+        if (evaluate_additional_policies
+            and not policy.check("baremetal:node:get:driver_info",
+                                 target_dict, cdict)):
             # Guard infrastructure intenral details from being visible.
             node['driver_info'] = {
                 'content': '** Redacted - requires baremetal:node:get:'
                            'driver_info permission. **'}
+        if not show_driver_secrets:
+            node['driver_info'] = strutils.mask_dict_password(
+                node['driver_info'], "******")
 
-    if not show_driver_secrets and node.get('driver_info'):
+    if not show_instance_secrets and 'instance_info' in node_keys:
-        node['driver_info'] = strutils.mask_dict_password(
-            node['driver_info'], "******")
-
-    if not show_instance_secrets and node.get('instance_info'):
         node['instance_info'] = strutils.mask_dict_password(
             node['instance_info'], "******")
         # NOTE(tenbrae): agent driver may store a swift temp_url on the
@@ -1434,11 +1445,12 @@ def node_sanitize(node, fields):
     if node.get('driver_internal_info', {}).get('agent_secret_token'):
         node['driver_internal_info']['agent_secret_token'] = "******"
 
-    update_state_in_older_versions(node)
+    if 'provision_state' in node_keys:
+        # Update legacy state data for provision state, but only if
+        # the key is present.
+        update_state_in_older_versions(node)
     hide_fields_in_newer_versions(node)
 
-    api_utils.sanitize_dict(node, fields)
-
     show_states_links = (
         api_utils.allow_links_node_states_and_driver_properties())
     show_portgroups = api_utils.allow_portgroups_subcontrollers()

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -17,6 +17,7 @@ import datetime
 from http import client as http_client
 import json
 import os
+import sys
 import tempfile
 from unittest import mock
 from urllib import parse as urlparse
@@ -141,6 +142,40 @@ class TestListNodes(test_api_base.BaseApiTest):
         self.assertNotIn('lessee', data['nodes'][0])
         self.assertNotIn('network_data', data['nodes'][0])
 
+    @mock.patch.object(policy, 'check', autospec=True)
+    @mock.patch.object(policy, 'check_policy', autospec=True)
+    def test_one_field_specific_santization(self, mock_check_policy,
+                                            mock_check):
+        py_ver = sys.version_info
+        if py_ver.major == 3 and py_ver.minor == 6:
+            self.skipTest('Test fails to work on python 3.6 when '
+                          'matching mock.ANY.')
+        obj_utils.create_test_node(self.context,
+                                   chassis_id=self.chassis.id,
+                                   last_error='meow')
+        mock_check_policy.return_value = False
+        data = self.get_json(
+            '/nodes?fields=uuid,provision_state,maintenance,instance_uuid,'
+            'last_error',
+            headers={api_base.Version.string: str(api_v1.max_version())})
+        self.assertIn('uuid', data['nodes'][0])
+        self.assertIn('provision_state', data['nodes'][0])
+        self.assertIn('maintenance', data['nodes'][0])
+        self.assertIn('instance_uuid', data['nodes'][0])
+        self.assertNotIn('driver_info', data['nodes'][0])
+        mock_check_policy.assert_has_calls([
+            mock.call('baremetal:node:get:filter_threshold',
+                      mock.ANY, mock.ANY)])
+        mock_check.assert_has_calls([
+            mock.call('is_admin', mock.ANY, mock.ANY),
+            mock.call('show_password', mock.ANY, mock.ANY),
+            mock.call('show_instance_secrets', mock.ANY, mock.ANY),
+            # Last error is populated above and should trigger a check.
+            mock.call('baremetal:node:get:last_error', mock.ANY, mock.ANY),
+            mock.call().__bool__(),
+            mock.call().__bool__(),
+        ])
+
     def test_get_one(self):
         node = obj_utils.create_test_node(self.context,
                                           chassis_id=self.chassis.id)

releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixes sub-optimal Ironic API performance where Secure RBAC related
+    field level policy checks were executing without first checking
+    if there were field results. This helps improve API performance when
+    only specific columns have been requested by the API consumer.