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

Adds ResolverConfigTest #51

Merged
merged 6 commits into from Jun 4, 2019

Conversation

Projects
None yet
2 participants
@kingle
Copy link
Collaborator

commented Jun 2, 2019

  • Adding some tests to help with upcoming refactoring of ResolverConfig to allow SPI and also to sanity check the move of the properties files still works when testing on Windows.
  • Changed many findX methods in ResolverConfig to return boolean on success/failure to aid in testing
  • Minor change to add a package-private fromConstantString method to set origin, also to aid in testing
  • Moved non-java related resources to appropriate src/main/resources and src/test/resources
  • Removed spi-up-to-java8 profile -- unneeded now in the master branch
if (OS.contains("95") ||
OS.contains("98") ||
OS.contains("ME"))
if (Stream.of("95", "98", "ME").anyMatch(OS::contains))

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

Do we still want to support these ancient Windows versions?

This comment has been minimized.

Copy link
@ibauersachs
@@ -4,4 +4,5 @@
.idea/
*.iml
target/
.DS_Store

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 2, 2019

Member

This piece of b*s* still exists?

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

yep, still exists even on macOS Mojave

public class ResolverConfig {

static final String DNS_SERVER_PROP = "dns.server";

This comment has been minimized.

Copy link
@ibauersachs

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.

This comment has been minimized.

Copy link
@kingle

kingle Jun 4, 2019

Author Collaborator

Made public, dropped indent (when is the mass test formatting happening?)

This comment has been minimized.

Copy link
@ibauersachs

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.

find95();
else
findNT();
findWin();

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 2, 2019

Member

Are you still working on a more SPI like interface, e.g. using ServiceLoader?

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

Yes

try {
Process p;
p = Runtime.getRuntime().exec("ipconfig /all");
findWin(p.getInputStream());
found = findWin(p.getInputStream());

This comment has been minimized.

Copy link
@ibauersachs

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).

This comment has been minimized.

Copy link
@kingle

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

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 3, 2019

Member

It lacks the search domains though, not sure atm. if there's another netsh call for those.

This comment has been minimized.

Copy link
@kingle

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?

This comment has been minimized.

Copy link
@ibauersachs

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.

This comment has been minimized.

Copy link
@kingle

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?

This comment has been minimized.

Copy link
@ibauersachs

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.

This comment has been minimized.

Copy link
@kingle

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.

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 3, 2019

Member

Sure, this was just about the question if netsh is also locale dependent. It is, so, bummer.

This comment has been minimized.

Copy link
@kingle

kingle Jun 3, 2019

Author Collaborator

Logged issue #55 to track the work to call Iphlpapi

String[] dnsSearch = { "dnsjava.org", "example.com",
"dnsjava.org" };
Name[] searchPath = Arrays.stream(dnsSearch)
.map(s -> Name.fromConstantString(s, Name.root))

This comment has been minimized.

Copy link
@ibauersachs

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?

This comment has been minimized.

Copy link
@kingle

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.

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 3, 2019

Member

The overload is anyway not necessary, this is equivalent: Name[] searchPath = Arrays.stream(dnsSearch).map(s -> Name.fromConstantString(s + "."))

This comment has been minimized.

Copy link
@kingle

kingle Jun 4, 2019

Author Collaborator

Updated and reverted the Name addition

}

@Test
@EnabledOnOs({OS.LINUX, OS.MAC, OS.AIX, OS.SOLARIS})

This comment has been minimized.

Copy link
@ibauersachs

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.

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

Agreed. Changed in latest

findUnix() {
findResolvConf("/etc/resolv.conf");
return findResolvConf("/etc/resolv.conf");

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 2, 2019

Member

Can we pass an InputStream to findResolvConf (and wrap the Files.newInputStream(Paths.get("...")) into an auto-close try)?

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

Yes! Done in latest commit.

@Test
void resolvConfLoaded() throws URISyntaxException {
assertTrue(ResolverConfig.getCurrentConfig()
.findResolvConf(Paths.get(getClass()

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 2, 2019

Member

ResolverConfigTest.class.getResourceAsStream("/...")

@@ -194,24 +194,6 @@
</dependencies>

<profiles>
<profile>

This comment has been minimized.

Copy link
@ibauersachs

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).

This comment has been minimized.

Copy link
@kingle

kingle Jun 2, 2019

Author Collaborator

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))

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 3, 2019

Member

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

This comment has been minimized.

Copy link
@ibauersachs

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).

This comment has been minimized.

Copy link
@kingle

kingle Jun 4, 2019

Author Collaborator

Logged issue #57

@ibauersachs ibauersachs merged commit 1f6bcc2 into dnsjava:master Jun 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.8%) to 45.113%
Details

@kingle kingle deleted the kingle:resolvconfig_initialtests branch Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.