smengcl commented on code in PR #11272:
URL: https://github.com/apache/ozone/pull/11272#discussion_r4229195982
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMNodeInfo.java:
##########
@@ -96,6 +96,7 @@ public static List<SCMNodeInfo>
buildNodeInfo(ConfigurationSource conf) {
if (scmAddress == null) {
throw new ConfigurationException(addressKey + "is not defined");
}
+ HddsUtils.validateAdvertisedHost(addressKey, scmAddress);
Review Comment:
SCM startup loads these addresses through
`SCMHANodeDetails.loadSCMHAConfig()`, which does not call this validator. I
reproduced that loader accepting local `0.0.0.0` and peer `169.254.1.1`, while
`SCMNodeInfo.buildNodeInfo()` rejects the same configuration. This leaves
non-secure SCM startup accepting advertised identities that clients and
datanodes now reject.
Could we apply the same check in the startup loader and add a rejection test
in `TestSCMConfiguration`?
```diff
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java
@@ -215,6 +215,7 @@
"%s. SCM RPC Address should be set for all nodes in a SCM " +
"service.", rpcAddrKey);
}
+ HddsUtils.validateAdvertisedHost(rpcAddrKey, rpcAddrStr);
isSCMddressSet = true;
String ratisPortKey =
ConfUtils.addKeySuffixes(OZONE_SCM_RATIS_PORT_KEY,
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java:
##########
@@ -893,6 +895,7 @@ public static Collection<HostAndPort>
getSCMAddressForDatanodes(ConfigurationSou
final Collection<HostAndPort> addresses = new HashSet<>(names.size());
for (String address : names) {
+ validateAdvertisedAddress(OZONE_SCM_NAMES, address);
Review Comment:
This throws `ConfigurationException`, but `InitDatanodeState.call()` only
catches `IllegalArgumentException`. With `ozone.scm.names=::1` or `0.0.0.0`,
the exception escapes through the future, and the datanode keeps retrying in
`INIT` instead of shutting down. I reproduced two cycles with
`shutdownOnError=false`. A wildcard Recon address has the same result.
Could we handle both address lookups in the existing shutdown path and
extend `testDatanodeStateMachineWithInvalidConfiguration` to cover these cases?
```diff
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/InitDatanodeState.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/InitDatanodeState.java
@@ -29,6 +29,7 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
+import org.apache.hadoop.hdds.conf.ConfigurationException;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.protocol.DatanodeDetails;
import org.apache.hadoop.hdds.scm.net.HostAndPort;
@@ -76,11 +77,13 @@
@Override
public DatanodeStateMachine.DatanodeStates call() throws Exception {
final Collection<HostAndPort> addresses;
+ final HostAndPort reconAddress;
try {
addresses = HddsServerUtil.getSCMAddressForDatanodes(conf);
- } catch (IllegalArgumentException e) {
+ reconAddress = getReconAddressForDatanodes(conf);
+ } catch (IllegalArgumentException | ConfigurationException e) {
if (!Strings.isNullOrEmpty(e.getMessage())) {
- LOG.error("Failed to get SCM addresses: {}", e.getMessage());
+ LOG.error("Failed to get SCM or Recon addresses: {}",
e.getMessage());
}
return DatanodeStateMachine.DatanodeStates.SHUTDOWN;
}
@@ -103,7 +106,6 @@
connectionManager.addSCMServer(addr, context.getThreadNamePrefix());
this.context.addEndpoint(addr);
}
- final HostAndPort reconAddress = getReconAddressForDatanodes(conf);
if (reconAddress != null) {
connectionManager.addReconServer(reconAddress,
context.getThreadNamePrefix());
```
--
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]
