Skip to content

Fixes #605 - Run PostgreSQL tools in containers - #906

Draft
jakduch wants to merge 1 commit into
theforeman:masterfrom
jakduch:issue-605-containerized-postgresql-tools
Draft

jakduch wants to merge 1 commit into
theforeman:masterfrom
jakduch:issue-605-containerized-postgresql-tools

Conversation

@jakduch

@jakduch jakduch commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Why are you introducing these changes? (Problem description, related links)

Backup and restore should use PostgreSQL client tools that match the configured server image, without requiring PostgreSQL packages on the target host. This also works when host clients are already present, for example because another component installed them.

Fixes #605.

What are the changes introduced in this pull request?

  • Run pg_isready, pg_dump and pg_restore in the configured PostgreSQL image with host networking. Mount the backup directory only for dump/restore operations and pass the password through the container environment.
  • Remove the backup role's host PostgreSQL client package dependency and load the image variables in backup/restore playbooks.
  • Remove the incorrect integration-test precondition that host clients must be absent. It blocked the IOP-enabled jobs before any backup test could run; existing checks still validate the command result, dump format, permissions, metadata and service health.
  • Use the canonical login_db parameter for postgresql_ext. Collection 4.0 supports it; collection 5 removed the old db alias. This clears the restore syntax warning without raising the minimum supported collection version.

How to test this pull request

Run the normal backup integration suite against internal and external databases, including IOP-enabled deployments. Verify the custom-format dumps and restore them in a disposable deployment. Test hosts with and without native PostgreSQL clients; both are supported.

Verification after rebasing on current master:

  • Targeted Ansible lint across the changed playbooks and their dependencies: 77 files, no failures or rule warnings.
  • Ruff across tests/, src/, development/scripts/ and inventories/: passed.
  • git diff --check: passed.

Earlier live qualification exercised backup and restore without host clients: deployment and post-restore redeployment completed successfully; 76 targeted application/health tests passed with one skip. That earlier result is not a deployment test of the current revision. The current complete deployment/backup matrix runs in CI; no new local VM or image was installed for this update.

Checklist

  • Tests added/updated (removed the invalid host-package precondition)
  • Documentation updated (not applicable: no new user-facing options)

AI assistance: Codex. Project signature: GPT_5.6 Sol.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 07502b0a-b311-4ff3-9e9f-b30c76e7b97b

📥 Commits

Reviewing files that changed from the base of the PR and between 42182ae and cc487d6.

📒 Files selected for processing (6)
  • src/playbooks/backup/backup.yaml
  • src/playbooks/restore/restore.yaml
  • src/roles/backup/tasks/database_dumps.yaml
  • src/roles/backup/tasks/main.yaml
  • src/roles/restore/tasks/restore_databases.yaml
  • tests/backup_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Backup and restore playbooks now prepare a configured PostgreSQL client image. Readiness checks, database dumps, and database restores run in Podman containers instead of using PostgreSQL client commands installed on the host.

Changes: PostgreSQL client containers

Layer / File(s) Summary
Prepare the PostgreSQL client image
src/playbooks/backup/backup.yaml, src/playbooks/restore/restore.yaml
Both playbooks load image variables and ensure the configured PostgreSQL client image is present. They disable image pulls in air-gapped mode.
Run backup commands in containers
src/roles/backup/tasks/main.yaml, src/roles/backup/tasks/database_dumps.yaml, tests/backup_test.py
The readiness check and pg_dump run in Podman containers. The test fixture checks that pg_dump and pg_isready are not installed on the host.
Run restore commands in containers
src/roles/restore/tasks/restore_databases.yaml
The readiness check and pg_restore run in Podman containers. The restore container mounts the backup directory read-only at /backup.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Suggested reviewers: sjha4

Merge Risk: Minimal · previous review cc487

The containerized PostgreSQL commands preserve the documented connection and backup paths, including restore password forwarding. No actionable merge-blocking risk was identified; reported integration results were not independently reproduced.

Security architecture risk: Moderate · up to cc487

Backup and restore now entrust database credentials and backup contents to a container image selected by a mutable tag. Temporary containers and read-only restore mounts limit exposure, but a compromised image could access sensitive data or alter database operations. The image was already trusted for internal PostgreSQL deployment; this change extends its authority to backup archives and external-database backups.

Retained concerns

  • Medium · security · inferred: A mutable PostgreSQL image becomes a credential-bearing backup client with host-network access and mounted backup storage, including for external-database backups. A compromised image could disclose database credentials, tamper with writable dumps, or read restore archives. The image already had internal-server authority before this PR, but these client and archive exposures are new. No digest constraint is visible in the inspected configuration; deployment-enforced image verification remains unknown.
Security review details

Security Blast Radius

  • inferred — A compromised client image could affect the configured databases within the forwarded accounts' privileges, the mounted backup directory, and services reachable through the host network. Restore supplies database-administrator authority. The inspected commands do not establish unrestricted host-filesystem access or cross-environment compromise.

Security Findings and Attack Paths

  • inferred — The supported attack path requires control of a selected image release or its delivery: image execution then exposes forwarded credentials and mounted data. No ordinary unprivileged request path or image compromise was demonstrated. Existing internal-server trust limits the incremental concern there, but does not cover newly mounted archives or external-database backup credentials.

Trust Boundaries and Controls

  • observed — Argument arrays avoid shell interpretation. Containers request neither --privileged nor added capabilities, use temporary-container removal, and restrict restore-volume writes. These controls do not prevent the selected image from reading its environment, accessing mounted contents, or using the host network.

Resilience and Maintainability Implications

  • inferred — Pre-role image preparation limits failures from initial image acquisition before backup service shutdown or database replacement. It does not make restore atomic: the existing drop, create, and restore sequence can leave incomplete databases after failure. No PR-specific atomicity regression was established.

Hardening Proposals

  • proposed — Bind credential-bearing client execution to an approved immutable image identity and verified provenance. Record that identity for reproducible recovery, and validate effective image-user, capability, and concurrent SELinux-mount behavior before treating these containers as a stronger isolation boundary.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed The changes satisfy issue [#605]. pg_dump, pg_isready, and pg_restore now run through podman run with the configured PostgreSQL image. The tasks use host networking and mount backup paths only…
Out of Scope Changes check Passed The changed playbooks, backup and restore tasks, and integration test directly support issue [#605]. Image availability is required for container execution. The host-tool checks verify the removal of …
Description check Passed The description clearly explains the move of PostgreSQL tools into containers, removal of host package dependencies, testing, and related compatibility changes.
Title check Passed The title clearly and concisely identifies the main change: running PostgreSQL tools in containers.
Full details: Linked Issues check

Explanation

The changes satisfy issue [#605]. pg_dump, pg_isready, and pg_restore now run through podman run with the configured PostgreSQL image. The tasks use host networking and mount backup paths only for dump and restore operations. The backup and restore playbooks ensure that the client image is available and remove the host postgresql package installation. The integration test verifies that pg_dump and pg_isready are not installed on the host.

Full details: Out of Scope Changes check

Explanation

The changed playbooks, backup and restore tasks, and integration test directly support issue [#605]. Image availability is required for container execution. The host-tool checks verify the removal of the host dependency. No unrelated change is demonstrated by the pull request diff.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jakduch

jakduch commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

I investigated the two current red jobs; neither failure is caused by this PR's PostgreSQL tooling change:

  • proxy deployment on CentOS Stream 9 with fapolicyd failed while downloading containers.podman from Ansible Galaxy with IncompleteRead(67561 bytes read, 17577 more expected). The deployment never started.
  • upgrade from 2.y-stable reached Pulp migration, then the published quay.io/foreman/pulp:foreman-3.19 image failed to import Psycopg because libpq is missing. The sosreport confirms image digest a8677e83... and pulpcore-manager-migrate.service fails before connecting to PostgreSQL.

I reproduced the second failure directly against the currently published image and reopened pulp-oci-images #115 until the stable tag is rebuilt with the already released packaging fix.

No code change to this PR is indicated by either failure. Once the full workflow finishes and the stable Pulp tag is repaired, I will trigger a fresh matrix run. The PR's own lint and the live backup/restore validation remain green.

@jakduch

jakduch commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

The remaining red matrix is now covered by the shared follow-ups rather than this PostgreSQL-container change: #910 handles recovered Dynflow workers, #893 covers the CentOS Stream 9 fapolicyd cache, #863 fixes the FIPS/backup privilege-escalation timeout, and pulp-oci-images#115 tracks the broken 3.19 Pulp image used by the upgrade job. This PR’s lint, Packit, external-database, CentOS Stream 10 security, capsule and targeted live backup/restore validation are green.

@jakduch
jakduch force-pushed the issue-605-containerized-postgresql-tools branch from cc487d6 to 047e8ed Compare October 5, 2026 19:38
@ehelms

ehelms commented Oct 6, 2026

Copy link
Copy Markdown
Member

Note that #605 talks about an example, but that we should consider how to address the use of PostgresSQL tools inside the container as a whole. This only touches a subset of that and does it in a very brute force method and does not consider why we currently use the postgresql collection, the benefits there in, or how to avoid re-implementing the same large podman run commands each and every time there is some interaction with PostgreSQL in the code base.

For me, this is an example of an issue that needs design discussion, not just trying to throw an agentic solution at it.

@jakduch

jakduch commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

I focused too narrowly on dump/restore. The design needs to cover database/user/extension management and readiness checks as well, and retain the community.postgresql modules rather than replace their behavior with repeated CLI commands. I would start by agreeing on a shared container execution mechanism for those modules and the few CLI-only operations, including internal/external databases, credentials and air-gapped image selection. This PR is not that complete solution yet, so I have left the implementation unchanged rather than treating another CI rerun as an answer to the design concern.

@ehelms
ehelms marked this pull request as draft October 21, 2026 01:29
@jakduch
jakduch force-pushed the issue-605-containerized-postgresql-tools branch from 047e8ed to 4eb6b8b Compare October 28, 2026 07:51
@jakduch

jakduch commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Fixed the backup test setup in 4eb6b8b. The IOP-enabled run had 466 passing tests but 14 backup setup errors because pg_dump was installed on the host. That is a valid deployment, so the fixture no longer rejects it. It still runs the backup and verifies the resulting dumps and metadata.

Also replaced the removed postgresql_ext parameter alias db with login_db, supported by the minimum collection version 4.0. Targeted Ansible lint passed across 77 files with no rule findings; Ruff and git diff --check passed. Rebased on current master; full deployment and backup CI is running again.

CI follow-up (28 October): the completed run had two failing jobs:

  • Upgrade: pulling the IOP remediation image from Quay ended with unexpected EOF; the dependent service could not start. This happened before backup validation.
  • EL10 fapolicyd: Nmap could not open its outlib.lua library. The audit log confirms a fapolicyd denial (subj_trust=0, obj_trust=0) for Nmap. This is a test-tool access failure, not a failed TLS negotiation. That job passed 329 tests, including its applicable backup checks.

The other 22 jobs passed. I refreshed the unchanged code as 7018c98 for one further CI attempt, preserving the tree, parent and single-commit PR. The new result is pending.

Run readiness checks, dumps and restores in the configured PostgreSQL
image instead of requiring host client packages. Keep backup tests usable
when an existing installation already has those packages installed.
Use login_db for postgresql_ext, supported since collection 4.0 and
required after its old db alias was removed.

Fixes theforeman#605

AI assistance: Codex. Project signature: GPT_5.6 Sol.
@jakduch
jakduch force-pushed the issue-605-containerized-postgresql-tools branch from 4eb6b8b to 7018c98 Compare October 28, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Waiting for review in other projects

Development

Successfully merging this pull request may close these issues.

Refactor: Use PostgreSQL container to run PostgreSQL commands instead of using the host.

2 participants