minetest / serverlist

The global Minetest server list server
GNU Lesser General Public License v2.1
52 stars 28 forks source link

Persist servers in separate database #45

Open ShadowNinja opened 3 years ago

ShadowNinja commented 3 years ago

Summary of Changes

Persistent SQL Database

Servers are now stored persistently in an SQL database. This offers the following advantages:

Persistent server state

We need a way to match announcements to servers in the database. The way this used to be done is with the announce_ip:port pair, however this is not a good option going forward because servers can change IPs (e.g. moving to different host or using a dynamic IP).

A better option is address:port because you can maintain the same address when moving the server. However this still not ideal because you can't change the port or move to a different domain name, and without address verification it can be spoofed.

The solution is to add a world_uuid. This is stored with the world and allows the world to be moved freely and still identified. This value is kept secret to prevent spoofing of the server's announcements.

An alternative possibility would be to use a pubkey/privkey pair to identify servers, but this would be more complicated.

Fallback

address:port is used as a fallback for old servers, which should also be unspoofable with address verification enabled.

Address verification is needed to prevent spoofing with servers that do not support world_uuid, however it can fail if a server is set up for IPv4 and announces over IPv6. Last I checked there were 71/315 servers with this issue, so it's not something we just want to ignore. Therefore, Address verification has been moved into the view function and a detailed error response is returned if it fails. The response includes detailed instructions on how to fix the issue. Address verification is skipped if world_uuid is sent.

There was also one server that (minetest.zarnica.org.ua:30001) that fails the verification check for a different reason: it uses IPv4, but announces over a different IPv4 address than it listens on. Perhaps this server has multiple NICs. It doesn't seems like this is a major concern since it's just one server with an odd networking config.

Background tasks

Celery is now used to handle background tasks. This requires a message broker (Redis or RabbitMQ) to be available.

We can probably use something simpler if this is deemed too complicated. We can probably move the entire request processing into the view function to remove the need for the request processing background task. This would also allow up to serve more useful error messages if there's an issue in the final verification steps before publishing the server.

The maintenance background task could be moved to a simpler scheduler like APScheduler, or even just cron with a cli command. However, it will have to run in a separate process so that there aren't multiple schedulers if there are multiple web processes.

List changes

The following fields are no longer included in the server list:

Also server_id was officially added. Multicraft clients set this to "multicraft". It worked before because the server list allowed arbitrary extra fields.

The list is not immediately updated when a new server announces any more, the server may not show up for up to a minute.

Ranking changes

The method used to calculate popularity (pop_v) has changed. It is now rolling average (like /proc/loadavg). This is needed because with persistent server storage the more recent popularity has to be weighted more heavily so that an old server that gets more popular isn't stuck with its old low popularity.

Ping checking

Servers are pinged a few times and the lowest value is used. Pings are also retried a few times in case of packet loss.

The server list re-pings every server every minute when the list is updated, rather that just sending one ping on start and never updating it until the server restarts. This could be tweaked if that's too often.

Misc

sfan5 commented 2 years ago

Last I checked there were 71/315 servers with this issue, so it's not something we just want to ignore. Therefore, Address verification has been moved into the view function and a detailed error response is returned if it fails.

But we can't just put the stricter validation into effect soon. Server owners cannot be properly notified until they have upgraded to 5.5.0, which will take some time even after it is released.

The server list re-pings every server every minute when the list is updated, rather that just sending one ping on start and never updating it until the server restarts.

I'm not convinced we need to update the ping between the 5 minutes a server usually announces.

Ranking changes

These sound mostly fine to me but it'd be better to move these into a future PR so that this one doesn't get tangled in discussions about ranking changes.

Celery is now used to handle background tasks. This requires a message broker (Redis or RabbitMQ) to be available. We can probably use something simpler if this is deemed too complicated. We can probably move the entire request processing into the view function to remove the need for the request processing background task.

Not sure if I like Celery yet, it's probably fine. Though if it's done simpler (just start a background thread from Flask?) I wouldn't mind. Moving processing into the requests isn't a good idea and not just for the obvious reasons. Minetest will currently(?) abort cURL requests after 5 seconds which might be too tight to do all the work.

Use Pipfile instead of requirements.txt.

pip can still read these and they not specific to Pipenv, are they?

rubenwardy commented 2 years ago

ContentDB uses Celery and SQLAlchemy - I recommend both. The problem with a background thread is when you have multiple web workers, you can end up potentially duplicating work

Also, I would suggest as an alternative to generating list.json to instead make it a Flask endpoint. You can then cache in Nginx or Apache. This feels cleaner

ShadowNinja commented 2 years ago

We can't just put the stricter validation into effect soon. Server owners cannot be properly notified until they have upgraded to 5.5.0, which will take some time even after it is released.

I have an idea to solve this: We could try to verify the server and if it succeeds set an address_verification_required bit in the database. This way servers with a compatible configuration still get spoof protection.

I'm not convinced we need to update the ping between the 5 minutes a server usually announces.

I could bump this to 5 minutes.

[Ranking changes] sound mostly fine to me but it'd be better to move these into a future PR so that this one doesn't get tangled in discussions about ranking changes.

OK. The popularity change will still be needed, but the rest can be separated.

pip can still read [Pipfiles] and they not specific to Pipenv, are they?

No, you need pipenv. I could also include a requirements.txt.

sfan5 commented 2 years ago

I have an idea to solve this: We could try to verify the server and if it succeeds set an address_verification_required bit in the database. This way servers with a compatible configuration still get spoof protection.

Good idea. Only side effect (that could result in more support questions) is that announces are no longer deterministic, if you pass domain verification once you may never fail it again.

I could bump this to 5 minutes.

Rather just remove that code entirely then? Servers are pinged on announce and those happen every 5 minutes.

ShadowNinja commented 2 years ago

Only side effect (that could result in more support questions) is that announces are no longer deterministic, if you pass domain verification once you may never fail it again.

Yes, not sure if there's much we can really do about this. It seems unlikely that a broken config would get fixed and then break again, so hopefully this won't be an issue in practice.

Servers are pinged on announce and those happen every 5 minutes.

Good point. I've removed the periodic server-initiated ping.

sfan5 commented 2 years ago

Can you rebase this? I want to put this to the test by mirroring all of the requests to it.

ShadowNinja commented 8 months ago

@sfan5 Rebased.