You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "anmolnar (via GitHub)" <gi...@apache.org> on 2023/06/10 09:12:44 UTC
[GitHub] [zookeeper] anmolnar opened a new pull request, #2005: ZOOKEEPER-3860 Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url
anmolnar opened a new pull request, #2005:
URL: https://github.com/apache/zookeeper/pull/2005
Instead of altering the behaviour of `ZKTrustManager`, I'll add the following improvements:
1. `NetUtils.formatInetAddr()` should prefer using the hostname of resolved addresses when converting InetSocketAddress to String. Previously it only picked the hostname if address was missing, e.g. the hostname was unresolved. This change has the advantage of sending the hostname in the InitialMessage instead of the IP address, which will help avoiding unnecessary reverse DNS lookups in the election protocol when TLS is enabled.
2. Add extra debug logs to `ZKTrustManager` and remove logging the exception stack trace when IP verification failed. Logging of stack traces makes sense to me in error log entries only. Many times we hit into this when user turns on debug logging, seeing a big stack trace and believes it's an error in ZooKeeper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #2005: ZOOKEEPER-3860 Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #2005: ZOOKEEPER-3860 Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url
URL: https://github.com/apache/zookeeper/pull/2005
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #2005: ZOOKEEPER-3860 Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2005:
URL: https://github.com/apache/zookeeper/pull/2005#discussion_r1225379186
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java:
##########
@@ -27,17 +27,18 @@
*/
public class NetUtils {
+ /**
+ * Prefer using the hostname for formatting, but without requesting reverse DNS lookup.
+ * Fall back to IP address if hostname is unavailable and use [] brackets for IPv6 literal.
+ */
public static String formatInetAddr(InetSocketAddress addr) {
+ String hostString = addr.getHostString();
InetAddress ia = addr.getAddress();
- if (ia == null) {
- return String.format("%s:%s", addr.getHostString(), addr.getPort());
- }
-
- if (ia instanceof Inet6Address) {
- return String.format("[%s]:%s", ia.getHostAddress(), addr.getPort());
+ if (ia instanceof Inet6Address && hostString.contains(":")) {
Review Comment:
Good point, need to check the source, but javadoc says:
> Returns the hostname, or the String form of the address if it doesn't have a hostname (it was created using a literal). This has the benefit of not attempting a reverse lookup.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetSocketAddress.html#getHostString()
How could be null? No hostname and no IP address either in the InetSocketAddress. What sort of socket address does it represent in such case?
I think we're good without the null check, but I'll take a look at the source.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #2005: ZOOKEEPER-3860 Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #2005:
URL: https://github.com/apache/zookeeper/pull/2005#discussion_r1225273214
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java:
##########
@@ -27,17 +27,18 @@
*/
public class NetUtils {
+ /**
+ * Prefer using the hostname for formatting, but without requesting reverse DNS lookup.
+ * Fall back to IP address if hostname is unavailable and use [] brackets for IPv6 literal.
+ */
public static String formatInetAddr(InetSocketAddress addr) {
+ String hostString = addr.getHostString();
InetAddress ia = addr.getAddress();
- if (ia == null) {
- return String.format("%s:%s", addr.getHostString(), addr.getPort());
- }
-
- if (ia instanceof Inet6Address) {
- return String.format("[%s]:%s", ia.getHostAddress(), addr.getPort());
+ if (ia instanceof Inet6Address && hostString.contains(":")) {
Review Comment:
Can hostString be null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org