jojochuang commented on code in PR #8669:
URL: https://github.com/apache/hadoop/pull/8669#discussion_r3876362721
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SecurityUtil.java:
##########
@@ -491,7 +491,7 @@ public static Text buildTokenService(InetSocketAddress
addr) {
} else {
host = StringUtils.toLowerCase(addr.getHostName());
}
- return new Text(host + ":" + addr.getPort());
+ return new Text(NetUtils.getHostPortString(host, addr.getPort()));
Review Comment:
When `hadoop.security.token.service.use_ip=true`, the service string will
use whatever canonical form `InetAddress.getHostAddress()` returns (e.g.
`0:0:0:0:0:0:0:1` vs `::1`). That is pre-existing JDK behavior, not introduced
here, but operators enabling IPv6 should know token service strings may use the
expanded form. A brief release note may help.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestSecurityUtil.java:
##########
@@ -366,6 +366,14 @@ public void testSocketAddrWithIP() {
verifyServiceAddr(staticHost, "127.0.0.1");
}
+ @Test
+ public void testSocketAddrWithIPv6() throws Exception {
+ SecurityUtil.setTokenServiceUseIp(false);
Review Comment:
Minor: `SecurityUtil.setTokenServiceUseIp(false)` here is redundant —
`verifyAddress()` already exercises both `use_ip=true` and `use_ip=false` via
`verifyTokenService()`.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestSecurityUtil.java:
##########
@@ -366,6 +366,14 @@ public void testSocketAddrWithIP() {
verifyServiceAddr(staticHost, "127.0.0.1");
}
+ @Test
+ public void testSocketAddrWithIPv6() throws Exception {
+ SecurityUtil.setTokenServiceUseIp(false);
+ String host = "::1";
+ InetSocketAddress addr = NetUtils.createSocketAddr("[::1]:123");
+ verifyAddress(addr, host, InetAddress.getByName(host).getHostAddress(),
123);
Review Comment:
`testSocketAddrWithIPv6` assumes the resolved hostname for `::1` remains
`"::1"`, but on JDK 17 (verified locally) `createSocketAddr("[::1]:123")`
yields `getHostName() == "localhost"`. That breaks `verifyValues()` when
`use_ip=false` and hostname-mode token service expectations
(`"[localhost]:123"` vs `"[::1]:123"`).
The PR description says both `use_ip=true` and `use_ip=false` round-trips
are verified; this test may not reliably cover hostname mode depending on
`/etc/hosts` and JDK reverse-DNS behavior.
Consider deriving the expected hostname from `addr.getHostName()` after
creation, using `NetUtils.addStaticResolution` for a stable name (consistent
with other tests in this class), or splitting explicit coverage for
`use_ip=true` vs hostname mode.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java:
##########
@@ -763,10 +766,29 @@ public static String getHostname() {
* Compose a "host:port" string from the address.
*
* @param addr address.
- * @return hort port string.
+ * @return host port string.
*/
public static String getHostPortString(InetSocketAddress addr) {
- return addr.getHostName() + ":" + addr.getPort();
+ return getHostPortString(addr.getHostName(), addr.getPort());
+ }
+
+ /**
+ * Compose a "host:port" string, bracketing IPv6 literals.
+ *
+ * @param host host name or IP address.
+ * @param port port number.
+ * @return host port string.
+ */
+ public static String getHostPortString(String host, int port) {
+ String normalizedHost = host;
+ if (normalizedHost != null && normalizedHost.startsWith("[")
+ && normalizedHost.endsWith("]")) {
+ normalizedHost = normalizedHost.substring(1, normalizedHost.length() -
1);
+ }
+ if (normalizedHost != null && normalizedHost.contains(":")) {
Review Comment:
This new helper emits bracketed IPv6 authorities (e.g. `"[::1]:123"`), but
the existing `getPortFromHostPortString()` (unchanged in this PR) still splits
on `":"` and requires exactly one colon — it cannot parse bracketed IPv6
strings.
Token round-trip via `SecurityUtil.getTokenServiceAddr()` →
`createSocketAddr()` is fine, but the two helpers are now inconsistent. Any
caller that formats with `getHostPortString` and parses with
`getPortFromHostPortString` will break on IPv6.
Worth updating `getPortFromHostPortString()` to accept `"[<ipv6>]:<port>"`
(and reject ambiguous unbracketed IPv6), with tests, either here or as an
immediate follow-up on the same JIRA.
--
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]
