Skip to content
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

[RFC] Detect system default resolver #52

Closed

Conversation

WyriHaximus
Copy link
Member

As discussed in #29 a PR that detects the system default resolver on *nix systems.

@jsor
Copy link
Member

jsor commented Feb 23, 2017

#48 adds a TimeoutExecutor, would it make sense to read the timeout option from resolve.conf and use that value to create the TimeoutExecutor?

@WyriHaximus
Copy link
Member Author

#48 adds a TimeoutExecutor, would it make sense to read the timeout option from resolve.conf and use that value to create the TimeoutExecutor?

Most certainly! If @reactphp/core likes the proposed external API I'll go more in depth and add more features like the timeout option 😄 .

@WyriHaximus WyriHaximus changed the title Detect system default resolver [RFC] Detect system default resolver Mar 2, 2017
@mailopl
Copy link

mailopl commented Jul 28, 2017

Any news on this guys? Any chance for it to being merged in next week?

@kelunik
Copy link

kelunik commented Jul 28, 2017

What's with Windows? This PR will result in many written apps not being able to run on Windows.

@WyriHaximus
Copy link
Member Author

Ping @jsor @clue, what do you think about the public facing API? If you like it I'll work it out into something that also works on windows and has a default fallback so it works if detection fails.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing this ticket @WyriHaximus, I'd love to get this feature in! 👍

I'm currently unsure about its API, but have you seen the (somewhat outdated) https://github.com/reactphp/dns/blob/master/src/Config/FilesystemFactory.php ?

I think it make more sense to keep this in a separate class and then pass the nameserver configuration to the default create*() methods.

What do you think about this?

@WyriHaximus
Copy link
Member Author

@clue we could go with something like this, where 8.8.8.8 is used as a fallback:

$loop = React\EventLoop\Factory::create();
$factory = new React\Dns\Resolver\Factory();
$dns = $factory->createCached(determineSystemDefaultServer('8.8.8.8'), $loop);

}

if (count($nameservers) > 0) {
shuffle($nameservers);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should use the first one, not a random one. I think the first one is considered the primary DNS and order should be respected in /etc/resolv.conf.

$nameserverPosition = stripos($line, 'nameserver');
if ($nameserverPosition !== false) {
$nameserverLine = trim(substr($line, $nameserverPosition + 11));
list ($nameservers[]) = explode(' ', $nameserverLine);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be only one nameserver per nameserver config option in /etc/resolv.conf.

return array_pop($nameservers);
}

throw new \Exception('Nameserver configuration missing');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man resolv.conf says it should default to the local nameserver if none are present, but Amp doesn't do this either, at least currently.

@kelunik
Copy link

kelunik commented Sep 13, 2017

I added some comments regarding the implementation.

While it will usually work to read /etc/resolv.conf once, the nameservers in there might change, e.g. because a software might be running on a notebook that changes the access point (WLAN).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants