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

nixos/postgresql: change option enableTCPIP to actually mean TCP/IP. #353707

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions nixos/modules/services/databases/postgresql.nix
Original file line number Diff line number Diff line change
Expand Up @@ -361,12 +361,25 @@ in
};

enableTCPIP = mkOption {
type = types.bool;
default = false;
type = types.nullOr types.bool;
default = null;
description = ''
Whether PostgreSQL should listen on all network interfaces.
If disabled, the database can only be accessed via its Unix
domain socket or via TCP connections to localhost.
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'.
Comment on lines +367 to +370
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

'';
};

listenAddresses = mkOption {
type = types.str;
default = "localhost";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

According to the PostgreSQL documentation:

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

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/current/runtime-config-connection.html#GUC-LISTEN-ADDRESSES).
'';
};

Expand Down Expand Up @@ -472,12 +485,16 @@ in
'';
}) 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

'';

services.postgresql.settings =
{
hba_file = "${pkgs.writeText "pg_hba.conf" cfg.authentication}";
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 "";
Copy link
Member

Choose a reason for hiding this comment

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

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

jit = mkDefault (if cfg.enableJIT then "on" else "off");
};

Expand Down
1 change: 1 addition & 0 deletions nixos/tests/all-tests.nix
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,7 @@ in {
apache_datasketches = handleTest ./apache_datasketches.nix {};
postgresql = handleTest ./postgresql.nix {};
postgresql-jit = handleTest ./postgresql-jit.nix {};
postgresql-listen-addresses = handleTest ./postgresql-listen-addresses.nix {};
postgresql-wal-receiver = handleTest ./postgresql-wal-receiver.nix {};
postgresql-tls-client-cert = handleTest ./postgresql-tls-client-cert.nix {};
powerdns = handleTest ./powerdns.nix {};
Expand Down
75 changes: 75 additions & 0 deletions nixos/tests/postgresql-listen-addresses.nix
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Nov 6, 2024

Choose a reason for hiding this comment

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

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).

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
{ system ? builtins.currentSystem
, config ? {}
, pkgs ? import ../.. { inherit system config; }
}:

pkgs.testers.runNixOSTest {
name = "postgresql-listen-addresses";
# Test that PostgreSQL defaults to "localhost" when enableTCPIP = true.

nodes = {
default = { pkgs, lib, ... }: {
# Default behaviour of the service
services.postgresql = {
enable = true;
};
environment.systemPackages = with pkgs; [
lsof
];
};
listenall = { pkgs, lib, ... }: {
services.postgresql = {
enable = true;
listenAddresses = "0.0.0.0";
enableTCPIP = true;
# settings.listen_addresses = "0.0.0.0";
};
environment.systemPackages = with pkgs; [
lsof
];
};
unixonly = { pkgs, lib, ... }: {
services.postgresql = {
enable = true;
enableTCPIP = false;
};
environment.systemPackages = with pkgs; [
lsof
];
};
};

testScript = ''
machines = [ default, listenall, unixonly ]

for machine in machines:
machine.start()
machine.wait_for_unit("postgresql.service")

with subtest("Configured to listen on localhost"):
default.succeed(
"sudo -u postgres psql <<<'SHOW listen_addresses' 2>/dev/null | grep 'localhost'")

with subtest("Actually listening on localhost"):
output = default.succeed("lsof -i tcp -P | grep 'localhost:5432'")

with subtest("Configured to listen on localhost"):
listenall.succeed(
"sudo -u postgres psql <<<'SHOW listen_addresses' 2>/dev/null | grep '0.0.0.0'")

with subtest("Actually listening on localhost"):
listenall.succeed("lsof -i tcp -P | grep '*:5432'")

with subtest("Configured not to listen on localhost or 0.0.0.0"):
unixonly.fail(
"sudo -u postgres psql <<<'SHOW listen_addresses' 2>/dev/null | grep 'localhost'")
unixonly.fail(
"sudo -u postgres psql <<<'SHOW listen_addresses' 2>/dev/null | grep '0.0.0.0'")

with subtest("Actually not listening on TCP"):
unixonly.fail("lsof -i tcp -P | grep ':5432'")

for machine in machines:
machine.shutdown()
'';
}
5 changes: 5 additions & 0 deletions pkgs/servers/sql/postgresql/generic.nix
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ let
pkgs = self;
package = this;
};
postgresql-listen-addresses = import ../../../../nixos/tests/postgresql-listen-addresses.nix {
inherit (stdenv) system;
pkgs = self;
package = this;
};
pkg-config = testers.testMetaPkgConfig finalAttrs.finalPackage;
} // lib.optionalAttrs jitSupport {
postgresql-jit = import ../../../../nixos/tests/postgresql-jit.nix {
Expand Down
Loading