postgrest icon indicating copy to clipboard operation
postgrest copied to clipboard

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

Open wolfgangwalther opened this issue 1 year ago • 6 comments

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?

wolfgangwalther avatar May 11 '24 19:05 wolfgangwalther

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 avatar May 13 '24 15:05 develop7

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 avatar May 14 '24 10:05 develop7

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.

develop7 avatar May 14 '24 11:05 develop7

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.

wolfgangwalther avatar May 15 '24 18:05 wolfgangwalther

So, to summarize:

  • an admin-server-host setting should be introduced, defaulting to server-host value
  • we have to make sure the host & port of both app and admin servers are not completely equal
  • we need to log both host & port either app got bound to (right now only the port is logged, which leads to confusion)

I'll cook a PR

develop7 avatar May 22 '24 15:05 develop7

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.

wolfgangwalther avatar May 22 '24 15:05 wolfgangwalther