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

tW4r · 2023-02-09T15:09:21Z

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:

cli now supports additional argument --bind (127.0.0.1:25565 by default) that outlines the global bind for TCPListener
cli argument --config now can point to either a single config file (lazymc.toml by default) or a whole directory including multiple config files
BREAKING: [public] address in server config is no longer the address TCPListener binds to, now it describes the server_address part of the handshake pattern, by which a server is selected
(terribly) reworked the routing to start the handshake before a server is decided and then decide and proxy the server based on the handshake

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:

feat: Add ability to support multiple different addresses for each server.
feat: More graceful
bug: After starting the server, it's status packets are not proxied correctly, it shows "Can't connect to server" even though connecting fully works.

* Added a new cli param `bind` ("127.0.0.1:25565" by default) for lazymc global binding * Routing now directs all traffic to status service rather than proxying it if server has stared as it is server dependent * Removed all ban checking from routing as it is server dependent * Status server now chooses config/server based on the handshake * Status server is now responsible for strating to proxy if server is started * BUG: After server has started the server status is not shown somewhy

src/config.rs

src/service/server.rs

src/status.rs

timvisee · 2023-02-10T10:03:50Z

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.

src/action/start.rs

timvisee · 2023-02-10T10:22:59Z

src/action/start.rs


    // Start server service
-    let config = Arc::new(config);
-    service::server::service(config)
+    let configs_arc = configs.into_iter().map(Arc::new).collect();


I don't think Arc<HashMap<_, Arc<Config>>> is desirable, as this is a very complex type.

I used Arc<Config> to make it easily sharable across async contexts with message passing.

I'll have to think about something that would be better here. I'm not sure right now. Maybe we need to create a custom type to manage these configurations for us rather than working with a hashmap directly.

Added a new type Router in src/router.rs to deal with this, what do you think?

src/config.rs

src/status.rs

src/proxy.rs

src/service/server.rs

timvisee · 2023-02-10T10:39:00Z

src/service/server.rs


-    if config.lockout.enabled {
-        warn!(
+    for config in configs.iter() {


We should probably extract this (the logic for doing something for many configuration files) into a separate function we can invoke for each configuration.

This is now a bit messy (and I admit my code already was).

timvisee · 2023-02-10T10:39:36Z

src/service/server.rs

-    // Load server state
-    let server = Arc::new(Server::default());
-
+pub async fn service(bind_addr: SocketAddr, configs: Vec<Arc<Config>>) -> Result<(), ()> {


I haven't inspected this in-depth right now. I'll try to do that later when the other changes have been made.

timvisee · 2023-02-10T10:41:35Z

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

* Moved Config under Server * Reverted `[public] address` removal * Removed cli `bind` argument

tW4r · 2023-02-11T12:55:33Z

Pushed initial implementation of the proposal,

it involved:

Moving Config under Server
Reverting [public] address removal
Removing cli bind argument

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 · 2023-02-14T07:18:45Z

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 · 2023-02-14T07:30:16Z

Closed by mistake

timvisee · 2023-02-14T13:31:58Z

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)

👍

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 · 2023-02-14T13:55:22Z

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?

Fixes timvisee#26

timvisee · 2023-02-21T16:41:24Z

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 · 2023-02-24T19:48:37Z

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:

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

Will look into reproducing this more

timvisee · 2023-02-24T19:51:54Z

Weird!

Minecraft has to be restarted to make it work again

The Minecraft client or server?

tW4r · 2023-03-02T09:00:07Z

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 · 2023-03-02T09:03:59Z

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.

tW4r mentioned this pull request Feb 9, 2023

Reverse proxy to multiple minecraft servers based on domain #40

Open

tW4r commented Feb 9, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

tW4r commented Feb 9, 2023

View reviewed changes

src/service/server.rs Outdated Show resolved Hide resolved

tW4r commented Feb 9, 2023

View reviewed changes

src/status.rs Outdated Show resolved Hide resolved

tW4r commented Feb 9, 2023

View reviewed changes

src/status.rs Outdated Show resolved Hide resolved

timvisee assigned timvisee and tW4r Feb 10, 2023

timvisee added the enhancement New feature or request label Feb 10, 2023

timvisee added 2 commits February 10, 2023 11:20

Merge branch 'master' of github.com:tW4r/lazymc into tW4r-master

b879047

Resolve clippy warnings

4d90011

timvisee requested changes Feb 10, 2023

View reviewed changes

tW4r added 2 commits February 10, 2023 15:01

Implement suggested syntax changes

2a55c92

Formatting

487c190

This comment was marked as outdated.

Sign in to view

Made lazymc listen on all [public] address

269db95

* Moved Config under Server * Reverted `[public] address` removal * Removed cli `bind` argument

Make it closer to original implementation

ce21d56

tW4r marked this pull request as ready for review February 14, 2023 07:18

tW4r deleted the branch timvisee:master February 14, 2023 07:29

tW4r closed this Feb 14, 2023

tW4r deleted the master branch February 14, 2023 07:29

tW4r restored the master branch February 14, 2023 07:29

tW4r reopened this Feb 14, 2023

tW4r and others added 4 commits February 15, 2023 16:42

Merge remote-tracking branch 'upstream/master'

dbc0d98

Allow exit code 143 and 130

023e46f

Fixes timvisee#26

Bump version to 0.2.10

e54025f

Merge branch 'master' of github.com:tW4r/lazymc into tW4r-master

02a8ba4

timvisee approved these changes Feb 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tW4r commented Feb 9, 2023

timvisee commented Feb 10, 2023

timvisee Feb 10, 2023

tW4r Feb 11, 2023

timvisee Feb 10, 2023

timvisee Feb 10, 2023

timvisee commented Feb 10, 2023 •

edited

This comment was marked as outdated.

tW4r commented Feb 11, 2023 •

edited

tW4r commented Feb 14, 2023

tW4r commented Feb 14, 2023

timvisee commented Feb 14, 2023 •

edited

1. Bind addresses, routes and their config

Initial implementation

Proposed implementation

2. Server config/state sharing between threads

tW4r commented Feb 14, 2023

timvisee commented Feb 21, 2023

tW4r commented Feb 24, 2023

timvisee commented Feb 24, 2023

tW4r commented Mar 2, 2023

timvisee commented Mar 2, 2023

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

Are you sure you want to change the base?

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

Conversation

tW4r commented Feb 9, 2023

timvisee commented Feb 10, 2023

timvisee Feb 10, 2023

Choose a reason for hiding this comment

tW4r Feb 11, 2023

Choose a reason for hiding this comment

timvisee Feb 10, 2023

Choose a reason for hiding this comment

timvisee Feb 10, 2023

Choose a reason for hiding this comment

timvisee commented Feb 10, 2023 • edited

This comment was marked as outdated.

tW4r commented Feb 11, 2023 • edited

tW4r commented Feb 14, 2023

tW4r commented Feb 14, 2023

timvisee commented Feb 14, 2023 • edited

1. Bind addresses, routes and their config

Initial implementation

Proposed implementation

2. Server config/state sharing between threads

tW4r commented Feb 14, 2023

timvisee commented Feb 21, 2023

tW4r commented Feb 24, 2023

timvisee commented Feb 24, 2023

tW4r commented Mar 2, 2023

timvisee commented Mar 2, 2023

timvisee commented Feb 10, 2023 •

edited

tW4r commented Feb 11, 2023 •

edited

timvisee commented Feb 14, 2023 •

edited