Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Halt upgrades if evr is not owned by foreman #953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions definitions/checks/foreman/check_external_db_evr_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
module Checks
module Foreman
class CheckExternalDbEvrPermissions < ForemanMaintain::Check
metadata do
label :external_db_evr_permissions
for_feature :foreman_database
description 'Check that external DBs have proper EVR extension permissions'
tags :pre_upgrade
confine do
feature(:foreman_database) && !feature(:foreman_database).local? && feature(:katello)
end
end

def run
# No check is needed if the evr extension does not exist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop this, the code reads well enough to tell me what's happening.

evr_exists = find_evr_exists
return if !evr_exists

Check failure on line 17 in definitions/checks/foreman/check_external_db_evr_permissions.rb

View workflow job for this annotation

GitHub Actions / rubocop / Rubocop

Style/NegatedIf: Favor `unless` over `if` for negative conditions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could just turn this into more of a evr_exists? method, so that you can reduce these two lines to:

return unless evr_exists?


error_msg = 'The evr extension is not owned by the foreman DB owner. Please run the following command to fix it: ' \

Check failure on line 19 in definitions/checks/foreman/check_external_db_evr_permissions.rb

View workflow job for this annotation

GitHub Actions / rubocop / Rubocop

Layout/LineLength: Line is too long. [124/100]
"UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman') WHERE extname='evr';"

Check failure on line 20 in definitions/checks/foreman/check_external_db_evr_permissions.rb

View workflow job for this annotation

GitHub Actions / rubocop / Rubocop

Layout/LineLength: Line is too long. [129/100]
foreman_owns_evr = find_foreman_owns_evr
fail!(error_msg) if !foreman_owns_evr

Check failure on line 22 in definitions/checks/foreman/check_external_db_evr_permissions.rb

View workflow job for this annotation

GitHub Actions / rubocop / Rubocop

Style/NegatedIf: Favor `unless` over `if` for negative conditions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea here, switch to foreman_owns_evr? method name and then do:

fail!(error_msg) unless foreman_own_evr?

end

def find_evr_exists
evr_exists = feature(:foreman_database).query(self.class.query_for_evr_existence)
if !evr_exists.empty? && evr_exists.first['evr_exists'] == '1'
return evr_exists.first['evr_exists'] == '1'
end
return false
end

def find_foreman_owns_evr
evr_owned_by_postgres = feature(:foreman_database).query(self.class.query_to_find_evr_owner)
if !evr_owned_by_postgres.empty?

Check failure on line 35 in definitions/checks/foreman/check_external_db_evr_permissions.rb

View workflow job for this annotation

GitHub Actions / rubocop / Rubocop

Style/NegatedIf: Favor `unless` over `if` for negative conditions.
return evr_owned_by_postgres.first['evr_owned_by_postgres'] == '0'
end
fail!('Could not determine if the evr extension is owned by the foreman DB owner')
end

def self.query_for_evr_existence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not need to be a class method

<<-SQL
SELECT 1 AS evr_exists FROM pg_extension WHERE extname = 'evr'
SQL
end

def self.query_to_find_evr_owner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not need to be a class method

<<-SQL
SELECT CASE WHEN r.rolname = 'foreman' THEN 0 ELSE 1 END AS evr_owned_by_postgres
FROM pg_extension e JOIN pg_roles r ON e.extowner = r.oid WHERE e.extname = 'evr'
SQL
end
end
end
end
1 change: 1 addition & 0 deletions definitions/scenarios/foreman_upgrade.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def compose
Checks::Disk::AvailableSpaceCandlepin, # if candlepin
Checks::Disk::AvailableSpacePostgresql13,
Checks::Foreman::ValidateExternalDbVersion, # if external database
Checks::Foreman::CheckExternalDbEvrPermissions, # if external database
Checks::Foreman::CheckCorruptedRoles,
Checks::Foreman::CheckDuplicatePermissions,
Checks::Foreman::TuningRequirements, # if katello present
Expand Down
1 change: 1 addition & 0 deletions definitions/scenarios/satellite_upgrade.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def compose
Checks::Disk::AvailableSpace,
Checks::Disk::AvailableSpaceCandlepin, # if candlepin
Checks::Foreman::ValidateExternalDbVersion, # if external database
Checks::Foreman::CheckExternalDbEvrPermissions, # if external database
Checks::Foreman::CheckCorruptedRoles,
Checks::Foreman::CheckDuplicatePermissions,
Checks::Foreman::TuningRequirements, # if katello present
Expand Down
Loading