From 77d30c3b5e06a4cc90195ca6f352b26c4bf2d2d8 Mon Sep 17 00:00:00 2001 From: Mateusz Kowalski Date: Tue, 24 Oct 2017 09:32:45 +0200 Subject: [PATCH] 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 (cherry picked from commit 909a1ea3a7aceb6e0637058b9c6a53d14043d6d1) --- oslo_policy/policy.py | 6 +- oslo_policy/tests/test_policy.py | 60 +++++++++++++++++++ ...cy-check-performance-fbad83c7a4afd7d7.yaml | 7 +++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/policy-check-performance-fbad83c7a4afd7d7.yaml diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index 4034fffc..09597f27 100644 --- a/oslo_policy/policy.py +++ b/oslo_policy/policy.py @@ -487,6 +487,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 = {} @@ -506,6 +507,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: @@ -627,7 +629,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.""" diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index 1bb97c26..7d220014 100644 --- a/oslo_policy/tests/test_policy.py +++ b/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, diff --git a/releasenotes/notes/policy-check-performance-fbad83c7a4afd7d7.yaml b/releasenotes/notes/policy-check-performance-fbad83c7a4afd7d7.yaml new file mode 100644 index 00000000..fba946b7 --- /dev/null +++ b/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. +