Conversation
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
This PR addresses ambiguity in delegation token service identifiers when the host is an IPv6 literal by introducing a consistent bracketed "[host]:port" representation and ensuring socket address creation accepts bracketed IPv6 authorities.
Changes:
- Add
NetUtils.getHostPortString(host, port)to formathost:portwhile bracketing IPv6 literals. - Update delegation token service string construction to use the shared host/port formatter.
- Add tests covering IPv6 host/port formatting, bracketed IPv6 socket address parsing, and delegation token service round-trips.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestSecurityUtil.java | Updates token-service string expectations and adds an IPv6 token service round-trip test. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java | Adds tests for IPv6 host/port formatting, bracketed parsing, and rejection of ambiguous unbracketed IPv6 authorities. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SecurityUtil.java | Uses the shared host/port formatter when building token service identifiers. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java | Adds IPv6-aware host/port formatting and strips brackets from parsed URI hosts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (normalizedHost != null && normalizedHost.contains(":")) { | ||
| return "[" + normalizedHost + "]:" + port; | ||
| } | ||
| return normalizedHost + ":" + port; |
|
💔 -1 overall
This message was automatically generated. |
ed1d90c to
52966be
Compare
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
Review of the rebased branch — inline comments on specific areas.
| SecurityUtil.setTokenServiceUseIp(false); | ||
| String host = "::1"; | ||
| InetSocketAddress addr = NetUtils.createSocketAddr("[::1]:123"); | ||
| verifyAddress(addr, host, InetAddress.getByName(host).getHostAddress(), 123); |
There was a problem hiding this 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.
| && normalizedHost.endsWith("]")) { | ||
| normalizedHost = normalizedHost.substring(1, normalizedHost.length() - 1); | ||
| } | ||
| if (normalizedHost != null && normalizedHost.contains(":")) { |
There was a problem hiding this 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.
| host = StringUtils.toLowerCase(addr.getHostName()); | ||
| } | ||
| return new Text(host + ":" + addr.getPort()); | ||
| return new Text(NetUtils.getHostPortString(host, addr.getPort())); |
There was a problem hiding this 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.
|
|
||
| @Test | ||
| public void testSocketAddrWithIPv6() throws Exception { | ||
| SecurityUtil.setTokenServiceUseIp(false); |
There was a problem hiding this comment.
Minor: SecurityUtil.setTokenServiceUseIp(false) here is redundant — verifyAddress() already exercises both use_ip=true and use_ip=false via verifyTokenService().
|
💔 -1 overall
This message was automatically generated. |
Generated-by: Codex (GPT-5.6 Sol)
Description of PR
Jira: HADOOP-19958
Parent Jira: HADOOP-11890
Delegation token services use
host:port. This representation is ambiguous when the host is an IPv6 literal.This change:
NetUtilshelper that formats IPv6 addresses as[host]:port;hadoop.security.token.service.use_ipistrueorfalse;How was this patch tested?
JAVA_HOME=<JDK17> mvn -B -pl :hadoop-common -Dtest=TestNetUtils,TestSecurityUtil test --no-transfer-progressgit diff --check asf/trunk...HEADFor code changes:
(e.g. 'HADOOP-17799. Your PR title ...')?
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required. Not applicable to this change.
under ASF 2.0? No new dependencies are added.
LICENSE,LICENSE-binary,NOTICE-binaryfiles?No license or notice changes are required.
AI Tooling
Contains content generated by Codex.
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html