diff options
author | Zuul <zuul@review.openstack.org> | 2018-07-19 01:02:12 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2018-07-19 01:02:12 +0000 |
commit | 0fc941f2802b787956c07e5aaf20f6272e689cf7 (patch) | |
tree | dc91754278789fffe0ae1bc76954905aabd36541 | |
parent | f319967738e671e1400052e39116ff3c23b9f9a0 (diff) | |
parent | 909a1ea3a7aceb6e0637058b9c6a53d14043d6d1 (diff) | |
download | oslo-policy-1.38.1.tar.gz |
-rw-r--r-- | oslo_policy/policy.py | 6 | ||||
-rw-r--r-- | oslo_policy/tests/test_policy.py | 60 | ||||
-rw-r--r-- | releasenotes/notes/policy-check-performance-fbad83c7a4afd7d7.yaml | 7 |
3 files changed, 72 insertions, 1 deletions
diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index be17647..1758579 100644 --- a/oslo_policy/policy.py +++ b/oslo_policy/policy.py @@ -496,6 +496,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 = {} @@ -515,6 +516,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: @@ -636,7 +638,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 13a00f2..d5e6686 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -391,6 +391,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 0000000..fba946b --- /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. + |