Re: [PR] HDDS-16308. Validate advertised addresses before startup, registration, and publication [ozone]

["smengcl (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org