Fixes #37717 - Update evr extension ownership to foreman

[sjha4]

Update ownership of evr extension to owner foreman to allow migrations to operate on it, specifically disable it.

[ianballou]

@ekohl mentioned before that the pg_shdepend table might also need updating, but at least functionally it doesn't seem necessary since the migration passes with @sjha4 's changes alone. The bits that would get updated in pg_shdepend may get dropped with the evr extension.

[ianballou]

I confirmed that dropping the extension also drops the related entry from pg_shdepend.

[sjha4]

I tested the installer with the chages here although I had to put the new code in a separate pre-hook file to be run..Suggestions on where to put this code if not this file?

[evgeni]

What about remote db users? Will those need to perform manual steps (we usually don't have enough permissions to change ownership of objects there) before the upgrade?

[sjha4]

What about remote db users? Will those need to perform manual steps (we usually don't have enough permissions to change ownership of objects there) before the upgrade?

Ya..They'll need to run the update ownership query manually as part of their upgrades. Will need to be documented.

[ianballou]

@evgeni docs update will go in theforeman/foreman-documentation#3167 unless someone gets to it first.

[ianballou]

Prepping a system for a test.

[ianballou]

Found an issue -- the postgres command cannot run because the postgres service is down during the installer run. We'll need to turn postgres on first.

[ianballou]

Aside from the postgres issue it's working.

[sjha4]

Aside from the postgres issue it's working.

Ok..So couple of changes..

1. Added a check to run only on local postgres.
2. Added code to check status of postgresql service, start it if stopped and stop it after the hook.

Should work better now.

[ianballou]

Tested again by upgrading Katello 4.12 to nightly via 4.13. Worked well.

[ekohl]

Please respect noop mode, which you can check with app_value(:noop)

[sjha4]

Updated code to respect no-op and address other review comments..

[sjha4]

Question: What are these numbers before the filenames? I picked 35 cause 34- existed. 🙊

[sjha4]

Looks like foreman release versions..I can update the filename to 311- to follow convention if that is what it is..

[ekohl]

They're for order. All hooks get sorted and that determines when they're executed. It allows you to ensure your hook is executed after the PostgreSQL migration itself

[ekohl]

I'd prefer to implement the noop mode slightly different. In our other hooks we log debug statements what would be done. Otherwise you can simply reduce it to if !app_value(:noop) &&local_postgresql? && execute("rpm -q postgresql-evr", false, false).

[sjha4]

Not sure if test failure is related.. 😕

[ehelms]

Not sure if test failure is related.. 😕

Looks like minitar 1.0.0 was released today and we made need to pin it for the time being -- https://rubygems.org/gems/minitar/versions/1.0.0

[sjha4]

Not sure if test failure is related.. 😕

Looks like minitar 1.0.0 was released today and we made need to pin it for the time being -- https://rubygems.org/gems/minitar/versions/1.0.0

Ya..That was it..

#956

[ekohl]

I'd prefer to implement the noop mode slightly different. In our other hooks we log debug statements what would be done. Otherwise you can simply reduce it to if !app_value(:noop) &&local_postgresql? && execute("rpm -q postgresql-evr", false, false).

[ekohl]

I think we should also have an else for local_postgresql? where we check if the extension exists with an incorrect owner. We have all the parameters to instruct psql to connect to the right location. You can't fix it, but you can halt the installer to prevent a half-upgraded system. Though that should probably be done in a pre_commit hook.

https://github.com/theforeman/puppet-foreman/blob/5f38479c870ef03a4a1565007be68b61e8950d53/manifests/init.pp#L73-L85 are the various answers. foreman_maintain has code that already does something similar, but based on database.yml:
https://github.com/theforeman/foreman_maintain/blob/6cc814b8c669bd02a15452fd488a0a6221ed1fd8/lib/foreman_maintain/concerns/base_database.rb#L148-L156

[sjha4]

Circling back to this, I will look at a pre-commit hook for non-local DBs to get this moving..

[ianballou]

I can add the pre_commit hook in a separate PR to keep this one more focused.

[sjha4]

@ekohl : Ian's got a PR out : #984 to add the pre-commit hook to check ownership on remote dbs. The local DBs would be handled with this PR..

[ekohl]

First of all, it can be simplified to use execute_as!. And I think you should look at the answers as well. Then the final version is:

Suggested change

	execute!("runuser -l postgres -c \"psql -d foreman -c \\\"UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman') WHERE extname='evr';\\\"\"", false, true)
	database = param_value('foreman', 'db_database')
	username = param_value('foreman', 'db_username')
	sql = "UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='#{username}') WHERE extname='evr'"
	execute_as!('postgres', "psql -d #{database} -c \"#{sql}\"", false, true)

But escaping will be tricky, so I'd enhance execute_as! to pass it as an array:

def execute_as!(user, command, do_say = true, do_log = true)
  runuser_command = ['runuser', '-l', user, '-c', command] 
  execute!(runuser_command, do_say, do_log)
end

Then I'd expect it to work if users chose a different DB and/or username as well.

[ekohl]

I just realized that Kafo::PuppetCommand.format_command probably doesn't deal with commands as arrays.

[sjha4]

Updated to do this:

  database = param_value('foreman', 'db_database') || 'foreman'
  username = param_value('foreman', 'db_username') || 'foreman'
  sql = "runuser -l postgres -c \"psql -d '#{database}' -c \\\"UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='#{username}') WHERE extname='evr';\\\"\""
  logger.debug("Executing: #{sql}")
  execute!(sql, false, true) unless app_value(:noop)

Still using execute! as escaping was being really painful.

[ekohl]

I opened theforeman/kafo#374 for that support.

[ianballou]

Suggested change

	# In Katello 4.14, the 'evr' extension is removed from PostgreSQL and integrated into the Katello database via a migration.
	# In Katello 4.15, the 'evr' extension is removed from PostgreSQL and integrated into the Katello database via a migration.