diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index 1736e982..3baaeae1 100644 --- a/oslo_policy/policy.py +++ b/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.""" diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index 38539518..7a6df7dd 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. +