Allow setting forwarding mode separately for different servers

[wafarm]

Fix #304

It is quite common to have a setup which includes both servers newer than 1.12 and older than 1.12. By allowing setting forwarding modes separately, we don't need to use the legacy forwarding mode all the time. Especially it is discontinued for newer fabric versions.

This PR is fully compatible with old config files and no changes to API are made.

You can find builds with this change included here: https://github.com/wafarm/Velocity/releases

[paulzzh]

Incompatible with Ambassador 1.4.4

[10:51:32 ERROR]: [server connection] Paulzzh -> SU: exception encountered in com.velocitypowered.proxy.connection.backend.LoginSessionHandler@5eb9676e
io.netty.handler.codec.EncoderException: java.lang.NoSuchMethodError: 'com.velocitypowered.proxy.config.PlayerInfoForwarding com.velocitypowered.proxy.config
.VelocityConfiguration.getPlayerInfoForwardingMode()'
Caused by: java.lang.NoSuchMethodError: 'com.velocitypowered.proxy.config.PlayerInfoForwarding com.velocitypowered.proxy.config.VelocityConfiguration.getPlayerInfoForwardingMode()'
        at org.adde0109.ambassador.velocity.backend.FMLMarkerAdder.encode(FMLMarkerAdder.java:32) ~[?:?]
        at org.adde0109.ambassador.velocity.backend.FMLMarkerAdder.encode(FMLMarkerAdder.java:19) ~[?:?]
        at io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:90) ~[velocity-3.3.0-SNAPSHOT-400.jar:3.3.0-SNAPSHOT (git-bb3d3b07)]
        ... 26 more

[wafarm]

Ambassador used getPlayerInfoForwardingMode, which is not part of velocity api and is renamed in this PR.

[powercasgamer]

Ambassador used getPlayerInfoForwardingMode, which is not part of velocity api and is renamed in this PR.

Don’t really see how that’s relevant? As you’ve already stated it’s not part of the API and thus subject to breaking changes.

[wafarm]

If a mod only uses com.velocitypowered:velocity-api as the dependency, it won't be affected. But Ambassador uses com.velocitypowered:velocity-proxy as well, and therefore it breaks due to the renamed method.

[powercasgamer]

If a mod only uses com.velocitypowered:velocity-api as the dependency, it won't be affected. But Ambassador uses com.velocitypowered:velocity-proxy as well, and therefore it breaks due to the renamed method.

Yes, but I don’t really see how that’s relevant? Using internals is not supported

[wafarm]

Yes, but I don’t really see how that’s relevant? Using internals is not supported

Yes, I'm just explaining why it's incompatible since he posted.

[Rattlyy]

bump?

[Timongcraft]

Shouldn't this be paired with a pull request to update the docs?

[Nacioszeczek]

Shouldn't this be paired with a pull request to update the docs?

Yes, but the docs might as well be changed after this is merged. You're free to open a PR if you want to though.

[liorsl]

Just a note here: should we consider also allowing to have forwarding secret configured for each server / each connection?

[wafarm]

Just a note here: should we consider also allowing to have forwarding secret configured for each server / each connection?

It would be possible, but I wonder what the use case is, i.e. why do you need to have different forwarding secrets for different servers?

[liorsl]

Just a note here: should we consider also allowing to have forwarding secret configured for each server / each connection?

It would be possible, but I wonder what the use case is, i.e. why do you need to have different forwarding secrets for different servers?

It might not be the most common one but I think its there, it will mainly make the secrets harder to get because there are multiple ones, so at the end it improves security

[wafarm]

It might not be the most common one but I think its there, it will mainly make the secrets harder to get because there are multiple ones, so at the end it improves security

I agree but I think configuring the firewall to reject direct WAN connections to your proxied servers may be a simpler and safer solution. I will put this on hold for now.

[Smart123s]

This PR breaks LimboAPI at https://github.com/Elytrium/LimboAPI/blob/e632cff20bd80f78b75a6634b7016464b650938a/plugin/src/main/java/net/elytrium/limboapi/injection/login/LoginListener.java#L223.

I have fixed it for myself with patch 0f4bef9.

[Smart123s]

Also, if player-info-forwarding-mode is set to MODERN, then it will kick players saying that the client must be on 1.13 or above, even if the server is on 1.8 (with BUNGEEGUARD forwarding) for example.

[theminecoder]

I think you should move the storage of the forwarding mode override to ServerInfo. Having it being in the config class means that it ends up being really annoying to use this from the api (which is needed for dynamic server registrations)