Handle deprecated rule only once

The policy engine converts simple strings into instances of rule
objects based on a policy DSL. This engine iterates checks and reduces
them after each iteration if performing the conversion on list of check
strings.

When we deprecate policies we apply a logical OR to make upgrades easier
for operators. The logical OR, implemented with an OrCheck, only needs
to be done once per deprecated rule. Today, we're re-initializing an
OrCheck instance each time we load rules, which happens every time
oslo_policy.policy.Enforcer.enforce() is called.

For most OpenStack usage, this isn't noticiable, especially if you're
only using it to enforce access to a specific endpoint. However, this
can get expensive if you're using the enforcer to protect the API,
protect each resource in a response, and protect each attrbute of the
resource (e.g., Neutron makes extensive usage of this pattern to
implement RBAC for resources it's responsible for).

This commit updates the RuleDefault object to track state of handling
deprecated logic ORs so that we only cast the check strings to OrCheck
instances once per rule no matter how many times we call load_rules().

Closes-Bug: 1913718

Change-Id: I539672fc220b8d7e3c47ab3dfa6670b88e3f4093

oslo_policy/policy.py

@@ -698,6 +698,11 @@ class Enforcer(object):
                         DeprecatedRule
         """
 
+        # Short-circuit the rule deprecation logic if we've already processed
+        # it for this particular rule.
+        if default._deprecated_rule_handled:
+            return
+
         deprecated_rule = default.deprecated_rule
 
         deprecated_msg = (
@@ -766,6 +771,8 @@ class Enforcer(object):
                     or self.suppress_default_change_warnings):
                 warnings.warn(deprecated_msg)
 
+        default._deprecated_rule_handled = True
+
     def _undefined_check(self, check):
         '''Check if a RuleCheck references an undefined rule.'''
         if isinstance(check, RuleCheck):
@@ -1180,6 +1187,7 @@ class RuleDefault(object):
         self.deprecated_for_removal = deprecated_for_removal
         self.deprecated_reason = deprecated_reason
         self.deprecated_since = deprecated_since
+        self._deprecated_rule_handled = False
 
         if self.deprecated_rule:
             if not isinstance(self.deprecated_rule, DeprecatedRule):

oslo_policy/tests/test_policy.py

@@ -1749,6 +1749,39 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
             enforcer.enforce('foo:create_bar', {}, {'roles': ['baz']})
         )
 
+    def test_deprecation_logic_is_only_performed_once_per_rule(self):
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:create_bar',
+            check_str='role:fizz'
+        )
+        rule = 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.register_defaults([rule])
+
+        # Check that rule deprecation handling hasn't been done, yet
+        self.assertFalse(rule._deprecated_rule_handled)
+
+        # Loading the rules will modify the rule check string to logically OR
+        # the new value with the deprecated value
+        enforcer.load_rules()
+        self.assertTrue(rule._deprecated_rule_handled)
+
+        # Make sure the original value is used instead of instantiating new
+        # OrCheck objects whenever we perform subsequent reloads
+        expected_check = rule.check
+        enforcer.load_rules()
+        self.assertTrue(rule.check is expected_check)
+        self.assertTrue(rule._deprecated_rule_handled)
+
 
 class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase):
 

releasenotes/notes/bug-1913718-f1b46bbff3231d98.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1913718 <https://launchpad.net/bugs/1913718>`_]
+    The `Enforcer()` object now only processes deprecated rules once at load or
+    enforcement time, improving performance for users that make extensive use
+    of policy enforcement.