From 892467dc9bc18a7174965484cd94fd7bd38cb5fd Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Thu, 1 Jun 2017 09:35:28 +0300 Subject: [PATCH] Move deploy_utils warnings to conductor start make these checks once during conductor start after config was parsed. This will prevent issuing those warnings on module imports. Without it sphinx-build is broken when warnings are treated as failures. Change-Id: I455bcc45edae82632aaf4eaebdcd597440d33ad3 Closes-Bug: #1694724 --- ironic/cmd/conductor.py | 28 ++++++++- ironic/drivers/modules/deploy_utils.py | 21 ------- ironic/tests/unit/cmd/test_conductor.py | 57 +++++++++++++++++++ .../unit/drivers/modules/test_deploy_utils.py | 31 ---------- 4 files changed, 83 insertions(+), 54 deletions(-) create mode 100644 ironic/tests/unit/cmd/test_conductor.py diff --git a/ironic/cmd/conductor.py b/ironic/cmd/conductor.py index 208ceb0cb6..6a8ddd41f4 100644 --- a/ironic/cmd/conductor.py +++ b/ironic/cmd/conductor.py @@ -40,7 +40,7 @@ SECTIONS_WITH_AUTH = ( # TODO(pas-ha) remove this check after deprecation period -def _check_auth_options(conf): +def check_auth_options(conf): missing = [] for section in SECTIONS_WITH_AUTH: if not auth.load_auth(conf, section): @@ -59,6 +59,30 @@ def _check_auth_options(conf): link=link)) +def warn_about_unsafe_shred_parameters(conf): + iterations = conf.deploy.shred_random_overwrite_iterations + overwrite_with_zeros = conf.deploy.shred_final_overwrite_with_zeros + if iterations == 0 and overwrite_with_zeros is False: + LOG.warning('With shred_random_overwrite_iterations set to 0 and ' + 'shred_final_overwrite_with_zeros set to False, disks ' + 'may NOT be shredded at all, unless they support ATA ' + 'Secure Erase. This is a possible SECURITY ISSUE!') + + +def warn_about_missing_default_boot_option(conf): + if not conf.deploy.default_boot_option: + LOG.warning('The default value of default_boot_option ' + 'configuration will change eventually from ' + '"netboot" to "local". It is recommended to set ' + 'an explicit value for it during the transition period') + + +def issue_startup_warnings(conf): + check_auth_options(conf) + warn_about_unsafe_shred_parameters(conf) + warn_about_missing_default_boot_option(conf) + + def main(): # NOTE(lucasagomes): Safeguard to prevent 'ironic.conductor.manager' # from being imported prior to the configuration options being loaded. @@ -77,7 +101,7 @@ def main(): 'ironic.conductor.manager', 'ConductorManager') - _check_auth_options(CONF) + issue_startup_warnings(CONF) launcher = service.launch(CONF, mgr) launcher.wait() diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 133b0355bc..a3c2c56dcc 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -68,27 +68,6 @@ SUPPORTED_CAPABILITIES = { DISK_LAYOUT_PARAMS = ('root_gb', 'swap_mb', 'ephemeral_gb') -def warn_about_unsafe_shred_parameters(): - iterations = CONF.deploy.shred_random_overwrite_iterations - overwrite_with_zeros = CONF.deploy.shred_final_overwrite_with_zeros - if iterations == 0 and overwrite_with_zeros is False: - LOG.warning('With shred_random_overwrite_iterations set to 0 and ' - 'shred_final_overwrite_with_zeros set to False, disks ' - 'may NOT be shredded at all, unless they support ATA ' - 'Secure Erase. This is a possible SECURITY ISSUE!') - - -def warn_about_missing_default_boot_option(): - if not CONF.deploy.default_boot_option: - LOG.warning('The default value of default_boot_option ' - 'configuration will change eventually from ' - '"netboot" to "local". It is recommended to set ' - 'an explicit value for it during the transition period') - - -warn_about_unsafe_shred_parameters() -warn_about_missing_default_boot_option() - # All functions are called from deploy() directly or indirectly. # They are split for stub-out. diff --git a/ironic/tests/unit/cmd/test_conductor.py b/ironic/tests/unit/cmd/test_conductor.py new file mode 100644 index 0000000000..882b9351bc --- /dev/null +++ b/ironic/tests/unit/cmd/test_conductor.py @@ -0,0 +1,57 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock +from oslo_config import cfg + +from ironic.cmd import conductor +from ironic.tests.unit.db import base + + +class ConductorStartTestCase(base.DbTestCase): + + @mock.patch.object(conductor, 'LOG', autospec=True) + def test_warn_about_unsafe_shred_parameters_defaults(self, log_mock): + conductor.warn_about_unsafe_shred_parameters(cfg.CONF) + self.assertFalse(log_mock.warning.called) + + @mock.patch.object(conductor, 'LOG', autospec=True) + def test_warn_about_unsafe_shred_parameters_zeros(self, log_mock): + cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy') + cfg.CONF.set_override('shred_final_overwrite_with_zeros', True, + 'deploy') + conductor.warn_about_unsafe_shred_parameters(cfg.CONF) + self.assertFalse(log_mock.warning.called) + + @mock.patch.object(conductor, 'LOG', autospec=True) + def test_warn_about_unsafe_shred_parameters_random_no_zeros(self, + log_mock): + cfg.CONF.set_override('shred_random_overwrite_iterations', 1, 'deploy') + cfg.CONF.set_override('shred_final_overwrite_with_zeros', False, + 'deploy') + conductor.warn_about_unsafe_shred_parameters(cfg.CONF) + self.assertFalse(log_mock.warning.called) + + @mock.patch.object(conductor, 'LOG', autospec=True) + def test_warn_about_unsafe_shred_parameters_produces_a_warning(self, + log_mock): + cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy') + cfg.CONF.set_override('shred_final_overwrite_with_zeros', False, + 'deploy') + conductor.warn_about_unsafe_shred_parameters(cfg.CONF) + self.assertTrue(log_mock.warning.called) + + @mock.patch.object(conductor, 'LOG', autospec=True) + def test_warn_on_missing_default_boot_option(self, log_mock): + conductor.warn_about_missing_default_boot_option(cfg.CONF) + self.assertTrue(log_mock.warning.called) diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 4aef3648cc..8ff0a0d8a1 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -1177,37 +1177,6 @@ class OtherFunctionTestCase(db_base.DbTestCase): mock_clean_up_caches.assert_called_once_with(None, 'master_dir', [('uuid', 'path')]) - @mock.patch.object(utils, 'LOG', autospec=True) - def test_warn_about_unsafe_shred_parameters_defaults(self, log_mock): - utils.warn_about_unsafe_shred_parameters() - self.assertFalse(log_mock.warning.called) - - @mock.patch.object(utils, 'LOG', autospec=True) - def test_warn_about_unsafe_shred_parameters_zeros(self, log_mock): - cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy') - cfg.CONF.set_override('shred_final_overwrite_with_zeros', True, - 'deploy') - utils.warn_about_unsafe_shred_parameters() - self.assertFalse(log_mock.warning.called) - - @mock.patch.object(utils, 'LOG', autospec=True) - def test_warn_about_unsafe_shred_parameters_random_no_zeros(self, - log_mock): - cfg.CONF.set_override('shred_random_overwrite_iterations', 1, 'deploy') - cfg.CONF.set_override('shred_final_overwrite_with_zeros', False, - 'deploy') - utils.warn_about_unsafe_shred_parameters() - self.assertFalse(log_mock.warning.called) - - @mock.patch.object(utils, 'LOG', autospec=True) - def test_warn_about_unsafe_shred_parameters_produces_a_warning(self, - log_mock): - cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy') - cfg.CONF.set_override('shred_final_overwrite_with_zeros', False, - 'deploy') - utils.warn_about_unsafe_shred_parameters() - self.assertTrue(log_mock.warning.called) - @mock.patch.object(utils, '_get_ironic_session') @mock.patch('ironic.common.keystone.get_service_url') def test_get_ironic_api_url_from_config(self, mock_get_url, mock_ks):