search

timvisee / lazymc

💤 Put your Minecraft server to rest when idle.
GNU General Public License v3.0
552 stars 14 forks source link

Add a very basic support for multi-server handling. #42

Open tW4r opened 1 year ago

tW4r commented 1 year ago

I have tried fixing #40 myself as a bit of a challenge to learn Rust. This is my first time touching Rust so the code quality is probably terrible, I am having tons of trouble understanding references, Arc, Mutex and RwLocks, and the borrow checker in general.

However, I have manage to create a very rough proof-of-concept that seems to work somewhat.

What has changed:

Here's an example of how to make it work

#/config/serverA.toml
[public]
address="server-a.example.com"
...

#/config/serverB.toml
[public]
address="server-b.example.com"
...
lazymc --config /config

Allows users to join either serverA or serverB via the same port depending on how it is described in their settings.

TODO:

timvisee commented 1 year ago

Thank you very much for taking a shot at this! That is very much appreciated.

I do have limited time right now to dive into this, but I'll try to leave a proper review. Some comments may be nit-picky.

timvisee commented 1 year ago

Well done thus far, especially since you're new to Rust!

Please see my review comments above.

I think that config: public.address should be kept intact (or be renamed to config: public.bin). Maybe we can use config: server.route or something to configure what address each individual server listens for, like:

[public]
address = "0.0.0.0:25565"

[server]
route = "example.com"
address = "internal_address"
# ...

Remember that clippy is your friend, its warnings are fantastic to improve code. Simply run cargo clippy (running rustup component add clippy once may be required).

Please note that I pushed some commits on your branch. Please make sure to pull again to prevent conflicts: git pull

tW4r commented 1 year ago

Thank you for such a quick review and support, implemented the small syntactic fixes you suggested.

Seems like two issues are arising in this PR:

  1. Bind addresses, routes and their config
  2. Server config/state sharing between threads I have some ideas on how to fix both, could you tell me if it seems like the right direction.

1. Bind addresses, routes and their config

Outlining some definitions because it might become confusing:

Initial implementation

I have implemented what seemed to me to be the easiest solution:

There is a single bind address that lazymc listens on, because it is global it cannot be defined in server configs, because then lazymc should bind to multiple bind addresses. Because of this the [public] address server config variable became irrelevant, and I chose to use it as the server name.

This has the following problems:

Proposed implementation

Even when starting I was heavily inspired by NGINX Server Blocks. I imagine the server config files could be equivalent to server blocks. Drawing from that I imagine we could have similar functionality.

Each server config would have:

Due to bind address being defined again in server config the cli argument is no longer necessary.

This could be achieved by:

This requires being able to find a server config based on (bind address+server name), to achieve this Arc<HashMap<_, Arc<Config>>> may need to be reworked.

2. Server config/state sharing between threads

With the growing complexity of the Config/Server sharing it may be beneficial to create a new data structure Servers that would have some way to retrieve references to individual server config and/or state based on bind address + server name, perhaps with a function like: Servers::find(bind_address: SocketAddr, server_name: String)

Additionally, would it make sense to move crate::Config into crate::Server on crate::Server::new?

Do you think ^ is the right direction to take, or have any insights?

tW4r commented 1 year ago

Pushed initial implementation of the proposal,

it involved:

Mostly seems to work, the only problem remaining is some trouble with ping packets, after a server it's started it's MOTD is shown as "Can't connect to server..." and status packets do not get through either, but joining the server works

Would also still like to support multiple [public] address and [server] name in cases users want to listen to multiple ports/server names, but this is not a requirement of mine

tW4r commented 1 year ago

I've made the new implementation work similar to original implementation by moving handshake packet parsing to the route function in src/service/server and then deciding whenever it should proxy there. The code definitely requires refactoring but it seems to fully work for now and I am testing it in my environment.

tW4r commented 1 year ago

Closed by mistake

timvisee commented 1 year ago

Fantastic work so far! You've hidden your main outline, but I think that represents a very nice goal.

1. Bind addresses, routes and their config

Outlining some definitions because it might become confusing:

* **Bind address** (public address) - address that lazymc binds to and listens for new connections from, e.g.: `0.0.0.0:25565`

* **Server name** (route) - the `server_address` present in the login packet presented by the Minecraft client to server according to which the backend server is chosen by lazymc, e.g.: `server-a.example.com`, `server-b.example.com`

* **Server address** - the backend (internal) address the actual minecraft `server.jar` is bound and listens to, e.g. `127.0.0.1:25566`

* **Server config** - lazymc.toml or /config/*.toml files that define the config for each individual minecraft server

Initial implementation

I have implemented what seemed to me to be the easiest solution:

There is a single bind address that lazymc listens on, because it is global it cannot be defined in server configs, because then lazymc should bind to multiple bind addresses. Because of this the [public] address server config variable became irrelevant, and I chose to use it as the server name.

This has the following problems:

* It is a breaking **server config** change.

* Each server can only have a single **server name**.

Proposed implementation

Even when starting I was heavily inspired by NGINX Server Blocks. I imagine the server config files could be equivalent to server blocks. Drawing from that I imagine we could have similar functionality.

Each server config would have:

* (possibly multiple) **bind address** as `[public] address`, multiple **server configs** can share the same **bind address**

* (possibly multiple) **server name** as `[server] name`

  * **server name** is not required, if it is not provided it works as a "catch-all" **server config**, if a client connects to lazymc and no **server names** match, the client is routed to the any "catch-all" **server config**

    * What to do when no "catch-all" servers exist? Should lazymc error, route to any server?
  * if multiple **server names** share the same server name the client is routed to any matching **server config**

Due to bind address being defined again in server config the cli argument is no longer necessary.

This could be achieved by:

* On startup lazymc reads all **server configs** and starts multiple `TCPListener` for all (unique) **bind addresses** found in configs

* On connection **server name** is read from handshake, and client is routed to the matching **server config** on the given **bind address**, or the "catch-all" server of the given **bind address** if no matching **server configs** found

This requires being able to find a server config based on (bind address+server name), to achieve this Arc<HashMap<_, Arc<Config>>> may need to be reworked.

Yes, this sounds very solid. Such an implementation would be great. It would also be surprisingly compatible with how the project currently works.

Eventually the server.name and public.bind values may optionally become a list, to allow to configure multiple names and binds. This is something to later, we can leave this for now.

if multiple server names share the same server name the client is routed to any matching server config

I don't think it should randomly pick one. I think better would be to always pick the first one. The configurations should then be ordered by: the order in which they're provided to lazymc, and for directories, in alphabetical order. This is a common approach on Unix systems. This may be somewhat advanced, so feel free to leave this for now.

I like the ability to have multiple (possible) bind addresses, and the ability to share them across servers.

2. Server config/state sharing between threads

With the growing complexity of the Config/Server sharing it may be beneficial to create a new data structure Servers that would have some way to retrieve references to individual server config and/or state based on bind address + server name, perhaps with a function like: Servers::find(bind_address: SocketAddr, server_name: String)

:+1:

Additionally, would it make sense to move crate::Config into crate::Server on crate::Server::new?

If I'm right you've already done this. Great!

Do you think ^ is the right direction to take, or have any insights?

Yes! If you're still wanting to do this.

The two insights I have is accepting an optional list of names and binds, and keeping the configurations in order. But we can do that later.

The code definitely requires refactoring but it seems to fully work for now and I am testing it in my environment.

Did this also fix the ping/status packets?

Please keep working on this if you'd like. I'm not available every day to look at this, but I'll try to properly review, test, tweak and merge eventually.

tW4r commented 1 year ago

Thanks again for the support! FIY: I've hidden the outline as it has been implemented already. (except the multiple server names and public addresses). I'll keep working on enabling multiple server namees and public addresses and deterministic choosing of the config.

What do you think about choosing the last server config instead of the first when multiple servers compete for the same server name?

timvisee commented 1 year ago

Based on a static code analysis, this looks great! I still have to actually test this on my machine, but I'll do that later.

It may be nicer to create a single router somehow, so we don't have to share Server instances. I couldn't really think of a better implementation architecturally, so I'll look into this while testing.

Thanks for your work!

tW4r commented 1 year ago

I've been testing this for a bit on my home server, and I have noticed there is some bug, not sure on how exactly to reproduce it, but sometimes it seems it has some trouble with the initial connection.

One scenario that has happened more than once:

  1. Log in to server and play normally
  2. Disconnect from server
  3. The server cannot be pinged anymore, and when trying to connect it says "Connecting to server..."
  4. Minecraft has to be restarted to make it work again

Will look into reproducing this more

timvisee commented 1 year ago

Weird!

Minecraft has to be restarted to make it work again

The Minecraft client or server?

tW4r commented 1 year ago

Weird!

Minecraft has to be restarted to make it work again

The Minecraft client or server?

Sorry for taking a bit to respond, it's the minecraft client

timvisee commented 1 year ago

Weird!

Minecraft has to be restarted to make it work again

The Minecraft client or server?

Sorry for taking a bit to respond, it's the minecraft client

That's super weird! I don't think a server should ever able to cause this, meaning it would be a client bug.