From af49d0b30abed07fa7631c59bebdbe285a1a8a8e Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Thu, 17 Sep 2020 17:20:15 -0500 Subject: [PATCH] Change default policy file from JSON to YAML As Cyborg is switching to new policy, this is required to avoid breaking the existing deployment using policy file in json format and relying on default value of 'CONF.oslo_policy.policy_file'. Default value of 'CONF.oslo_policy.policy_file' config option has been changed from 'policy.json' to 'policy.yaml'. If new default file 'policy.yaml' does not exist but old default 'policy.json' exist then fallback to use old default file. An upgrade checks is added to check the policy_file format and fail upgrade checks if it is JSON formatted. Added a warning in policy doc about JSON formatted file is deprecated, also removed all the reference to policy.json file in doc as well as in tests. Related Blueprint: https://blueprints.launchpad.net/oslo.policy/+spec/policy-json-to-yaml Change-Id: I865227e516dc7505c463ac279309169d95ea6a22 --- .gitignore | 1 + cyborg/cmd/status.py | 30 ++++++-- cyborg/common/authorize_wsgi.py | 41 +++++++++- cyborg/common/policy.py | 2 +- cyborg/tests/unit/cmd/test_status.py | 74 +++++++++++++++++-- cyborg/tests/unit/policy_fixture.py | 2 +- devstack/settings | 2 +- doc/source/configuration/sample_policy.rst | 9 +++ ...policy.json.txt => README.policy.yaml.txt} | 2 +- etc/cyborg/{policy.json => policy.yaml} | 0 ...default-value-change-de14a3688357b081.yaml | 14 ++++ requirements.txt | 4 +- setup.cfg | 2 +- tools/config/cyborg-policy-generator.conf | 2 +- 14 files changed, 159 insertions(+), 26 deletions(-) rename etc/cyborg/{README.policy.json.txt => README.policy.yaml.txt} (51%) rename etc/cyborg/{policy.json => policy.yaml} (100%) create mode 100644 releasenotes/notes/policy-file-default-value-change-de14a3688357b081.yaml diff --git a/.gitignore b/.gitignore index 80de8ab7..c0d0d098 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ doc/source/_static/cyborg.conf.sample doc/source/_static/cyborg.policy.yaml.sample # Sample profile etc/cyborg/policy.json.sample +etc/cyborg/policy.yaml.sample etc/cyborg/cyborg.conf.sample .idea/* .vscode/* diff --git a/cyborg/cmd/status.py b/cyborg/cmd/status.py index 84799597..63925b7a 100644 --- a/cyborg/cmd/status.py +++ b/cyborg/cmd/status.py @@ -16,20 +16,38 @@ import sys from oslo_config import cfg from oslo_upgradecheck import upgradecheck +from oslo_utils import fileutils from cyborg.common.i18n import _ +CONF = cfg.CONF + + class Checks(upgradecheck.UpgradeCommands): """Various upgrade checks should be added as separate methods in this class and added to _upgrade_checks tuple. """ - def _check_placeholder(self): - # TODO(whoami-rajat):This is just a placeholder for upgrade checks, - # it should be removed when the actual checks are added - return upgradecheck.Result(upgradecheck.Code.SUCCESS) + 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 (Cyborg 5.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) + # NOTE(gmann): Check if policy file exist and is in + # JSON format by actually loading the file not just + # by checking the extension. + 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 # oslo_upgradecheck.upgradecheck.Result @@ -39,8 +57,8 @@ class Checks(upgradecheck.UpgradeCommands): # in the returned Result's "details" attribute. The # summary will be rolled up at the end of the check() method. _upgrade_checks = ( - # In the future there should be some real checks added here - (_('Placeholder'), _check_placeholder), + # Added in Victoria + (_('Policy File JSON to YAML Migration'), _check_policy_json), ) diff --git a/cyborg/common/authorize_wsgi.py b/cyborg/common/authorize_wsgi.py index 51d8c048..1c3db892 100644 --- a/cyborg/common/authorize_wsgi.py +++ b/cyborg/common/authorize_wsgi.py @@ -16,6 +16,7 @@ import sys from oslo_concurrency import lockutils from oslo_config import cfg from oslo_log import log +from oslo_policy import opts from oslo_policy import policy from oslo_versionedobjects import base as object_base import pecan @@ -30,6 +31,36 @@ CONF = cfg.CONF LOG = log.getLogger(__name__) +# 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(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.yaml) 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 + + @lockutils.synchronized('policy_enforcer', 'cyborg-') def init_enforcer(policy_file=None, rules=None, default_rule=None, use_conf=True, @@ -55,10 +86,12 @@ def init_enforcer(policy_file=None, rules=None, # loaded exactly once - when this module-global is initialized. # Defining these in the relevant API modules won't work # because API classes lack singletons and don't use globals. - _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) if suppress_deprecation_warnings: _ENFORCER.suppress_deprecation_warnings = True _ENFORCER.register_defaults(policies.list_policies()) diff --git a/cyborg/common/policy.py b/cyborg/common/policy.py index 01e4cbe9..77dd9aa7 100644 --- a/cyborg/common/policy.py +++ b/cyborg/common/policy.py @@ -19,7 +19,7 @@ """ from oslo_policy import policy # NOTE: to follow policy-in-code spec, we define defaults for -# the granular policies in code, rather than in policy.json. +# the granular policies in code, rather than in policy.yaml. # All of these may be overridden by configuration, but we can # depend on their existence throughout the code. diff --git a/cyborg/tests/unit/cmd/test_status.py b/cyborg/tests/unit/cmd/test_status.py index 0c97c94f..90900a53 100644 --- a/cyborg/tests/unit/cmd/test_status.py +++ b/cyborg/tests/unit/cmd/test_status.py @@ -12,19 +12,77 @@ # License for the specific language governing permissions and limitations # under the License. -from oslo_upgradecheck.upgradecheck import Code +import fixtures +import os +import tempfile +import yaml + +from oslo_config import cfg +from oslo_serialization import jsonutils +from oslo_upgradecheck import upgradecheck from cyborg.cmd import status +from cyborg.common import authorize_wsgi from cyborg.tests import base -class TestUpgradeChecks(base.TestCase): +class TestUpgradeCheckPolicyJSON(base.TestCase): def setUp(self): - super(TestUpgradeChecks, self).setUp() - self.cmd = status.Checks() + super(TestUpgradeCheckPolicyJSON, self).setUp() + self.cmd = status.UpgradeCommands() + authorize_wsgi.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) - def test__check_placeholder(self): - check_result = self.cmd._check_placeholder() - self.assertEqual( - Code.SUCCESS, check_result.code) + 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.mock_search = self.useFixture(fixtures.MockPatch( + 'oslo_config.cfg._search_dirs')).mock + self.mock_search.side_effect = 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) diff --git a/cyborg/tests/unit/policy_fixture.py b/cyborg/tests/unit/policy_fixture.py index e8bd5828..64c9be98 100644 --- a/cyborg/tests/unit/policy_fixture.py +++ b/cyborg/tests/unit/policy_fixture.py @@ -36,7 +36,7 @@ class PolicyFixture(fixtures.Fixture): super(PolicyFixture, self).setUp() self.policy_dir = self.useFixture(fixtures.TempDir()) self.policy_file_name = os.path.join(self.policy_dir.path, - 'policy.json') + 'policy.yaml') with open(self.policy_file_name, 'w') as policy_file: policy_file.write(policy_data) policy_opts.set_defaults(CONF) diff --git a/devstack/settings b/devstack/settings index acbb0b6e..4cef8e58 100644 --- a/devstack/settings +++ b/devstack/settings @@ -15,7 +15,7 @@ CYBORG_AUTH_CACHE_DIR=${CYBORG_AUTH_CACHE_DIR:-/var/cache/cyborg} CYBORG_CONF_DIR=${CYBORG_CONF_DIR:-/etc/cyborg} CYBORG_CONF_FILE=$CYBORG_CONF_DIR/cyborg.conf CYBORG_API_PASTE_INI=$CYBORG_CONF_DIR/api-paste.ini -CYBORG_POLICY_JSON=$CYBORG_CONF_DIR/policy.json +CYBORG_POLICY_JSON=$CYBORG_CONF_DIR/policy.yaml CYBORG_SERVICE_HOST=${CYBORG_SERVICE_HOST:-$SERVICE_HOST} CYBORG_SERVICE_PORT=${CYBORG_SERVICE_PORT:-6666} CYBORG_SERVICE_PROTOCOL=${CYBORG_SERVICE_PROTOCOL:-$SERVICE_PROTOCOL} diff --git a/doc/source/configuration/sample_policy.rst b/doc/source/configuration/sample_policy.rst index ce3823e7..20d5df05 100644 --- a/doc/source/configuration/sample_policy.rst +++ b/doc/source/configuration/sample_policy.rst @@ -2,6 +2,15 @@ Cyborg Sample Policy ==================== +.. warning:: + + JSON formatted policy file is deprecated since Cyborg 5.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 + The following is a sample cyborg policy file that has been auto-generated from default policy values in code. If you're using the default policies, then the maintenance of this file is not necessary, and it should not be copied into diff --git a/etc/cyborg/README.policy.json.txt b/etc/cyborg/README.policy.yaml.txt similarity index 51% rename from etc/cyborg/README.policy.json.txt rename to etc/cyborg/README.policy.yaml.txt index 70285457..1b99e45d 100644 --- a/etc/cyborg/README.policy.json.txt +++ b/etc/cyborg/README.policy.yaml.txt @@ -1,4 +1,4 @@ -To generate the sample policy.json file, run the following command from the top +To generate the sample policy.yaml file, run the following command from the top level of the cyborg directory: tox -egenpolicy diff --git a/etc/cyborg/policy.json b/etc/cyborg/policy.yaml similarity index 100% rename from etc/cyborg/policy.json rename to etc/cyborg/policy.yaml diff --git a/releasenotes/notes/policy-file-default-value-change-de14a3688357b081.yaml b/releasenotes/notes/policy-file-default-value-change-de14a3688357b081.yaml new file mode 100644 index 00000000..2624dc86 --- /dev/null +++ b/releasenotes/notes/policy-file-default-value-change-de14a3688357b081.yaml @@ -0,0 +1,14 @@ +--- +upgrade: + - | + The default value of ``[oslo_policy] policy_file`` config option has been + changed from ``policy.json`` + to ``policy.yaml``. Cyborg policy new defaults since 5.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 `_ tool. + Refer to `bug 1875418 `_ + for more details. + Also check `oslopolicy-convert-json-to-yaml `_ + tool to convert the JSON to YAML formatted policy file in + backward compatible way. diff --git a/requirements.txt b/requirements.txt index bb98b327..02b0adf7 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,9 +17,9 @@ oslo.service>=1.0.0,!=1.28.1 # Apache-2.0 oslo.db>=4.1.0 # Apache-2.0 os-resource-classes>=0.5.0 # Apache-2.0 oslo.upgradecheck>=0.1.0 # Apache-2.0 -oslo.utils>=3.33.0 # Apache-2.0 +oslo.utils>=4.5.0 # Apache-2.0 oslo.versionedobjects>=1.31.2 # Apache-2.0 -oslo.policy>=2.3.0 # Apache-2.0 +oslo.policy>=3.4.0 # Apache-2.0 SQLAlchemy>=0.9.0,!=1.1.5,!=1.1.6,!=1.1.7,!=1.1.8 # MIT alembic>=0.8.10 # MIT stevedore>=1.5.0 # Apache-2.0 diff --git a/setup.cfg b/setup.cfg index f4eef3d8..38a88bcc 100644 --- a/setup.cfg +++ b/setup.cfg @@ -26,7 +26,7 @@ packages = cyborg data_files = etc/cyborg = - etc/cyborg/policy.json + etc/cyborg/policy.yaml etc/cyborg/api-paste.ini [entry_points] diff --git a/tools/config/cyborg-policy-generator.conf b/tools/config/cyborg-policy-generator.conf index 7c7748ca..a928b7b8 100644 --- a/tools/config/cyborg-policy-generator.conf +++ b/tools/config/cyborg-policy-generator.conf @@ -1,3 +1,3 @@ [DEFAULT] -output_file = etc/cyborg/policy.json.sample +output_file = etc/cyborg/policy.yaml.sample namespace = cyborg.api