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

[FC-42163] fc-postgresql: support upgrades with custom extensions #1191

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Dec 9, 2024

Upgrading postgresql to a new major with fc-postgresql fails when using custom extensions, defined via services.postgresql.extraPlugins.

This patch modifies the relevant agent code to use a postgresql package WITH all needed plugins for upgrades:

  • for automatic upgrades, these extensions are discovered automatically.
  • for manual upgrades, the user must specify those manually with --extension-names.

The original use-case of a customer was to upgrade postgresql to a newer version with postgresqlPackages.anonymizer installed (FC-42089). This is a little trickier since it requires anon to be listed in shared_preload_libraries. For this, all shared_preload_libraries are now extracted from postgresql.conf and passed to pg_upgrade via CLI. This code is based on the assumption that the old postgresql.conf is generated by Nix and thus ignores all edge-cases, the postgresql config format may have.

@flyingcircusio/release-managers

Release process

  • Created changelog entry using ./changelog.sh

PR release workflow (internal)

  • PR has internal ticket
  • internal issue ID (PL-…) part of branch name
  • internal issue ID mentioned in PR description text
  • ticket is on Platform agile board
  • ticket state set to Pull request ready
  • if ticket is more urgent than within the next few days, directly contact a member of the Platform team

Design notes

  • Provide a feature toggle if the change might need to be adjusted/reverted quickly depending on context. Consider whether the default should be on or off. Example: rate limiting.
  • All customer-facing features and (NixOS) options need to be discoverable from documentation. Add or update relevant documentation such that hosted and guided customers can understand it as well.

Security implications

  • Security requirements defined? (WHERE)
    • Be able to upgrade postgresql, even with custom extensions.
    • Anonymization (pg_anon is used) is kept during upgrades.
  • Security requirements tested? (EVIDENCE)
    • Implemented an integration test for both automatic and manual upgrades, based on the original one.
    • Verified a manual upgrade (12 -> 14 with inactive anonymization, 14 -> 15 with active anonymization) on test68.

FC-42163

Upgrading postgresql to a new major with fc-postgresql fails when using
custom extensions, defined via `services.postgresql.extraPlugins`.

This patch modifies the relevant agent code to use a postgresql package
WITH all needed plugins for upgrades:

* for automatic upgrades, these extensions are discovered automatically.
* for manual upgrades, the user must specify those manually with
  `--extension-names`.

The original use-case of a customer was to upgrade postgresql to a newer
version with `postgresqlPackages.anonymizer` installed (FC-42089). This
is a little trickier since it requires `anon` to be listed in
`shared_preload_libraries`. For this, all `shared_preload_libraries` are
now extracted from `postgresql.conf` and passed to `pg_upgrade` via CLI.
This code is based on the assumption that the old `postgresql.conf` is
generated by Nix and thus ignores all edge-cases, the postgresql config
format may have.
@Ma27 Ma27 force-pushed the FC-42163-fc-postgresql-extension-support branch from f201c91 to 6229e7b Compare December 9, 2024 11:55
nixos/services/postgresql/default.nix Show resolved Hide resolved
settings.shared_preload_libraries = lib.mkForce "auto_explain, pg_stat_statements, anon";
};

specialisation = {
Copy link
Member

Choose a reason for hiding this comment

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

This boilerplate of postgres version definitions is mostly duplicated from the main postgresql update tests. The last times the postgres versions have been adapted there, this turned out to be a brittle and annoying manual task – whis is now also necessary twice.
Hence, refactoring the tests to be modular and share some code should be the goal. This can be postponed though until when the autoupgrade tests are generally refactored.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be postponed though until when the autoupgrade tests are generally refactored.

Does this mean there was work on this already?
But yeah, I'd prefer to postpone this for now. I considered using the upgrade test for that, but splitting it off made it easier to iterate than having to invent a structure to embed these testcases into the existing ones on the fly.

Regarding modular tests: just in case you haven't seen it already, tests are also just modules which makes it relatively straightforward to write test helpers for a class of tests. See e.g. https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/nextcloud/default.nix.

Copy link
Member

Choose a reason for hiding this comment

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

No, there was no proper work on refactoring so far, only on manually updating the versions in the test before. And that was when it became apparent that this just incurs a lot of manual labour.

@osnyx osnyx merged commit 1ab7377 into fc-24.05-dev Dec 11, 2024
2 checks passed
@osnyx osnyx deleted the FC-42163-fc-postgresql-extension-support branch December 11, 2024 09:19
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.

2 participants