From 6721fd7c80ed2b97a23bd27090c53a0c5a775998 Mon Sep 17 00:00:00 2001 From: "Eric D. Helms" Date: Mon, 3 Apr 2023 10:52:10 -0400 Subject: [PATCH] Require steps to be explicitly marked as skippable for whitelisting This makes a change to whitelist logic to require steps to be explicitly marked as 'skippable' in the code to be whitelistable by users. Previously, everything was assumed to be skippable and steps had to be marked that they could not be whitelisted. This can lead to steps in processes like upgrades being skipped by users when they should not be causing more problems. This may have an impact on users used to skipping certain steps, but will enforce more forethought on what a user should be able to skip. --- .../foreman/check_tuning_requirements.rb | 1 - .../repositories/check_upstream_repository.rb | 1 + definitions/checks/repositories/validate.rb | 2 + definitions/procedures/content/prepare.rb | 1 - definitions/procedures/content/switchover.rb | 1 - definitions/procedures/repositories/setup.rb | 1 + lib/foreman_maintain/concerns/metadata.rb | 4 +- lib/foreman_maintain/reporter/cli_reporter.rb | 8 +- lib/foreman_maintain/runner.rb | 10 +++ .../foreman/check_tuning_profile_test.rb | 4 - test/lib/cli/upgrade_command_test.rb | 1 + test/lib/reporter_test.rb | 4 +- test/lib/support/definitions/checks/dummy.rb | 2 +- .../definitions/checks/service_is_stopped.rb | 2 +- .../definitions/features/present_service.rb | 2 +- test/lib/test_helper.rb | 4 +- test/lib/upgrade_runner_test.rb | 86 ++++++++----------- 17 files changed, 62 insertions(+), 72 deletions(-) diff --git a/definitions/checks/foreman/check_tuning_requirements.rb b/definitions/checks/foreman/check_tuning_requirements.rb index 00c639c5e..5ea777d64 100644 --- a/definitions/checks/foreman/check_tuning_requirements.rb +++ b/definitions/checks/foreman/check_tuning_requirements.rb @@ -8,7 +8,6 @@ class TuningRequirements < ForemanMaintain::Check confine do feature(:katello) end - do_not_whitelist end def run diff --git a/definitions/checks/repositories/check_upstream_repository.rb b/definitions/checks/repositories/check_upstream_repository.rb index 59ad3f5b1..edea3f515 100644 --- a/definitions/checks/repositories/check_upstream_repository.rb +++ b/definitions/checks/repositories/check_upstream_repository.rb @@ -5,6 +5,7 @@ class Checks::CheckUpstreamRepository < ForemanMaintain::Check label :check_upstream_repository description 'Check if any upstream repositories are enabled on system' tags :pre_upgrade + skippable preparation_steps do [Checks::Repositories::CheckNonRhRepository.new, Procedures::Packages::Install.new(:packages => %w[yum-utils])] diff --git a/definitions/checks/repositories/validate.rb b/definitions/checks/repositories/validate.rb index 5c195053f..9a7145aa7 100644 --- a/definitions/checks/repositories/validate.rb +++ b/definitions/checks/repositories/validate.rb @@ -2,6 +2,8 @@ module Checks::Repositories class Validate < ForemanMaintain::Check metadata do description 'Validate availability of repositories' + skippable + preparation_steps do Checks::Repositories::CheckNonRhRepository.new end diff --git a/definitions/procedures/content/prepare.rb b/definitions/procedures/content/prepare.rb index 10bb07207..fb6312bfa 100644 --- a/definitions/procedures/content/prepare.rb +++ b/definitions/procedures/content/prepare.rb @@ -4,7 +4,6 @@ class Prepare < ForemanMaintain::Procedure description 'Prepare content for Pulp 3' for_feature :pulpcore param :quiet, 'Keep the output on a single line', :flag => true, :default => false - do_not_whitelist end def run diff --git a/definitions/procedures/content/switchover.rb b/definitions/procedures/content/switchover.rb index 47b3cf5d9..5b42e43c5 100644 --- a/definitions/procedures/content/switchover.rb +++ b/definitions/procedures/content/switchover.rb @@ -9,7 +9,6 @@ class Switchover < ForemanMaintain::Procedure end param :skip_deb, 'Do not run debian options in installer.' - do_not_whitelist end def run diff --git a/definitions/procedures/repositories/setup.rb b/definitions/procedures/repositories/setup.rb index 890a120d7..4360a4b3f 100644 --- a/definitions/procedures/repositories/setup.rb +++ b/definitions/procedures/repositories/setup.rb @@ -2,6 +2,7 @@ module Procedures::Repositories class Setup < ForemanMaintain::Procedure metadata do description 'Setup repositories' + skippable confine do feature(:instance).downstream || feature(:upstream) end diff --git a/lib/foreman_maintain/concerns/metadata.rb b/lib/foreman_maintain/concerns/metadata.rb index bd3a78846..bf5d2a926 100644 --- a/lib/foreman_maintain/concerns/metadata.rb +++ b/lib/foreman_maintain/concerns/metadata.rb @@ -100,8 +100,8 @@ def advanced_run(advanced_run) @data[:advanced_run] = advanced_run end - def do_not_whitelist - @data[:do_not_whitelist] = true + def skippable + @data[:skippable] = true end def self.eval_dsl(metadata, &block) diff --git a/lib/foreman_maintain/reporter/cli_reporter.rb b/lib/foreman_maintain/reporter/cli_reporter.rb index 71e95563f..6ca35c416 100644 --- a/lib/foreman_maintain/reporter/cli_reporter.rb +++ b/lib/foreman_maintain/reporter/cli_reporter.rb @@ -315,19 +315,17 @@ def scenario_failure_message(scenario) end steps_with_error = scenario.steps_with_error(:whitelisted => false) - steps_with_skipped = scenario.steps_with_skipped(:whitelisted => true) - not_skippable_steps = scenario.steps_with_error.select do |step| - step.metadata[:do_not_whitelist] == true + skippable_steps = scenario.steps_with_error.select do |step| + step.metadata[:skippable] == true end - steps_to_whitelist = steps_with_error + steps_with_skipped - not_skippable_steps unless steps_with_error.empty? message << format(<<-MESSAGE.strip_heredoc, format_steps(steps_with_error, "\n", 2)) The following steps ended up in failing state: %s MESSAGE - whitelist_labels = steps_to_whitelist.map(&:label_dashed).join(',') + whitelist_labels = skippable_steps.map(&:label_dashed).join(',') unless whitelist_labels.empty? recommend << if scenario.detector.feature(:instance).downstream format(<<-MESSAGE.strip_heredoc, whitelist_labels) diff --git a/lib/foreman_maintain/runner.rb b/lib/foreman_maintain/runner.rb index ab73d236a..019719766 100644 --- a/lib/foreman_maintain/runner.rb +++ b/lib/foreman_maintain/runner.rb @@ -52,6 +52,8 @@ def run_scenario(scenario) return if scenario.steps.empty? raise 'The runner is already in quit state' if quit? && !rescue? + validate_steps(scenario.steps) + confirm_scenario(scenario) return if quit? && !rescue? @@ -177,5 +179,13 @@ def ask_about_offered_steps(step, scenario) def rerun_check?(step) @last_decision_step == step end + + def validate_steps(steps) + steps.each do |step| + if whitelisted_step?(step) && !step.metadata[:skippable] + raise "#{step} is not skippable. Please remove from whitelist." + end + end + end end end diff --git a/test/definitions/checks/foreman/check_tuning_profile_test.rb b/test/definitions/checks/foreman/check_tuning_profile_test.rb index 9db915f04..4134f4185 100644 --- a/test/definitions/checks/foreman/check_tuning_profile_test.rb +++ b/test/definitions/checks/foreman/check_tuning_profile_test.rb @@ -42,8 +42,4 @@ assert_includes result.output, 'The number of CPU cores for the system is 6 but the currently configured tuning profile requires 8.' # rubocop:disable Metrics/LineLength assert result.fail? end - - it 'does not allow being whitelisted' do - assert subject.metadata[:do_not_whitelist] - end end diff --git a/test/lib/cli/upgrade_command_test.rb b/test/lib/cli/upgrade_command_test.rb index 561342764..8c5ead434 100644 --- a/test/lib/cli/upgrade_command_test.rb +++ b/test/lib/cli/upgrade_command_test.rb @@ -230,6 +230,7 @@ def foreman_maintain_update_unavailable end it 'with --phase it runs only a specific phase of the upgrade' do + foreman_maintain_update_unavailable UpgradeRunner.any_instance.expects(:run_phase).with(:pre_migrations) assert_cmd(<<-OUTPUT.strip_heredoc, ['--phase=pre_migrations', '--target-version=1.15']) Checking for new version of #{ForemanMaintain.main_package_name}... diff --git a/test/lib/reporter_test.rb b/test/lib/reporter_test.rb index 1a965aaaf..87b374506 100644 --- a/test/lib/reporter_test.rb +++ b/test/lib/reporter_test.rb @@ -132,7 +132,7 @@ def decision_question(description) contact Red Hat Technical Support. In case the failures are false positives, use - --whitelist="dummy-check-fail,dummy-check-fail2" + --whitelist="dummy-check-fail" MESSAGE end @@ -160,7 +160,7 @@ def decision_question(description) Resolve the failed steps and rerun the command. In case the failures are false positives, use - --whitelist="dummy-check-fail,dummy-check-fail2" + --whitelist="dummy-check-fail" MESSAGE end diff --git a/test/lib/support/definitions/checks/dummy.rb b/test/lib/support/definitions/checks/dummy.rb index 83e2d8968..4fdddeea7 100644 --- a/test/lib/support/definitions/checks/dummy.rb +++ b/test/lib/support/definitions/checks/dummy.rb @@ -13,6 +13,7 @@ class Fail < ForemanMaintain::Check metadata do label :dummy_check_fail description 'Check that ends up with fail' + skippable end def run @@ -35,7 +36,6 @@ class FailSkipWhitelist < ForemanMaintain::Check metadata do label :dummy_check_fail_skipwhitelist description 'Check that ends up with fail' - do_not_whitelist end def run diff --git a/test/lib/support/definitions/checks/service_is_stopped.rb b/test/lib/support/definitions/checks/service_is_stopped.rb index 79ca45f55..bc3101355 100644 --- a/test/lib/support/definitions/checks/service_is_stopped.rb +++ b/test/lib/support/definitions/checks/service_is_stopped.rb @@ -9,7 +9,7 @@ class Checks::ServiceIsStopped < ForemanMaintain::Check end def run - assert(false, 'service is running', + assert(TestHelper.service_is_stopped, 'service is running', :next_steps => Procedures::StopService.new) end end diff --git a/test/lib/support/definitions/features/present_service.rb b/test/lib/support/definitions/features/present_service.rb index 340dcf812..2a1a96086 100644 --- a/test/lib/support/definitions/features/present_service.rb +++ b/test/lib/support/definitions/features/present_service.rb @@ -20,6 +20,6 @@ def restart end def running? - false + TestHelper.present_service_is_running end end diff --git a/test/lib/test_helper.rb b/test/lib/test_helper.rb index 67e2d47f7..bb1000afe 100644 --- a/test/lib/test_helper.rb +++ b/test/lib/test_helper.rb @@ -2,11 +2,13 @@ class TestHelper class << self - attr_accessor :use_present_service_2, :present_service_is_running, :migrations_fail_at + attr_accessor :use_present_service_2, :present_service_is_running, :migrations_fail_at, + :service_is_stopped def reset self.use_present_service_2 = false self.present_service_is_running = false + self.service_is_stopped = false self.migrations_fail_at = nil end end diff --git a/test/lib/upgrade_runner_test.rb b/test/lib/upgrade_runner_test.rb index 90c7e8929..b79acaaf3 100644 --- a/test/lib/upgrade_runner_test.rb +++ b/test/lib/upgrade_runner_test.rb @@ -15,11 +15,6 @@ module ForemanMaintain UpgradeRunner.new('1.15', reporter) end - let(:upgrade_runner_with_whitelist) do - UpgradeRunner.new('1.15', reporter, - :whitelist => %w[present-service-is-running service-is-stopped]) - end - it 'lists versions available for upgrading, based on available scenarios' do _(UpgradeRunner.available_targets).must_equal ['1.15'] end @@ -36,55 +31,43 @@ module ForemanMaintain end it 'asks for confirmation before getting into pre_migrations from pre upgrade checks' do - upgrade_runner_with_whitelist.run + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true + upgrade_runner.run + _(reporter.log.last).must_equal ['ask', <<-MESSAGE.strip_heredoc.strip] The pre-upgrade checks indicate that the system is ready for upgrade. It's recommended to perform a backup at this stage. Confirm to continue with the modification part of the upgrade, [y(yes), n(no), q(quit)] MESSAGE - assert_equal(:pre_upgrade_checks, upgrade_runner_with_whitelist.phase, + assert_equal(:pre_upgrade_checks, upgrade_runner.phase, 'The phase should not be switched until confirmed') end - it 'remembers the state of the previous run of the upgrade' do - TestHelper.migrations_fail_at = :migrations - reporter.input << 'y' - upgrade_runner_with_whitelist.storage.data.clear - upgrade_runner_with_whitelist.run - upgrade_runner_with_whitelist.save - original_scenario = upgrade_runner_with_whitelist.scenario(:pre_upgrade_checks) - - ForemanMaintain.detector.refresh - new_upgrade_runner = UpgradeRunner.new('1.15', reporter) - new_upgrade_runner.load - _(new_upgrade_runner.phase).must_equal :migrations - restored_scenario = new_upgrade_runner.scenario(:pre_upgrade_checks) - _(restored_scenario.steps.map { |s| s.execution.status }). - must_equal(original_scenario.steps.map { |step| step.execution.status }) - end - it 'remembers the current target version' do TestHelper.migrations_fail_at = :migrations + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true reporter.input << 'y' - upgrade_runner_with_whitelist.storage.data.clear - upgrade_runner_with_whitelist.run - upgrade_runner_with_whitelist.save + upgrade_runner.storage.data.clear + upgrade_runner.run + upgrade_runner.save _(UpgradeRunner.current_target_version).must_equal '1.15' _(UpgradeRunner.available_targets).must_equal(['1.15']) end it 'does not remember the current target version when failed on pre_upgrade_checks ===' do TestHelper.migrations_fail_at = :pre_upgrade_checks - upgrade_runner_with_whitelist.run - upgrade_runner_with_whitelist.save + upgrade_runner.run + upgrade_runner.save _(UpgradeRunner.current_target_version).must_be_nil end it 'cleans the state when the upgrade finished successfully' do reporter.input << 'y' - upgrade_runner_with_whitelist.storage.data.clear - upgrade_runner_with_whitelist.run - upgrade_runner_with_whitelist.save + upgrade_runner.storage.data.clear + upgrade_runner.run + upgrade_runner.save new_upgrade_runner = UpgradeRunner.new('1.15', reporter) new_upgrade_runner.load @@ -100,48 +83,47 @@ module ForemanMaintain end it 'runs migrations if pre_migrations succeed' do + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true reporter.input << 'y' - upgrade_runner_with_whitelist.run + upgrade_runner.run _(reporter.log).must_include ['before_execution_starts', :upgrade_migration] end - it 'runs post_migrations and post_upgrade checks if pre_migrations fail' do - reporter.input << 'y' - TestHelper.migrations_fail_at = :pre_migrations - upgrade_runner_with_whitelist.run - _(upgrade_runner_with_whitelist.phase).must_equal :pre_upgrade_checks - _(reporter.log).wont_include ['before_execution_starts', :upgrade_migration] - _(reporter.log).must_include ['before_execution_starts', :upgrade_post_migration] - _(reporter.log).must_include ['before_execution_starts', :upgrade_post_upgrade_check] - _(upgrade_runner_with_whitelist.exit_code).must_equal 1 - end - it 'runs post_migrations if migrations succeed' do reporter.input << 'y' - upgrade_runner_with_whitelist.run + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true + upgrade_runner.run _(reporter.log).must_include ['before_execution_starts', :upgrade_post_migration] end it 'fails if migrations fail' do reporter.input << 'y' TestHelper.migrations_fail_at = :migrations - upgrade_runner_with_whitelist.run - _(upgrade_runner_with_whitelist.phase).must_equal :migrations - _(upgrade_runner_with_whitelist.exit_code).must_equal 1 + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true + upgrade_runner.run + _(upgrade_runner.phase).must_equal :migrations + _(upgrade_runner.exit_code).must_equal 1 end it 'runs post_upgrade_checks if post_migrations succeed' do reporter.input << 'y' - upgrade_runner_with_whitelist.run + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true + upgrade_runner.run _(reporter.log).must_include ['before_execution_starts', :upgrade_post_upgrade_check] end it 'fails if post_migrations fail' do reporter.input << 'y' TestHelper.migrations_fail_at = :post_migrations - upgrade_runner_with_whitelist.run - _(upgrade_runner_with_whitelist.phase).must_equal :post_migrations - _(upgrade_runner_with_whitelist.exit_code).must_equal 1 + TestHelper.present_service_is_running = true + TestHelper.service_is_stopped = true + upgrade_runner.run + _(upgrade_runner.phase).must_equal :post_migrations + _(upgrade_runner.exit_code).must_equal 1 end end end