Deprecate [placement]/policy_file config option
The [placement]/policy_file option was necessary when placement was in nova since nova uses the standard [oslo_policy]/policy_file option for defining custom policy rules. Now that placement is extracted (+1 release) we can deprecate the placement-specific option and use the standard [oslo_policy]/policy_file option as well. The tricky thing with this is both options define a default value but those values are different, and neither need to exist or can exist but be empty and we'll use policy defaults from code. So some logic is necessary for detecting which option we should pass to the oslo.policy Enforcer object. We prefer to use [oslo_policy]/policy_file if it exists but will fallback to use [placement]/policy_file for backward compatibility. We also check for a couple of edge cases to try and detect misconfiguration and usage of the deprecated option. The config generation docs are updated to include the [oslo_policy] options as well as registering the options from that library for runtime code. Change-Id: Ifb14d2c14b17fc5bcdf7d124744ac2e1b58fd063 Story: #2005171 Task: #29913
This commit is contained in:
Matt Riedemann
parent
10146978ea
commit
9ece3d35ab
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
Loading…
Reference in New Issue