Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdds ResolverConfigTest #51
Conversation
| if (OS.contains("95") || | ||
| OS.contains("98") || | ||
| OS.contains("ME")) | ||
| if (Stream.of("95", "98", "ME").anyMatch(OS::contains)) |
kingle
Jun 2, 2019
Author
Collaborator
Do we still want to support these ancient Windows versions?
Do we still want to support these ancient Windows versions?
ibauersachs
Jun 2, 2019
Member
No
No
| @@ -4,4 +4,5 @@ | |||
| .idea/ | |||
| *.iml | |||
| target/ | |||
| .DS_Store | |||
ibauersachs
Jun 2, 2019
Member
This piece of b*s* still exists?
This piece of b*s* still exists?
kingle
Jun 2, 2019
Author
Collaborator
yep, still exists even on macOS Mojave
yep, still exists even on macOS Mojave
| public class ResolverConfig { | ||
|
|
||
| static final String DNS_SERVER_PROP = "dns.server"; |
ibauersachs
Jun 2, 2019
Member
I'd probably make these public to allow users to reference the properties via this constant. And please drop the indent for now.
I'd probably make these public to allow users to reference the properties via this constant. And please drop the indent for now.
kingle
Jun 4, 2019
Author
Collaborator
Made public, dropped indent (when is the mass test formatting happening?)
Made public, dropped indent (when is the mass test formatting happening?)
ibauersachs
Jun 4, 2019
Member
Not sure yet, if no pull requests are open. And only on test sources anyway. I haven't decided to apply it on the main sources yet since they're not so messy.
Not sure yet, if no pull requests are open. And only on test sources anyway. I haven't decided to apply it on the main sources yet since they're not so messy.
| find95(); | ||
| else | ||
| findNT(); | ||
| findWin(); |
ibauersachs
Jun 2, 2019
Member
Are you still working on a more SPI like interface, e.g. using ServiceLoader?
Are you still working on a more SPI like interface, e.g. using ServiceLoader?
kingle
Jun 2, 2019
Author
Collaborator
Yes
Yes
| try { | ||
| Process p; | ||
| p = Runtime.getRuntime().exec("ipconfig /all"); | ||
| findWin(p.getInputStream()); | ||
| found = findWin(p.getInputStream()); |
ibauersachs
Jun 2, 2019
Member
Is netsh int ipv4 show dnsservers or netsh int ipv6 show dnsservers also locale-dependent? I don't have a non-english Windows at hand right now. Otherwise we could switch to it (netsh should be available on Win 7+, and I don't care about MS unsupported Windows versions).
Is netsh int ipv4 show dnsservers or netsh int ipv6 show dnsservers also locale-dependent? I don't have a non-english Windows at hand right now. Otherwise we could switch to it (netsh should be available on Win 7+, and I don't care about MS unsupported Windows versions).
kingle
Jun 2, 2019
Author
Collaborator
netsh is still locale-dependent (on my Windows 10 with Spanish set):
C:\...>netsh int ipv4 show dnsservers
Configuración para la interfaz "Ethernet"
Servidores DNS configurados a través de DHCP: 192.168.1.1
Registrar con el sufijo: Solo el principal
...
That said, it's probably a better source to parse since it eliminates most of the noise
netsh is still locale-dependent (on my Windows 10 with Spanish set):
C:\...>netsh int ipv4 show dnsservers
Configuración para la interfaz "Ethernet"
Servidores DNS configurados a través de DHCP: 192.168.1.1
Registrar con el sufijo: Solo el principal
...
That said, it's probably a better source to parse since it eliminates most of the noise
ibauersachs
Jun 3, 2019
Member
It lacks the search domains though, not sure atm. if there's another netsh call for those.
It lacks the search domains though, not sure atm. if there's another netsh call for those.
kingle
Jun 3, 2019
Author
Collaborator
I still think we should consider the registry option. What's wrong with looking at:
regedit /e TEMPFILE "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters"
and parse those?
I still think we should consider the registry option. What's wrong with looking at:
regedit /e TEMPFILE "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters"
and parse those?
ibauersachs
Jun 3, 2019
Member
You'll get nameservers of NICs with a disconnected link. Example: laptop with a wired and WiFi NIC. WiFi is connected at home to your router and the DHCP server assigns an IP and DNS addresses. Later you go to work, connect via cable (e.g. docking station), WiFi is disconnected. Your regedit call (and what Java does internally with probing the registry and calling Windows 95 (!) networking APIs) will get the inaccessible nameservers of your home WiFi.
Or the other way around (office nameservers still on the wired NIC, now only a WiFi connection). Querying the registry for this info is just completely wrong. See also JDK-7006496.
What I'd be fine with though is adding an optional (i.e. scope=provided) dependency to JNA, probing at runtime if it's available and then doing proper calls to Iphlpapi. But that API isn't easy.
You'll get nameservers of NICs with a disconnected link. Example: laptop with a wired and WiFi NIC. WiFi is connected at home to your router and the DHCP server assigns an IP and DNS addresses. Later you go to work, connect via cable (e.g. docking station), WiFi is disconnected. Your regedit call (and what Java does internally with probing the registry and calling Windows 95 (!) networking APIs) will get the inaccessible nameservers of your home WiFi.
Or the other way around (office nameservers still on the wired NIC, now only a WiFi connection). Querying the registry for this info is just completely wrong. See also JDK-7006496.
What I'd be fine with though is adding an optional (i.e. scope=provided) dependency to JNA, probing at runtime if it's available and then doing proper calls to Iphlpapi. But that API isn't easy.
kingle
Jun 3, 2019
Author
Collaborator
name servers sure, but what about search suffixes? Regardless of whether you disconnect and reconnect at different places, do the DNS Search Suffixes change?
name servers sure, but what about search suffixes? Regardless of whether you disconnect and reconnect at different places, do the DNS Search Suffixes change?
ibauersachs
Jun 3, 2019
Member
Yes, they do change, at least their priority. At work I have ds.example.com, at home my cable router hands out home. Fritz Boxes (popular routers/modems in Europe and especially Germany) assign fritz.box.
Yes, they do change, at least their priority. At work I have ds.example.com, at home my cable router hands out home. Fritz Boxes (popular routers/modems in Europe and especially Germany) assign fritz.box.
kingle
Jun 3, 2019
Author
Collaborator
Interesting. Can we table this for a different PR/issue? How we go about adding new functionality to better handle Windows resolution specifically could go on for awhile and we probably should discuss it in a separate issue. I definitely don't want to tackle native code additions here.
Interesting. Can we table this for a different PR/issue? How we go about adding new functionality to better handle Windows resolution specifically could go on for awhile and we probably should discuss it in a separate issue. I definitely don't want to tackle native code additions here.
ibauersachs
Jun 3, 2019
Member
Sure, this was just about the question if netsh is also locale dependent. It is, so, bummer.
Sure, this was just about the question if netsh is also locale dependent. It is, so, bummer.
| String[] dnsSearch = { "dnsjava.org", "example.com", | ||
| "dnsjava.org" }; | ||
| Name[] searchPath = Arrays.stream(dnsSearch) | ||
| .map(s -> Name.fromConstantString(s, Name.root)) |
ibauersachs
Jun 2, 2019
Member
If this call is the only reason you added Name.fromConstantString(String, Name), why don't you simply put the trailing dot for the root to the string array?
If this call is the only reason you added Name.fromConstantString(String, Name), why don't you simply put the trailing dot for the root to the string array?
kingle
Jun 2, 2019
Author
Collaborator
Future tests may look at the inputs from other sources for search path (ipconfig output), and those sources usually don't have the trailing dot. We could adjust the inputs, but I feel conflicted relying on massaging test resources to have trailing dots. That being said, it's your call. I can go either way.
Future tests may look at the inputs from other sources for search path (ipconfig output), and those sources usually don't have the trailing dot. We could adjust the inputs, but I feel conflicted relying on massaging test resources to have trailing dots. That being said, it's your call. I can go either way.
ibauersachs
Jun 3, 2019
Member
The overload is anyway not necessary, this is equivalent: Name[] searchPath = Arrays.stream(dnsSearch).map(s -> Name.fromConstantString(s + "."))
The overload is anyway not necessary, this is equivalent: Name[] searchPath = Arrays.stream(dnsSearch).map(s -> Name.fromConstantString(s + "."))
kingle
Jun 4, 2019
Author
Collaborator
Updated and reverted the Name addition
Updated and reverted the Name addition
| } | ||
|
|
||
| @Test | ||
| @EnabledOnOs({OS.LINUX, OS.MAC, OS.AIX, OS.SOLARIS}) |
ibauersachs
Jun 2, 2019
Member
Or @DisabledOnOs({OS.WINDOWS})? I doubt any test run will ever be executed on Android - if it's detected at all.
Or @DisabledOnOs({OS.WINDOWS})? I doubt any test run will ever be executed on Android - if it's detected at all.
kingle
Jun 2, 2019
Author
Collaborator
Agreed. Changed in latest
Agreed. Changed in latest
| findUnix() { | ||
| findResolvConf("/etc/resolv.conf"); | ||
| return findResolvConf("/etc/resolv.conf"); |
ibauersachs
Jun 2, 2019
Member
Can we pass an InputStream to findResolvConf (and wrap the Files.newInputStream(Paths.get("...")) into an auto-close try)?
Can we pass an InputStream to findResolvConf (and wrap the Files.newInputStream(Paths.get("...")) into an auto-close try)?
kingle
Jun 2, 2019
Author
Collaborator
Yes! Done in latest commit.
Yes! Done in latest commit.
| @Test | ||
| void resolvConfLoaded() throws URISyntaxException { | ||
| assertTrue(ResolverConfig.getCurrentConfig() | ||
| .findResolvConf(Paths.get(getClass() |
ibauersachs
Jun 2, 2019
Member
ResolverConfigTest.class.getResourceAsStream("/...")
ResolverConfigTest.class.getResourceAsStream("/...")
| @@ -194,24 +194,6 @@ | |||
| </dependencies> | |||
|
|
|||
| <profiles> | |||
| <profile> | |||
ibauersachs
Jun 2, 2019
Member
The service descriptor still needs to be excluded from the JAR on Java 9+. But it can probably be moved to the no-spi-on-java9 profile with an exclusion instead of an inclusion if you move it (correctly so, I missed that).
The service descriptor still needs to be excluded from the JAR on Java 9+. But it can probably be moved to the no-spi-on-java9 profile with an exclusion instead of an inclusion if you move it (correctly so, I missed that).
kingle
Jun 2, 2019
Author
Collaborator
Good call. Excluded in that profile.
Good call. Excluded in that profile.
| String[] dnsSearch = { "dnsjava.org", "example.com", | ||
| "dnsjava.org" }; | ||
| Name[] searchPath = Arrays.stream(dnsSearch) | ||
| .map(s -> Name.fromConstantString(s, Name.root)) |
ibauersachs
Jun 3, 2019
Member
The overload is anyway not necessary, this is equivalent: Name[] searchPath = Arrays.stream(dnsSearch).map(s -> Name.fromConstantString(s + "."))
The overload is anyway not necessary, this is equivalent: Name[] searchPath = Arrays.stream(dnsSearch).map(s -> Name.fromConstantString(s + "."))
| nameserver 192.168.1.1 | ||
| domain domain.com | ||
| search example.com dnsjava.org | ||
| options ndots:5 |
ibauersachs
Jun 3, 2019
Member
Interesting. I wasn't aware there were this many options for resolv.conf. I wonder if we want to support some of them in the future (not for this PR, just a remark).
Interesting. I wasn't aware there were this many options for resolv.conf. I wonder if we want to support some of them in the future (not for this PR, just a remark).
src/main/resourcesandsrc/test/resources