Skip to content

HDDS-15774. Make SCM Ratis roles string parsing IPv6-safe #10831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AdyChechani wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HDDS-15774. Make SCM Ratis roles string parsing IPv6-safe #10831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
View file
Open in desktop

@smengcl smengcl Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix HostAndPort class as well:

diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/HostAndPort.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/HostAndPort.java
index ... .. ...
--- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/HostAndPort.java
+++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/HostAndPort.java
@@ -18,6 +18,7 @@
 package org.apache.hadoop.hdds.scm.net;

 import java.net.InetSocketAddress;
+import org.apache.hadoop.hdds.HddsUtils;
 import org.apache.hadoop.net.NetUtils;

 public class HostAndPort {
@@ -35,7 +36,7 @@ public class HostAndPort {
   public HostAndPort(String host, int port) {
     this.host = host;
     this.port = port;
-    this.hostAndPortString = host + ":" + port;
+    this.hostAndPortString = HddsUtils.getHostPortString(host, port);
     this.hash = host.hashCode() ^ Integer.hashCode(port);
     // TODO: HDDS-15533 change the address resolution logic and make this.address threadsafe.
     this.address = NetUtils.createSocketAddr(hostAndPortString);

Otherwise SCMNodeInfo.buildNodeInfo() can still produce invalid unbracketed authority.

Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,52 @@ public static String getHostPortString(String host, int port) {
return HostAndPort.fromParts(host, port).toString();
}

/**
* Parse a Ratis role string produced by
* {@code SCMRatisServerImpl.getRatisRoles()} into its constituent fields.
* The format is {@code [host]:port:ROLE:id:hostIP} where host and hostIP
* may be bracketed IPv6 literals.
*
* @param roleString the encoded role string
* @return a 5-element array: {host, port, role, id, hostIP}
*/
public static String[] parseRatisRoleString(String roleString) {
// Parse from the right: the last field is hostIP (possibly bracketed),
// then id (uuid, no colons), then role (LEADER/FOLLOWER, no colons),
// and the remainder is host:port (which may be bracketed IPv6).
int idx = roleString.length();

// Field 5: hostIP — may be bracketed IPv6 like [2001:db8::1]
String hostIp;
if (idx > 0 && roleString.charAt(idx - 1) == ']') {
int bracket = roleString.lastIndexOf('[');
hostIp = roleString.substring(bracket + 1, idx - 1);
idx = bracket - 1; // skip the ':' before '['
} else {
int sep = roleString.lastIndexOf(':');
hostIp = roleString.substring(sep + 1);
idx = sep;
}

// Field 4: id (uuid or peer id, no colons)
int sep3 = roleString.lastIndexOf(':', idx - 1);
String id = roleString.substring(sep3 + 1, idx);

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we validate the input before calling substring()? Inputs such as "" or "host:9894" currently produce StringIndexOutOfBoundsException but older SCMs can actually emit.

idx = sep3;

// Field 3: role (LEADER/FOLLOWER, no colons)
int sep2 = roleString.lastIndexOf(':', idx - 1);
String role = roleString.substring(sep2 + 1, idx);
idx = sep2;

// Remainder is host:port — use HostAndPort to parse safely
String hostPort = roleString.substring(0, idx);
HostAndPort hp = HostAndPort.fromString(hostPort);
String host = hp.getHost();
String port = String.valueOf(hp.getPort());

return new String[]{host, port, role, id, hostIp};
}

/**
* Retrieve a number, trying the supplied config keys in order.
* Each config value may be absent
Expand Down
44 changes: 44 additions & 0 deletions hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/TestHddsUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,50 @@ void testGetHostPortString() {
assertEquals("[2001:db8::1]:9858", HddsUtils.getHostPortString("[2001:db8::1]", 9858));
}

@Test
void testParseRatisRoleStringIPv4() {
String input = "hostname1:9894:LEADER:peer-uuid-123:192.168.1.1";
String[] result = HddsUtils.parseRatisRoleString(input);
assertEquals("hostname1", result[0]);
assertEquals("9894", result[1]);
assertEquals("LEADER", result[2]);
assertEquals("peer-uuid-123", result[3]);
assertEquals("192.168.1.1", result[4]);
}

@Test
void testParseRatisRoleStringIPv6() {
String input = "[2001:db8::1]:9894:LEADER:peer1:[2001:db8:0:0:0:0:0:1]";
String[] result = HddsUtils.parseRatisRoleString(input);
assertEquals("2001:db8::1", result[0]);
assertEquals("9894", result[1]);
assertEquals("LEADER", result[2]);
assertEquals("peer1", result[3]);
assertEquals("2001:db8:0:0:0:0:0:1", result[4]);
}

@Test
void testParseRatisRoleStringIPv6Follower() {
String input = "[::1]:9894:FOLLOWER:abc-def:[0:0:0:0:0:0:0:1]";
String[] result = HddsUtils.parseRatisRoleString(input);
assertEquals("::1", result[0]);
assertEquals("9894", result[1]);
assertEquals("FOLLOWER", result[2]);
assertEquals("abc-def", result[3]);
assertEquals("0:0:0:0:0:0:0:1", result[4]);
}

@Test
void testParseRatisRoleStringEmptyHostIp() {
String input = "scm-host:9894:LEADER:uuid123:";
String[] result = HddsUtils.parseRatisRoleString(input);
assertEquals("scm-host", result[0]);
assertEquals("9894", result[1]);
assertEquals("LEADER", result[2]);
assertEquals("uuid123", result[3]);
assertEquals("", result[4]);
}

static List<Arguments> validPaths() {
return Arrays.asList(
Arguments.of("/", "/"),
Expand Down
25 changes: 18 additions & 7 deletions hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,24 @@ public List<String> getRatisRoles() {
LOG.error("SCM Ratis PeerInetAddress {} is unresolvable",
peer.getAddress());
}
ratisRoles.add((peer.getAddress() == null ? "" :
peer.getAddress().concat(peer.equals(leader) ?
":".concat(RaftProtos.RaftPeerRole.LEADER.toString()) :
":".concat(RaftProtos.RaftPeerRole.FOLLOWER.toString()))
.concat(":".concat(peer.getId().toString()))
.concat(":".concat(peerInetAddress == null ? "" :
peerInetAddress.getHostAddress()))));
if (peer.getAddress() == null) {
ratisRoles.add("");
continue;
}
String host = HddsUtils.getHostName(peer.getAddress()).orElse("");
int port = HddsUtils.getHostPort(peer.getAddress()).orElse(0);

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a missing Ratis port valid here? Defaulting to 0 makes the response look valid and consumers may display it as the actual port. If the port is required, failing with a clear message may be safer than silently using 0.

String normalizedAddress = HddsUtils.getHostPortString(host, port);
String role = peer.equals(leader)
? RaftProtos.RaftPeerRole.LEADER.toString()
: RaftProtos.RaftPeerRole.FOLLOWER.toString();
String hostIp = "";
if (peerInetAddress != null) {
String rawIp = peerInetAddress.getHostAddress();
hostIp = rawIp.contains(":") ? "[" + rawIp + "]" : rawIp;

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion: since we already have the InetAddress, InetAddresses.toUriString(peerInetAddress) handles this directly—IPv4 stays unchanged and IPv6 is bracketed. That may make the formatting intent a little clearer.

}
String roleEntry = normalizedAddress + ":" + role + ":"
+ peer.getId().toString() + ":" + hostIp;
ratisRoles.add(roleEntry);
}
return ratisRoles;
}
Expand Down
2 changes: 1 addition & 1 deletion ...s/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2190,7 +2190,7 @@ public List<List<String>> getScmRatisRoles() {
List<String> ratisRoles = server.getRatisRoles();
List<List<String>> result = new ArrayList<>();
for (String role : ratisRoles) {
String[] roleArr = role.split(":");
String[] roleArr = HddsUtils.parseRatisRoleString(role);
List<String> scmInfo = new ArrayList<>();
// Host Name
scmInfo.add(roleArr[0]);
Expand Down
62 changes: 62 additions & 0 deletions ...p-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
Expand All @@ -27,11 +28,15 @@
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;

import java.util.List;
import java.util.UUID;
import org.apache.hadoop.hdds.HddsUtils;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
import org.apache.hadoop.hdds.security.SecurityConfig;
import org.apache.ratis.conf.RaftProperties;
import org.apache.ratis.protocol.RaftGroup;
import org.apache.ratis.protocol.RaftGroupId;
import org.apache.ratis.protocol.RaftPeer;
import org.apache.ratis.protocol.RaftPeerId;
import org.apache.ratis.server.RaftServer;
Expand Down Expand Up @@ -103,4 +108,61 @@ public void testGetLeaderId() throws Exception {
}
}

@Test
public void testGetRatisRolesWithIPv6() throws Exception {
try (
MockedConstruction<SecurityConfig> mockedSecurityConfigConstruction = mockConstruction(SecurityConfig.class);
MockedStatic<RaftServer> staticMockedRaftServer = mockStatic(RaftServer.class);
MockedStatic<RatisUtil> staticMockedRatisUtil = mockStatic(RatisUtil.class);
) {
ConfigurationSource conf = mock(ConfigurationSource.class);
StorageContainerManager scm = mock(StorageContainerManager.class);
when(scm.getClusterId()).thenReturn("CID-" + UUID.randomUUID());
SCMHADBTransactionBuffer dbTransactionBuffer = mock(SCMHADBTransactionBuffer.class);

RaftServer.Builder raftServerBuilder = mock(RaftServer.Builder.class);
when(raftServerBuilder.setServerId(any())).thenReturn(raftServerBuilder);
when(raftServerBuilder.setProperties(any())).thenReturn(raftServerBuilder);
when(raftServerBuilder.setStateMachineRegistry(any())).thenReturn(raftServerBuilder);
when(raftServerBuilder.setOption(any())).thenReturn(raftServerBuilder);
when(raftServerBuilder.setGroup(any())).thenReturn(raftServerBuilder);
when(raftServerBuilder.setParameters(any())).thenReturn(raftServerBuilder);

RaftServer raftServer = mock(RaftServer.class);
RaftServer.Division division = mock(RaftServer.Division.class);
when(raftServer.getDivision(any())).thenReturn(division);
when(raftServerBuilder.build()).thenReturn(raftServer);
staticMockedRaftServer.when(RaftServer::newBuilder).thenReturn(raftServerBuilder);

RaftProperties raftProperties = mock(RaftProperties.class);
staticMockedRatisUtil.when(() -> RatisUtil.newRaftProperties(conf)).thenReturn(raftProperties);

SecurityConfig sc = new SecurityConfig(conf);
when(sc.isSecurityEnabled()).thenReturn(false);

SCMRatisServerImpl scmRatisServer = spy(new SCMRatisServerImpl(conf, scm, dbTransactionBuffer));

// IPv6 peer address in the bracketed format that getRatisHostPortStr() produces
RaftPeer ipv6Peer = RaftPeer.newBuilder()
.setId(RaftPeerId.valueOf("peer1"))
.setAddress("[2001:db8::1]:9894")
.build();

RaftGroup raftGroup = RaftGroup.valueOf(RaftGroupId.randomId(), ipv6Peer);
when(division.getGroup()).thenReturn(raftGroup);
doReturn(ipv6Peer).when(scmRatisServer).getLeader();

List<String> roles = scmRatisServer.getRatisRoles();
assertEquals(1, roles.size());

String roleString = roles.get(0);
String[] parsed = HddsUtils.parseRatisRoleString(roleString);
assertEquals("2001:db8::1", parsed[0]);
assertEquals("9894", parsed[1]);
assertEquals("LEADER", parsed[2]);
assertEquals("peer1", parsed[3]);
assertTrue(!parsed[4].isEmpty(), "hostIP should be resolved");
}
}

}
10 changes: 5 additions & 5 deletions ...ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/SafeModeCheckSubcommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,16 @@ private SCMNodeInfo findLeaderNode(ScmClient scmClient) throws IOException {
try {
List<String> roles = scmClient.getScmRoles();
for (String role : roles) {
String[] parts = role.split(":");
if (parts.length < 3 || !"LEADER".equalsIgnoreCase(parts[2])) {
String[] parts = HddsUtils.parseRatisRoleString(role);
if (!"LEADER".equalsIgnoreCase(parts[2])) {
continue;
}
String leaderHost = parts[0];
String leaderIp = parts.length >= 5 ? parts[4] : null;
String leaderIp = parts[4];
for (SCMNodeInfo node : nodes) {
String nodeHost = node.getScmClientAddress().split(":")[0];
String nodeHost = HddsUtils.getHostName(node.getScmClientAddress()).orElse("");

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This correctly extracts the IPv6 host, but matchesAddress() still parses its inputs with split(":", 2). Equivalent forms such as 2001:db8::1 and 2001:db8:0:0:0:0:0:1 therefore do not match. Could we make matchesAddress() IPv6-aware as part of this fix?

https://github.com/AdyChechani/ozone/blob/0530be6310670bea08e54d711d929b7214cc25b0/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/SafeModeCheckSubcommand.java#L189-L190


if (matchesAddress(leaderHost, nodeHost) || (leaderIp != null && !leaderIp.isEmpty() &&
if (matchesAddress(leaderHost, nodeHost) || (!leaderIp.isEmpty() &&
matchesAddress(leaderIp, nodeHost))) {
return node;
}
Expand Down
29 changes: 8 additions & 21 deletions ...cli-admin/src/main/java/org/apache/hadoop/ozone/admin/scm/GetScmRatisRolesSubcommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@

package org.apache.hadoop.ozone.admin.scm;

import static java.lang.System.err;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.apache.hadoop.hdds.HddsUtils;
import org.apache.hadoop.hdds.cli.HddsVersionProvider;
import org.apache.hadoop.hdds.scm.cli.ScmSubcommand;
import org.apache.hadoop.hdds.scm.client.ScmClient;
Expand Down Expand Up @@ -73,10 +71,7 @@ public void execute(ScmClient scmClient) throws IOException {
formattingCLIUtils.addHeaders(SCM_ROLES_HEADER);

for (String role : peerRoles) {
String[] roleItems = role.split(":");
if (roleItems.length < 2) {
err.println("Invalid response received for ScmRatisRoles.");
}
String[] roleItems = HddsUtils.parseRatisRoleString(role);

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table path previously handled short or invalid responses without throwing. Since the new parser requires five fields, should we preserve the existing validation/skip behavior here?

formattingCLIUtils.addLine(roleItems);
}
System.out.println(formattingCLIUtils.render());
Expand All @@ -92,20 +87,12 @@ private Map<String, Map<String, String>> parseScmRoles(
Map<String, Map<String, String>> allRoles = new HashMap<>();
for (String role : peerRoles) {
Map<String, String> roleDetails = new HashMap<>();
String[] roles = role.split(":");
if (roles.length < 2) {
err.println("Invalid response received for ScmRatisRoles.");
return Collections.emptyMap();
}
// In case, there is no ratis, there is no ratis role.
// This will just print the hostname with ratis port as the address
roleDetails.put("address", roles[0].concat(":").concat(roles[1]));
if (roles.length == 5) {
roleDetails.put("raftPeerRole", roles[2]);
roleDetails.put("ID", roles[3]);
roleDetails.put("InetAddress", roles[4]);
}
allRoles.put(roles[0], roleDetails);
String[] fields = HddsUtils.parseRatisRoleString(role);

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we preserve the existing host:port handling here? The previous code explicitly supported this non-Ratis/two-field response, but the new parser requires all five fields and crashes on "host:9894". This looks like an unintended behavior change.

roleDetails.put("address", fields[0] + ":" + fields[1]);

@chihsuan chihsuan Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For IPv6 this produces an ambiguous address such as 2001:db8::1:9894. Could we use HddsUtils.getHostPortString(...) here so the JSON output remains [2001:db8::1]:9894?

roleDetails.put("raftPeerRole", fields[2]);
roleDetails.put("ID", fields[3]);
roleDetails.put("InetAddress", fields[4]);
allRoles.put(fields[0], roleDetails);
}
return allRoles;
}
Expand Down
24 changes: 24 additions & 0 deletions ...e/cli-admin/src/test/java/org/apache/hadoop/ozone/scm/TestGetScmRatisRolesSubcommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,28 @@ public void testGetScmHARatisRoles() throws Exception {
}
}

@Test
public void testGetScmHARatisRolesIPv6() throws Exception {
GetScmRatisRolesSubcommand cmd = new GetScmRatisRolesSubcommand();
ScmClient client = mock(ScmClient.class);
CommandLine c = new CommandLine(cmd);
c.parseArgs("--table");

List<String> result = new ArrayList<>();
result.add("[2001:db8::1]:9894:LEADER:e428ca07-b2a3-4756-bf9b-a4abb033c7d1:[2001:db8:0:0:0:0:0:1]");
result.add("[2001:db8::2]:9894:FOLLOWER:61b1c8e5-da40-4567-8a17-96a0234ba14e:[2001:db8:0:0:0:0:0:2]");

when(client.getScmRoles()).thenAnswer(invocation -> result);

try (GenericTestUtils.SystemOutCapturer capture =
new GenericTestUtils.SystemOutCapturer()) {
cmd.execute(client);
assertThat(capture.getOutput()).contains("2001:db8::1");
assertThat(capture.getOutput()).contains("9894");
assertThat(capture.getOutput()).contains("LEADER");
assertThat(capture.getOutput()).contains("e428ca07-b2a3-4756-bf9b-a4abb033c7d1");
assertThat(capture.getOutput()).contains("FOLLOWER");
}
}

}
3 changes: 2 additions & 1 deletion ...main/java/org/apache/hadoop/ozone/recon/spi/impl/StorageContainerServiceProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.concurrent.atomic.AtomicBoolean;
import javax.inject.Inject;
import org.apache.commons.io.FileUtils;
import org.apache.hadoop.hdds.HddsUtils;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.protocolPB.SCMSecurityProtocolClientSideTranslatorPB;
Expand Down Expand Up @@ -138,7 +139,7 @@ public DBCheckpoint getSCMDBSnapshot() {
try {
List<String> ratisRoles = scmClient.getScmInfo().getPeerRoles();
for (String ratisRole : ratisRoles) {
String[] role = ratisRole.split(":");
String[] role = HddsUtils.parseRatisRoleString(ratisRole);
if (role[2].equals(RaftProtos.RaftPeerRole.LEADER.toString())) {
String hostAddress = role[4].trim();
int grpcPort = configuration.getInt(
Expand Down