Merge "Migrate default policy file from JSON to YAML"

Author: Zuul

doc/source/cli/nova-status.rst

@@ -139,6 +139,10 @@ Upgrade
   * Checks for the policy files are not automatically overwritten with
     new defaults.
 
+  **22.0.0 (Victoria)**
+
+  * Checks for the policy files is not JSON-formatted.
+
 See Also
 ========
 

doc/source/configuration/policy-concepts.rst

@@ -1,10 +1,19 @@
 Understanding Nova Policies
 ===========================
 
+.. warning::
+
+   JSON formatted policy file is deprecated since Nova 22.0.0(Victoria).
+   Use YAML formatted file. Use `oslopolicy-convert-json-to-yaml`__ tool
+   to convert the existing JSON to YAML formatted policy file in backward
+   compatible way.
+
+.. __: https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-convert-json-to-yaml.html
+
 Nova supports a rich policy system that has evolved significantly over its
 lifetime. Initially, this took the form of a large, mostly hand-written
-``policy.json`` file but, starting in the Newton (14.0.0) release, policy
-defaults have been defined in the codebase, requiring the ``policy.json``
+``policy.yaml`` file but, starting in the Newton (14.0.0) release, policy
+defaults have been defined in the codebase, requiring the ``policy.yaml``
 file only to override these defaults.
 
 In the Ussuri (21.0.0) release, further work was undertaken to address some

doc/source/configuration/policy.rst

@@ -4,6 +4,15 @@ Nova Policies
 
 The following is an overview of all available policies in Nova.
 
+.. warning::
+
+   JSON formatted policy file is deprecated since Nova 22.0.0(Victoria).
+   Use YAML formatted file. Use `oslopolicy-convert-json-to-yaml`__ tool
+   to convert the existing JSON to YAML formatted policy file in backward
+   compatible way.
+
+.. __: https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-convert-json-to-yaml.html
+
 .. only:: html
 
    For a sample configuration file, refer to :doc:`sample-policy`.

doc/source/install/verify.rst

@@ -127,3 +127,7 @@ Verify operation of the Compute service.
       | Result: Success                                                    |
       | Details: None                                                      |
       +--------------------------------------------------------------------+
+      | Check: Policy File JSON to YAML Migration                          |
+      | Result: Success                                                    |
+      | Details: None                                                      |
+      +--------------------------------------------------------------------+

lower-constraints.txt

@@ -69,21 +69,21 @@ os-xenapi==0.3.4
 osc-lib==1.10.0
 oslo.cache==1.26.0
 oslo.concurrency==3.29.0
-oslo.config==6.1.0
+oslo.config==6.8.0
 oslo.context==2.22.0
 oslo.db==4.44.0
 oslo.i18n==3.15.3
 oslo.log==3.36.0
 oslo.messaging==10.3.0
 oslo.middleware==3.31.0
-oslo.policy==3.1.0
+oslo.policy==3.4.0
 oslo.privsep==1.33.2
 oslo.reports==1.18.0
 oslo.rootwrap==5.8.0
 oslo.serialization==2.21.1
 oslo.service==1.40.1
 oslo.upgradecheck==0.1.1
-oslo.utils==4.1.0
+oslo.utils==4.5.0
 oslo.versionedobjects==1.35.0
 oslo.vmware==2.17.0
 oslotest==3.8.0
@@ -127,11 +127,11 @@ python-subunit==1.4.0
 pytz==2018.3
 PyYAML==3.13
 repoze.lru==0.7
-requests==2.14.2
+requests==2.23.0
 requests-mock==1.2.0
 requestsexceptions==1.4.0
 retrying==1.3.3
-rfc3986==1.1.0
+rfc3986==1.2.0
 Routes==2.3.1
 simplejson==3.13.2
 six==1.11.0

nova/cmd/status.py

@@ -25,6 +25,7 @@
 from oslo_config import cfg
 from oslo_serialization import jsonutils
 from oslo_upgradecheck import upgradecheck
+from oslo_utils import fileutils
 import pkg_resources
 import six
 from sqlalchemy import func as sqlfunc
@@ -409,6 +410,23 @@ def _check_policy(self):
         policy.reset()
         return status
 
+    def _check_policy_json(self):
+        "Checks to see if policy file is JSON-formatted policy file."
+        msg = _("Your policy file  is JSON-formatted which is "
+                "deprecated since Victoria release (Nova 22.0.0). "
+                "You need to switch to YAML-formatted file. You can use the "
+                "``oslopolicy-convert-json-to-yaml`` tool to convert existing "
+                "JSON-formatted files to YAML-formatted files in a "
+                "backwards-compatible manner: "
+                "https://docs.openstack.org/oslo.policy/"
+                "latest/cli/oslopolicy-convert-json-to-yaml.html.")
+        status = upgradecheck.Result(upgradecheck.Code.SUCCESS)
+        # Check if policy file exist and is JSON-formatted.
+        policy_path = CONF.find_file(CONF.oslo_policy.policy_file)
+        if policy_path and fileutils.is_json(policy_path):
+            status = upgradecheck.Result(upgradecheck.Code.FAILURE, msg)
+        return status
+
     # The format of the check functions is to return an upgradecheck.Result
     # object with the appropriate upgradecheck.Code and details set. If the
     # check hits warnings or failures then those should be stored in the
@@ -427,6 +445,8 @@ def _check_policy(self):
         (_('Cinder API'), _check_cinder),
         # Added in Ussuri
         (_('Policy Scope-based Defaults'), _check_policy),
+        # Added in Victoria
+        (_('Policy File JSON to YAML Migration'), _check_policy_json),
     )
 
 

nova/policy.py

@@ -19,6 +19,7 @@
 
 from oslo_config import cfg
 from oslo_log import log as logging
+from oslo_policy import opts
 from oslo_policy import policy
 from oslo_utils import excutils
 
@@ -40,6 +41,35 @@
 saved_file_rules = []
 KEY_EXPR = re.compile(r'%\((\w+)\)s')
 
+# TODO(gmann): Remove setting the default value of config policy_file
+# once oslo_policy change the default value to 'policy.yaml'.
+# https://github.com/openstack/oslo.policy/blob/a626ad12fe5a3abd49d70e3e5b95589d279ab578/oslo_policy/opts.py#L49
+DEFAULT_POLICY_FILE = 'policy.yaml'
+opts.set_defaults(cfg.CONF, DEFAULT_POLICY_FILE)
+
+
+def pick_policy_file(policy_file):
+    # TODO(gmann): We have changed the default value of
+    # CONF.oslo_policy.policy_file option to 'policy.yaml' in Victoria
+    # release. To avoid breaking any deployment relying on default
+    # value, we need to add this is fallback logic to pick the old default
+    # policy file (policy.json) if exist. We can to remove this fallback
+    # logic sometime in future.
+    if policy_file:
+        return policy_file
+
+    if CONF.oslo_policy.policy_file == DEFAULT_POLICY_FILE:
+        location = CONF.get_location('policy_file', 'oslo_policy').location
+        if CONF.find_file(CONF.oslo_policy.policy_file):
+            return CONF.oslo_policy.policy_file
+        elif location in [cfg.Locations.opt_default,
+                          cfg.Locations.set_default]:
+            old_default = 'policy.json'
+            if CONF.find_file(old_default):
+                return old_default
+    # Return overridden value
+    return CONF.oslo_policy.policy_file
+
 
 def reset():
     global _ENFORCER
@@ -67,11 +97,12 @@ def init(policy_file=None, rules=None, default_rule=None, use_conf=True,
     global saved_file_rules
 
     if not _ENFORCER:
-        _ENFORCER = policy.Enforcer(CONF,
-                                    policy_file=policy_file,
-                                    rules=rules,
-                                    default_rule=default_rule,
-                                    use_conf=use_conf)
+        _ENFORCER = policy.Enforcer(
+            CONF,
+            policy_file=pick_policy_file(policy_file),
+            rules=rules,
+            default_rule=default_rule,
+            use_conf=use_conf)
         # NOTE(gmann): Explictly disable the warnings for policies
         # changing their default check_str. During policy-defaults-refresh
         # work, all the policy defaults have been changed and warning for

nova/tests/unit/cmd/test_status.py

@@ -22,11 +22,16 @@
 
 import fixtures
 import mock
+import os
 from six.moves import StringIO
+import tempfile
+import yaml
 
 from keystoneauth1 import exceptions as ks_exc
 from keystoneauth1 import loading as keystone
 from keystoneauth1 import session
+from oslo_config import cfg
+from oslo_serialization import jsonutils
 from oslo_upgradecheck import upgradecheck
 from oslo_utils.fixture import uuidsentinel as uuids
 from oslo_utils import uuidutils
@@ -602,3 +607,63 @@ class TestUpgradeCheckPolicyEnableScope(TestUpgradeCheckPolicy):
     def setUp(self):
         super(TestUpgradeCheckPolicyEnableScope, self).setUp()
         self.flags(enforce_scope=True, group="oslo_policy")
+
+
+class TestUpgradeCheckPolicyJSON(test.NoDBTestCase):
+
+    def setUp(self):
+        super(TestUpgradeCheckPolicyJSON, self).setUp()
+        self.cmd = status.UpgradeCommands()
+        policy.CONF.clear_override('policy_file', group='oslo_policy')
+        self.data = {
+            'rule_admin': 'True',
+            'rule_admin2': 'is_admin:True'
+        }
+        self.temp_dir = self.useFixture(fixtures.TempDir())
+        fd, self.json_file = tempfile.mkstemp(dir=self.temp_dir.path)
+        fd, self.yaml_file = tempfile.mkstemp(dir=self.temp_dir.path)
+
+        with open(self.json_file, 'w') as fh:
+            jsonutils.dump(self.data, fh)
+        with open(self.yaml_file, 'w') as fh:
+            yaml.dump(self.data, fh)
+
+        original_search_dirs = cfg._search_dirs
+
+        def fake_search_dirs(dirs, name):
+            dirs.append(self.temp_dir.path)
+            return original_search_dirs(dirs, name)
+
+        self.stub_out('oslo_config.cfg._search_dirs', fake_search_dirs)
+
+    def test_policy_json_file_fail_upgrade(self):
+        # Test with policy json file full path set in config.
+        self.flags(policy_file=self.json_file, group="oslo_policy")
+        self.assertEqual(upgradecheck.Code.FAILURE,
+                         self.cmd._check_policy_json().code)
+
+    def test_policy_yaml_file_pass_upgrade(self):
+        # Test with full policy yaml file path set in config.
+        self.flags(policy_file=self.yaml_file, group="oslo_policy")
+        self.assertEqual(upgradecheck.Code.SUCCESS,
+                         self.cmd._check_policy_json().code)
+
+    def test_no_policy_file_pass_upgrade(self):
+        # Test with no policy file exist.
+        self.assertEqual(upgradecheck.Code.SUCCESS,
+                         self.cmd._check_policy_json().code)
+
+    def test_default_policy_yaml_file_pass_upgrade(self):
+        tmpfilename = os.path.join(self.temp_dir.path, 'policy.yaml')
+        with open(tmpfilename, 'w') as fh:
+            yaml.dump(self.data, fh)
+        self.assertEqual(upgradecheck.Code.SUCCESS,
+                         self.cmd._check_policy_json().code)
+
+    def test_old_default_policy_json_file_fail_upgrade(self):
+        self.flags(policy_file='policy.json', group="oslo_policy")
+        tmpfilename = os.path.join(self.temp_dir.path, 'policy.json')
+        with open(tmpfilename, 'w') as fh:
+            jsonutils.dump(self.data, fh)
+        self.assertEqual(upgradecheck.Code.FAILURE,
+                         self.cmd._check_policy_json().code)

nova/tests/unit/test_policy.py

@@ -17,10 +17,13 @@
 
 import os.path
 
+import fixtures
 import mock
+from oslo_config import cfg
 from oslo_policy import policy as oslo_policy
 from oslo_serialization import jsonutils
 import requests_mock
+import yaml
 
 from nova import context
 from nova import exception
@@ -572,3 +575,70 @@ def test_rule_missing(self):
             self.system_reader_or_owner_rules +
             self.allow_nobody_rules + special_rules)
         self.assertEqual(set([]), result)
+
+
+class PickPolicyFileTestCase(test.NoDBTestCase):
+
+    def setUp(self):
+        super(PickPolicyFileTestCase, self).setUp()
+        self.data = {
+            'rule_admin': 'True',
+            'rule_admin2': 'is_admin:True'
+        }
+        policy.CONF.clear_override('policy_file', group='oslo_policy')
+        self.tmpdir = self.useFixture(fixtures.TempDir())
+        original_search_dirs = cfg._search_dirs
+
+        def fake_search_dirs(dirs, name):
+            dirs.append(self.tmpdir.path)
+            return original_search_dirs(dirs, name)
+
+        self.stub_out('oslo_config.cfg._search_dirs', fake_search_dirs)
+
+    def test_non_config_policy_file(self):
+        tmpfilename = 'nova-policy.yaml'
+        self.flags(policy_file=tmpfilename, group='oslo_policy')
+        selected_policy_file = policy.pick_policy_file(
+            policy_file='non-config-file')
+        self.assertEqual(policy.CONF.oslo_policy.policy_file, tmpfilename)
+        self.assertEqual(selected_policy_file, 'non-config-file')
+
+    def test_overridden_policy_file(self):
+        tmpfilename = 'nova-policy.yaml'
+        self.flags(policy_file=tmpfilename, group='oslo_policy')
+        selected_policy_file = policy.pick_policy_file(policy_file=None)
+        self.assertEqual(policy.CONF.oslo_policy.policy_file, tmpfilename)
+        self.assertEqual(selected_policy_file, tmpfilename)
+
+    def test_only_new_default_policy_file_exist(self):
+        tmpfilename = os.path.join(self.tmpdir.path, 'policy.yaml')
+        with open(tmpfilename, 'w') as fh:
+            yaml.dump(self.data, fh)
+
+        selected_policy_file = policy.pick_policy_file(policy_file=None)
+        self.assertEqual(policy.CONF.oslo_policy.policy_file, 'policy.yaml')
+        self.assertEqual(selected_policy_file, 'policy.yaml')
+
+    @mock.patch.object(policy.CONF, 'get_location')
+    def test_only_old_default_policy_file_exist(self, mock_get):
+        mock_get.return_value = cfg.LocationInfo(cfg.Locations.set_default,
+                                                 'None')
+        tmpfilename = os.path.join(self.tmpdir.path, 'policy.json')
+        with open(tmpfilename, 'w') as fh:
+            jsonutils.dump(self.data, fh)
+
+        selected_policy_file = policy.pick_policy_file(policy_file=None)
+        self.assertEqual(policy.CONF.oslo_policy.policy_file, 'policy.yaml')
+        self.assertEqual(selected_policy_file, 'policy.json')
+
+    def test_both_default_policy_file_exist(self):
+        tmpfilename1 = os.path.join(self.tmpdir.path, 'policy.json')
+        with open(tmpfilename1, 'w') as fh:
+            jsonutils.dump(self.data, fh)
+        tmpfilename2 = os.path.join(self.tmpdir.path, 'policy.yaml')
+        with open(tmpfilename2, 'w') as fh:
+            yaml.dump(self.data, fh)
+
+        selected_policy_file = policy.pick_policy_file(policy_file=None)
+        self.assertEqual(policy.CONF.oslo_policy.policy_file, 'policy.yaml')
+        self.assertEqual(selected_policy_file, 'policy.yaml')

releasenotes/notes/bug-1875418-default-policy-file-change-22bd4cc6e27e0091.yaml

@@ -0,0 +1,19 @@
+---
+upgrade:
+  - |
+    The default value of ``[oslo_policy] policy_file`` config option has been
+    changed from ``policy.json``
+    to ``policy.yaml``. Nova policy new defaults since 21.0.0 and current
+    default value of ``[oslo_policy] policy_file`` config option (``policy.json``)
+    does not work when ``policy.json`` is generated by
+    `oslopolicy-sample-generator <https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-sample-generator.html>`_  tool.
+    Refer to `bug 1875418 <https://bugs.launchpad.net/nova/+bug/1875418>`_
+    for more details.
+    Also check `oslopolicy-convert-json-to-yaml <https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-convert-json-to-yaml.html>`_
+    tool to convert the JSON to YAML formatted policy file in
+    backward compatible way.
+fixes:
+  - |
+    Bug `1875418 <https://bugs.launchpad.net/nova/+bug/1875418>`_ is fixed
+    by changing the default value of ``[oslo_policy] policy_file`` config
+    option to YAML format.