-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix resolve ip address. #3069
Fix resolve ip address. #3069
Conversation
Processing names in hosts file, DNS (including localhost from DNS), IP addresses
This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request. |
addrOrHost.Equals("localhost", StringComparison.OrdinalIgnoreCase) || | ||
addrOrHost.Equals("127.0.0.1", StringComparison.OrdinalIgnoreCase)) | ||
// Fix StreamFilteringTests_SMS tests | ||
if (addrOrHost.Equals("loopback", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we lost 'localhost'?
127.0.0.1 and 0.0.0.0 are covered by IPAddress.TryParse, but I don't see how ''localhost' is covered..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for not being clear the first time I commented.
What I was getting at was that we specifically handled certain cases, and I wanted to ensure that we continued to do so.
Those cases were:
0.0.0.0 - which returned IPAddress.Any, and is currently handled by the IPAddress.TryParse.
127.0.0.1 - which returned loopback, and is currently handled by the IPAddress.TryParse.
"loopback" - which returned loopback, and is currently doing the same, with the last commit.
"localhost" - which returned loopback, and is not being handled as previously.
I understand that I'm being a bit of a stickler here, but we've many services using this, so we need to ensure we're not breaking any existing users without good cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we lost 'localhost'?
The function Dns.GetHostAddressesAsync(addrOrHost)
returns loopback IP in case addrOrHost == "localhost"
. It works if OS contains loopback interface (all current OS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks!
Thanks for the fix, and quick turnaround on comments! |
Fix resolve ip address.
We have a problem when configuring silo hosts. If I setup in hosts file network name (for example 'orleans') with IP 127.0.0.1 and set it as Address in SeedNode, Networking or ProxyingGateway the load configuration will trow
ArgumentException
. The host name 'orleans' is a valid host with valid loopback IP in this case.So this fix checks any valid host name from hosts file, DNS entries (including case whena DNS responds 'localhost' or '127.0.0.1'). In additional I add parsing IP addresses (IPv4 and IPv6). So it possible to set not only 127.0.0.1, but any valid IP address.