fix: Accept single words as valid hostnames #3591
Conversation
com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs
Outdated
Show resolved
Hide resolved
|
FYI the CMB Service Test failure is a know issue that Noel is addressing *update, it was fixed and I merged latest content from |
There was a problem hiding this comment.
@simon-lemay-unity
A bunch of good stuff in here, but I have concerns on how this will impact the connection flow with users. Primarily, with this update the behavior changes the default behavior which is to provide meaningful information if the user is using an FQDN (which is what I believe vast majority of usage would be) that is malformed, just like we would with an IP address, to the behavior of not doing anything and allowing everything to run as if it is "ok" until the hostname resolution fails due to a malformed FQDN. This was my original concern when hostname support was initially added.
This also removes script that actually reduces user pain:
- Before: If a user uses an invalid FQDN it would immediately throw an error with a clear message as to the problem and NetworkManager would shutdown.
- The user can quickly determine the issue.
- Now: If a user uses an invalid FQDN it waits for before transport trickles up a disconnect/unable to connect message.
- Albeit, we could improve the error message via the PR mentioned...but we still are changing the connection flow behavior where upon starting
NetworkManagerif it returns "true" then that meant a bunch of things are "correct"...and if we detected something wrong with the configuration (i.e. invalid IP address or the like) then we would log an error and shutdown theNetworkManager.
- Albeit, we could improve the error message via the PR mentioned...but we still are changing the connection flow behavior where upon starting
There are a very few special case scenarios where someone would want to use an unqualified name as opposed to a FQDN. The localhost is a default internal machine specific unqualified domain name, which we could have just as easily checked to see if the address provided only contained the word "localhost" (removing any capitalization) since that would (most likely) be the most widely used single word hostname.
Any other unqualified hostnames would require being specifically configured in the local machine's DNS table or on a local network name server or it would be a NetBIOS name. Which I think a very small percentage of our users would have that kind of configuration...which the ones that do would most likely be using it for development purposes. The vast majority of netcode projects are connecting via the internet where FQDNs are required.
I guess my question is why remove this check that would reduce user pain as opposed to adding something like an isFQDN flag that could be disabled for the special cases and some additional script to check for the basic kinds of things like "localhost" when isFQDN is still enabled (which would be the default)? (We could even provide users a way to configure a set of "valid" unqualified names that it would treat as FQDNs and allow/bypass the check.)
|
My main objective with this PR is getting "localhost" resolution to work, because if someone sees in a release note that they can now use domain resolution and they then attempt to use the obvious test case of "localhost" and it doesn't work, then it's a pretty bad look. The rationale behind removing the validation is just that this was the simplest way to achieve that goal and had the benefit of simplifying the code to boot. If you feel however that a validation check is important at the moment of starting a client, I don't mind adding that in. It won't be through that regex however since it's wrong in other ways too (e.g. it doesn't accept FQDNs with their trailing dot). |
|
@NoelStephensUnity I added a validation step in |
There was a problem hiding this comment.
@simon-lemay-unity
Thank you very much... and I like your way of handling the host name check (way simpler with more coverage than what I had put together).
🥇
Purpose of this PR
The regex we used to validate hostnames did not accept single words as valid hostnames. But single words can be valid hostnames. The most common is of course "localhost" but one can edit one's hosts file to define any word as something the local resolver will resolve to an IP address.
This PR addresses this by removing any validation of the provided hostname. We could modify the validity check to accept single words too, but the regex is pretty indigestible already and it's simpler to just let the resolver fail if the user provided garbage. UTP will signal this through a disconnection event with an appropriate reason (which we'll be able to map to a nice error message once this PR lands).
While I was at it, I also made a few cleanups and improvements:
UTP_TRANSPORT_2_4_ABOVEdefine. NGO depends on UTP 2.4.0 so it can be safely assumed that users will have it installed. No need to conditionally compile the code that depends on it.Connectmethod when connecting to a hostname.ConnectionAddressData.ServerEndPoint. We don't use it anymore, it doesn't work with hostnames, and it's not providing any value over just callingNetworkEndpoint.Parse. And worst of all: its capitalization of "endpoint" doesn't match what we use elsewhere.SetConnectionData. The reason for this change is that the resolver in UTP prioritizes IPv6 addresses over IPv4. So if we listen on IPv4 by default, we're likely to get issues if the resolver then ends up with an IPv6 address. In current versions of UTP for instance, this causes errors on Windows (a fix is on the way). I'm looking into changing the behavior of the resolver to prefer IPv4, but that's an engine change so might take a while to land. In the meantime defaulting to IPv6 seems like the best approach.Changelog
UnityTransportwould not accept single words as valid hostnames (notably "localhost").UnityTransport.ConnectionAddressData.ServerEndPointas obsolete. It can't work when using hostnames as the server address, and its functionality can easily be replicated usingNetworkEndpoint.Parse.Documentation
No documentation changes or additions were necessary.
Testing & QA
Tested with manual and automated tests.
Backport
Hostname resolution is only supported in UTP 2.4+ and Unity 6.1+, so no backport necessary.