Skip to content

migrate pgsql installation from rpm to container-based#62

Merged
ehelms merged 1 commit intotheforeman:masterfrom
archanaserver:pgsql-container
Mar 19, 2025
Merged

migrate pgsql installation from rpm to container-based#62
ehelms merged 1 commit intotheforeman:masterfrom
archanaserver:pgsql-container

Conversation

@archanaserver
Copy link
Copy Markdown
Contributor

@archanaserver archanaserver commented Nov 27, 2024

This PR transitions the pgsql installation from an RPM-based installation to a containerized based.

@archanaserver
Copy link
Copy Markdown
Contributor Author

idk what is wrong with image, it is failing everytime reading manifest in registry.access.redhat.io/rhel9/postgresql-16:latest?

@archanaserver
Copy link
Copy Markdown
Contributor Author

also i do not like doing this way, should i go with creating a separate role for postgresql and do the containerization work there? would it be good? any thoughts?

@ehelms
Copy link
Copy Markdown
Member

ehelms commented Dec 3, 2024

idk what is wrong with image, it is failing everytime reading manifest in registry.access.redhat.io/rhel9/postgresql-16:latest?

This is an official Red Hat image locked behind subscriptions, only ubi is available for wide usage but I think we've realized it does not come in flavors like the rhel images.

@ehelms
Copy link
Copy Markdown
Member

ehelms commented Dec 3, 2024

also i do not like doing this way, should i go with creating a separate role for postgresql and do the containerization work there? would it be good? any thoughts?

100% -- give it a role like all the other services

@archanaserver archanaserver marked this pull request as ready for review January 7, 2025 05:19
@archanaserver archanaserver force-pushed the pgsql-container branch 4 times, most recently from ed4479f to 5b01376 Compare January 27, 2025 12:04
@archanaserver archanaserver marked this pull request as draft January 29, 2025 12:34
@archanaserver archanaserver force-pushed the pgsql-container branch 2 times, most recently from b394a56 to 733a488 Compare January 29, 2025 12:49
@archanaserver archanaserver force-pushed the pgsql-container branch 3 times, most recently from a9cf160 to 1699fff Compare February 6, 2025 10:03
@archanaserver archanaserver marked this pull request as ready for review February 6, 2025 10:03
@archanaserver
Copy link
Copy Markdown
Contributor Author

archanaserver commented Feb 6, 2025

@ehelms how can we re run the workflow?

edit: got it!

@ehelms
Copy link
Copy Markdown
Member

ehelms commented Feb 6, 2025

With Candlepin, the thing to usually check is the candlepin.log file:

vi /var/log/candlepin/candlepin.log

And the output I see is:

2025-02-06 13:59:51,400 [thread=main] [=, org=, csid=] ERROR org.candlepin.database.DatabaseConnectionManager - Connection to localhost:5432 refused. Check that the hostname and port are correct and that the postmaster is accepting TCP/IP connections.
org.postgresql.util.PSQLException: Connection to localhost:5432 refused. Check that the hostname and port are correct and that the postmaster is accepting TCP/IP connections.

Comment on lines +12 to +13
CREATE DATABASE {{ db.name }};
ALTER DATABASE {{ db.name }} OWNER TO {{ db.owner }};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Per https://www.postgresql.org/docs/current/sql-createdatabase.html you can set the owner on creation

Suggested change
CREATE DATABASE {{ db.name }};
ALTER DATABASE {{ db.name }} OWNER TO {{ db.owner }};
CREATE DATABASE {{ db.name }} OWNER {{ db.owner }};

Comment on lines +4 to +7
{% for user in postgresql_users %}
psql -v ON_ERROR_STOP=1 --username "postgres" <<-EOSQL
CREATE USER {{ user.name }} WITH PASSWORD '{{ user.password }}';
EOSQL
{% endfor %}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was debating using createuser but reading the documentation I wonder how to properly deal with passing in the password.

Then my other thought was whether if it's better to create the users in a single query:

Suggested change
{% for user in postgresql_users %}
psql -v ON_ERROR_STOP=1 --username "postgres" <<-EOSQL
CREATE USER {{ user.name }} WITH PASSWORD '{{ user.password }}';
EOSQL
{% endfor %}
psql -v ON_ERROR_STOP=1 --username "postgres" <<-EOSQL
{% for user in postgresql_users %}
CREATE USER {{ user.name }} WITH PASSWORD '{{ user.password }}';
{% endfor %}
EOSQL

And if you take that latter step to the extreme: should there just be a single script that creates the users and the databases.

Network=host
Volume={{ postgresql_data_dir }}:/var/lib/pgsql/data:Z
Volume={{ playbook_dir }}/postgresql-init:/opt/app-root/src/postgresql-init:Z
Environment=POSTGRESQL_ADMIN_PASSWORD={{ postgresql_admin_password }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These files will be world readable, meaning any user on the system can read the admin password. This should be supplied via a secret.

Having said that: why do we need an admin password? https://github.com/sclorg/postgresql-container/tree/master/13#postgresql-admin-account says you can still use local connections. Or is that insecure because we use the host network?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These files will be world readable, meaning any user on the system can read the admin password. This should be supplied via a secret.

That's right! To address this i've moved the password storage to a dedicated secrets file and restrict file permission to 0600 so only container process can now access it. But I'm trying to do this with Podman secrets.

Having said that: why do we need an admin password? https://github.com/sclorg/postgresql-container/tree/master/13#postgresql-admin-account says you can still use local connections. Or is that insecure because we use the host network?

While the container allows local connections without a password here by default, but host networking exposes PostgreSQL to all network interfaces I believe(not just localhost). So without a password, anyone on the network could access the database with postgres privileges. And admin password is that is gonna adds a layer of security here, even if local connections are allowed. If we switch to a non-host network later, maybe the password isn't needed, but for now, it's safer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By using init you don't guarantee the password is correct. Perhaps not relevant now, but for the long term: do we need to provide a mechanism to rotate passwords?

Another thing I'm worried about: this will contain plain text passwords in the container. Will that be a security risk?

By the default the container already contains code to create a single user (POSTGRESQL_USER, POSTGRESQL_PASSWORD, POSTGRESQL_DATABASE) which could be provided via secrets. It makes me wonder what would be a good pattern. Should we have a PostgreSQL container for every database?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

By using init you don't guarantee the password is correct. Perhaps not relevant now, but for the long term: do we need to provide a mechanism to rotate passwords?

It only works for initial step for now and isn't idempotent. I suppose we can add separate tasks to manage users/passwords or maybe we have Ansible modules to handle this, not sure. For now, yes we need that.
https://docs.ansible.com/ansible/latest/collections/community/postgresql/postgresql_user_module.html

Another thing I'm worried about: this will contain plain text passwords in the container. Will that be a security risk?

After looking at it that's valid point. Here the init script run once during container initialization, but the file is in postgresql-init/ could expose secrets. So I restricted now dir permission to owner access but better would be do it with Podman secrets or file based, so that's another to-do.

By the default the container already contains code to create a single user (POSTGRESQL_USER, POSTGRESQL_PASSWORD, POSTGRESQL_DATABASE) which could be provided via secrets. It makes me wonder what would be a good pattern. Should we have a PostgreSQL container for every database?

Can't think of much but If we want strict isolation then we should adopt multiple containers, each with its own secrets and we gonna definitely need a private podman network in that case. Or I guess starting with a single container would be good but with a private network so we can revisit it later for multi-container isolation?

@archanaserver archanaserver force-pushed the pgsql-container branch 3 times, most recently from 74f315b to 910cb46 Compare February 25, 2025 06:47
@archanaserver archanaserver force-pushed the pgsql-container branch 4 times, most recently from ec9d4c4 to 7a63df8 Compare March 17, 2025 17:18
login_host: localhost
state: present
loop: "{{ postgresql_users }}"
when: postgresql_users | length > 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can omit the when here, if the array is empty, the loop will not be executed :)

login_host: localhost
state: present
loop: "{{ postgresql_databases }}"
when: postgresql_databases | length > 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can omit the when here, if the array is empty, the loop will not be executed :)

Comment on lines +15 to +19
- name: Configure host based authentication
ansible.builtin.template:
src: pg_hba.conf.j2
dest: "{{ postgresql_data_dir }}/pg_hba.conf"
mode: "0644"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I am reading https://github.com/sclorg/postgresql-container/blob/master/13/root/usr/share/container-scripts/postgresql/common.sh#L228-L240 correctly, we don't need to configure hba in the container case, as the container is already configured as "everyone can connect"

Comment on lines +28 to +30
def test_postgresql_hba_config(server):
result = server.run("podman exec postgresql cat /var/lib/pgsql/data/pg_hba.conf")
assert re.search(r"host\s+all\s+all\s+127\.0\.0\.1/32\s+md5", result.stdout)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You dropped the hba editing, so I think this test can be removed (and then the re import too)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +30 to +31
env:
POSTGRESQL_ADMIN_PASSWORD: "{{ postgresql_admin_password }}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You created a secret for that above, so let's use it

Suggested change
env:
POSTGRESQL_ADMIN_PASSWORD: "{{ postgresql_admin_password }}"
secrets:
- 'postgresql_admin_password,target=POSTGRESQL_ADMIN_PASSWORD,type=env'


postgresql_databases: []
postgresql_users: []
postgresql_hba_entries: []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be removed now (and so can the setting of it in deploy.yaml)

@archanaserver archanaserver force-pushed the pgsql-container branch 2 times, most recently from 11ca8fc to ed5a8f5 Compare March 18, 2025 09:55
@ehelms
Copy link
Copy Markdown
Member

ehelms commented Mar 18, 2025

Great to see this working! How much data do we want on how upgrades work (#66) before we merge this change?

@evgeni
Copy link
Copy Markdown
Member

evgeni commented Mar 18, 2025

I would see those as separate problems (= no data required to merge this).

It seems the container does support running pg_upgrade if it starts with existing data and POSTGRESQL_UPGRADE variable was set:

So we might be rather lucky here and be able to orchestrate updates in a very similar way we do on base rhel

@ehelms
Copy link
Copy Markdown
Member

ehelms commented Mar 18, 2025

I'll do a final review of this PR then. @evgeni could you add those upgrade notes to #66 ?

@ehelms
Copy link
Copy Markdown
Member

ehelms commented Mar 18, 2025

@archanaserver Do you know if this container setup satisfies #106 ?

@archanaserver
Copy link
Copy Markdown
Contributor Author

@archanaserver Do you know if this container setup satisfies #106 ?

No it doesn't, currently container setup by default uses md5. So I can take look into adding explici settings for scram-sha-256.

Copy link
Copy Markdown
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

The SCRAM update can be a follow up.

@ehelms ehelms merged commit d7c7801 into theforeman:master Mar 19, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Containerization Mar 19, 2025
@ehelms ehelms added this to the 1.0 milestone Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants