theforeman / puppet-foreman_proxy

Puppet module for Foreman Smart Proxy
GNU General Public License v3.0
43 stars 130 forks source link

Fixes #37325 - make postgres the container gateway default DB #835

Closed ianballou closed 4 months ago

ianballou commented 5 months ago

Draft PR for adding postgres support to the container gateway class. Settings should match the in-progress new settings here: https://github.com/Katello/smart_proxy_container_gateway/pull/33/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R42-R47

I'm not certain that this is the right direction for adding postgres support -- so far I'm thinking that the actual database should be create in puppet-foreman_proxy_content so I don't add a hard postgres library requirement to puppet-foreman_proxy.

foreman_proxy_content PR: https://github.com/theforeman/puppet-foreman_proxy_content/pull/479

Another question was about testing the random database password -- is there a way to stub the extlib::random_password(32) call?

ianballou commented 5 months ago

What we can do is add a soft dependency, where it's mentioned in the README.

To make sure I understand -- so we'll just tell users that they need to ensure the postgresql module is available on the system to install a foreman proxy with the container gateway?

ianballou commented 5 months ago

Marking ready for review. I tested this and it does indeed create a postgres DB for the container gateway.

ianballou commented 5 months ago

Adding postgres to .fixtures.yml since it seems it is necessary for any testing of this new feature to work. I suppose having that module as a dependency isn't a very heavy change.

ianballou commented 5 months ago

Hm looks like the latest test issue is "Could not find resource 'Package[glibc-langpack-en]' in parameter 'require'"

Edit: adding that package to the acceptance node setup.

ianballou commented 4 months ago

I just updated the code to work with https://github.com/Katello/smart_proxy_container_gateway/pull/36.

ianballou commented 4 months ago

The failure is a bit hard to debug due to the way the errors are logged:

  the missing elements were:      [":connection_string: ***:/container_gateway"]
  the extra elements were:        [":connection_string: ***:/container_gateway"]
ianballou commented 4 months ago

I had a bit of a time thinking how the variables should be organized to support both sqlite or postgres. What I landed on for now is having postgres-titled variables for the postgres connection string, and then just reuse the sqlite_db_path variable for the connection string since the string for that should be a file rather than a network path.

ianballou commented 4 months ago

@ekohl before I dive into the tests again, I'm curious if the general design here is acceptable.

ianballou commented 4 months ago

Just tested this again, it's still working with default settings on a centos stream 9 proxy machine.

ianballou commented 4 months ago

I'm not seeing the connection between the test errors and my changes.

I'd like to request that this gets in for Katello 4.13 since postgres support for the container gateway is such a significant improvement.

ianballou commented 4 months ago

As requested in an earlier comment, I've implemented the use of lest. The new behavior removes the database password from the container gateway smart proxy config file unless the user sets it.

I've tested it on a smart proxy and it still works.

ianballou commented 4 months ago

After a chat with @wbclark , I'm gonna look into adding an acceptance test to check that postgres is installed properly.

ianballou commented 4 months ago

I'm gonna aim for addressing these new comments first. Due to the short timeline, I might add the acceptance tests in later in a second PR.

ianballou commented 4 months ago

@ekohl I made the changes and tested them on a foreman proxy.

I added a quick service check for postgresql to 'test the waters' on acceptance testing. Eventually I'd like to check the database and ping the container gateway.

ianballou commented 4 months ago

Doing a curl in the acceptance tests is more complicated than I expected since the container gateway defines the /v2/ endpoint and expects the httpd config to be available to proxy /v2/ to /container_gateway/v2/.

ianballou commented 4 months ago

I confirmed that this still works on a real proxy with Pulp.

ekohl commented 4 months ago

Argh, realized I forgot to squash. I blame the mobile app

ianballou commented 4 months ago

Argh, realized I forgot to squash. I blame the mobile app

I tried to make my commit messages somewhat intelligible, so hopefully not a big deal xD

ekohl commented 4 months ago

Yes, could have been worse 😅

evgeni commented 3 months ago

This needs additions to foreman_maintain (for backups) and upgrade notes so that people who have external DBs don't end up with Pulp on external and Container GW on internal DB.