minidns
minidns copied to clipboard
Allow DNSServerLookupMechanism to specify ports for the servers
As far as I can see there currently is no way to alter the port a query is sent to when using ResolverAPI or DnssecResolverApi. This would be possible if DNSServerLookupMechanism specified ports for the servers.
As this is a breaking change for projects using DNSServerLookupMechanism
or AbstractDNSServerLookupMechanism
it might be useful to keep the old getDnsServerAddresses()
inside AbstractDNSServerLookupMechanism
and mark it as deprecated, returning an empty list by default. A possible alteration could look like this:
/**
* Port-aware replacement of {@link #getDnsServerAddresses()}
*
* @return A list of upstream DNS servers with their corresponding ports.
*
*/
@Override
public List<IPPortPair> getDnsServerAddressesWithPorts(){
List<String> servers = getDnsServerAddresses();
// Throw some exception here if the list is empty?
List<IPPortPair> pairs = new ArrayList<>(servers.size());
for(String server: servers){
pairs.add(new IPPortPair(server, IPPortPair.DEFAULT_PORT));
}
return pairs;
}
/**
* @return A List containing non-null DNS servers represented as IP addresses
*
* @deprecated override {@link #getDnsServerAddressesWithPorts()} instead
*/
public List<String> getDnsServerAddresses(){
return Collections.emptyList();
}
This however would break the concept of abstract classes as you would no longer be forced to implement getDnsServerAddressesWithPorts() in a subclass - that's why it isn't included in this commit.
Another way could be using REGEX to parse strings returned with getDnsServerAddresses()
, extract possible ports from them (or fallback to 53) and return these for getDnsServerAddressesWithPorts by default.
Pull Request Test Coverage Report for Build 160
- 38 of 58 (65.52%) changed or added relevant lines in 7 files are covered.
- 1 unchanged line in 1 file lost coverage.
- Overall coverage increased (+0.08%) to 53.02%
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
---|---|---|---|
minidns-android21/src/main/java/org/minidns/dnsserverlookup/android21/AndroidUsingLinkProperties.java | 0 | 1 | 0.0% |
minidns-client/src/main/java/org/minidns/dnsserverlookup/AndroidUsingExec.java | 0 | 1 | 0.0% |
minidns-client/src/main/java/org/minidns/dnsserverlookup/AndroidUsingReflection.java | 0 | 1 | 0.0% |
minidns-client/src/main/java/org/minidns/dnsserverlookup/IPPortPair.java | 6 | 8 | 75.0% |
minidns-client/src/main/java/org/minidns/dnsserverlookup/AbstractDnsServerLookupMechanism.java | 5 | 11 | 45.45% |
minidns-client/src/main/java/org/minidns/DnsClient.java | 26 | 35 | 74.29% |
<!-- | Total: | 38 | 58 |
Files with Coverage Reduction | New Missed Lines | % |
---|---|---|
minidns-client/src/main/java/org/minidns/DnsClient.java | 1 | 52.8% |
<!-- | Total: | 1 |
Totals | |
---|---|
Change from base Build 159: | 0.08% |
Covered Lines: | 2897 |
Relevant Lines: | 5464 |
💛 - Coveralls
Using non-default ports is pretty rare. My initial thought was to add support for non-default ports without breaking too much existing stuff. If this doesn't get merged (and is done properly some time in the future) I'm totally fine with it as I can just use my fork.
I've changed the reviewed parts. The copyright header was auto-generated by Android studio and slipped through my sight.
I merged everything into this PR. If wanted/needed I can integrate the suggested changes regarding server capabilities as well, but that'd require more changes as DNS over TLS (or other capabilities as DNS over HTTPS) most of the time isn't wanted as it increases traffic by a lot - there'd need to be some sort of input from the developer then if those capabilities should be used
I'm in support of having alternative port support. This is something we use to get around wireless carriers who like to intercept dns and redirect to their own servers (transparent dns proxying at the carrier level)