Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Use an actual DNS resolver #31

Merged
merged 1 commit into from
Dec 10, 2018
Merged

Use an actual DNS resolver #31

merged 1 commit into from
Dec 10, 2018

Conversation

stayallive
Copy link
Contributor

@stayallive stayallive commented Dec 6, 2018

Without this the statistics requests will never reach the server if it's not available on 127.0.0.1 directly (think load balanced or distributed services).

Let the system supply a DNS resolver or use Cloudflare's 1.1.1.1 as an default.

@mpociot
Copy link
Member

mpociot commented Dec 6, 2018

The Problem is that the systems dns resolver will never resolve domains while developing locally. For example when using Laravel Valet.
Maybe we could make this dependent on wether or not the app environment is set to production.

What do you think @freekmurze ?

@stayallive
Copy link
Contributor Author

@mpociot I noticed that too 👍 I made an config option for it 😄

@freekmurze
Copy link
Contributor

Wouldn't that DNS lookup block our server?

@stayallive
Copy link
Contributor Author

It shouldn't since it's using the Reacts Async DNS lookup and also uses the in-memory cache for lookups.

But otherwise some other way needs to be devised to get the statistics across since it just silently fails on our production cluster without it 😄

config/websockets.php Outdated Show resolved Hide resolved
src/Console/StartWebSocketServer.php Outdated Show resolved Hide resolved
@freekmurze
Copy link
Contributor

I'm ok with adding this to the package. I don't know how, but it would be nice if we could have an automated test for this.

Could you also PR that docs (update the config file there)

@stayallive
Copy link
Contributor Author

stayallive commented Dec 6, 2018

Hmm we could test if it resolves not to 127.0.0.1 with that config option set possibly. Although that probably means it has to be extracted out of the command.

Then it could be tested if the correct dns resolver is provided.

But testing if the resolver actually resolves something seems silly, and making a really advanced testing suite that only works on not 127.0.0.1 to prove the point might be going a bit to far 😂

Open for ideas!

@WyriHaximus
Copy link

A small note FYI, we actually support the hosts file of *nix and Windows systems through the HostsFileExecutor which is used by default on both the non-cached and the cached resolvers. And as long as you try to resolve localhost it will always return 127.0.0.1.

@stayallive
Copy link
Contributor Author

@WyriHaximus interesting... so as long as the entry is in the hosts file it will never even hit the network? Very nice!

This will still not allow us to enable it by default because Valet does not write to the hosts file but good to know that adding entries to the hosts file will make sure no network requests are made for DNS resolving when this is enabled.

@WyriHaximus
Copy link

@stayallive correct.

To be honest I don't have the slightest clue on how Valet sets the DNS for .test up then. But I assume it sets up DNS entries somewhere, if that is a DNS server and it is in /etc/resolv.conf, the PR in it's current state will also pick that up.

@mpociot
Copy link
Member

mpociot commented Dec 10, 2018

@WyriHaximus Valet places a custom configuration, depending on the TLD that you choose inside the etc/resolver/ folder. And this DNS will then point to 127.0.0.1.
Too bad that it's not possible to add multiple DNS servers yet.

@stayallive thanks for the PR!

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

Successfully merging this pull request may close these issues.

4 participants