From 106b694460257f1c6acfefc1e0a92ad01c851407 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Fri, 22 May 2020 09:30:00 -0400 Subject: [PATCH] Add "yes" prompt for update/upgrades commands Update/Upgrade commands have now a prompt by default that ask for confirmation before proceeding. It'll prevent an user to run the command that may cause the problems to infrastructure. This prompt can be skipped with --yes/-y argument. Note: putting "UPDATE" and "UPGRADE" in uppercase to make sure this is visible and clear. We have seen many users running the wrong command and ending up doing an upgrade instead of an update. Note2: this prompt will be ported to the upgrade and FFWD workflows to prevent unexpected execution to prevent potential harm to infrastructures. Depends-On: https://review.opendev.org/741657 Change-Id: I838e6748879c668dd004ca2243b7b00b857c2a7b (cherry picked from commit 7a044ca2517d26ea4ea763cc76d373ad4b39f7f9) --- ...pgrade_update_prompt-f6ace53f02b62fa0.yaml | 7 ++++ tripleoclient/constants.py | 13 ++++-- tripleoclient/exceptions.py | 8 ++++ .../overcloud_update/test_overcloud_update.py | 18 ++++++-- .../test_overcloud_upgrade.py | 19 ++++++--- tripleoclient/v1/overcloud_external_update.py | 14 +++++++ .../v1/overcloud_external_upgrade.py | 15 +++++++ tripleoclient/v1/overcloud_update.py | 41 +++++++++++++++++++ tripleoclient/v1/overcloud_upgrade.py | 29 ++++++++++++- 9 files changed, 150 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/upgrade_update_prompt-f6ace53f02b62fa0.yaml diff --git a/releasenotes/notes/upgrade_update_prompt-f6ace53f02b62fa0.yaml b/releasenotes/notes/upgrade_update_prompt-f6ace53f02b62fa0.yaml new file mode 100644 index 000000000..3fa17f5d8 --- /dev/null +++ b/releasenotes/notes/upgrade_update_prompt-f6ace53f02b62fa0.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + The upgrade/update commands have a prompt by default now that ask for + confirmation before proceeding. It'll prevent an operator to run the + command and cause the problems to infrastructure. + This prompt can be skipped with --yes/-y argument. diff --git a/tripleoclient/constants.py b/tripleoclient/constants.py index 7a83af1a7..51ac2fcf9 100644 --- a/tripleoclient/constants.py +++ b/tripleoclient/constants.py @@ -196,10 +196,17 @@ UNDERCLOUD_EXTRA_PACKAGES = [ "tripleo-ansible" ] -# UPGRADE_PROMPT -UPGRADE_PROMPT = _('It is strongly recommended to perform a backup ' +UPGRADE_PROMPT = _('You are about to run a UPGRADE command. ' + 'It is strongly recommended to perform a backup ' 'before the upgrade. Are you sure you want to ' 'upgrade [y/N]?') UPGRADE_NO = _('User did not confirm upgrade, so exiting. ' - 'Consider using the --yes parameter if you ' + 'Consider using the --yes/-y parameter if you ' 'prefer to skip this warning in the future') +UPDATE_PROMPT = _('You are about to run a UPDATE command. ' + 'It is strongly recommended to perform a backup ' + 'before the update. Are you sure you want to ' + 'update [y/N]?') +UPDATE_NO = _('User did not confirm update, so exiting. ' + 'Consider using the --yes/-y parameter if you ' + 'prefer to skip this warning in the future') diff --git a/tripleoclient/exceptions.py b/tripleoclient/exceptions.py index b6840a117..9141c3e11 100644 --- a/tripleoclient/exceptions.py +++ b/tripleoclient/exceptions.py @@ -121,6 +121,14 @@ class UndercloudUpgradeNotConfirmed(Base): """Undercloud upgrade security question not confirmed.""" +class OvercloudUpdateNotConfirmed(Base): + """Overcloud Update security question not confirmed.""" + + +class OvercloudUpgradeNotConfirmed(Base): + """Overcloud Update security question not confirmed.""" + + class CellExportError(Base): """Cell export failed""" diff --git a/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py b/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py index c435fe574..1ef4231e6 100644 --- a/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py +++ b/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py @@ -36,6 +36,8 @@ class TestOvercloudUpdatePrepare(fakes.TestOvercloudUpdatePrepare): self.mock_uuid4 = uuid4_patcher.start() self.addCleanup(self.mock_uuid4.stop) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch('tripleoclient.v1.overcloud_deploy.DeployOvercloud.' '_get_undercloud_host_entry', autospec=True, return_value='192.168.0.1 uc.ctlplane.localhost uc.ctlplane') @@ -53,7 +55,8 @@ class TestOvercloudUpdatePrepare(fakes.TestOvercloudUpdatePrepare): '_deploy_tripleo_heat_templates_tmpdir', autospec=True) def test_update_out(self, mock_deploy, mock_open, mock_copy, mock_yaml, mock_abspath, mock_update, mock_logger, - mock_get_stack, mock_get_undercloud_host_entry): + mock_get_stack, mock_get_undercloud_host_entry, + mock_confirm): mock_stack = mock.Mock(parameters={'DeployIdentifier': ''}) mock_stack.stack_name = 'overcloud' mock_get_stack.return_value = mock_stack @@ -77,6 +80,8 @@ class TestOvercloudUpdatePrepare(fakes.TestOvercloudUpdatePrepare): container='overcloud', ) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch('tripleoclient.utils.get_stack', autospec=True) @mock.patch('tripleoclient.workflows.package_update.update', @@ -89,7 +94,7 @@ class TestOvercloudUpdatePrepare(fakes.TestOvercloudUpdatePrepare): '_deploy_tripleo_heat_templates', autospec=True) def test_update_failed(self, mock_deploy, mock_copy, mock_yaml, mock_abspath, mock_open, mock_update, - mock_get_stack): + mock_get_stack, mock_confirm): mock_stack = mock.Mock(parameters={'DeployIdentifier': ''}) mock_stack.stack_name = 'overcloud' mock_get_stack.return_value = mock_stack @@ -124,13 +129,16 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): self.mock_uuid4 = uuid4_patcher.start() self.addCleanup(self.mock_uuid4.stop) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch('tripleoclient.utils.run_ansible_playbook', autospec=True) @mock.patch('os.path.expanduser') @mock.patch('oslo_concurrency.processutils.execute') @mock.patch('six.moves.builtins.open') def test_update_with_no_limit( - self, mock_open, mock_execute, mock_expanduser, update_ansible): + self, mock_open, mock_execute, mock_expanduser, update_ansible, + mock_confirm): mock_expanduser.return_value = '/home/fake/' argslist = [] verifylist = [ @@ -151,9 +159,11 @@ class TestOvercloudUpdateConverge(fakes.TestOvercloudUpdateConverge): app_args.verbose_level = 1 self.cmd = overcloud_update.UpdateConverge(self.app, app_args) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch( 'tripleoclient.v1.overcloud_deploy.DeployOvercloud.take_action') - def test_update_converge(self, deploy_action): + def test_update_converge(self, deploy_action, mock_confirm): argslist = ['--templates', '--stack', 'cloud'] verifylist = [ ('stack', 'cloud') diff --git a/tripleoclient/tests/v1/overcloud_upgrade/test_overcloud_upgrade.py b/tripleoclient/tests/v1/overcloud_upgrade/test_overcloud_upgrade.py index 260e5ae61..4418ff470 100644 --- a/tripleoclient/tests/v1/overcloud_upgrade/test_overcloud_upgrade.py +++ b/tripleoclient/tests/v1/overcloud_upgrade/test_overcloud_upgrade.py @@ -37,7 +37,8 @@ class TestOvercloudUpgradePrepare(fakes.TestOvercloudUpgradePrepare): uuid4_patcher = mock.patch('uuid.uuid4', return_value="UUID4") self.mock_uuid4 = uuid4_patcher.start() self.addCleanup(self.mock_uuid4.stop) - + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch('tripleoclient.v1.overcloud_deploy.DeployOvercloud.' 'take_action') @mock.patch('tripleoclient.workflows.deployment.' @@ -56,7 +57,8 @@ class TestOvercloudUpgradePrepare(fakes.TestOvercloudUpgradePrepare): mock_get_stack, add_env, mock_enable_ssh_admin, - mock_overcloud_deploy): + mock_overcloud_deploy, + mock_confirm): mock_stack = mock.Mock(parameters={'DeployIdentifier': ''}) mock_stack.stack_name = 'overcloud' @@ -90,6 +92,8 @@ class TestOvercloudUpgradePrepare(fakes.TestOvercloudUpgradePrepare): mock.ANY ) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch('tripleoclient.v1.overcloud_deploy.DeployOvercloud.' 'take_action') @mock.patch('tripleoclient.utils.get_stack', @@ -98,7 +102,8 @@ class TestOvercloudUpgradePrepare(fakes.TestOvercloudUpgradePrepare): @mock.patch('six.moves.builtins.open') @mock.patch('yaml.safe_load') def test_upgrade_failed(self, mock_yaml, mock_open, - add_env, mock_get_stack, mock_overcloud_deploy): + add_env, mock_get_stack, mock_overcloud_deploy, + mock_confirm): mock_overcloud_deploy.side_effect = exceptions.DeploymentError() mock_yaml.return_value = {'fake_container': 'fake_value'} mock_stack = mock.Mock(parameters={'DeployIdentifier': ''}) @@ -142,6 +147,8 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun): self.mock_uuid4 = uuid4_patcher.start() self.addCleanup(self.mock_uuid4.stop) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch( 'ansible_runner.runner_config.RunnerConfig', autospec=True, @@ -158,7 +165,7 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun): @mock.patch('six.moves.builtins.open') def test_upgrade_limit_with_playbook_and_user( self, mock_open, mock_execute, mock_expanduser, upgrade_ansible, - mock_run, mock_run_prepare): + mock_run, mock_run_prepare, mock_confirm): mock_expanduser.return_value = '/home/fake/' argslist = ['--limit', 'Compute, Controller', '--playbook', 'fake-playbook1.yaml', @@ -171,6 +178,8 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun): self.check_parser(self.cmd, argslist, verifylist) + @mock.patch('tripleoclient.utils.prompt_user_for_confirmation', + return_value=True) @mock.patch( 'ansible_runner.runner_config.RunnerConfig', autospec=True, @@ -187,7 +196,7 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun): @mock.patch('six.moves.builtins.open') def test_upgrade_nodes_with_playbook_no_skip_tags( self, mock_open, mock_execute, mock_expanduser, upgrade_ansible, - mock_run, mock_run_prepare): + mock_run, mock_run_prepare, mock_confirm): mock_expanduser.return_value = '/home/fake/' argslist = ['--limit', 'compute-0,compute-1', '--playbook', 'fake-playbook.yaml', ] diff --git a/tripleoclient/v1/overcloud_external_update.py b/tripleoclient/v1/overcloud_external_update.py index 73cd38712..0445f6ba3 100644 --- a/tripleoclient/v1/overcloud_external_update.py +++ b/tripleoclient/v1/overcloud_external_update.py @@ -19,6 +19,8 @@ from oslo_log import log as logging from osc_lib.i18n import _ from osc_lib import utils +from tripleoclient.exceptions import OvercloudUpdateNotConfirmed + from tripleoclient import command from tripleoclient import constants from tripleoclient import utils as oooutils @@ -91,6 +93,12 @@ class ExternalUpdateRun(command.Command): default=True, help=_('This option no longer has any effect.') ) + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any upgrade " + "operation. Use this with caution! "), + ) parser.add_argument( '--limit', action='store', @@ -105,6 +113,12 @@ class ExternalUpdateRun(command.Command): def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPDATE_PROMPT, self.log)): + raise OvercloudUpdateNotConfirmed(constants.UPDATE_NO) + _, ansible_dir = self.get_ansible_key_and_dir( no_workflow=True, stack=parsed_args.stack, diff --git a/tripleoclient/v1/overcloud_external_upgrade.py b/tripleoclient/v1/overcloud_external_upgrade.py index c22241965..c17867138 100644 --- a/tripleoclient/v1/overcloud_external_upgrade.py +++ b/tripleoclient/v1/overcloud_external_upgrade.py @@ -18,6 +18,8 @@ from oslo_log import log as logging from osc_lib.i18n import _ from osc_lib import utils +from tripleoclient.exceptions import OvercloudUpgradeNotConfirmed + from tripleoclient import command from tripleoclient import constants from tripleoclient import utils as oooutils @@ -90,6 +92,13 @@ class ExternalUpgradeRun(command.Command): default=True, help=_('This option no longer has any effect.') ) + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any upgrade " + "operation. Use this with caution! "), + ) + parser.add_argument( '--limit', action='store', @@ -104,6 +113,12 @@ class ExternalUpgradeRun(command.Command): def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPGRADE_PROMPT, self.log)): + raise OvercloudUpgradeNotConfirmed(constants.UPGRADE_NO) + _, ansible_dir = self.get_ansible_key_and_dir( no_workflow=True, stack=parsed_args.stack, diff --git a/tripleoclient/v1/overcloud_update.py b/tripleoclient/v1/overcloud_update.py index 668471f0e..02605a1ba 100644 --- a/tripleoclient/v1/overcloud_update.py +++ b/tripleoclient/v1/overcloud_update.py @@ -18,6 +18,8 @@ from oslo_log import log as logging from osc_lib.i18n import _ from osc_lib import utils +from tripleoclient.exceptions import OvercloudUpdateNotConfirmed + from tripleoclient import command from tripleoclient import constants from tripleoclient import utils as oooutils @@ -43,10 +45,22 @@ class UpdatePrepare(DeployOvercloud): def get_parser(self, prog_name): parser = super(UpdatePrepare, self).get_parser(prog_name) + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any update operation. " + "Use this with caution! "), + ) return parser def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPDATE_PROMPT, self.log)): + raise OvercloudUpdateNotConfirmed(constants.UPDATE_NO) + clients = self.app.client_manager stack = oooutils.get_stack(clients.orchestration, @@ -147,10 +161,22 @@ class UpdateRun(command.Command): help=_('A list of tags to skip when running the the' ' config-download ansible-playbook command.') ) + parser.add_argument( + '-y', '--yes', + default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation required before " + "any update operation. Use this with caution! "), + ) return parser def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPDATE_PROMPT, self.log)): + raise OvercloudUpdateNotConfirmed(constants.UPDATE_NO) # NOTE(cloudnull): The string option "all" was a special default # that is no longer relevant. To retain compatibility # this condition has been put in place. @@ -198,9 +224,24 @@ class UpdateConverge(DeployOvercloud): log = logging.getLogger(__name__ + ".UpdateConverge") + def get_parser(self, prog_name): + parser = super(UpdateConverge, self).get_parser(prog_name) + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any update operation. " + "Use this with caution! "), + ) + return parser + def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPDATE_PROMPT, self.log)): + raise OvercloudUpdateNotConfirmed(constants.UPDATE_NO) + # Add the update-converge.yaml environment to unset noops templates_dir = (parsed_args.templates or constants.TRIPLEO_HEAT_TEMPLATES) diff --git a/tripleoclient/v1/overcloud_upgrade.py b/tripleoclient/v1/overcloud_upgrade.py index a7914814f..dd713bfcb 100644 --- a/tripleoclient/v1/overcloud_upgrade.py +++ b/tripleoclient/v1/overcloud_upgrade.py @@ -18,6 +18,8 @@ from oslo_log import log as logging from osc_lib.i18n import _ from osc_lib import utils +from tripleoclient.exceptions import OvercloudUpgradeNotConfirmed + from tripleoclient import command from tripleoclient import constants from tripleoclient import exceptions @@ -49,11 +51,22 @@ class UpgradePrepare(DeployOvercloud): def get_parser(self, prog_name): parser = super(UpgradePrepare, self).get_parser(prog_name) + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any upgrade " + "operation. Use this with caution! "), + ) return parser def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPGRADE_PROMPT, self.log)): + raise OvercloudUpgradeNotConfirmed(constants.UPGRADE_NO) + # Throw deprecation warning if service is enabled and # ask user if upgrade should still be continued. if parsed_args.environment_files: @@ -178,13 +191,19 @@ class UpgradeRun(command.Command): help=_('Name or ID of heat stack ' '(default=Env: OVERCLOUD_STACK_NAME)'), default=utils.env('OVERCLOUD_STACK_NAME', - default='overcloud')) + default='overcloud') + ) parser.add_argument('--no-workflow', dest='no_workflow', action='store_true', default=True, help=_('This option no longer has any effect.') ) - + parser.add_argument('-y', '--yes', default=False, + action='store_true', + help=_("Use -y or --yes to skip the confirmation " + "required before any upgrade " + "operation. Use this with caution! ") + ) return parser def _validate_skip_tags(self, skip_tags): @@ -199,6 +218,12 @@ class UpgradeRun(command.Command): def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) + + if (not parsed_args.yes + and not oooutils.prompt_user_for_confirmation( + constants.UPGRADE_PROMPT, self.log)): + raise OvercloudUpgradeNotConfirmed(constants.UPGRADE_NO) + # NOTE(cloudnull): The string option "all" was a special default # that is no longer relevant. To retain compatibility # this condition has been put in place.