nixos/postgresql: change option `enableTCPIP` to actually mean TCP/IP. by hoh · Pull Request #353707 · NixOS/nixpkgs

hoh · 2024-11-04T20:37:43Z

This is an attempt at fixing #346013, started during NixCon 2024 with the help of @jfroche .

Setting the option services.postgresql.enableTCPIP to false did not disable TCP/IP. Instead, this makes PostgreSQL listen on localhost.

This was confusing, as a security minded user may want to disable TCP/IP entirely, even for localhost.

It also prevented running multiple instances of PostgreSQL on the same host without manually assigning distinct TCP/IP ports.

This commit adds a new option, services.postgres.listenAddresses, that defaults to localhost.

The default behaviour of services.postgresql is maintained: the service listens on localhost using TCP/IP with the new default enableTCPIP = null.

A warning informs users that setting enableTCPIP = true now only listens on localhost. Setting listenAddresses is now required to listen on all or multiple interfaces. This option follows the upstream PostgreSQL syntax for liste_addresses.

New tests validate the new behavior.

Things done

Add a 👍 reaction to pull requests you find important.

Setting the option `services.postgresql.enableTCPIP` to `false` did not disable TCP/IP. Instead, this makes PostgreSQL listen on `localhost`. This was confusing, as a security minded user may want to disable TCP/IP entirely, even for localhost. It also prevented running multiple instances of PostgreSQL on the same host without manually assigning distinct TCP/IP ports. This commit adds a new option, `services.postgres.listenAddresses`, that defaults to `localhost`. The default behaviour of `services.postgresql` is maintained: the service listens on localhost using TCP/IP with the new default `enableTCPIP = null`. A warning informs users that setting `enableTCPIP = true` now only listens on localhost. Setting `listenAddresses` is now required to listen on all or multiple interfaces. This option follows the upstream PostgreSQL syntax for `liste_addresses`.

m1dugh · 2024-11-04T21:08:38Z

nixos/modules/services/databases/postgresql.nix

+          Whether PostgreSQL should listen on network interfaces.
+          If 'false', the database can only be accessed via its Unix
+          domain socket.
+          A value of 'null' defaults to 'true'.


Why setting the type to nullOr bool when the default is true ?

The new default is null. This has the same behavior as true but this makes it possible to show a warning only to users who have this option configured, not to users who left the default values.

m1dugh · 2024-11-04T21:09:28Z

nixos/modules/services/databases/postgresql.nix

+      listenAddresses = mkOption {
+        type = types.str;
+        default = "localhost";
+        description = ''
+          The TCP/IP address(es) on which the server is to listen for connections from client applications.
+          The value takes the form of a comma-separated list of host names and/or numeric IP addresses.
+          The special entry * corresponds to all available IP interfaces.
+
+          See the PostgreSQL documentation on [listen_address](https://www.postgresql.org/docs/16/runtime-config-connection.html#GUC-LISTEN-ADDRESSES).
        '';


It would be clearer if it was a list of strings later joined with comma in implementation

I agree with this, however I am not sure whether the Nixpkgs policy is to map the settings of the services as close as possible ("The value takes the form of a comma-separated list of host names and/or numeric IP addresses") or to provide a more abstract interface.

On a similar topic, other packages use alternative names for this such as hosts, bind, listen, interface, ports, ... listen_addresses is the name of the setting in PostgreSQL (and a few other services).

I think it you want to keep it like this, I suggest you change the type from string to lib.types.commas which are comma separated strings, which provide an additional check that the definition is valid (see nixos manual).

I would prefer update this branch with a list of strings (or IP addresses if there is a type for that) if that is compatible with the Nixpkgs policy.

nixos/modules/services/databases/postgresql.nix

SuperSandro2000 · 2024-11-05T14:08:42Z

nixos/modules/services/databases/postgresql.nix

        ident_file = "${pkgs.writeText "pg_ident.conf" cfg.identMap}";
        log_destination = "stderr";
-        listen_addresses = if cfg.enableTCPIP then "*" else "localhost";
+        listen_addresses = if (cfg.enableTCPIP != false) then cfg.listenAddresses else "";


Suggested change

listen_addresses = if (cfg.enableTCPIP != false) then cfg.listenAddresses else "";

listen_addresses = lib.optionalString (cfg.enableTCPIP != false) cfg.listenAddresses;

SuperSandro2000 · 2024-11-05T14:09:27Z

nixos/modules/services/databases/postgresql.nix

+
+      listenAddresses = mkOption {
+        type = types.str;
+        default = "localhost";


Is that ipv6 localhost ::1 and 127.0.0.1 on ipv4 only systems?

According to the PostgreSQL documentation:

The default value is localhost, which allows only local TCP/IP “loopback” connections to be made.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

wolfgangwalther · 2024-11-05T15:56:05Z

nixos/tests/postgresql-listen-addresses.nix

Depending on what goes in first, we need to either adjust here or #352966, where we move all the postgres tests into nixos/tests/postgresql and make sure to run them against all PG versions etc.

Thanks for linking to that PR, I didn't know about it. Do you think it would be relevant to test the behavior of these options with all PG versions ? My understanding is that this is not affected by the different versions.

Do you think it would be relevant to test the behavior of these options with all PG versions ?

No, I don't think so. But I haven't made up my mind about where to draw the line, yet.

The "and make sure to run them against all PG versions" was meant as a description for that PR, not necessarily as a requirement for this one.

The requirement is*, that we have it in the same folder eventually, so that we're getting pinged via ci/OWNERS :)

* And that we have it working nicely via passthru, but you already have that in principle, I think. Just avoiding the long relative path would be good (see my PR, too, for how to).

#352966 has been merged, so you could rebase and move the file to nixos/tests/postgresql/ accordingly.

Thanks, I will look into that.

Ma27 · 2024-11-06T22:09:59Z

nixos/modules/services/databases/postgresql.nix

    }) cfg.ensureUsers;

+    warnings = lib.optional (cfg.enableTCPIP == true) ''
+      Behaviour of `services.postgresql.enableTCPIP` changed from binding on all interfaces to binding on localhost in addition to the unix socket.


This is wrong: there's now an option called listenAddresses that controls where to listen.

Generally, I'm wondering if we shouldn't just deprecate the enableTCPIP option and give users fine-grained control over the listen address part.

In fact, we don't even need a listenAddresses option, we can just define the option in the freeform settings type (I mean settings.listen_addresses).

I.e.

options.services.postgresql.enableTCPIP gives a deprecation warning.

if enableTCPIP is set, keep the old behavior (and remove it after a while - given that this is a rather central piece, we may want to go that route).

when it's null, don't touch listen_addresses.

Does that make sense?

Scrumplex · 2024-11-29T22:37:08Z

conflicting

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 4, 2024

nix-owners bot requested a review from thoughtpolice November 4, 2024 20:39

m1dugh reviewed Nov 4, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 5, 2024

SuperSandro2000 reviewed Nov 5, 2024

View reviewed changes

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

SuperSandro2000 reviewed Nov 5, 2024

View reviewed changes

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

SuperSandro2000 reviewed Nov 5, 2024

View reviewed changes

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

SuperSandro2000 reviewed Nov 5, 2024

View reviewed changes

hoh and others added 3 commits November 5, 2024 16:46

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

eb007a3

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

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

b3dbad3

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

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

2b3a2f4

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

nix-owners bot requested review from Ma27 and wolfgangwalther November 5, 2024 15:47

wolfgangwalther reviewed Nov 5, 2024

View reviewed changes

Ma27 reviewed Nov 6, 2024

View reviewed changes

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024

drupol marked this pull request as draft December 10, 2024 14:07

wolfgangwalther mentioned this pull request Feb 9, 2025

postgresql: add listenAddress #169458

Open

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 25, 2025

	listen_addresses = if (cfg.enableTCPIP != false) then cfg.listenAddresses else "";
	listen_addresses = lib.optionalString (cfg.enableTCPIP != false) cfg.listenAddresses;

Uh oh!

Comments

Conversation

hoh commented Nov 4, 2024

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scrumplex commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wolfgangwalther Nov 6, 2024 •

edited

Loading

Scrumplex commented Nov 29, 2024 •

edited

Loading