Add ServerHandshakeEvent

[techchrism]

The ServerHandshakeEvent hooks before the login handshake packet is sent and enables the values to be modified (after the various forwarding mode and connection type rules are applied).

This is useful to me for changing the address sent to the server as an early means of sending custom data from velocity->paper without sending additional packets. This was previously possible with BungeeCord and is an impediment to porting plugins.

I have not previously contributed to Velocity and I am unsure if this is the proper approach for implementing this behavior. Changes are welcome.

[CoreyShupe]

Considering this PR exposes a few values which would break quite a lot of logic in the backend - we should write a better API for interacting with certain scenarios.

Changing things like the protocol version of a player will put the player in a position of being sent improper data.

Supporting something like this in a "good" way is probably not going to happen.

TL;DR there's way too much plainly exposed as API; there should be better ways to interact with the system to achieve the same result.

[techchrism]

It would be fairly simple to move the protocolVersion and nextStatus variables from the immutable (but reconstructable) data object up to the event itself, preventing them from being modified.

The other two, port and serverAddress, are not used by the notchian server and should be safe to modify without backend consequences.

[techchrism]

https://github.com/PaperMC/Velocity/pull/916/commits/a21da3689a126eeeb0a686f7b14fac457aff3b5f addresses this. As far as I'm aware, using this api to set arbitrary values will have no effect on backend logic.

[CoreyShupe]

So if I'm understanding correctly the entire point of this would be to update the virtual host used to connect to the remote. Nothing else seems to necessarily matter here. Why not a ServerAddressPrepare event which provides/modifies specifically the server address it will use for the server connection.

I think that would be much more useful and we could provide better details like the original value; a potential new value; and other functional things based on the server which the event is tied to.

EDIT: This could potentially be added logic in the ServerPreConnectEvent

[techchrism]

From what I understand of what you're saying, that's precisely what this is but with a different name. The sole purpose is to modify the address and port fields of the handshake packet from the proxy to the server, not to modify any other connection logic or backend data.

I briefly looked into the connection flow to see if the ServerPreConnect event would be more appropriate and while it could work, there's a few layers that data would need to be passed through until the packet gets sent out. This seemed to be a simpler approach.

[CoreyShupe]

From what I understand of what you're saying, that's precisely what this is but with a different name. The sole purpose is to modify the address and port fields of the handshake packet, not to modify any other connection logic or backend data.

I briefly looked into the connection flow to see if the ServerPreConnect or event would be more appropriate and while it could work, there's a few layers that data would need to be passed through until the packet gets sent out. This seemed to be a simpler approach.

            ServerPreConnectEvent event = new ServerPreConnectEvent(ConnectedPlayer.this,
                toConnect, previousServer);
            return server.getEventManager().fire(event)
                .thenComposeAsync(newEvent -> {
                  Optional<RegisteredServer> newDest = newEvent.getResult().getServer();
                  if (!newDest.isPresent()) {
                    return completedFuture(
                        plainResult(ConnectionRequestBuilder.Status.CONNECTION_CANCELLED, toConnect)
                    );
                  }

                  RegisteredServer realDestination = newDest.get();
                  Optional<ConnectionRequestBuilder.Status> check = checkServer(realDestination);
                  if (check.isPresent()) {
                    return completedFuture(plainResult(check.get(), realDestination));
                  }

                  VelocityRegisteredServer vrs = (VelocityRegisteredServer) realDestination;
                  VelocityServerConnection con = new VelocityServerConnection(vrs,
                      previousServer, ConnectedPlayer.this, server);
                  connectionInFlight = con;
                  return con.connect().whenCompleteAsync(
                      (result, exception) -> this.resetIfInFlightIs(con), connection.eventLoop());
                }, connection.eventLoop());

Looks like there's a way by passing in the result to the VelocityServerConnection - or even overriding it post-creation; then the VelocityServerConnection handles all handshake creation operations. If we pass it in as a nullable field we can use it as a "first option" for the server address

I can't think of a reason port would matter but I don't think there's any harm in allowing modification of that.

I think it fits in the ServerPreConnectEvent since that's meant to prepare the system for the connection - whatever "pre connect" behavior needs to be defined to properly handle that should most likely go there.

I think if we do that; and drop all the other fields from even appearing API-side that would be better. The player should be able to manage and scope their own protocol version (which is provided through the event anyways). Providing both doesn't necessarily make sense.

I think NextState could be fine in there only as an immutable identifier for what the system is planning on doing.

I don't think a new event which exposes everything in a new place is necessarily the right way to go.

I think the logic right now for determining what virtual host to use is a bit ambiguous so we might want to add in more documentation to make it more clear. I think the system around that could definitely be more clear in what host it's going to use for the connection.

[techchrism]

I agree that the virtual host logic is somewhat unclear but that was one of the reasons I decided to trigger the event after the default virtual host was decided. This allows plugins to observe or content-aware modify (eg. append) the address instead of a comparatively blind "overwrite before the value is known".

If all of that logic was moved to a place before the ServerPreConnectEvent was fired, this approach could be maintained.

[CoreyShupe]

I think it might be time to move the logic into the pre connect and change the behavior of pre-connect to be more of a full-fledged "we need to decide how to setup a full-connection". When a VelocityServerConnection is created we should expect to have a full understanding of the server address used.

In the case of bungeecord forwarding "appending"; I don't think the user should be aware - that is completely an implementation detail of a "legacy" platform. Providing API support for something like that doesn't necessarily make sense.

[techchrism]

I'll see what I can do with regards to moving the logic.

For the appending example, who is the "user" in this case?

[CoreyShupe]

I'll see what I can do with regards to moving the logic.

For the appending example, who is the "user" in this case?

Referring to the user of the library; the plug-in developer. The bungeecord encoding is simply an implementation detail of the forwarding; exposing it in the "server address" doesn't make sense since that's not what's passed through.

The bungeecord encoding does put the "server address" at the end so that's what should be expected as the output.

I think including the appending could cause issues - especially with people using legacy forwarding and expecting to modify the server address rather than the encoding scheme for bungeecord forwarding.

[techchrism]

The "appending" was just an example of content-aware modification, it of course wouldn't actually be useful because for bungee encoding, the server checks the number of null-separated segments (meaning another segment can't be added) and the last segment must be either valid json or a UUID.

Ensuring the user (plugin) can read the original to-be-sent data and use that to craft an override makes handling legacy encodings the responsibility of the plugin and not Velocity.

One possible way of doing this is to recalculate the "original" handshake packet "hostname" (to use Mojang mapped terminology) and port whenever the ServerPreConnectEvent result server is changed.

[techchrism]

There are two main things I believe:

• Modifying the handshake hostname and port is a niche but useful feature that should be added to the Velocity API
• Content-aware modification functionality through exposing the original, unmodified data is useful to avoid plugin developers reimplementing legacy encoding logic in an otherwise blind override. It is also a pattern used elsewhere in the Velocity API.

By combining these, it makes the most sense to me for Velocity to first be certain of the server it's attempting to connect to, then begin crafting handshake data. While it is possible to craft handshake data "on the fly" while the destination server is unknown, this approach doesn't feel quite right and seems like a shoehorned attempt to force both in the same space.

This is why I feel a separate event, after the server is decided but before the handshake is sent, makes the most sense. From what I could tell, there wasn't another event between those two that I could piggyback off of.