From 8c6355f3ccb64cb949a8280b6cd98a37e13bfaa7 Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Tue, 26 Mar 2019 16:56:30 -0500 Subject: [PATCH] Add upgrade check for presence of policy.json file Correct file is policy.yaml and only needed if overriding defaults. This only warns if the file is present in case it was left by a previous version and not actually needed or used. Also checks for the policy_file being overridden in config, and if so, warns if the specified file does not exist. Change-Id: Ia46e843ad245cf8263b97b773fac9bc6c6fe6647 Signed-off-by: Sean McGinnis --- cinder/cmd/status.py | 68 ++++++++++++++++++- cinder/tests/unit/cmd/test_status.py | 62 +++++++++++++++-- doc/source/cli/cinder-status.rst | 2 + ...tus-check-policyjson-ef61826eab95372b.yaml | 7 ++ 4 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/cinder-status-check-policyjson-ef61826eab95372b.yaml diff --git a/cinder/cmd/status.py b/cinder/cmd/status.py index d0c3b43fd3a..f6a88caac57 100644 --- a/cinder/cmd/status.py +++ b/cinder/cmd/status.py @@ -15,14 +15,15 @@ """CLI interface for cinder status commands.""" +import os import sys from oslo_config import cfg from oslo_upgradecheck import upgradecheck as uc +from cinder.policy import DEFAULT_POLICY_FILENAME import cinder.service # noqa - CONF = cfg.CONF SUCCESS = uc.Code.SUCCESS @@ -33,6 +34,10 @@ WARNING = uc.Code.WARNING class Checks(uc.UpgradeCommands): """Upgrade checks to run.""" + def _file_exists(self, path): + """Helper for mocking check of os.path.exists.""" + return os.path.exists(path) + def _check_backup_module(self): """Checks for the use of backup driver module paths. @@ -55,14 +60,71 @@ class Checks(uc.UpgradeCommands): return uc.Result(SUCCESS) + def _check_policy_file(self): + """Checks if a policy.json file is present. + + With the switch to policy-in-code, policy files should be policy.yaml + and should only be present if overriding default policy. Just checks + and warns if the old file is present to make sure they are aware it is + not being used. + """ + # make sure we know where to look for the policy file + config_dir = CONF.find_file('cinder.conf') + if not config_dir: + return uc.Result( + WARNING, + 'Cannot locate your cinder configuration directory. ' + 'Please re-run using the --config-dir option.') + + policy_file = CONF.oslo_policy.policy_file + json_file = os.path.join(os.path.dirname(config_dir), 'policy.json') + + if policy_file == DEFAULT_POLICY_FILENAME: + # Default is being used, check for old json file + if self._file_exists(json_file): + return uc.Result( + WARNING, + 'policy.json file is present. Make sure any changes from ' + 'the default policies are present in a policy.yaml file ' + 'instead. If you really intend to use a policy.json file, ' + 'make sure that its absolute path is set as the value of ' + "the 'policy_file' configuration option in the " + '[oslo_policy] section of your cinder.conf file.') + + else: + # They have configured a custom policy file. It is OK if it does + # not exist, but we should check and warn about it while we're + # checking. + if not policy_file.startswith('/'): + # policy_file is relative to config_dir + policy_file = os.path.join(os.path.dirname(config_dir), + policy_file) + if not self._file_exists(policy_file): + return uc.Result( + WARNING, + "Configured policy file '%s' does not exist. This may be " + "expected, but default policies will be used until any " + "desired overrides are added to the configured file." % + policy_file) + + return uc.Result(SUCCESS) + _upgrade_checks = ( ('Backup Driver Path', _check_backup_module), + ('Use of Policy File', _check_policy_file), ) def main(): - return uc.main(CONF, 'cinder', Checks()) - + # TODO(rosmaita): need to do this because we suggest using the + # --config-dir option, and if the user gives a bogus value, we + # get a stacktrace. Needs to be fixed in oslo_upgradecheck + try: + return uc.main(CONF, 'cinder', Checks()) + except cfg.ConfigDirNotFoundError: + return('ERROR: cannot read the cinder configuration directory.\n' + 'Please re-run using the --config-dir option ' + 'with a valid cinder configuration directory.') if __name__ == '__main__': sys.exit(main()) diff --git a/cinder/tests/unit/cmd/test_status.py b/cinder/tests/unit/cmd/test_status.py index c5759cff979..647abff7a65 100644 --- a/cinder/tests/unit/cmd/test_status.py +++ b/cinder/tests/unit/cmd/test_status.py @@ -12,6 +12,7 @@ """Unit tests for the cinder-status CLI interfaces.""" +import mock from oslo_config import cfg from oslo_upgradecheck import upgradecheck as uc import testtools @@ -28,18 +29,69 @@ class TestCinderStatus(testtools.TestCase): super(TestCinderStatus, self).setUp() self.checks = status.Checks() - def _set_backup_driver(self, driver_path): - CONF.set_override('backup_driver', driver_path) - self.addCleanup(CONF.clear_override, 'backup_driver') + # Make sure configuration is initialized + try: + CONF([], project='cinder') + except cfg.RequiredOptError: + # Doesn't matter in this situation + pass + + # Make sure our expected path is returned + patcher = mock.patch.object(CONF, 'find_file') + self.addCleanup(patcher.stop) + self.find_file = patcher.start() + self.find_file.return_value = '/etc/cinder/' + + def _set_config(self, key, value, group=None): + CONF.set_override(key, value, group=group) + self.addCleanup(CONF.clear_override, key, group=group) def test_check_backup_module(self): - self._set_backup_driver( + self._set_config( + 'backup_driver', 'cinder.backup.drivers.swift.SwiftBackupDriver') result = self.checks._check_backup_module() self.assertEqual(uc.Code.SUCCESS, result.code) def test_check_backup_module_not_class(self): - self._set_backup_driver('cinder.backup.drivers.swift') + self._set_config('backup_driver', 'cinder.backup.drivers.swift') result = self.checks._check_backup_module() self.assertEqual(uc.Code.FAILURE, result.code) self.assertIn('requires the full path', result.details) + + def test_check_policy_file(self): + with mock.patch.object(self.checks, '_file_exists') as fe: + fe.return_value = False + result = self.checks._check_policy_file() + + self.assertEqual(uc.Code.SUCCESS, result.code) + + def test_check_policy_file_exists(self): + with mock.patch.object(self.checks, '_file_exists') as fe: + fe.return_value = True + result = self.checks._check_policy_file() + + self.assertEqual(uc.Code.WARNING, result.code) + self.assertIn('policy.json file is present', result.details) + + def test_check_policy_file_custom_path(self): + policy_path = '/my/awesome/configs/policy.yaml' + self._set_config('policy_file', policy_path, group='oslo_policy') + with mock.patch.object(self.checks, '_file_exists') as fe: + fe.return_value = False + result = self.checks._check_policy_file() + fe.assert_called_with(policy_path) + + self.assertEqual(uc.Code.WARNING, result.code) + self.assertIn(policy_path, result.details) + + def test_check_policy_file_custom_file(self): + policy_path = 'mypolicy.yaml' + self._set_config('policy_file', policy_path, group='oslo_policy') + with mock.patch.object(self.checks, '_file_exists') as fe: + fe.return_value = False + result = self.checks._check_policy_file() + fe.assert_called_with('/etc/cinder/%s' % policy_path) + + self.assertEqual(uc.Code.WARNING, result.code) + self.assertIn(policy_path, result.details) diff --git a/doc/source/cli/cinder-status.rst b/doc/source/cli/cinder-status.rst index 9da190290c4..fb7d835a284 100644 --- a/doc/source/cli/cinder-status.rst +++ b/doc/source/cli/cinder-status.rst @@ -87,6 +87,8 @@ Upgrade * Check added to ensure the backup_driver setting is using the full driver class path and not just the module path. + * Checks for the presence of a **policy.json** file have been added to warn + if policy changes should be present in a **policy.yaml** file. See Also ======== diff --git a/releasenotes/notes/cinder-status-check-policyjson-ef61826eab95372b.yaml b/releasenotes/notes/cinder-status-check-policyjson-ef61826eab95372b.yaml new file mode 100644 index 00000000000..7074ee8ad4d --- /dev/null +++ b/releasenotes/notes/cinder-status-check-policyjson-ef61826eab95372b.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - | + A warning has been added to the ``cinder-status upgrade check`` CLI if a + ``policy.json`` file is present. Documentation has been updated to + correct the file as ``policy.yaml`` if any policies need to be changed from + their defaults.