chihsuan commented on code in PR #11130:
URL: https://github.com/apache/ozone/pull/11130#discussion_r3885487380
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/http/TestBaseHttpServer.java:
##########
@@ -137,6 +138,23 @@ protected String getHttpAuthConfigPrefix() {
assertEquals("/1.2.3.4:1234", baseHttpServer
.getBindAddress("bindhostkey", "addresskey",
"default", 65).toString());
+
+ // An IPv6 bind host, wildcard or literal, must survive being combined with
+ // the port. Assert on the address rather than toString(), whose bracketing
+ // of IPv6 literals is a JDK detail.
+ conf.set("bindhostkey", "::");
+
+ InetSocketAddress wildcard = baseHttpServer
+ .getBindAddress("bindhostkey", "addresskey", "default", 65);
+ assertEquals(InetAddress.getByName("::"), wildcard.getAddress());
Review Comment:
Could we also cover the endpoint URI path here? I found that an unbracketed
IPv6 authority makes `URI.create` return a `null` host instead of throwing, so
reverting that line keeps the suite green.
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/TestHddsUtils.java:
##########
@@ -102,6 +106,31 @@ void testGetHostPortString() {
assertEquals("[2001:db8::1]:9858",
HddsUtils.getHostPortString("[2001:db8::1]", 9858));
}
+ @Test
+ void testGetHostPortStringFromAddress() {
+ assertEquals("1.2.3.4:9858",
+ HddsUtils.getHostPortString(new InetSocketAddress("1.2.3.4", 9858)));
Review Comment:
Worth using `192.0.2.1` here? I noticed this expectation holds only while
the reverse lookup fails, and `1.2.3.4` is routable space that may have a PTR
record on some resolvers.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -243,6 +243,19 @@ public static String getHostPortString(String host, int
port) {
return HostAndPort.fromParts(host, port).toString();
}
+ /**
+ * Render an address as a "host:port" string, wrapping the host in square
+ * brackets when it is an IPv6 literal. Hadoop's
+ * {@code NetUtils.getHostPortString(InetSocketAddress)} joins the two with a
+ * plain colon, which yields an ambiguous authority for IPv6.
+ *
+ * @param addr the address to render
+ * @return the combined address, bracketed for IPv6 literals
+ */
+ public static String getHostPortString(InetSocketAddress addr) {
+ return getHostPortString(addr.getHostName(), addr.getPort());
Review Comment:
I wonder if we need this overload at all. The call sites could pass
`addr.getHostName()` to the two-arg version, keeping parity with Hadoop. That
avoids a new public helper and leaves `getHostName()`'s reverse lookup visible
at each call site.
It would also avoid baking in `getHostName()`, which does a reverse lookup
when the address carries no hostname.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
