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.

Sign up for GitHub

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

Eureka Client doesn't respect DNS server change at runtime #1157

Merged
merged 3 commits into from
Dec 18, 2018
Merged

Eureka Client doesn't respect DNS server change at runtime #1157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b11040
initialize DirContext on each request to DNS (make DirContext non-sta…
krutsko Nov 14, 2018
755d5a8
fix exception handling
krutsko Nov 16, 2018
d6ba8c1
revert to static DNSResolver
krutsko Dec 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions eureka-client/src/main/java/com/netflix/discovery/endpoint/DnsResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,10 @@ public final class DnsResolver {
private static final String CNAME_RECORD_TYPE = "CNAME";
private static final String TXT_RECORD_TYPE = "TXT";

static final DirContext dirContext = getDirContext();

private DnsResolver() {
}

/**
* Load up the DNS JNDI context provider.
*/
public static DirContext getDirContext() {
private DirContext getDirContext() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can see that this method can throw new RuntimeException. It's okay to throw this when application starts: this is a critical problem which has rights to prevent application startup. But did you check what are the consequences of this happening at some point in time when application is already operating and just wants to refresh list of Eureka nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I have changed all invocations and ensure that this RuntimeException properly caught and logged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is now called N+1 times, where N is number of zones. Are we okay with this? Isn't it time-consuming? (IMHO even if it is, it's not critical, but maybe somebody has another opinion).

Copy link
Contributor Author

@krutsko krutsko Nov 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not an expensive request, we don't make a network call, just initialize context where it re-check dns server on the local machine.

Here the implementation of dns server resolution for Linux on AdoptOpenJDK 11 (a first link i can found) https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java
It caches "/etc/resolv.conf" parsing for 5 minutes.

Hashtable<String, String> env = new Hashtable<String, String>();
env.put(JAVA_NAMING_FACTORY_INITIAL, DNS_NAMING_FACTORY);
env.put(JAVA_NAMING_PROVIDER_URL, DNS_PROVIDER_URL);
Expand All @@ -58,15 +53,15 @@ public static DirContext getDirContext() {
*
* @return resolved host name
*/
public static String resolve(String originalHost) {
public String resolve(String originalHost) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DnsResolver was written as a utility class with only static methods. This PR changes an implementation detail of this class so there's no need to make it possible to instantiate DnsResolver directly. Also, I'm concerned about making such breaking changes to public methods. The PR can be greatly simplified if the static change were to be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elandau thanks for looking into this.
Yes, i have reverted to static DnsResolver, somehow missed this at first place.

String currentHost = originalHost;
if (isLocalOrIp(currentHost)) {
return originalHost;
}
try {
String targetHost = null;
do {
Attributes attrs = dirContext.getAttributes(currentHost, new String[]{A_RECORD_TYPE, CNAME_RECORD_TYPE});
Attributes attrs = getDirContext().getAttributes(currentHost, new String[]{A_RECORD_TYPE, CNAME_RECORD_TYPE});
Attribute attr = attrs.get(A_RECORD_TYPE);
if (attr != null) {
targetHost = attr.get().toString();
Expand All @@ -92,12 +87,12 @@ public static String resolve(String originalHost) {
* @return resolved IP addresses or null if no A-record was present
*/
@Nullable
public static List<String> resolveARecord(String rootDomainName) {
public List<String> resolveARecord(String rootDomainName) {
if (isLocalOrIp(rootDomainName)) {
return null;
}
try {
Attributes attrs = dirContext.getAttributes(rootDomainName, new String[]{A_RECORD_TYPE, CNAME_RECORD_TYPE});
Attributes attrs = getDirContext().getAttributes(rootDomainName, new String[]{A_RECORD_TYPE, CNAME_RECORD_TYPE});
Attribute aRecord = attrs.get(A_RECORD_TYPE);
Attribute cRecord = attrs.get(CNAME_RECORD_TYPE);
if (aRecord != null && cRecord == null) {
Expand Down Expand Up @@ -128,8 +123,8 @@ private static boolean isLocalOrIp(String currentHost) {
/**
* Looks up the DNS name provided in the JNDI context.
*/
public static Set<String> getCNamesFromTxtRecord(String discoveryDnsName) throws NamingException {
Attributes attrs = dirContext.getAttributes(discoveryDnsName, new String[]{TXT_RECORD_TYPE});
public Set<String> getCNamesFromTxtRecord(String discoveryDnsName) throws NamingException {
Attributes attrs = getDirContext().getAttributes(discoveryDnsName, new String[]{TXT_RECORD_TYPE});
Attribute attr = attrs.get(TXT_RECORD_TYPE);
String txtRecord = null;
if (attr != null) {
Expand Down
4 changes: 2 additions & 2 deletions eureka-client/src/main/java/com/netflix/discovery/endpoint/EndpointUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ public static Set<String> getEC2DiscoveryUrlsFromZone(String dnsName, DiscoveryU
try {
dnsName = "txt." + dnsName;
logger.debug("The zone url to be looked up is {} :", dnsName);
Set<String> ec2UrlsForZone = DnsResolver.getCNamesFromTxtRecord(dnsName);
Set<String> ec2UrlsForZone = new DnsResolver().getCNamesFromTxtRecord(dnsName);
for (String ec2Url : ec2UrlsForZone) {
logger.debug("The eureka url for the dns name {} is {}", dnsName, ec2Url);
ec2UrlsForZone.add(ec2Url);
Expand Down Expand Up @@ -320,7 +320,7 @@ public static Map<String, List<String>> getZoneBasedDiscoveryUrlsFromRegion(Eure
discoveryDnsName = "txt." + region + "." + clientConfig.getEurekaServerDNSName();

logger.debug("The region url to be looked up is {} :", discoveryDnsName);
Set<String> zoneCnamesForRegion = new TreeSet<String>(DnsResolver.getCNamesFromTxtRecord(discoveryDnsName));
Set<String> zoneCnamesForRegion = new TreeSet<String>(new DnsResolver().getCNamesFromTxtRecord(discoveryDnsName));
Map<String, List<String>> zoneCnameMapForRegion = new TreeMap<String, List<String>>();
for (String zoneCname : zoneCnamesForRegion) {
String zone = null;
Expand Down
10 changes: 8 additions & 2 deletions eureka-client/src/main/java/com/netflix/discovery/shared/dns/DnsServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,20 @@
* @author Tomasz Bak
*/
public class DnsServiceImpl implements DnsService {
private final DnsResolver dnsResolver;

public DnsServiceImpl(DnsResolver dnsResolver) {
this.dnsResolver = dnsResolver;
}

@Override
public String resolveIp(String hostName) {
return DnsResolver.resolve(hostName);
return dnsResolver.resolve(hostName);
}

@Nullable
@Override
public List<String> resolveARecord(String rootDomainName) {
return DnsResolver.resolveARecord(rootDomainName);
return dnsResolver.resolveARecord(rootDomainName);
}
}
4 changes: 3 additions & 1 deletion eureka-client/src/main/java/com/netflix/discovery/shared/resolver/LegacyClusterResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;

import com.netflix.discovery.EurekaClientConfig;
import com.netflix.discovery.endpoint.DnsResolver;
import com.netflix.discovery.endpoint.EndpointUtils;
import com.netflix.discovery.shared.resolver.aws.AwsEndpoint;
import com.netflix.discovery.shared.resolver.aws.DnsTxtRecordClusterResolver;
Expand Down Expand Up @@ -89,7 +90,8 @@ public ClusterResolver<AwsEndpoint> createClusterResolver() {
true,
Integer.parseInt(clientConfig.getEurekaServerPort()),
false,
clientConfig.getEurekaServerURLContext()
clientConfig.getEurekaServerURLContext(),
new DnsResolver()
);
newResolver = new ZoneAffinityClusterResolver(newResolver, myZone, clientConfig.shouldPreferSameZoneEureka());
} else {
Expand Down
4 changes: 3 additions & 1 deletion ...client/src/main/java/com/netflix/discovery/shared/resolver/aws/ConfigClusterResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.netflix.appinfo.InstanceInfo;
import com.netflix.discovery.EurekaClientConfig;
import com.netflix.discovery.endpoint.DnsResolver;
import com.netflix.discovery.endpoint.EndpointUtils;
import com.netflix.discovery.shared.resolver.ClusterResolver;
import org.slf4j.Logger;
Expand Down Expand Up @@ -56,7 +57,8 @@ private List<AwsEndpoint> getClusterEndpointsFromDns() {
true,
port,
false,
clientConfig.getEurekaServerURLContext()
clientConfig.getEurekaServerURLContext(),
new DnsResolver()
);

List<AwsEndpoint> endpoints = dnsResolver.getClusterEndpoints();
Expand Down
15 changes: 9 additions & 6 deletions .../src/main/java/com/netflix/discovery/shared/resolver/aws/DnsTxtRecordClusterResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,21 +85,24 @@ public class DnsTxtRecordClusterResolver implements ClusterResolver<AwsEndpoint>
private final int port;
private final boolean isSecure;
private final String relativeUri;
private DnsResolver dnsResolver;

/**
* @param rootClusterDNS top level domain name, in the two level hierarchy (see {@link DnsTxtRecordClusterResolver} documentation).
* @param extractZoneFromDNS if set to true, zone information will be extract from zone DNS name. It assumed that the zone
* name is the name part immediately followint 'txt.' suffix.
* @param port Eureka sever port number
* @param relativeUri service relative URI that will be appended to server address
* @param dnsResolver used to send DNS requests
*/
public DnsTxtRecordClusterResolver(String region, String rootClusterDNS, boolean extractZoneFromDNS, int port, boolean isSecure, String relativeUri) {
public DnsTxtRecordClusterResolver(String region, String rootClusterDNS, boolean extractZoneFromDNS, int port, boolean isSecure, String relativeUri, DnsResolver dnsResolver) {
this.region = region;
this.rootClusterDNS = rootClusterDNS;
this.extractZoneFromDNS = extractZoneFromDNS;
this.port = port;
this.isSecure = isSecure;
this.relativeUri = relativeUri;
this.dnsResolver = dnsResolver;
}

@Override
Expand All @@ -115,7 +118,7 @@ public List<AwsEndpoint> getClusterEndpoints() {
return eurekaEndpoints;
}

private static List<AwsEndpoint> resolve(String region, String rootClusterDNS, boolean extractZone, int port, boolean isSecure, String relativeUri) {
private List<AwsEndpoint> resolve(String region, String rootClusterDNS, boolean extractZone, int port, boolean isSecure, String relativeUri) {
try {
Set<String> zoneDomainNames = resolve(rootClusterDNS);
if (zoneDomainNames.isEmpty()) {
Expand All @@ -135,16 +138,16 @@ private static List<AwsEndpoint> resolve(String region, String rootClusterDNS, b
}
}

private static Set<String> resolve(String rootClusterDNS) throws NamingException {
private Set<String> resolve(String rootClusterDNS) throws NamingException {
Set<String> result;
try {
result = DnsResolver.getCNamesFromTxtRecord(rootClusterDNS);
result = dnsResolver.getCNamesFromTxtRecord(rootClusterDNS);
if (!rootClusterDNS.startsWith("txt.")) {
result = DnsResolver.getCNamesFromTxtRecord("txt." + rootClusterDNS);
result = dnsResolver.getCNamesFromTxtRecord("txt." + rootClusterDNS);
}
} catch (NamingException e) {
if (!rootClusterDNS.startsWith("txt.")) {
result = DnsResolver.getCNamesFromTxtRecord("txt." + rootClusterDNS);
result = dnsResolver.getCNamesFromTxtRecord("txt." + rootClusterDNS);
} else {
throw e;
}
Expand Down
3 changes: 2 additions & 1 deletion ...in/java/com/netflix/discovery/shared/transport/decorator/RedirectingEurekaHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import com.netflix.discovery.endpoint.DnsResolver;
import com.netflix.discovery.shared.dns.DnsService;
import com.netflix.discovery.shared.dns.DnsServiceImpl;
import com.netflix.discovery.shared.resolver.DefaultEndpoint;
Expand Down Expand Up @@ -97,7 +98,7 @@ protected <R> EurekaHttpResponse<R> execute(RequestExecutor<R> requestExecutor)
}

public static TransportClientFactory createFactory(final TransportClientFactory delegateFactory) {
final DnsServiceImpl dnsService = new DnsServiceImpl();
final DnsServiceImpl dnsService = new DnsServiceImpl(new DnsResolver());
return new TransportClientFactory() {
@Override
public EurekaHttpClient newClient(EurekaEndpoint endpoint) {
Expand Down
3 changes: 2 additions & 1 deletion eureka-core/src/main/java/com/netflix/eureka/transport/EurekaServerHttpClients.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.eureka.transport;

import com.netflix.discovery.endpoint.DnsResolver;
import com.netflix.discovery.shared.dns.DnsServiceImpl;
import com.netflix.discovery.shared.resolver.ClusterResolver;
import com.netflix.discovery.shared.resolver.EurekaEndpoint;
Expand Down Expand Up @@ -66,7 +67,7 @@ public static EurekaHttpClient createRemoteRegionClient(EurekaServerConfig serve
}

public static TransportClientFactory createFactory(final TransportClientFactory delegateFactory) {
final DnsServiceImpl dnsService = new DnsServiceImpl();
final DnsServiceImpl dnsService = new DnsServiceImpl(new DnsResolver());
return new TransportClientFactory() {
@Override
public EurekaHttpClient newClient(EurekaEndpoint endpoint) {
Expand Down