From 4d40252748bd31584f1cbb72d4641dec62cf1303 Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Wed, 31 Jul 2019 18:18:17 -0700 Subject: [PATCH] Add attribute to suppress deprecation warnings Without this patch, if a project is going through a heavy policy refactor, significant numbers of deprecation warnings are emitted. When the enforcer is repeated reinitialized, as is the case when unit testing, the vast amount of logs resulting from the warnings is both unnecessary and harmful as it impedes log readability and explodes the size of the logs, thereby causing CI instability as the infrastructure struggles to process the logs. This change adds a public attribute to the enforcer object to allow callers to suppress these logs. This is not exposed as a configuration option because we do not want to allow operators to suppress these logs, and the warnings that occur when the enforcer is only reinitialized when the process is reloaded are not nearly so debilitating as they are during, e.g., a unit test run when the enforcer is generally reinitialized for every test. The Python warnings module allows for setting global attributes to filter logs, and it might have been useful for the consuming project to filter these logs at that level rather than modifying the policy enforcer to turn log emissions on and off. The problem with this approach is that if the number of deprecations is extreme, as may be the case during a heavy refactor, the warnings filter can become so inefficient that the test run can take much longer, causing even further CI stability as test runs reach timeout limits. Needed-by: https://review.opendev.org/673933 Change-Id: Ibfc7d4fca02b896953f80ddf1a9a8b9a19444f72 Related-bug: #1836568 --- oslo_policy/policy.py | 12 ++++-- oslo_policy/tests/test_policy.py | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index dd53ba7f..0a1173cb 100644 --- a/oslo_policy/policy.py +++ b/oslo_policy/policy.py @@ -503,6 +503,8 @@ class Enforcer(object): self._policy_dir_mtimes = {} self._file_cache = {} self._informed_no_policy_file = False + # FOR TESTING ONLY + self.suppress_deprecation_warnings = False def set_rules(self, rules, overwrite=True, use_conf=False): """Create a new :class:`Rules` based on the provided dict of rules. @@ -538,6 +540,7 @@ class Enforcer(object): self.registered_rules = {} self.file_rules = {} self._informed_no_policy_file = False + self.suppress_deprecation_warnings = False def load_rules(self, force_reload=False): """Loads policy_path's rules. @@ -618,7 +621,8 @@ class Enforcer(object): # know that the policy is going to be silently ignored in the future # and they can remove it from their overrides since it isn't being # replaced by another policy. - if default.name in self.file_rules: + if not self.suppress_deprecation_warnings and \ + default.name in self.file_rules: warnings.warn( 'Policy "%(policy)s":"%(check_str)s" was deprecated for ' 'removal in %(release)s. Reason: %(reason)s. Its value may be ' @@ -662,7 +666,8 @@ class Enforcer(object): if deprecated_rule.name != default.name and ( deprecated_rule.name in self.file_rules): - warnings.warn(deprecated_msg) + if not self.suppress_deprecation_warnings: + warnings.warn(deprecated_msg) # If the deprecated policy is being overridden and doesn't match # the default deprecated policy, override the new policy's default @@ -692,7 +697,8 @@ class Enforcer(object): default.check_str + ' or ' + deprecated_rule.check_str ) - warnings.warn(deprecated_msg) + if not self.suppress_deprecation_warnings: + warnings.warn(deprecated_msg) def _undefined_check(self, check): '''Check if a RuleCheck references an undefined rule.''' diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index 9a7f6a41..ba4896b0 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -1281,6 +1281,76 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): enforcer.load_rules() mock_warn.assert_not_called() + def test_deprecate_check_str_suppress_does_not_log_warning(self): + deprecated_rule = policy.DeprecatedRule( + name='foo:create_bar', + check_str='role:fizz' + ) + + rule_list = [policy.DocumentedRuleDefault( + name='foo:create_bar', + check_str='role:bang', + description='Create a bar.', + operations=[{'path': '/v1/bars', 'method': 'POST'}], + deprecated_rule=deprecated_rule, + deprecated_reason='"role:bang" is a better default', + deprecated_since='N' + )] + enforcer = policy.Enforcer(self.conf) + enforcer.suppress_deprecation_warnings = True + enforcer.register_defaults(rule_list) + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules() + mock_warn.assert_not_called() + + def test_deprecate_name_suppress_does_not_log_warning(self): + deprecated_rule = policy.DeprecatedRule( + name='foo:bar', + check_str='role:baz' + ) + + rule_list = [policy.DocumentedRuleDefault( + name='foo:create_bar', + check_str='role:baz', + description='Create a bar.', + operations=[{'path': '/v1/bars/', 'method': 'POST'}], + deprecated_rule=deprecated_rule, + deprecated_reason='"foo:bar" is not granular enough.', + deprecated_since='N' + )] + + rules = jsonutils.dumps({'foo:bar': 'role:bang'}) + self.create_config_file('policy.json', rules) + enforcer = policy.Enforcer(self.conf) + enforcer.suppress_deprecation_warnings = True + enforcer.register_defaults(rule_list) + + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules() + mock_warn.assert_not_called() + + def test_deprecate_for_removal_suppress_does_not_log_warning(self): + rule_list = [policy.DocumentedRuleDefault( + name='foo:bar', + check_str='role:baz', + description='Create a foo.', + operations=[{'path': '/v1/foos/', 'method': 'POST'}], + deprecated_for_removal=True, + deprecated_reason=( + '"foo:bar" is no longer a policy used by the service' + ), + deprecated_since='N' + )] + rules = jsonutils.dumps({'foo:bar': 'role:bang'}) + self.create_config_file('policy.json', rules) + enforcer = policy.Enforcer(self.conf) + enforcer.suppress_deprecation_warnings = True + enforcer.register_defaults(rule_list) + + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules() + mock_warn.assert_not_called() + def test_deprecated_policy_for_removal_must_include_deprecated_since(self): self.assertRaises( ValueError,