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

Netty DNS resolver ignores provided DNS server address #6573

Closed
tomasherman opened this issue Mar 27, 2017 · 5 comments
Closed

Netty DNS resolver ignores provided DNS server address #6573

tomasherman opened this issue Mar 27, 2017 · 5 comments
Assignees
Labels
Milestone

Comments

@tomasherman
Copy link

Expected behavior

In netty 4.1.8.Final, following code works fine. However, in 4.1.9 it does not send the request to provided address and sends it to default name server stream instead.

import java.net.{InetAddress, InetSocketAddress}

import io.netty.channel.nio.NioEventLoopGroup
import io.netty.channel.socket.nio.NioDatagramChannel
import io.netty.resolver.dns.{DnsNameResolverBuilder, DnsServerAddresses}

/** Simple dns resolver for testing stuff
  */
object Cli extends App {
  private val bossGroup = new NioEventLoopGroup()

  val res = new DnsNameResolverBuilder(bossGroup.next())
            .channelType(classOf[NioDatagramChannel])
            .nameServerAddresses(DnsServerAddresses.singleton(new InetSocketAddress("customdns.com", 33220)))
            .build()

  println(res.resolve("testaddress.com").await().get())
}

If you run this code with netty 4.1.9 in debug mode, you can see it's sending requests to system resolvers.

I suspect the problem is the doResolveUncached in DnsNameResolver where you check whether the address stream is null. I think you should check whether the nameServerAddress is null and if it is, you should default to system resolvers. if it is defined however, you should make it into a stream and use it. But maybe im missing something.

Netty version

4.1.8.Final is fine, 4.1.9.Final is broken

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

mac os

@normanmaurer
Copy link
Member

@Scottmitch can you please check as this was introduced by your change in 54c9ecf . I think @tomasherman is correct that we should ensure we not override the name servers set by the user in this case.

@Scottmitch
Copy link
Member

Scottmitch commented Mar 27, 2017

temporary workaround: Use NoopDnsServerAddressStreamProvider.

There is currently an issue with the APIs. nameServerAddresses (e.g. DnsServerAddresses) doesn't have any knowledge of the hostname when selecting the DNS server, but DnsServerAddressStreamProvider does have knowledge of the hostname when selecting the DNS server. When they are both used it is not clear which should have precedence. Currently the DnsServerAddressStreamProvider has precedence and we fall back to nameServerAddresses [1], and it can't really work the other way around (because nameServerAddresses would always be used). So we have a few options:

  1. Keep the current APIs. If the user wants to specify nameServerAddresses then they must ensure DnsServerAddressStreamProvider is properly configured for their use case.
  2. Remove nameServerAddresses and just accept a DnsServerAddressStreamProvider. This way the user has fine grained control (per hostname being resolved) as to what DNS server should be used for resolution. This approach is also easy to cascade. For example the user could specify a DnsServerAddressStreamProvider which programmatically overrides the DNS servers for some/all hostnames, and fallback to system default otherwise.

I'm leaning toward approach 2. I think it represents what we are logically trying to achieve and limits the surface area to a single interface. Since the DNS codec is labeled as UnstableApi we can also make this breaking change now. @normanmaurer @trustin - WDYT?

[1] https://github.com/netty/netty/blob/4.1/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java#L641

@tomasherman
Copy link
Author

i think the #2 makes sense ... i think having to configure two things in order to set dns server will be confusing

@normanmaurer
Copy link
Member

@Scottmitch yep go with [2].

@Scottmitch
Copy link
Member

see #6588

Scottmitch added a commit to Scottmitch/netty that referenced this issue Mar 31, 2017
Motivation:
Recently DnsServerAddressStreamProvider was introduced to allow control for each query as to which DNS server should be used for resolution to respect the local host's default DNS server configuration. However resolver-dns also accepts a stream of DNS servers to use by default, but this stream is not host name aware. This creates an ambiguity as to which method is used to determine the DNS server to user during resolution, and in which order. We can remove this ambiguity and provide a more general API by just supporting DnsServerAddressStreamProvider.

Modifications:
- Remove the fixed DnsServerAddresses and instead only accept a DnsServerAddressStreamProvider.
- Add utility methods to help use DnsServerAddressStreamProvider for a single entry, a list of entries, and get the default for the current machine.

Result:
Fixes netty#6573.
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this issue Sep 10, 2017
Motivation:
Recently DnsServerAddressStreamProvider was introduced to allow control for each query as to which DNS server should be used for resolution to respect the local host's default DNS server configuration. However resolver-dns also accepts a stream of DNS servers to use by default, but this stream is not host name aware. This creates an ambiguity as to which method is used to determine the DNS server to user during resolution, and in which order. We can remove this ambiguity and provide a more general API by just supporting DnsServerAddressStreamProvider.

Modifications:
- Remove the fixed DnsServerAddresses and instead only accept a DnsServerAddressStreamProvider.
- Add utility methods to help use DnsServerAddressStreamProvider for a single entry, a list of entries, and get the default for the current machine.

Result:
Fixes netty#6573.
kiril-me pushed a commit to kiril-me/netty that referenced this issue Feb 28, 2018
Motivation:
Recently DnsServerAddressStreamProvider was introduced to allow control for each query as to which DNS server should be used for resolution to respect the local host's default DNS server configuration. However resolver-dns also accepts a stream of DNS servers to use by default, but this stream is not host name aware. This creates an ambiguity as to which method is used to determine the DNS server to user during resolution, and in which order. We can remove this ambiguity and provide a more general API by just supporting DnsServerAddressStreamProvider.

Modifications:
- Remove the fixed DnsServerAddresses and instead only accept a DnsServerAddressStreamProvider.
- Add utility methods to help use DnsServerAddressStreamProvider for a single entry, a list of entries, and get the default for the current machine.

Result:
Fixes netty#6573.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants