fix issue #480 : UDP target host address is cached

[lxhoan]

This PR is to resolve host address for UDP writing, period is 5 mins
(follow the default eviction period of OKHttp conection pool)

[fmachado]

@lxhoan what is the benefit of using this approach instead of networkaddress.cache.ttl as explained on PR #481 ? Am I missing something here?

[majst01]

Hi @lxhoan
I would also prefer to document the usage of the security manager configuration like here:
https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/java-dg-jvm-ttl.html

Writing 100 lines of code of a already existing feature is waste of time.

[lxhoan]

Hi All,
That property (networkaddress.cache.ttl) is for JVM/JRE DNS lookup service and enabled if a security manager is installed

Once we have parsed and cached the host IP address, as in

this.hostAddress = parseHostAddress(url);

then hostAddress stayed unchanged and UDP writing will fail if the target host IP gets changed. networkaddress.cache.ttl will not help this case

@fmachado, apropos of PR #481, in discussion with PR author I suggest that we can use a socket adress as follows

datagramSocket.send(new DatagramPacket(bytes, bytes.length, new InetSocketAddress(hostname,  udpPort)));

that means we accept to resolve DNS each UDP writing. This approach is not a good idea if the JVM is not configured with a appropriate networkaddress.cache.ttl or no security manager is installed for the JVM. So I would like to introduce this PR

[lxhoan]

In my opinion the issue #480 (UDP target host address is cached) is valid and we should address it somehow

[majst01]

Hi @lxhoan
according to: https://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

networkaddress.cache.ttl
Specified in java.security to indicate the caching policy for successful name lookups from the name service.. The value is specified as integer to indicate the number of seconds to cache the successful lookup.
A value of -1 indicates "cache forever". The default behavior is to cache forever when a security manager is installed, and to cache for an implementation specific period of time, when a security manager is not installed.

networkaddress.cache.ttl is evaluated even if no security manager is in place. So i cant see why this will not solve this specific Problem ?

[lxhoan]

networkaddress.cache.ttl is evaluated even if no security manager is in place. So i cant see why this will not solve this specific Problem ?

this scenario (what mentioned in issue #480)
1/ Currently we resolve the the hostname, say abc.com, to to its ip address in InfluxDBImpl constructor

this.hostAddress = parseHostAddress(url);

2/ at a certain time, host abc.com changes to a new IP address
3/ then if user writes to abc.com use udp, the writing will fail because hostAdress still holds the old IP address

[fmachado]

3/ then if user writes to abc.com use udp, the writing will fail because hostAdress still holds the old IP address

I may be wrong but I think if the hostAddress is changing every 2m, your solution will also fail.

With the scenario I mentioned here, if you set networkaddress.cache.ttl to 1m30s then you solve your problem.

[majst01]

What if we do not resolv in parseAddress() ?

[lxhoan]

What if we do not resolv in parseAddress() ?

I originally in PR #481 suggested to resolve on UDP writing, as follow

datagramSocket.send(new DatagramPacket(bytes, bytes.length, new InetSocketAddress(hostname,  udpPort)));

you can see InetSocketAddress constructor will resolve hostname to the IP address. but this way can overwork the DNS server or make the DNS server think you are trying a DOS attack on it.

Then I think to resolve it periodically 5min (as the default eviction time of OKHttp Connection pool)

However, how about introducing one more parameter to InfluxDBImpl so user can configure this resolving period ?

[lxhoan]

I may be wrong but I think if the hostAddress is changing every 2m, your solution will also fail.

With the scenario I mentioned here, if you set networkaddress.cache.ttl to 1m30s then you solve your problem.

Yes, so how about introducing one more parameter to InfluxDBImpl so user can configure this resolving period ? (setting networkaddress.cache.ttl cannot refresh the hostAddress to new IP adress)

[fmachado]

(setting networkaddress.cache.ttl cannot refresh the hostAddress to new IP adress)

?!?

But this is exactly what this option is offering. After n seconds, records are considered expired and the next time a hostname needs to be resolved, JVM will perform a DNS lookup.

@lxhoan you mentioned networkaddress.cache.ttl "(...) approach is not a good idea if the JVM is not configured with a appropriate networkaddress.cache.ttl or no security manager is installed for the JVM".

If the JVM allows the user to solve his problem and the user didn't configure properly the JVM, why we are supposed to do it?

So, again:

• use networkaddress.cache.ttl by changing the existing java.security file
• copy java.security to a new file, change the new file and pass it as parameter to the JVM;
• use sun.net.inetaddr.ttl that can be passed as command-line parameter (I mentioned it would be possible to do it with networkaddress.cache.ttl but it seems I was wrong)

Anyway, I vote for do not merge this PR.

[lxhoan]

@fmachado , I think we are misunderstanding

After n seconds, records are considered expired and the next time a hostname needs to be resolved, JVM will perform a DNS lookup.

But we have already resolved it in the InfluxDBImpl constructor : InfluxDBImple, line 119

this.hostAddress = parseHostAddress(url);

So how can this.hostAddress get updated in case the host changes to new IP Address ?
(the method parseHostAddress returns an InetAddress that hold the current IP address of the host)

[lxhoan]

Hi All, does my last comment make some sense ?

[lxhoan]

closed (PR #502 as the alternative)