diff --git a/etc/placement/config-generator.conf b/etc/placement/config-generator.conf index dd7bf32a0..67cd782ec 100644 --- a/etc/placement/config-generator.conf +++ b/etc/placement/config-generator.conf @@ -5,6 +5,7 @@ namespace = placement.conf namespace = keystonemiddleware.auth_token namespace = oslo.log namespace = oslo.middleware.cors +namespace = oslo.policy namespace = osprofiler # FIXME(mriedem): There are likely other missing 3rd party oslo library -# options that should show up in the placement.conf docs, like oslo.policy. +# options that should show up in the placement.conf docs, like oslo.concurrency diff --git a/placement/conf/__init__.py b/placement/conf/__init__.py index 8a6711983..5e04d974b 100644 --- a/placement/conf/__init__.py +++ b/placement/conf/__init__.py @@ -16,6 +16,7 @@ from __future__ import absolute_import from oslo_log import log as logging from oslo_middleware import cors +from oslo_policy import opts as policy_opts from placement.conf import api from placement.conf import base @@ -34,6 +35,7 @@ def register_opts(conf): paths.register_opts(conf) placement.register_opts(conf) logging.register_options(conf) + policy_opts.set_defaults(conf) # The CORS middleware does not present a register_opts method, instead # it shares a list of available opts. conf.register_opts(cors.CORS_OPTS, 'cors') diff --git a/placement/conf/placement.py b/placement/conf/placement.py index 44580ce8c..e3e275253 100644 --- a/placement/conf/placement.py +++ b/placement/conf/placement.py @@ -40,6 +40,17 @@ is determined. # This default matches what is in # etc/nova/policy-generator.conf default='policy.yaml', + deprecated_for_removal=True, + deprecated_since='2.0.0', + deprecated_reason=""" +This option was necessary when placement was part of nova but can now be +replaced with the more standard usage of ``[oslo_policy]/policy_file`` which +has the same semantics but a different default value. If you have a custom +policy.yaml file and were not setting this option but just relying on the +default value, you need to configure placement to use +``[oslo_policy]/policy_file`` with policy.yaml specifically since otherwise +that option defaults to policy.json. +""", help='The file that defines placement policies. This can be an ' 'absolute path or relative to the configuration file.'), cfg.StrOpt( diff --git a/placement/policy.py b/placement/policy.py index 4696f78e6..58645d5c4 100644 --- a/placement/policy.py +++ b/placement/policy.py @@ -13,6 +13,7 @@ from oslo_config import cfg from oslo_log import log as logging +from oslo_policy import opts as policy_opts from oslo_policy import policy from oslo_utils import excutils @@ -36,13 +37,43 @@ def init(conf): """Init an Enforcer class. Sets the _ENFORCER global.""" global _ENFORCER if not _ENFORCER: - # NOTE(mriedem): We have to explicitly pass in the - # [placement]/policy_file path because otherwise oslo_policy defaults - # to read the policy file from config option [oslo_policy]/policy_file - # which is used by nova. In other words, to have separate policy files - # for placement and nova, we have to use separate policy_file options. - _enforcer = policy.Enforcer( - conf, policy_file=conf.placement.policy_file) + # TODO(mriedem): This compat code can be removed when the + # [placement]/policy_file config option is removed. + # First check to see if [oslo_policy]/policy_file exists since that's + # what we want people using. That option defaults to policy.json while + # [placement]/policy_file defaults to policy.yaml so if + # [oslo_policy]/policy_file does not exist it means either someone with + # custom policy has not migrated or they are using defaults in code. + if conf.find_file(conf.oslo_policy.policy_file): + # [oslo_policy]/policy_file exists so use it. + policy_file = conf.oslo_policy.policy_file + # Do a sanity check to see if [placement]/policy_file exists but + # with a different name because if so we could be loading up the + # wrong file. For example, maybe someone's packaging or deployment + # tooling creates an empty policy.json but placement.conf is + # actually configured to use [placement]/policy_file=policy.yaml + # with custom rules. + if (conf.placement.policy_file != conf.oslo_policy.policy_file and + conf.find_file(conf.placement.policy_file)): + LOG.error('Found [oslo_policy]/policy_file and ' + '[placement]/policy_file and not sure which to use. ' + 'Using [oslo_policy]/policy_file since ' + '[placement]/policy_file is deprecated but you need ' + 'to clean up your configuration file to stop using ' + '[placement]/policy_file.') + else: + # Check to see if a custom [placement]/policy_file is being used + # and if so, log a warning to migrate to [oslo_policy]/policy_file. + if conf.find_file(conf.placement.policy_file): + LOG.warning('[placement]/policy_file is deprecated. Use ' + '[oslo_policy]/policy_file instead.') + # For backward compatibility use [placement]/policy_file. Even if + # the file does not exist we can specify this since we will load up + # default rules from code. Once we remove the compat code we can + # just stop passing the policy_file kwarg to Enforcer. + policy_file = conf.placement.policy_file + + _enforcer = policy.Enforcer(conf, policy_file=policy_file) _enforcer.register_defaults(policies.list_rules()) _enforcer.load_rules() _ENFORCER = _enforcer @@ -53,6 +84,7 @@ def get_enforcer(): # files from overrides on disk and defaults in code. We can just pass an # empty list and let oslo do the config lifting for us. cfg.CONF([], project='placement') + policy_opts.set_defaults(cfg.CONF) return _get_enforcer(cfg.CONF) diff --git a/placement/tests/unit/policy_fixture.py b/placement/tests/unit/policy_fixture.py index 394365cf4..00dab91fe 100644 --- a/placement/tests/unit/policy_fixture.py +++ b/placement/tests/unit/policy_fixture.py @@ -29,9 +29,8 @@ class PolicyFixture(fixtures.Fixture): """Load the default placement policy for tests.""" def setUp(self): super(PolicyFixture, self).setUp() - policy_file = paths.state_path_def( - 'etc/placement/placement-policy.yaml') - self.conf_fixture.config(group='placement', policy_file=policy_file) + policy_file = paths.state_path_def('etc/placement/policy.yaml') + self.conf_fixture.config(group='oslo_policy', policy_file=policy_file) placement_policy.reset() placement_policy.init(self.conf_fixture.conf) self.addCleanup(placement_policy.reset) diff --git a/placement/tests/unit/test_policy.py b/placement/tests/unit/test_policy.py index 6dcf30101..0386f2f17 100644 --- a/placement/tests/unit/test_policy.py +++ b/placement/tests/unit/test_policy.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import mock import os import fixtures @@ -43,15 +44,22 @@ class PlacementPolicyTestCase(base.ContextTestCase): self.addCleanup(policy.reset) def test_modified_policy_reloads(self): - """Creates a temporary placement-policy.yaml file and tests + """Creates a temporary policy.yaml file and tests authorizations against a fake rule between updates to the physical policy file. """ tempdir = self.useFixture(fixtures.TempDir()) - tmpfilename = os.path.join(tempdir.path, 'placement-policy.yaml') + tmpfilename = os.path.join(tempdir.path, 'policy.yaml') self.conf_fixture.config( - group='placement', policy_file=tmpfilename) + group='oslo_policy', policy_file=tmpfilename) + + # We have to create the file before initializing the policy enforcer + # otherwise it falls back to using CONF.placement.policy_file. This + # can be removed when the deprecated CONF.placement.policy_file option + # is removed. + with open(tmpfilename, "w") as policyfile: + policyfile.write('# The policy file is empty.') action = 'placement:test' @@ -82,3 +90,122 @@ class PlacementPolicyTestCase(base.ContextTestCase): self.assertFalse( policy.authorize( self.ctxt, 'placement', self.target, do_raise=False)) + + @mock.patch('placement.policy.LOG.warning') + def test_default_fallback_placement_policy_file_no_exist(self, mock_warn): + """Tests that by default the policy enforcer will fallback to the + [placement]/policy_file when [oslo_policy]/policy_file does not + exist. In this case the placement policy file does not exist so no + warning about using it should be logged. + """ + # Make sure oslo_policy and placement use different policy_file + # defaults (the former uses policy.json, the latter uses policy.yaml). + config = self.conf_fixture.conf + self.assertNotEqual(config.oslo_policy.policy_file, + config.placement.policy_file) + enforcer = policy._get_enforcer(config) + self.assertEqual(config.placement.policy_file, enforcer.policy_file) + # There should not be a warning logged since the policy file does not + # actually exist. + mock_warn.assert_not_called() + + @mock.patch('placement.policy.LOG.warning') + def test_default_fallback_placement_policy_file(self, mock_warn): + """Tests that by default the policy enforcer will fallback to the + [placement]/policy_file when [oslo_policy]/policy_file does not + exist. In this case the plcaement policy file exists, like in the case + of using it to define custom rules, so a warning is logged. + """ + tempdir = self.useFixture(fixtures.TempDir()) + tmpfilename = os.path.join(tempdir.path, 'policy.yaml') + self.conf_fixture.config(group='placement', policy_file=tmpfilename) + # We have to create the file before initializing the policy enforcer + # otherwise it falls back to using CONF.placement.policy_file. This + # can be removed when the deprecated CONF.placement.policy_file option + # is removed. + with open(tmpfilename, "w") as policyfile: + policyfile.write('# I would normally have custom rules in here.') + config = self.conf_fixture.conf + enforcer = policy._get_enforcer(config) + self.assertEqual(config.placement.policy_file, enforcer.policy_file) + # There should not be a warning logged since the policy file does not + # actually exist. + mock_warn.assert_called_once_with( + '[placement]/policy_file is deprecated. Use ' + '[oslo_policy]/policy_file instead.') + + @mock.patch('placement.policy.LOG.error') + def test_init_from_oslo_policy_file_exists_same_policy_file_name( + self, mock_log_error): + """Tests a scenario where the [oslo_policy]/policy_file exists and + is the same name as the [placement]/policy_file so no error is logged + since we'll use the file from oslo_policy config. + """ + # Configure [oslo_policy]/policy_file and [placement]/policy_file with + # the same name. + tempdir = self.useFixture(fixtures.TempDir()) + tmpfilename = os.path.join(tempdir.path, 'policy.yaml') + self.conf_fixture.config(group='oslo_policy', policy_file=tmpfilename) + self.conf_fixture.config(group='placement', policy_file=tmpfilename) + # Create the [oslo_policy]/policy_file. + with open(tmpfilename, "w") as policyfile: + policyfile.write('# Assume upgrade with existing custom policy.') + config = self.conf_fixture.conf + policy._get_enforcer(config) + # Checking what the Enforcer is using for a policy file does not really + # matter too much since they are pointing at the same file, just make + # sure we did not log an error. + mock_log_error.assert_not_called() + + @mock.patch('placement.policy.LOG.error') + def test_init_from_oslo_file_exists_different_name_no_placement_file( + self, mock_log_error): + """Tests a scenario where the [oslo_policy]/policy_file exists and + has a different name from the [placement]/policy_file but the + [placement]/policy_file does not exist so no error is logged. + """ + # Configure [oslo_policy]/policy_file and [placement]/policy_file with + # different names. + tempdir = self.useFixture(fixtures.TempDir()) + tmpfilename = os.path.join(tempdir.path, 'policy.yaml') + self.conf_fixture.config(group='oslo_policy', policy_file=tmpfilename) + self.conf_fixture.config(group='placement', policy_file='policy.json') + # Create the [oslo_policy]/policy_file. + with open(tmpfilename, "w") as policyfile: + policyfile.write('# Assume upgrade with existing custom policy.') + config = self.conf_fixture.conf + enforcer = policy._get_enforcer(config) + self.assertEqual(config.oslo_policy.policy_file, enforcer.policy_file) + # Though the policy file names are different, the placement version + # does not exist while the oslo policy one does so no error is logged. + mock_log_error.assert_not_called() + + @mock.patch('placement.policy.LOG.error') + def test_init_from_oslo_file_exists_different_name_placement_file_exists( + self, mock_log_error): + """Tests a scenario where the [oslo_policy]/policy_file exists and + has a different name from the [placement]/policy_file and the + [placement]/policy_file exists so an error is logged. + """ + # Configure [oslo_policy]/policy_file and [placement]/policy_file with + # different names. + tempdir = self.useFixture(fixtures.TempDir()) + oslo_name = os.path.join(tempdir.path, 'policy.yaml') + self.conf_fixture.config(group='oslo_policy', policy_file=oslo_name) + placement_name = os.path.join(tempdir.path, 'placement-policy.yaml') + self.conf_fixture.config(group='placement', policy_file=placement_name) + # Create the [oslo_policy]/policy_file. + with open(oslo_name, "w") as oslo_policy_file: + oslo_policy_file.write('# New oslo policy config.') + # Create the [placement]/policy_file. + with open(placement_name, "w") as placement_policy_file: + placement_policy_file.write('# Old placement policy file.') + config = self.conf_fixture.conf + enforcer = policy._get_enforcer(config) + self.assertEqual(config.oslo_policy.policy_file, enforcer.policy_file) + # An error should be logged since we're going to use the oslo policy + # file but there is a placement policy file with a different name that + # also exists. + mock_log_error.assert_called_once() + self.assertIn('you need to clean up your configuration file', + mock_log_error.call_args[0][0]) diff --git a/placement/wsgi.py b/placement/wsgi.py index 655b79eb2..2e02b05bc 100644 --- a/placement/wsgi.py +++ b/placement/wsgi.py @@ -21,7 +21,6 @@ import os.path from oslo_config import cfg from oslo_log import log as logging from oslo_middleware import cors -from oslo_policy import opts as policy_opts from oslo_utils import importutils import pbr.version @@ -79,10 +78,6 @@ def _parse_args(config, argv, default_config_files): _set_middleware_defaults() - # This is needed so we can check [oslo_policy]/enforce_scope in the - # deploy module. - policy_opts.set_defaults(config) - config(argv[1:], project='placement', version=version_info.version_string(), default_config_files=default_config_files) diff --git a/releasenotes/notes/deprecate-placement-policy-file-1777dc2e92d8363c.yaml b/releasenotes/notes/deprecate-placement-policy-file-1777dc2e92d8363c.yaml new file mode 100644 index 000000000..bd499a892 --- /dev/null +++ b/releasenotes/notes/deprecate-placement-policy-file-1777dc2e92d8363c.yaml @@ -0,0 +1,11 @@ +--- +deprecations: + - | + The ``[placement]/policy_file`` configuration option is deprecated and its + usage is being replaced with the more standard + ``[oslo_policy]/policy_file`` option. If you do not override policy with + custom rules you will have nothing to do. If you do override the placement + default policy then you will need to update your configuration to use the + ``[oslo_policy]/policy_file`` option. By default, the + ``[oslo_policy]/policy_file`` option will be used if the file it points at + exists.