diff --git a/oslo_policy/generator.py b/oslo_policy/generator.py index 0ca7d1d0..0a6fde23 100644 --- a/oslo_policy/generator.py +++ b/oslo_policy/generator.py @@ -127,6 +127,28 @@ def _format_rule_default_yaml(default, include_help=True): 'op': op, 'text': text}) + if default.deprecated_for_removal: + text = ( + '# DEPRECATED\n# "%(name)s" has been deprecated since ' + '%(since)s.\n%(reason)s\n%(text)s' + ) % {'name': default.name, + 'since': default.deprecated_since, + 'reason': _format_help_text(default.deprecated_reason), + 'text': text} + elif default.deprecated_rule: + text = ( + '# DEPRECATED\n# "%(old_name)s":"%(old_check_str)s" has been ' + 'deprecated since %(since)s in favor of ' + '"%(name)s":"%(check_str)s".\n' + '%(reason)s\n%(text)s' + ) % {'old_name': default.deprecated_rule.name, + 'old_check_str': default.deprecated_rule.check_str, + 'since': default.deprecated_since, + 'name': default.name, + 'check_str': default.check_str, + 'reason': _format_help_text(default.deprecated_reason), + 'text': text} + return text diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index 1736e982..8abd35f1 100644 --- a/oslo_policy/policy.py +++ b/oslo_policy/policy.py @@ -221,6 +221,7 @@ by setting the ``policy_default_rule`` configuration setting to the desired rule name. """ +import copy import logging import os import warnings @@ -547,6 +548,66 @@ class Enforcer(object): force_reload, False) for default in self.registered_rules.values(): + if default.deprecated_rule: + deprecated_msg = ( + 'Policy "%(old_name)s":"%(old_check_str)s" was ' + 'deprecated in %(release)s in favor of "%(name)s":' + '"%(check_str)s". Reason: %(reason)s. Either ensure ' + 'your deployment is ready for the new default or ' + 'copy/paste the deprecated policy into your policy ' + 'file and maintain it manually.' % { + 'old_name': default.deprecated_rule.name, + 'old_check_str': default.deprecated_rule.check_str, + 'release': default.deprecated_since, + 'name': default.name, + 'check_str': default.check_str, + 'reason': default.deprecated_reason + } + ) + if default.deprecated_rule.name != default.name and ( + default.deprecated_rule.name in self.rules): + # Print a warning because the actual policy name is + # changing. If deployers are relying on an override for + # foo:bar and it's getting renamed to foo:create_bar + # then they need to be able to see that before they + # roll out the next release. + warnings.warn(deprecated_msg) + if (default.deprecated_rule.check_str != + default.check_str and default.name not in + self.rules): + # In this case, the default check_str is changing. We + # need to let operators know that this is going to + # change. If they don't want to override it, they are + # going to have to make sure the right infrastructure + # exists before they upgrade. This overrides the new + # check with an OrCheck that combines the new and old + # check_str attributes from the new and deprecated + # policies. This will make it so that deployments don't + # break on upgrade, but they receive log messages + # telling them stuff is going to change if they don't + # maintain the policy manually or add infrastructure to + # their deployment to support the new policy. + default.check = _parser.parse_rule( + default.check_str + ' or ' + + default.deprecated_rule.check_str + ) + warnings.warn(deprecated_msg) + if default.deprecated_for_removal and ( + default.name in self.rules): + # If a policy is going to be removed altogether, then we + # need to make sure we let operators know so they can clean + # up their policy files, if they are overriding it. + warnings.warn( + 'Policy "%(policy)s":"%(check_str)s" was ' + 'deprecated for removal in %(release)s. Reason: ' + '%(reason)s. Its value may be silently ignored in ' + 'the future.' % { + 'policy': default.name, + 'check_str': default.check_str, + 'release': default.deprecated_since, + 'reason': default.deprecated_reason + } + ) if default.name not in self.rules: self.rules[default.name] = default.check @@ -822,12 +883,54 @@ class RuleDefault(object): :param description: A plain text description of the policy. This will be used to comment sample policy files for use by deployers. + :param deprecated_rule: :class:`.DeprecatedRule` + :param deprecated_for_removal: indicates whether the policy is planned for + removal in a future release. + :param deprecated_reason: indicates why this policy is planned for removal + in a future release. Silently ignored if + deprecated_for_removal is False. + :param deprecated_since: indicates which release this policy was deprecated + in. Accepts any string, though valid version + strings are encouraged. Silently ignored if + deprecated_for_removal is False. + + .. versionchanged 1.29 + Added *deprecated_rule* parameter. + + .. versionchanged 1.29 + Added *deprecated_for_removal* parameter. + + .. versionchanged 1.29 + Added *deprecated_reason* parameter. + + .. versionchanged 1.29 + Added *deprecated_since* parameter. """ - def __init__(self, name, check_str, description=None): + def __init__(self, name, check_str, description=None, + deprecated_rule=None, deprecated_for_removal=False, + deprecated_reason=None, deprecated_since=None): self.name = name self.check_str = check_str self.check = _parser.parse_rule(check_str) self.description = description + self.deprecated_rule = copy.deepcopy(deprecated_rule) or [] + self.deprecated_for_removal = deprecated_for_removal + self.deprecated_reason = deprecated_reason + self.deprecated_since = deprecated_since + + if self.deprecated_rule: + if not isinstance(self.deprecated_rule, DeprecatedRule): + raise ValueError( + 'deprecated_rule must be a DeprecatedRule object.' + ) + + if (deprecated_for_removal or deprecated_rule) and ( + deprecated_reason is None or deprecated_since is None): + raise ValueError( + '%(name)s deprecated without deprecated_reason or ' + 'deprecated_since. Both must be supplied if deprecating a ' + 'policy' % {'name': self.name} + ) def __str__(self): return '"%(name)s": "%(check_str)s"' % {'name': self.name, @@ -871,9 +974,16 @@ class DocumentedRuleDefault(RuleDefault): operations=[{'path': '/foo', 'method': 'GET'}, {'path': '/some', 'method': 'POST'}] """ - def __init__(self, name, check_str, description, operations): + def __init__(self, name, check_str, description, operations, + deprecated_rule=None, deprecated_for_removal=False, + deprecated_reason=None, deprecated_since=None): super(DocumentedRuleDefault, self).__init__( - name, check_str, description) + name, check_str, description, + deprecated_rule=deprecated_rule, + deprecated_for_removal=deprecated_for_removal, + deprecated_reason=deprecated_reason, + deprecated_since=deprecated_since + ) self.operations = operations @property @@ -906,3 +1016,172 @@ class DocumentedRuleDefault(RuleDefault): if len(op.keys()) > 2: raise InvalidRuleDefault('Operation contains > 2 keys') self._operations = ops + + +class DeprecatedRule(object): + + """Represents a Deprecated policy or rule. + + Here's how you can use it to change a policy's default role or rule. Assume + the following policy exists in code:: + + from oslo_policy import policy + + policy.DocumentedRuleDefault( + name='foo:create_bar', + check_str='role:fizz', + description='Create a bar.', + operations=[{'path': '/v1/bars', 'method': 'POST'}] + ) + + The next snippet will maintain the deprecated option, but allow + ``foo:create_bar`` to default to ``role:bang`` instead of ``role:fizz``:: + + deprecated_rule = policy.DeprecatedRule( + name='foo:create_bar', + check_str='role:fizz' + ) + + 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' + ) + + DeprecatedRule can be used to change the policy name itself. Assume the + following policy exists in code:: + + from oslo_policy import policy + + policy.DocumentedRuleDefault( + name='foo:post_bar', + check_str='role:fizz', + description='Create a bar.', + operations=[{'path': '/v1/bars', 'method': 'POST'}] + ) + + For the sake of consistency, let's say we want to replace ``foo:post_bar`` + with ``foo:create_bar``, but keep the same ``check_str`` as the default. We + can accomplish this by doing:: + + deprecated_rule = policy.DeprecatedRule( + name='foo:post_bar', + check_str='role:fizz' + ) + + policy.DocumentedRuleDefault( + name='foo:create_bar', + check_str='role:fizz', + description='Create a bar.', + operations=[{'path': '/v1/bars', 'method': 'POST'}], + deprecated_rule=deprecated_rule, + deprecated_reason='foo:create_bar is more consistent', + deprecated_since='N' + ) + + Finally, let's use DeprecatedRule to break a policy into more granular + policies. Let's assume the following policy exists in code:: + + policy.DocumentedRuleDefault( + name='foo:bar', + check_str='role:bazz', + description='Create, read, update, or delete a bar.', + operations=[ + { + 'path': '/v1/bars', + 'method': 'POST' + }, + { + 'path': '/v1/bars', + 'method': 'GET' + }, + { + 'path': '/v1/bars/{bar_id}', + 'method': 'GET' + }, + { + 'path': '/v1/bars/{bar_id}', + 'method': 'PATCH' + }, + { + 'path': '/v1/bars/{bar_id}', + 'method': 'DELETE' + } + ] + ) + + Here we can see the same policy is used to protect multiple operations on + bars. This prevents operators from being able to assign different roles to + different actions that can be taken on bar. For example, what if an + operator wanted to require a less restrictive role or rule to list bars but + a more restrictive rule to delete them? The following will introduce a + policy that helps achieve that and deprecate the original, overly-broad + policy:: + + deprecated_rule = policy.DeprecatedRule( + name='foo:bar', + check_str='role:bazz' + ) + + 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='foo:create_bar is more granular than foo:bar', + deprecated_since='N' + ) + policy.DocumentedRuleDefault( + name='foo:list_bars', + check_str='role:bazz', + description='List bars.', + operations=[{'path': '/v1/bars', 'method': 'GET'}], + deprecated_rule=deprecated_rule, + deprecated_reason='foo:list_bars is more granular than foo:bar', + deprecated_since='N' + ) + policy.DocumentedRuleDefault( + name='foo:get_bar', + check_str='role:bazz', + description='Get a bar.', + operations=[{'path': '/v1/bars/{bar_id}', 'method': 'GET'}], + deprecated_rule=deprecated_rule, + deprecated_reason='foo:get_bar is more granular than foo:bar', + deprecated_since='N' + ) + policy.DocumentedRuleDefault( + name='foo:update_bar', + check_str='role:bang', + description='Update a bar.', + operations=[{'path': '/v1/bars/{bar_id}', 'method': 'PATCH'}], + deprecated_rule=deprecated_rule, + deprecated_reason='foo:update_bar is more granular than foo:bar', + deprecated_since='N' + ) + policy.DocumentedRuleDefault( + name='foo:delete_bar', + check_str='role:bang', + description='Delete a bar.', + operations=[{'path': '/v1/bars/{bar_id}', 'method': 'DELETE'}], + deprecated_rule=deprecated_rule, + deprecated_reason='foo:delete_bar is more granular than foo:bar', + deprecated_since='N' + ) + + .. versionchanged 1.29 + Added *DeprecatedRule* object. + """ + + def __init__(self, name, check_str): + """Construct a DeprecatedRule object. + + :param name: the policy name + :param check_str: the value of the policy's check string + """ + self.name = name + self.check_str = check_str diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index 2406fcb5..8db44d0b 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -844,6 +844,161 @@ class RuleDefaultTestCase(base.PolicyBaseTestCase): self.assertNotEqual(opt1, opt2) +class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): + + def test_deprecate_a_policy_check_string(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.register_defaults(rule_list) + expected_msg = ( + 'Policy "foo:create_bar":"role:fizz" was deprecated in N in favor ' + 'of "foo:create_bar":"role:bang". Reason: "role:bang" is a better ' + 'default. Either ensure your deployment is ready for the new ' + 'default or copy/paste the deprecated policy into your policy ' + 'file and maintain it manually.' + ) + + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules() + mock_warn.assert_called_once_with(expected_msg) + + def test_deprecate_a_policy_name(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. If your deployment has ' + 'overridden "foo:bar", ensure you override the new policies ' + 'with same role or rule. Not doing this will require the ' + 'service to assume the new defaults for "foo:bar:create", ' + '"foo:bar:update", "foo:bar:list", and "foo:bar:delete", ' + 'which might be backwards incompatible for your deployment' + ), + deprecated_since='N' + )] + expected_msg = ( + 'Policy "foo:bar":"role:baz" was deprecated in N in favor of ' + '"foo:create_bar":"role:baz". Reason: "foo:bar" is not granular ' + 'enough. If your deployment has overridden "foo:bar", ensure you ' + 'override the new policies with same role or rule. Not doing this ' + 'will require the service to assume the new defaults for ' + '"foo:bar:create", "foo:bar:update", "foo:bar:list", and ' + '"foo:bar:delete", which might be backwards incompatible for your ' + 'deployment. Either ensure your deployment is ready for the new ' + 'default or copy/paste the deprecated policy into your policy ' + 'file and maintain it manually.' + ) + + rules = jsonutils.dumps({'foo:bar': 'role:bang'}) + self.create_config_file('policy.json', rules) + enforcer = policy.Enforcer(self.conf) + enforcer.register_defaults(rule_list) + + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules(True) + mock_warn.assert_called_once_with(expected_msg) + + def test_deprecate_a_policy_for_removal(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' + )] + expected_msg = ( + 'Policy "foo:bar":"role:baz" was deprecated for removal in N. ' + 'Reason: "foo:bar" is no longer a policy used by the service. Its ' + 'value may be silently ignored in the future.' + ) + rules = jsonutils.dumps({'foo:bar': 'role:bang'}) + self.create_config_file('policy.json', rules) + enforcer = policy.Enforcer(self.conf) + enforcer.register_defaults(rule_list) + + with mock.patch('warnings.warn') as mock_warn: + enforcer.load_rules() + mock_warn.assert_called_once_with(expected_msg) + + def test_deprecated_policy_for_removal_must_include_deprecated_since(self): + self.assertRaises( + ValueError, + policy.DocumentedRuleDefault, + name='foo:bar', + check_str='rule:baz', + description='Create a foo.', + operations=[{'path': '/v1/foos/', 'method': 'POST'}], + deprecated_for_removal=True, + deprecated_reason='Some reason.' + ) + + def test_deprecated_policy_must_include_deprecated_since(self): + deprecated_rule = policy.DeprecatedRule( + name='foo:bar', + check_str='rule:baz' + ) + + self.assertRaises( + ValueError, + policy.DocumentedRuleDefault, + name='foo:bar', + check_str='rule:baz', + description='Create a foo.', + operations=[{'path': '/v1/foos/', 'method': 'POST'}], + deprecated_rule=deprecated_rule, + deprecated_reason='Some reason.' + ) + + def test_deprecated_rule_requires_deprecated_rule_object(self): + self.assertRaises( + ValueError, + policy.DocumentedRuleDefault, + name='foo:bar', + check_str='rule:baz', + description='Create a foo.', + operations=[{'path': '/v1/foos/', 'method': 'POST'}], + deprecated_rule='foo:bar', + deprecated_reason='Some reason.' + ) + + def test_deprecated_policy_must_include_deprecated_reason(self): + self.assertRaises( + ValueError, + policy.DocumentedRuleDefault, + name='foo:bar', + check_str='rule:baz', + description='Create a foo.', + operations=[{'path': '/v1/foos/', 'method': 'POST'}], + deprecated_for_removal=True, + deprecated_since='N' + ) + + class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase): def test_contain_operations(self):