nixos/postgresql: add ensureDatabases & ensureUsers options

[aanderse]

Motivation for this change

Bring feature parity with mysql to postgresql.

https://discourse.nixos.org/t/postgresql-module-vs-mysql-module/2243

I see several modules (like gitea, for example) create a postgresql user and database by adding relatively messy code to the preStart script, which then requires raised privileges. If the postgresql module had ensureDatabases and ensureUsers options like mysql did all this database code could be cleaned up and simplified greatly, and many modules wouldn’t require any privilege escalation at all in preStart.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

---

[nixos-discourse]

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/postgresql-module-vs-mysql-module/2243/5

[danbst]

Huh, pretty simple! The only thing I'm worrying is that it will generate errors in logs (database already exists, role already exists), even that service boots OK.

On the other hand, when there will be real configuration error, it won't be visible after deploy. So I'd propose to remove || true to make service fail when configuration is incorrect. These options shouldn't be used for adding role permission dynamically.

I've sketched (untested, but based on my config) how I'd like to see the SQL generation part:

# nix eval '(import ./this-file.nix)' --raw
let
  pkgs = import <nixpkgs> {};
  lib = pkgs.lib;
  inherit (lib) flip;
  
  cfg = {
      superUser = "postgres";
      ensureDatabases = [ "redmine" ];
      ensureUsers = [
          { name = "redmine";
            roleAttributes = "LOGIN NOCREATEDB NOCREATEROLE";
            ensurePermissions = { "redmine.*" = "ALL PRIVILEGES"; };
          }
      ];
  };

  script = ''
    PSQL="${pkgs.sudo}/bin/sudo -u ${cfg.superUser} psql"

    function createUser {
        local user="$1"
        local attributes="$2"
        if $( $PSQL -tAc "SELECT 1 FROM pg_roles WHERE rolname='$user'" | grep -q 1 ); then
          $PSQL -tAc "ALTER ROLE $user WITH $attributes"
        else
          $PSQL -tAc "CREATE ROLE $user WITH $attributes"
        fi
    }
    function createDB {
        local db="$1"
        $PSQL -tAc "SELECT 1 FROM pg_database WHERE datname = '$db'" | grep -q 1 \
            || $PSQL -tAc "CREATE DATABASE \"$db\""
    }
  '' + lib.concatStringsSep "\n" (
       map (db: ''createDB "${db}"'') cfg.ensureDatabases
    ++ map (user: ''createUser ${user.name} "${user.roleAttributes}"'') cfg.ensureUsers
    ++ flip lib.concatMap cfg.ensureUsers (user:
            flip lib.mapAttrsToList user.ensurePermissions (db: permissions:
                ''$PSQL -tAc "GRANT ${permissions} ON ${db} TO ${user.name}"''
            )
       )
  );
in script

It generates following Bash code:

PSQL="/nix/store/hy1k7xgjggn3yvafi7wlcqdg42s6byc2-sudo-1.8.26/bin/sudo -u postgres psql"

function createUser {
    local user="$1"
    local attributes="$2"
    if $( $PSQL -tAc "SELECT 1 FROM pg_roles WHERE rolname='$user'" | grep -q 1 ); then
      $PSQL -tAc "ALTER ROLE $user WITH $attributes"
    else
      $PSQL -tAc "CREATE ROLE $user WITH $attributes"
    fi
}
function createDB {
    local db="$1"
    $PSQL -tAc "SELECT 1 FROM pg_database WHERE datname = '$db'" | grep -q 1 \
        || $PSQL -tAc "CREATE DATABASE \"$db\""
}
createDB "redmine"
createUser redmine "LOGIN NOCREATEDB NOCREATEROLE"
$PSQL -tAc "GRANT ALL PRIVILEGES ON redmine.* TO redmine"

[danbst]

And also, this supports the quirks like:

      ensureUsers = [
          { name = "superuser";
            ensurePermissions = { "ALL TABLES IN SCHEMA public" = "ALL PRIVILEGES"; };
          }
      ];

Maybe add as example?

[aanderse]

@danbst wow thank you so much!!

I'm assuming ALTER ROLE will remove permissions if existing permissions aren't listed?

This option will never delete existing users or remove permissions, especially not when the value of this option is changed.

This is how mysql operates. While it isn't necessary to keep this behavior the exact same between mysql and postgres so long as properly documented... it would be nice if we could.

It's there an easy way to modify this so permissions won't be removed?

[danbst]

@aanderse as I read docs, alter role won't remove permission unless you do ask it. For example, if you change LOGIN to NOLOGIN, then LOGIN permission will be removed. But this exactly what is asked, no?

Also, note that roleAttributes is my addition, which is absent in mysql options. We can drop it, as interested parties can set adhoc:

systemd.services.postgres.postStart = lib.mkAfter ''
  $PSQL -c "ALTER ROLE redmine WITH NOLOGIN"
'';

Code will be simplified a bit.

[aanderse]

@aanderse as I read docs, alter role won't remove permission unless you do ask it.

Great! Please disregard my last comment then.

[aanderse]

Hey @danbst I've tried a couple variants of the "GRANT" sql you posted but I'm still having some trouble. Given I'm not very familiar with postgresql dialect I'm probably missing something very obvious and thought you might save me a few more minutes banging my head on it.

$PSQL -tAc "GRANT ${permission} ON DATABASE ${database} TO ${user.name}"

This generates the code you posted $PSQL -tAc "GRANT ALL PRIVILEGES ON redmine.* TO redmine" but I get this error:

ERROR: syntax error at or near "."
LINE 1: GRANT ALL PRIVILEGES ON redmine.* TO redmine

Aside from that, everything else look good?

[danbst]

I've discovered bug-report #56889, which is an example of what can happen when option is not typed. I think "ensureUsers" should be converted to typed submodule, both here and in MySQL.

[aanderse]

@GrahamcOfBorg test redmine.v4-mysql redmine.v4-pgsql

[aanderse]

I've discovered bug-report #56889, which is an example of what can happen when option is not typed. I think "ensureUsers" should be converted to typed submodule, both here and in MySQL.

@florianjacob As the original author of the ensureDatabases & ensureUsers options in the mysql module would you be motivated to tackle this?

[florianjacob]

@aanderse I'll see what I can do this evening for the MySQL part, which you then can integrate here as well (don't have much experience with postgresql). At the time of writing ensurePermissions, I just didn't knew any better.

👍 for bringing this to postgresql too, the amount of code to create postgresql databases splattered all over different services is indeed painstaking to see.

[florianjacob]

@aanderse done: #57550

[aanderse]

@florianjacob Thank you very much!

[aanderse]

@aszlig as someone who just made a postgresql PR do you have any opinions on this change? More opinions and feedback on this is always welcome.

[aszlig]

@aanderse: Hmmm... given that we (want to) have this possibly for MySQL as well, what about having a generic databases option, which defines a common interface to implement for any database? I did something like this a while ago, but without the ensurePermissions part (only owners can be set).

For example like this:

{
  databases.foo = {
    type = "mysql";
    owner = "someuser";
  };

  databases.bar = {
    type = "postgresql";
    schemaFile = "/path/to/foo.sql";
  };
}

Database creation then would be an extra systemd service, so units requiring bar could just order it after database-bar.target for example. Using requires = [ "database-bar.target" ]; then also ensures that if database/user creation fails the dependent service will also fail.

Of course, the model mentioned above does have some limitations, for example you can't have the same database name for both MySQL and PostgreSQL and I probably would separate user creation from database creation like you did here. Maybe something like sql.databases and sql.users or whatever.

Anyway, don't take this as a "you're wrong do it like this" thing, just random thoughts that sprung into my mind.

[aszlig]

I would move all this into a separate service that runs as cfg.superUser instead, because using sudo here uses PAM and also is dependant on the configuration of sudoers. While at it I'd probably also remove the postStart step altogether and use systemd notify, since PostgreSQL has support for that since quite a while.

[aanderse]

@aszlig For other services which depend on having a postgresql database provisioned and have after = [ "postgresql.service" ]; in their systemd unit will this work? My thought was that systemd would spawn both the separate service you are talking about and the service which depends on having a postgresql database provisioned at the same time, which could lead to problems. My systemd skills are very novice, so please forgive my ignorance here... 😞

[aanderse]

@aszlig very neat! I'm glad to see another way to approach the problem.

I think it would be great to have consistent syntax for creating databases and users so ultimately we could do something like:

type = "mysql"; # or postgresql"
# ...
services."${type}" = {
  ensureDatabases = [ "db-for-my-service" ];
  ensureUsers = [ { name = "service-user"; ensurePermissions = "db-for-my-service.*" = "SELECT, INSERT, UPDATE, etc..."; } ];
};

The problem so far being that I don't think it is an easy thing to do to make a consistent syntax between mysql and pgsql for granting access to all tables in a database? There was some discussion between @florianjacob and myself in #50403 (comment) (though I think I've since decided the "with grant option" might not really be appropriate as I think these ensureUsers+ensureDatabases options are moreso for provisioning databases for services, less for provisioning regular users...).

Any thoughts on a consistent syntax?

[danbst]

Somehwat 👍 for separate service, it is not fun to restart PG if you want to add a declarative DB.

[florianjacob]

I'd personally be very cautious regarding the unified frontend to both databases, as whatever differences the two databases have or will develop in the future might bite us, similar to the issues with networking.interfaces backed by either systemd-networkd or the scripted backend.

I'd focus more on two separate, well-designed frontends for each database type, but them being as similar as possible, and think later whether we can combine those interfaces under a common umbrella somehow. Though I have to say I don't have enough postgresql experience to really be able to tell how different they behave, might work out well in this specific case.

[aanderse]

I'd personally be very cautious regarding the unified frontend to both databases, as whatever differences the two databases have or will develop in the future might bite us, similar to the issues with networking.interfaces backed by either systemd-networkd or the scripted backend.

I'd focus more on two separate, well-designed frontends for each database type, but them being as similar as possible, and think later whether we can combine those interfaces under a common umbrella somehow. Though I have to say I don't have enough postgresql experience to really be able to tell how different they behave, might work out well in this specific case.

Hmm... That is a really good point. A little convenience now might end up as a huge pain later. Thanks for mentioning.

[aanderse]

@danbst Given the mysql module has the same issue of creating database+users+permissions in the services postStart would you be happy to merge without a separate service? The separate service sounds like a good idea, but possibly out of scope for what I'm trying to do here (ie. make postgresql on par with mysql, which currently creates databases+users+permissions in the postStart).

[aanderse]

@aszlig What are your thoughts on merging as is? My goal was to bring feature parity between mysql and postgresql as far as ensureUsers and ensureDatabases and this PR achieves that. There is room for improvement that you have highlighted, but since mysql already exists without a separate service that you recommended I think that a separate PR should be created to "fix" both mysql and postgreqsl after this PR has been merged.

I'm not very familiar with postgresql so I'm relying on the tests I ran plus the expertise of the reviewers listed here to confirm everything is correct... but do you have an issue with this PR being merged as is?

[aanderse]

ping @danbst just in case you have some time freed up and can comment on #56720 (comment).

[flokli]

I'd say it's fine to first bring feature parity between mysql and postgresql in this PR then sketch a solution to allow changing ensureUsers and ensureDatabases for both mysql and postgresql.

[aanderse]

@thoughtpolice any thoughts on this PR?

[flokli]

Lets merge this in, we can split it up into multiple services later.

Ran nixosTests.redmine.v4-pgsql, looks good!

[dasJ]

Is it possible that this breaks the manual?

<3>
<3>manual-combined.xml:4808: element xref: validity error : IDREF attribute linkend references an unknown ID "opt-xdg.portal.extraPortals"
<3>  4804    be installed. If you run GNOME, this will be handled automatically for you;
<3>  4805    in other cases, you will need to add something like the following to your
<3>  4806    <filename>configuration.nix</filename>:
<3>  4807  <programlisting>
<3>  4808    <xref linkend="opt-xdg.portal.extraPortals"/> = [ pkgs.xdg-desktop-portal-gtk ];
<3>  4809  </programlisting>
<3>  4810   </para>
<3>
<3>manual-combined.xml:3512: element link: validity error : IDREF attribute linkend references an unknown ID "opt-services.postgresql.ensureDatabases"
<3>  3508    };
<3>  3509
<3>  3510    services.postgresql = {
<3>  3511      <link linkend="opt-services.postgresql.enable">enable</link> = true;
<3>  3512      <link linkend="opt-services.postgresql.ensureDatabases">ensureDatabases</link> = [ "nextcloud" ];
<3>  3513      <link linkend="opt-services.postgresql.ensureUsers">ensureUsers</link> = [
<3>  3514       { name = "nextcloud";
<3>
<3>manual-combined.xml:3513: element link: validity error : IDREF attribute linkend references an unknown ID "opt-services.postgresql.ensureUsers"
<3>  3509
<3>  3510    services.postgresql = {
<3>  3511      <link linkend="opt-services.postgresql.enable">enable</link> = true;
<3>  3512      <link linkend="opt-services.postgresql.ensureDatabases">ensureDatabases</link> = [ "nextcloud" ];
<3>  3513      <link linkend="opt-services.postgresql.ensureUsers">ensureUsers</link> = [
<3>  3514       { name = "nextcloud";
<3>  3515         ensurePermissions."DATABASE nextcloud" = "ALL PRIVILEGES";
<3>
<3>manual-combined.xml fails to validate
<3>builder for '/nix/store/lnk75f0zrfzhs6bybb6xz64i5c1sy58x-nixos-manual-combined.drv' failed with exit code 3

[aanderse]

@dasJ I haven't heard anyone complain about a broken manual... and generally you hear about a broken manual within hours of it breaking. 🤷‍♂️

[markasoftware]

I don't see this anywhere on https://nixos.org/nixos/manual/options.html. Maybe it did break something?

Don't mind me, just forgetting how releases work :)