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
This commit is contained in:
Colleen Murphy 2019-07-31 18:18:17 -07:00
parent b7da7a92ad
commit 4d40252748
2 changed files with 79 additions and 3 deletions

View File

@ -503,6 +503,8 @@ class Enforcer(object):
self._policy_dir_mtimes = {} self._policy_dir_mtimes = {}
self._file_cache = {} self._file_cache = {}
self._informed_no_policy_file = False self._informed_no_policy_file = False
# FOR TESTING ONLY
self.suppress_deprecation_warnings = False
def set_rules(self, rules, overwrite=True, use_conf=False): def set_rules(self, rules, overwrite=True, use_conf=False):
"""Create a new :class:`Rules` based on the provided dict of rules. """Create a new :class:`Rules` based on the provided dict of rules.
@ -538,6 +540,7 @@ class Enforcer(object):
self.registered_rules = {} self.registered_rules = {}
self.file_rules = {} self.file_rules = {}
self._informed_no_policy_file = False self._informed_no_policy_file = False
self.suppress_deprecation_warnings = False
def load_rules(self, force_reload=False): def load_rules(self, force_reload=False):
"""Loads policy_path's rules. """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 # 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 # and they can remove it from their overrides since it isn't being
# replaced by another policy. # 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( warnings.warn(
'Policy "%(policy)s":"%(check_str)s" was deprecated for ' 'Policy "%(policy)s":"%(check_str)s" was deprecated for '
'removal in %(release)s. Reason: %(reason)s. Its value may be ' '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 ( if deprecated_rule.name != default.name and (
deprecated_rule.name in self.file_rules): 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 # If the deprecated policy is being overridden and doesn't match
# the default deprecated policy, override the new policy's default # the default deprecated policy, override the new policy's default
@ -692,7 +697,8 @@ class Enforcer(object):
default.check_str + ' or ' + default.check_str + ' or ' +
deprecated_rule.check_str deprecated_rule.check_str
) )
warnings.warn(deprecated_msg) if not self.suppress_deprecation_warnings:
warnings.warn(deprecated_msg)
def _undefined_check(self, check): def _undefined_check(self, check):
'''Check if a RuleCheck references an undefined rule.''' '''Check if a RuleCheck references an undefined rule.'''

View File

@ -1281,6 +1281,76 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
enforcer.load_rules() enforcer.load_rules()
mock_warn.assert_not_called() 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): def test_deprecated_policy_for_removal_must_include_deprecated_since(self):
self.assertRaises( self.assertRaises(
ValueError, ValueError,