Avoid redundant policy syntax checks

Introduce a private variable inside Enforcer class to remember
status of the last policy syntax checks in order to avoid
redundant calls to the check_rules() method.

Having this flag makes the whole rules mechanism faster, as under
certain conditions check_rules() method was being executed
multiple times even when not needed.

Change-Id: Id3992fc0cb567451049a12ebdc6851e737573bb8
Closes-bug: #1723030
Co-Authored-By: Ben Nemec <bnemec@redhat.com>
(cherry picked from commit 909a1ea3a7)

oslo_policy/policy.py

@@ -471,6 +471,7 @@ class Enforcer(object):
 
         self.policy_file = policy_file or self.conf.oslo_policy.policy_file
         self.use_conf = use_conf
+        self._need_check_rule = True
         self.overwrite = overwrite
         self._loaded_files = []
         self._policy_dir_mtimes = {}
@@ -490,6 +491,7 @@ class Enforcer(object):
             raise TypeError(_('Rules must be an instance of dict or Rules, '
                             'got %s instead') % type(rules))
         self.use_conf = use_conf
+        self._need_check_rule = True
         if overwrite:
             self.rules = Rules(rules, self.default_rule)
         else:
@@ -551,7 +553,9 @@ class Enforcer(object):
                     self.rules[default.name] = default.check
 
             # Detect and log obvious incorrect rule definitions
-            self.check_rules()
+            if self._need_check_rule:
+                self.check_rules()
+                self._need_check_rule = False
 
     def check_rules(self, raise_on_violation=False):
         """Look for rule definitions that are obviously incorrect."""

oslo_policy/tests/test_policy.py

@@ -390,6 +390,66 @@ class EnforcerTest(base.PolicyBaseTestCase):
                                group='oslo_policy')
         self.assertRaises(ValueError, self.enforcer.load_rules, True)
 
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_rules_twice(self, mock_check_rules):
+        self.enforcer.load_rules()
+        self.enforcer.load_rules()
+        self.assertEqual(1, mock_check_rules.call_count)
+
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_rules_twice_force(self, mock_check_rules):
+        self.enforcer.load_rules(True)
+        self.enforcer.load_rules(True)
+        self.assertEqual(2, mock_check_rules.call_count)
+
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_rules_twice_clear(self, mock_check_rules):
+        self.enforcer.load_rules()
+        self.enforcer.clear()
+        # NOTE(bnemec): It's weird that we have to pass True here, but clear
+        # sets enforcer.use_conf to False, which causes load_rules to be a
+        # noop when called with no parameters.  This is probably a bug.
+        self.enforcer.load_rules(True)
+        self.assertEqual(2, mock_check_rules.call_count)
+
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_directory_twice(self, mock_check_rules):
+        self.create_config_file(
+            os.path.join('policy.d', 'a.conf'), POLICY_A_CONTENTS)
+        self.create_config_file(
+            os.path.join('policy.d', 'b.conf'), POLICY_B_CONTENTS)
+        self.enforcer.load_rules()
+        self.enforcer.load_rules()
+        self.assertEqual(1, mock_check_rules.call_count)
+        self.assertIsNotNone(self.enforcer.rules)
+
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_directory_twice_force(self, mock_check_rules):
+        self.create_config_file(
+            os.path.join('policy.d', 'a.conf'), POLICY_A_CONTENTS)
+        self.create_config_file(
+            os.path.join('policy.d', 'b.conf'), POLICY_B_CONTENTS)
+        self.enforcer.load_rules(True)
+        self.enforcer.load_rules(True)
+        self.assertEqual(2, mock_check_rules.call_count)
+        self.assertIsNotNone(self.enforcer.rules)
+
+    @mock.patch('oslo_policy.policy.Enforcer.check_rules')
+    def test_load_directory_twice_changed(self, mock_check_rules):
+        self.create_config_file(
+            os.path.join('policy.d', 'a.conf'), POLICY_A_CONTENTS)
+        self.enforcer.load_rules()
+
+        # Touch the file
+        conf_path = os.path.join(self.config_dir, os.path.join(
+            'policy.d', 'a.conf'))
+        stinfo = os.stat(conf_path)
+        os.utime(conf_path, (stinfo.st_atime + 10, stinfo.st_mtime + 10))
+
+        self.enforcer.load_rules()
+        self.assertEqual(2, mock_check_rules.call_count)
+        self.assertIsNotNone(self.enforcer.rules)
+
     def test_set_rules_type(self):
         self.assertRaises(TypeError,
                           self.enforcer.set_rules,

releasenotes/notes/policy-check-performance-fbad83c7a4afd7d7.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    As reported in launchpad bug 1723030, under some circumstances policy
+    checks caused a significant performance degradation. This release includes
+    improved logic around rule validation to prevent that.
+