search

PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
22.52k stars 996 forks source link

Server port and admin server port can end up with the same value #3508

Open wolfgangwalther opened 2 weeks ago

wolfgangwalther commented 2 weeks ago

As found out in https://github.com/PostgREST/postgrest/issues/3424#issuecomment-2106000902.

Do this:

PGRST_SERVER_PORT=3000 PGRST_SERVER_HOST="localhost" PGRST_ADMIN_SERVER_PORT=3000 postgrest-with-postgresql-16 postgrest-run

and observe:

11/May/2024:21:30:27 +0200: Admin server listening on port 3000
11/May/2024:21:30:27 +0200: Listening on port 3000

One of them is listening on ipv4, the other on ipv6.

We should probably reject this case on the config level? Admin port should always be different from server port?

develop7 commented 2 weeks ago

One of them is listening on ipv4, the other on ipv6.

Generally, a user expects us to stick to one protocol, right? Meanwhile, one way to reduce confusion would be reporting URLs of apps to reach them at instead of just port numbers (and we do have all the info required for it).

develop7 commented 2 weeks ago

The problem here is the app server binds to [::1]:3000 and admin server binds to 127.0.0.1:3000 because bindPortTCP binds to whatever address available and localhost resolves in both [::1] and 127.0.0.1 addresses.

Anyway, with #3512 admin server is bound to the same address as main, so specifying the same port results in a runtime error:

❯  PGRST_SERVER_PORT=3000 PGRST_SERVER_HOST='localhost' PGRST_ADMIN_SERVER_PORT=3000 postgrest-with-postgresql-16 postgrest-run
postgrest-with-postgresql-16: You can connect with: psql 'postgres:///postgres?host=/run/user/1000/postgrest/postgrest-with-postgresql-16-BBy/socket' -U postgres
postgrest-with-postgresql-16: You can tail the logs with: tail -f /run/user/1000/postgrest/postgrest-with-postgresql-16-BBy/db.log
postgrest: Network.Socket.bind: resource busy (Address already in use)
Temporary directory kept at: /run/user/1000/postgrest/postgrest-with-postgresql-16-BBy

Although the above runtime error is rather uninformative. I wonder if I should validate the config and fail if port numbers match, or just add logs, like binding main app to <hostname>:<port>; binding admin app to <hostname>:<port>? The latter could be useful to admins either way, and the runtime error would make perfect sense either way: when ports are equal — the problem is with the PostgREST config; otherwise there's some other service occupying the same port.

develop7 commented 2 weeks ago

Okay, we actually do diagnostic messages "[Admin server] listening on port <port>", but in the case of the port conflict neither of those is shown before the runtime error.

wolfgangwalther commented 2 weeks ago

Generally, a user expects us to stick to one protocol, right?

This sounds very plausible - but I'm not 100% sure.

I can easily imagine scenarios where I don't want this to be the case.

Isn't the whole problem here that we only have server-host, but not admin-server-host?

I think it's quite reasonable to have the admin server listen on 127.0.0.1 only, but the regular server on 0.0.0.0.

So even if it's the same protocol, we probably still need a way to allow setting different hosts.

Meanwhile, one way to reduce confusion would be reporting URLs of apps to reach them at instead of just port numbers (and we do have all the info required for it).

Agreed, that would be really good.

Overall, even given both admin-server-host and the display of URLs, I think we should still prevent using the same port number on both on the config level already. This only leads to confusion and ambiguity.

develop7 commented 1 week ago

So, to summarize:

I'll cook a PR

wolfgangwalther commented 1 week ago

we have to make sure the host & port of both app and admin servers are not completely equal

I think it would be fine to block on the same port only. This leads to less confusion and is also much easier to do, because you know the port beforehand. The host could be "localhost" and "127.0.0.1" - and then it becomes much harder to tell whether those are equal or not. You could actually only check after binding to the interface. So I'd like the port-only check at startup, way before trying to open any sockets, more.