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

Fixes #37883 - halt if remote DB does not own EVR #984

Merged

Conversation

ianballou
Copy link
Contributor

Checks if the postgres user owns the evr extension. If it does, halt the foreman-installer and report an error with the command to run to fix the permissions.

Also check if postgres is the foreman DB owner. In that case, don't do anything because the migration will succeed.

There's no provision if the admin user is called something other than postgres. Should there be?

@ianballou
Copy link
Contributor Author

ianballou commented Oct 5, 2024

To-do: skip the entire hook if the database is local.
Edit: done

@ehelms
Copy link
Member

ehelms commented Oct 24, 2024

Tested and I see the proper error message:

    2024-10-24 13:55:54 [NOTICE] [root] Loading installer configuration. This will take some time.
    2024-10-24 13:55:56 [NOTICE] [root] Running installer with log based terminal output at level NOTICE.
    2024-10-24 13:55:56 [NOTICE] [root] Use -l to set the terminal output log level to ERROR, WARN, NOTICE, INFO, or DEBUG. See --full-help for definitions.
    2024-10-24 13:55:57 [NOTICE] [checks] System checks passed
    The evr extension is owned by postgres and not the foreman DB owner. Please run the following on the foreman DB to fix it: UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman');
    2024-10-24 13:55:59 [ERROR ] [root] The evr extension is owned by postgres and not the foreman DB owner. Please run the following on the foreman DB to fix it: UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman');

I ran (at least I think I ran) the recommended update UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman'); command on the database but it keeps telling me that things are not correct on the remote database. Any ideas?

I tested this using some new Forklift code to make it easier (theforeman/forklift#1871). This is needed locally to get all the PRs:

diff --git a/pipelines/external_database.yml b/pipelines/external_database.yml
index 1e0021c4..0f1b85c8 100644
--- a/pipelines/external_database.yml
+++ b/pipelines/external_database.yml
@@ -19,6 +19,11 @@
   become: yes
   vars_files:
     - vars/external_database.yml
+  vars:
+    packit_prs:
+      - https://github.com/theforeman/foreman-installer/pull/984
+      - https://github.com/Katello/katello/pull/11159
+    postgresql_use_evr: false
   roles:
     - role: forklift_versions
       scenario: "{{ pipeline_type }}"
@@ -27,6 +32,7 @@
     - role: foreman_server_repositories
     - role: etc_hosts
     - role: update_os_packages
+    - role: packit
     - role: foreman_installer
       foreman_installer_options_internal_use_only:
         - "--foreman-db-manage false"

@ehelms
Copy link
Member

ehelms commented Oct 24, 2024

From discussion with @ianballou, a check that evr extension will be added to this hook since for fresh installations the EVR extension would not be expected to have been installed.

@ianballou ianballou force-pushed the 37883-halt-installer-evr-permissions branch from 174cb40 to 308b53b Compare October 24, 2024 19:52
@ianballou
Copy link
Contributor Author

@ehelms I fixed up the PR, it should be okay now. I haven't tested it yet for nil outputs, but the nil check there should do the trick.

@ehelms
Copy link
Member

ehelms commented Oct 25, 2024

@ianballou Can you fix up the rubocop issue?

@ianballou ianballou force-pushed the 37883-halt-installer-evr-permissions branch from 308b53b to d2ee8f5 Compare October 25, 2024 14:19
@ehelms
Copy link
Member

ehelms commented Oct 28, 2024

We should also add a similar check to foreman-maintain to test the ownership before upgrading for the best user experience.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

At least the logging of the password needs to be addressed.

hooks/pre_commit/42-evr_extension_permissions.rb Outdated Show resolved Hide resolved
hooks/pre_commit/42-evr_extension_permissions.rb Outdated Show resolved Hide resolved
hooks/pre_commit/42-evr_extension_permissions.rb Outdated Show resolved Hide resolved
hooks/pre_commit/42-evr_extension_permissions.rb Outdated Show resolved Hide resolved
@ianballou
Copy link
Contributor Author

@ekohl I believe I've addressed all of your comments. I had to move some methods around to use them in both this hook and the database reset one.

@ianballou ianballou requested a review from ekohl October 29, 2024 21:44
@ianballou ianballou force-pushed the 37883-halt-installer-evr-permissions branch from e37efb5 to ea941fd Compare November 1, 2024 19:57
@evgeni
Copy link
Member

evgeni commented Nov 6, 2024

We should also add a similar check to foreman-maintain to test the ownership before upgrading for the best user experience.

I consider not having this check a blocker, TBH.

@ianballou ianballou force-pushed the 37883-halt-installer-evr-permissions branch from ea941fd to 60b31b8 Compare November 12, 2024 19:31
@ianballou ianballou force-pushed the 37883-halt-installer-evr-permissions branch from 5aae8d8 to 3c6ef7e Compare November 13, 2024 19:40
@ianballou
Copy link
Contributor Author

I realized that we don't need to check the evr permissions if the extension does not exist, so I added a check for that earlier in the script.

@ianballou
Copy link
Contributor Author

Foreman maintain side: theforeman/foreman_maintain#953

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

Should be squashed

@ehelms ehelms merged commit 7c45365 into theforeman:develop Nov 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants