services.postgresql: Allow configuration of user roles in ensureUser #200724

JonathanLorimer · 2022-11-11T16:43:29Z

Description of changes

This adds an attribute to the ensureUsers option on the services.postgresql module. The attribute allows you to set a list of roles that you want to applied to the user when the postgresql service starts up.

Things done

nixos/modules/services/databases/postgresql.nix

bjornfor · 2022-11-12T15:33:31Z

nixos/modules/services/databases/postgresql.nix

+              default = [ ];
+              description = lib.mdDoc ''
+                A list of roles to grant to the user. Under the hood this uses the
+                [ALTER USER syntax](https://www.postgresql.org/docs/current/sql-alteruser.html) for each role  in the list:


Double space in "each role in". (Hehe, it seems github removes double space in these comments.)

jtojnar · 2022-11-14T15:22:03Z

Note that there is some uncertainty whether the database modules should have ensure* options, see e.g. #107342

roberth

I have a couple of suggestions, and this should be tested. You could extend nixos/tests/postgresql.nix for this purpose.

roberth · 2022-11-17T15:44:45Z

nixos/modules/services/databases/postgresql.nix

@@ -168,6 +168,18 @@ in
                }
              '';
            };
+            ensureRoles = mkOption {


Roles and Users are synonymous in PostgreSQL.

The linked page calls them options. This page calls them clauses. That seems like an even better name.

roberth · 2022-11-17T15:44:49Z

nixos/modules/services/databases/postgresql.nix

+              description = lib.mdDoc ''
+                A list of roles to grant to the user. Under the hood this uses the
+                [ALTER USER syntax](https://www.postgresql.org/docs/current/sql-alteruser.html) for each role in the list:
+                `ALTER USER user.name WITH role`


Can clauses be removed? We don't seem to have enough knowledge to remove unmentioned roles.

The linked page lists negatives as well: NOSUPERUSER, etc. Could those be used for this purpose?
A better option representation would be types.attrsOf types.bool. (or even individual options, to restrict it to known-valid attribute names; not sure if that's better)

With this infrastructure in place, we can also provide an option (off by default, not to break configs) for fully declarative user permissions; a matter of conditionally assigning some lib.mkDefault defaults to those options, so that they cover all possible clauses.

Great suggestion. I will try to find some time this weekend to do this, as well as adding tests.

roberth · 2022-11-23T15:52:15Z

nixos/tests/postgresql.nix

+          assert clauses['rolreplication'], 'expected user with clauses to have replication clause'
+          assert clauses['rolbypassrls'], 'expected user with clauses to have bypassrls clause'
+
+      with subtest("All user permissions default to false when ensureClauses is not provided"):


This is the default for NixOS admins who are unaware of this new feature, so it'd be a breaking change to remove all clauses. I think the bool types need to be nullOr bool, to represent "leave as is" with null.
Then change the defaults to null, or perhaps add a new option so the defaults can be if config.services.postgresql.declarativeRoleClauses then false else null?

Oh yeah, great point. I will try to get to this next week when I have some time. Thanks for all the direction on this.

jtojnar · 2022-11-25T02:21:58Z

Thinking about it a bit more, maybe it would be better to move all these ensure options into a separate module (accessible under e.g. services.postgresql.provision to follow what Grafana does). That way it would be clearer that this is something extra.

roberth · 2022-11-25T11:56:27Z

move all these ensure options into a separate module (accessible under e.g. services.postgresql.provision to follow what Grafana does).

Sounds ok to me, though I wouldn't rush it because it seems that we need to solve some declarativeness problems that need interface changes. It'd be nicer for users to do one migration than to do a rename and then a migration. Also if we rename options first, then options with the old semantics may collide with the names we want to use after making things more declarative.

That way it would be clearer that this is something extra.

Moving it to a separate module isn't trivial, because it needs to run as part of postStart, which isn't something that the postgres module exposes for extension. I suppose it could, or perhaps it's ok for other modules to add extra definitions to the ExecStartPost? Though I think not, because it's not a great idea to make assumptions about the ordering of those lines.
I'd be slightly more at ease doing those things after something like #154123, although that doesn't implement an explicit ordering relation between scripts either.
For now it's definitely easier to have the script in a single place, rather than broken up into modules.

roberth · 2022-11-25T11:59:15Z

nixos/modules/services/databases/postgresql.nix

+
+                  clauseSqlStatements = attrValues (mapAttrs (n: v: if v then n else "no${n}") filteredClauses);
+
+                  alterClause = role: ''$PSQL -tAc 'ALTER USER "${user.name}" WITH ${role}' '';


Is it possible to set all clauses in one $PSQL invocation?
It seems that that would save some latency, especially when you have multiple users and the connection isn't local and uses TLS.

Good catch, just pushed a fix.

roberth

Some nitpicks. Otherwise LGTM

nixos/tests/postgresql.nix

roberth · 2022-11-27T12:01:28Z

nixos/modules/services/databases/postgresql.nix

+
+                      More information on postgres roles can be found [here](https://www.postgresql.org/docs/current/role-attributes.html)
+                    '';
+                    default = null;


null was underdocumented. Could you do the following? And maybe create a let binding for it, so you don't have to copy paste.

defaultText = lib.literalMD '' `null`: do not set. For newly created roles, use PostgreSQL's default. For existing roles, do not touch this clause. '';

roberth · 2022-11-27T12:10:29Z

Then change the defaults to null, or perhaps add a new option so the defaults can be if config.services.postgresql.declarativeRoleClauses then false else null?

I don't know. This might be a bit out of place if we don't make the other things more declarative. Can be done in a follow up PR if desired.

The PR contains many commits, which should be squashed. I doesn't look like there's an intermediate change that would warrant having more than one commit. @JonathanLorimer could you squash them?

remove trailing whitespace switch docs to markdown use mdDoc remove trailing whitespace get rid of double space add tests and update options to use submodule remove whitespace remove whitespace use mdDoc remove whitespace make default a no-op make ALTER ROLE a single sql statement document null case

nixos/modules/services/databases/postgresql.nix

Co-authored-by: Robert Hensing <[email protected]>

RaitoBezarius · 2022-12-01T13:48:36Z

There might be features conflicts with #203474 ?

…on-of-roles

roberth · 2022-12-02T18:16:04Z

@ofborg test postgresql

roberth · 2022-12-03T01:37:56Z

LGTM.
Deferring to postgres maintainers @thoughtpolice @danbst @globin @marsam or @ivan for review/approval.

JonathanLorimer · 2022-12-13T20:54:42Z

There might be features conflicts with #203474 ?

I think we could make what I have work with that PR.

roberth · 2022-12-17T23:40:54Z

LGTM. Deferring to postgres maintainers @thoughtpolice @danbst @globin @marsam or @ivan for review/approval.

2 weeks have passed, or 10 maintainer*weeks, without any response.

Still LGTM.

roberth · 2022-12-17T23:41:59Z

Probably the maintainers are only interested in the package. Next time I'll cc them but not ask for anything.

JonathanLorimer requested a review from thoughtpolice as a code owner November 11, 2022 16:43

JonathanLorimer changed the title ~~Allow configuration of postgres roles~~ Allow configuration of postgres user roles Nov 11, 2022

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 11, 2022

JonathanLorimer changed the title ~~Allow configuration of postgres user roles~~ services.postgresql: Allow configuration of user roles in ensureUser Nov 11, 2022

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 11, 2022

jtojnar mentioned this pull request Nov 11, 2022

nixos/*: deprecate docbook option docs #189318

Merged

13 tasks

jtojnar reviewed Nov 11, 2022

View reviewed changes

nixos/modules/services/databases/postgresql.nix Outdated Show resolved Hide resolved

JonathanLorimer requested review from jtojnar and removed request for thoughtpolice November 11, 2022 19:23

bjornfor reviewed Nov 12, 2022

View reviewed changes

JonathanLorimer requested review from bjornfor and removed request for jtojnar November 13, 2022 15:52

pennae mentioned this pull request Nov 14, 2022

nixos/manual: re-add mention of mdDoc marker #201187

Merged

13 tasks

JonathanLorimer requested review from jtojnar and removed request for bjornfor November 14, 2022 15:12

roberth requested changes Nov 17, 2022

View reviewed changes

JonathanLorimer requested review from roberth and removed request for jtojnar November 20, 2022 15:49

roberth reviewed Nov 23, 2022

View reviewed changes

JonathanLorimer requested a review from roberth November 24, 2022 22:51

roberth reviewed Nov 25, 2022

View reviewed changes

JonathanLorimer requested a review from roberth November 25, 2022 13:43

roberth reviewed Nov 27, 2022

View reviewed changes

JonathanLorimer force-pushed the allow-configuration-of-roles branch from 67e3912 to 193aa6f Compare November 28, 2022 14:45

JonathanLorimer requested a review from roberth November 28, 2022 14:47

roberth reviewed Nov 28, 2022

View reviewed changes

nixos/modules/services/databases/postgresql.nix Outdated Show resolved Hide resolved

Update nixos/modules/services/databases/postgresql.nix

4e502a1

Co-authored-by: Robert Hensing <[email protected]>

JonathanLorimer requested a review from roberth November 28, 2022 16:21

roberth added 2 commits December 2, 2022 18:12

Merge remote-tracking branch 'upstream/master' into allow-configurati…

7bfbf03

…on-of-roles

nixosTests.postgresql: Fix attribute name shadowing

1466203

roberth requested review from thoughtpolice, danbst, globin, marsam and ivan December 3, 2022 01:36

RaitoBezarius mentioned this pull request Dec 16, 2022

RFC: ensure-style options in NixOS modules #206467

Open

roberth merged commit cf150c5 into NixOS:master Dec 17, 2022

Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

services.postgresql: Allow configuration of user roles in ensureUser #200724

services.postgresql: Allow configuration of user roles in ensureUser #200724

JonathanLorimer commented Nov 11, 2022

bjornfor Nov 12, 2022

jtojnar commented Nov 14, 2022 •

edited

Loading

roberth left a comment

roberth Nov 17, 2022

roberth Nov 17, 2022

JonathanLorimer Nov 17, 2022

roberth Nov 23, 2022

JonathanLorimer Nov 24, 2022

jtojnar commented Nov 25, 2022

roberth commented Nov 25, 2022

roberth Nov 25, 2022

JonathanLorimer Nov 25, 2022

roberth left a comment

roberth Nov 27, 2022

roberth commented Nov 27, 2022

RaitoBezarius commented Dec 1, 2022

roberth commented Dec 2, 2022

roberth commented Dec 3, 2022

JonathanLorimer commented Dec 13, 2022

roberth commented Dec 17, 2022

roberth commented Dec 17, 2022


		clauseSqlStatements = attrValues (mapAttrs (n: v: if v then n else "no${n}") filteredClauses);

		alterClause = role: ''$PSQL -tAc 'ALTER USER "${user.name}" WITH ${role}' '';

services.postgresql: Allow configuration of user roles in ensureUser #200724

services.postgresql: Allow configuration of user roles in ensureUser #200724

Conversation

JonathanLorimer commented Nov 11, 2022

Description of changes

Things done

Choose a reason for hiding this comment

jtojnar commented Nov 14, 2022 • edited Loading

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar commented Nov 25, 2022

roberth commented Nov 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Nov 27, 2022

RaitoBezarius commented Dec 1, 2022

roberth commented Dec 2, 2022

roberth commented Dec 3, 2022

JonathanLorimer commented Dec 13, 2022

roberth commented Dec 17, 2022

roberth commented Dec 17, 2022

jtojnar commented Nov 14, 2022 •

edited

Loading