-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Eureka Client doesn't respect DNS server change at runtime #1157
Conversation
/** | ||
* Load up the DNS JNDI context provider. | ||
*/ | ||
public static DirContext getDirContext() { | ||
private DirContext getDirContext() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
/** | ||
* Load up the DNS JNDI context provider. | ||
*/ | ||
public static DirContext getDirContext() { | ||
private DirContext getDirContext() { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Shouldn't we open an issue first, linked to this PR? |
Can someone check this PR please? |
Can someone take a look at the PR, and re-run the tests (they all pass on my local machine) ? |
@@ -58,15 +53,15 @@ public static DirContext getDirContext() { | |||
* | |||
* @return resolved host name | |||
*/ | |||
public static String resolve(String originalHost) { | |||
public String resolve(String originalHost) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Eureka Client doesn't respect DNS server change at runtime.
E.g. try to adjust DNS server at
/etc/resolv.conf
file , but Eureka Client keep communicate with old address.The reason is that Eureka Client initializes InitialDirContext once and keep it in static variable , so it never re-read resolv.conf.
I didn't spent much time to write a proper unit test for this (let me know if it needed), but here the code which could help to check this with DnsResolver.