Skip to content

test(refactor): add withToxiproxy* in withTools #4833

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.

Sign up for GitHub

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

Closed
mkleczek wants to merge 1 commit into from
+122 −11
Closed

test(refactor): add withToxiproxy* in withTools #4833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
123 changes: 115 additions & 8 deletions nix/tools/withTools.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
, python3Packages
, writeText
, writers
, toxiproxy
}:
let
withTmpDb =
Expand Down Expand Up @@ -54,16 +55,25 @@ let

export PGDATA="$tmpdir/db"
export PGHOST="$tmpdir/socket"
PGPORT=$(${randomPort})
export PGPORT
export PGUSER
export PGDATABASE
export PGRST_DB_SCHEMAS
export PGTZ
export PGOPTIONS

HBA_FILE="$tmpdir/pg_hba.conf"
echo "local $PGDATABASE some_protected_user password" > "$HBA_FILE"
echo "local $PGDATABASE all trust" >> "$HBA_FILE"
echo "local replication all trust" >> "$HBA_FILE"
{
echo "local $PGDATABASE some_protected_user password"
echo "local $PGDATABASE all trust"
echo "local replication all trust"
echo "host $PGDATABASE some_protected_user localhost password"
echo "host $PGDATABASE all localhost trust"
} >> "$HBA_FILE"

UNIX_PGHOST="$PGHOST"
export TCP_PGHOST="localhost"
Comment on lines +75 to +76

@wolfgangwalther wolfgangwalther Apr 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure whether we still need to keep unix sockets here at all. We originally did this to prevent port-in-use errors during tests. We now need to add the complexity of finding the right port etc. - so there is no point in keeping additional complexity in supporting both unix sockets and tcp.

@mkleczek mkleczek Apr 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd agree. The only thing is load tests and the performance impact (if any) of running them with TCP.

@steve-chavez steve-chavez May 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't we create another dedicated test suite like postgrest-test-recovery (suggested before here) only focused on TCP? That way we don't have to modify previous test infra?

@mkleczek mkleczek May 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Why don't we create another dedicated test suite like postgrest-test-recovery (suggested before here) only focused on TCP?

Fine for me.
In #4860 I've extracted a (private) library test-utils that should be shared across different test suites and will make creating additional test suites easier.
But I don't think it is relevant to this PR really (see below).

That way we don't have to modify previous test infra?

This PR does not really change anything in existing infra (except starting the db server on TCP in addition to unix socket, which required very small adjustments in tests - adding explicit PGPORT)

@steve-chavez steve-chavez May 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

except starting the db server on TCP in addition to unix socket, which required very small adjustments in tests - adding explicit PGPORT

That looks like a change, unless if it was gated by bool env var like:

postgrest/nix/tools/withTools.nix

Line 33 in 08ec8d1

"ARG_OPTIONAL_BOOLEAN([replica],, [Enable a replica for the database])"

@wolfgangwalther wolfgangwalther May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't we create another dedicated test suite like postgrest-test-recovery (suggested before here) only focused on TCP?

Essentially, I conclude the same in #4868. I would not call it "recovery", see my reasoning in that issue. The key thing is: I don't want to run TCP connections on the host system, because that just leads to port conflicts down the road - we've always had these eventually.

In #4860 I've extracted a (private) library test-utils that should be shared across different test suites and will make creating additional test suites easier.
But I don't think it is relevant to this PR really (see below).

See that same issue above why I think what we're currently doing in the observability test suite is fundamentally wrong - and as such that PR is, too. We can't really use haskell code to "inspect" PostgREST, because we'd always only test the library, not the executable in its entirety. Which is fine for some things, but surely not for listener related things.

@mkleczek mkleczek May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See that same issue above why I think what we're currently doing in the observability test suite is fundamentally wrong - and as such that PR is, too. We can't really use haskell code to "inspect" PostgREST, because we'd always only test the library, not the executable in its entirety. Which is fine for some things, but surely not for listener related things.

That's obviously not true, see for example https://github.com/mkleczek/postgrest/blob/9af29496ad943381233d2752ca8f6dd6c10487c4/test/observability/Observation/ToxiSpec.hs#L43

@wolfgangwalther wolfgangwalther May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure what the link is supposed to tell me, but it's certainly not a counter point to my argument. Just because you are testing the listener there, doesn't invalidate my point which is:

  • We will very likely want to split library from executable.
  • Listener should be in the executable (even if it was technically implemented as a cabal library, that's not the point)
  • You can't test the full executable by referencing it as a library - and we certainly are not doing so right now, because the observability tests are setting the config via Haskell, for example.
  • Once you run the executable, you can't use haskell code to inspect it anymore.

Your tests are not running the executable, they are using the still-not-split postgrest as a library.

@mkleczek mkleczek May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what the link is supposed to tell me, but it's certainly not a counter point to my argument. Just because you are testing the listener there, doesn't invalidate my point which is:

  • We will very likely want to split library from executable.

Then it is even more important to test the library, isn't it?

  • Listener should be in the executable (even if it was technically implemented as a cabal library, that's not the point)

I am not sure I understand that. Listener is both a library and in the executable.

  • You can't test the full executable by referencing it as a library - and we certainly are not doing so right now, because the observability tests are setting the config via Haskell, for example.
  • Once you run the executable, you can't use haskell code to inspect it anymore.

Your tests are not running the executable, they are using the still-not-split postgrest as a library.

I know that, but I don't understand why it would invalidate the tests. There are various kinds of tests necessary to ensure high quality: unit tests, white box tests, black box tests - all with different trade-offs.

You could use the same argument for postgrest-test-spec - they don't test the whole executable. It does not make them invalid or not useful.

And again - there are good arguments for writing tests in Haskell - that already was discussed in #4671.

@wolfgangwalther wolfgangwalther May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this discussion is probably better off in #4868, because many of the points you are addressing here, I already did there.


log "Initializing database cluster..."
# We try to make the database cluster as independent as possible from the host
Expand All @@ -80,7 +90,7 @@ let
# On MacOS, it's 104 chars
# See: https://serverfault.com/questions/641347/check-if-a-path-exceeds-maximum-for-unix-domain-socket

pg_ctl -l "$tmpdir/db.log" -w start -o "-F -c listen_addresses=\"\" -c hba_file=$HBA_FILE -k $PGHOST -c log_statement=\"all\" " \
pg_ctl -l "$tmpdir/db.log" -w start -o "-F -c listen_addresses=\"$TCP_PGHOST\" -c hba_file=$HBA_FILE -k $UNIX_PGHOST -c log_statement=\"all\" " \
>> "$setuplog"

log "Creating a minimally privileged $PGUSER connection role..."
Expand All @@ -93,6 +103,7 @@ let
replica_slot="replica_$RANDOM"
replica_dir="$tmpdir/$replica_slot"
replica_host="$tmpdir/socket_$replica_slot"
replica_port=$(${randomPort})

mkdir -p "$replica_host"

Expand All @@ -106,15 +117,16 @@ let
log "Starting replica on $replica_host"

# We set a low max_standby_streaming_delay to make the replication conflict fail faster in tests (otherwise it waits for the default 30s)
pg_ctl -D "$replica_dir" -l "$replica_dblog" -w start -o "-F -c listen_addresses=\"\" -c hba_file=$HBA_FILE -k $replica_host -c log_statement=\"all\" -c max_standby_streaming_delay=\"3s\" " \
pg_ctl -D "$replica_dir" -l "$replica_dblog" -w start -o "-F -c listen_addresses=\"$TCP_PGHOST\" -c port=$replica_port -c hba_file=$HBA_FILE -k $replica_host -c log_statement=\"all\" -c max_standby_streaming_delay=\"3s\" " \
>> "$setuplog"

>&2 echo "${commandName}: Replica enabled. You can connect to it with: psql 'postgres:///$PGDATABASE?host=$replica_host' -U postgres"
>&2 echo "${commandName}: You can tail the replica logs with: tail -f $replica_dblog"

export PGREPLICAHOST="$replica_host"
export PGREPLICAPORT="$replica_port"
export PGREPLICASLOT="$replica_slot"
export PGRST_DB_URI="postgres:///$PGDATABASE?host=$PGREPLICAHOST,$PGHOST"
export PGRST_DB_URI="postgres:///$PGDATABASE?host=$PGREPLICAHOST,$PGHOST&port=$replica_port,$PGPORT"
fi

# shellcheck disable=SC2329
Expand Down Expand Up @@ -372,6 +384,100 @@ let
libraries = [ python3Packages.pandas python3Packages.tabulate python3Packages.psutil ];
}
(builtins.readFile ./monitor_pid.py);

randomPort =
writers.writePython3 "postgrest-random-port"
{ }
''
import socket
s = socket.socket()
s.bind(("127.0.0.1", 0))
print(s.getsockname()[1])
s.close()
'';

withToxiproxyProxy =
checkedShellScript
{
name = "postgrest-with-toxiproxy-proxy";
docs = "Run <command> with Toxiproxy proxy created.";
args =
[
"ARG_POSITIONAL_SINGLE([command], [Command to run])"
"ARG_LEFTOVERS([command arguments])"
"ARG_OPTIONAL_SINGLE([listen], [l], [Proxy will listen on this address])"
"ARG_OPTIONAL_SINGLE([upstream], [u], [Proxy will forward to this address])"
];
positionalCompletion = "_command";
workingDir = "/";
withPath = [ toxiproxy ];
}
''
proxyname="tp$RANDOM"
toxiproxy-cli create -l "$_arg_listen" -u "$_arg_upstream" "$proxyname"

# shellcheck disable=SC2329
stop () {
Comment thread
wolfgangwalther marked this conversation as resolved.
toxiproxy-cli delete "$proxyname" || true
}
trap stop EXIT

(TOXI_PROXY_NAME="$proxyname" "$_arg_command" "''${_arg_leftovers[@]}")
'';

withToxiproxyPg =
checkedShellScript
{
name = "postgrest-with-toxiproxy-pg";
docs = "Run <command> with a Toxiproxy proxy to PostgreSQL.";
args =
[
"ARG_POSITIONAL_SINGLE([command], [Command to run])"
"ARG_LEFTOVERS([command arguments])"
"ARG_USE_ENV([TCP_PGHOST], [], [PG host name])"
"ARG_USE_ENV([PGPORT], [], [PG port])"
];
positionalCompletion = "_command";
workingDir = "/";
}
''
proxy_port=''$(${randomPort})

${withToxiproxy} ${withToxiproxyProxy} -l "$TCP_PGHOST:$proxy_port" -u "$TCP_PGHOST:$PGPORT" \
env "TOXI_PGPORT=$proxy_port" "$_arg_command" "''${_arg_leftovers[@]}"
Comment on lines +444 to +447

@wolfgangwalther wolfgangwalther Apr 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a better user-interface would be something where PGPORT was replaced with the proxy port, so that downstream consumers don't need to know whether they need to connect to TOXI_PGPORT or to PGPORT.

The addition of postgrest-with-toxiproxy-xxx should ideally be transparent.

WDYT?

@mkleczek mkleczek Apr 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point - if we don't need unix socket access then I agree.

'';

withToxiproxy =
checkedShellScript
{
name = "postgrest-with-toxiproxy";
docs = "Run <command> with toxiproxy-server";
args =
[
"ARG_POSITIONAL_SINGLE([command], [Command to run])"
"ARG_LEFTOVERS([command arguments])"
];
positionalCompletion = "_command";
workingDir = "/";
withPath = [ toxiproxy ];
}
''
if ! test -v TOXI_PROXY; then
export TOXI_PROXY=""
LOG_LEVEL=error toxiproxy-server&
TOXIPROXY_PID=$!
sleep 1 # give the server a moment to start

# shellcheck disable=SC2329
stop () {
kill "$TOXIPROXY_PID" || true
wait "$TOXIPROXY_PID" || true
}
trap stop EXIT
fi
("$_arg_command" "''${_arg_leftovers[@]}")
'';

in
buildToolbox
{
Expand All @@ -380,11 +486,12 @@ buildToolbox
inherit
withGit
withPgAll
withPgrst;
withPgrst
withToxiproxyPg;
} // builtins.listToAttrs (
# Create a `postgrest-with-pg-` for each PostgreSQL version
builtins.map (pg: { inherit (pg) name; value = withTmpDb pg; }) postgresqlVersions
);
# make latest withPg available for other nix files
extra = { inherit withPg; };
extra = { inherit withPg withToxiproxyPg; };
}
6 changes: 5 additions & 1 deletion test/io/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ def dburi():
"Postgres database connection URI."
dbname = os.environ["PGDATABASE"]
host = os.environ["PGHOST"]
port = os.environ["PGPORT"]
user = os.environ["PGUSER"]
return f"postgresql://?dbname={dbname}&host={host}&user={user}".encode()
return f"postgresql://?dbname={dbname}&host={host}&port={port}&user={user}".encode()


@pytest.fixture
Expand All @@ -19,6 +20,7 @@ def baseenv():
return {
"PGDATABASE": os.environ["PGDATABASE"],
"PGHOST": os.environ["PGHOST"],
"PGPORT": os.environ["PGPORT"],
"PGUSER": os.environ["PGUSER"],
}

Expand Down Expand Up @@ -51,6 +53,7 @@ def replicaenv(defaultenv):
**defaultenv,
**conf,
"PGHOST": os.environ["PGREPLICAHOST"] + "," + os.environ["PGHOST"],
"PGPORT": os.environ["PGREPLICAPORT"] + "," + os.environ["PGPORT"],
"PGREPLICASLOT": os.environ["PGREPLICASLOT"],
},
}
Expand All @@ -76,6 +79,7 @@ def metapostgrest():
env = {
"PGDATABASE": os.environ["PGDATABASE"],
"PGHOST": os.environ["PGHOST"],
"PGPORT": os.environ["PGPORT"],
"PGUSER": role,
"PGRST_DB_ANON_ROLE": role,
"PGRST_DB_CONFIG": "true",
Expand Down
2 changes: 1 addition & 1 deletion test/io/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def test_jwt_errors(defaultenv):

def test_fail_with_invalid_password(defaultenv):
"Connecting with an invalid password should fail without retries."
uri = f'postgresql://?dbname={defaultenv["PGDATABASE"]}&host={defaultenv["PGHOST"]}&user=some_protected_user&password=invalid_pass'
uri = f'postgresql://?dbname={defaultenv["PGDATABASE"]}&host={defaultenv["PGHOST"]}&port={defaultenv["PGPORT"]}&user=some_protected_user&password=invalid_pass'
env = {**defaultenv, "PGRST_DB_URI": uri}
with run(env=env, wait_for_readiness=False) as postgrest:
exitCode = wait_until_exit(postgrest)
Expand Down
2 changes: 1 addition & 1 deletion test/io/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2022,7 +2022,7 @@ def test_log_listener_connection_start(defaultenv):
# Check for the listener start message containing host and port
# Do not check if pg version is displayed properly as it is tricky to test it
assert any(
f'"{defaultenv["PGHOST"]}:5432" and listening for database notifications on the "pgrst" channel'
f'"{defaultenv["PGHOST"]}:{defaultenv["PGPORT"]}" and listening for database notifications on the "pgrst" channel'
in line
for line in output
)
Expand Down
Loading