[ISSUE #8345] Check cluster name in client when registerInstance (#8618)

* Check cluster name in client when registerInstance

* Use `Pattern.compile` to check number to avoid performance lost.

api/src/main/java/com/alibaba/nacos/api/common/Constants.java

@@ -188,7 +188,7 @@ public class Constants {
     
     public static final String NULL_STRING = "null";
     
-    public static final String NUMBER_PATTERN = "^\\d+$";
+    public static final String NUMBER_PATTERN_STRING = "^\\d+$";
     
     public static final String ANY_PATTERN = ".*";
     
@@ -214,6 +214,8 @@ public class Constants {
     
     public static final String CHARSET_KEY = "charset";
     
+    public static final String CLUSTER_NAME_PATTERN_STRING = "^[0-9a-zA-Z-]+$";
+    
     /**
      * The constants in config directory.
      */

api/src/main/java/com/alibaba/nacos/api/naming/pojo/Instance.java

@@ -18,7 +18,7 @@ package com.alibaba.nacos.api.naming.pojo;
 
 import com.alibaba.nacos.api.common.Constants;
 import com.alibaba.nacos.api.naming.PreservedMetadataKeys;
-import com.alibaba.nacos.api.utils.StringUtils;
+import com.alibaba.nacos.api.naming.utils.NamingUtils;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonInclude.Include;
 
@@ -26,8 +26,6 @@ import java.io.Serializable;
 import java.util.HashMap;
 import java.util.Map;
 
-import static com.alibaba.nacos.api.common.Constants.NUMBER_PATTERN;
-
 /**
  * Instance.
  *
@@ -37,8 +35,6 @@ import static com.alibaba.nacos.api.common.Constants.NUMBER_PATTERN;
 public class Instance implements Serializable {
     
     private static final long serialVersionUID = -742906310567291979L;
-
-    private static final String CLUSTER_NAME_SYNTAX = "[0-9a-zA-Z-]+";
     
     /**
      * unique id of this instance.
@@ -138,7 +134,6 @@ public class Instance implements Serializable {
     
     public void setClusterName(final String clusterName) {
         this.clusterName = clusterName;
-        checkClusterNameFormat();
     }
     
     public String getServiceName() {
@@ -255,7 +250,7 @@ public class Instance implements Serializable {
             return defaultValue;
         }
         final String value = getMetadata().get(key);
-        if (!StringUtils.isEmpty(value) && value.matches(NUMBER_PATTERN)) {
+        if (NamingUtils.isNumber(value)) {
             return Long.parseLong(value);
         }
         return defaultValue;
@@ -267,19 +262,5 @@ public class Instance implements Serializable {
         }
         return getMetadata().get(key);
     }
-
-    /**
-     * validate the cluster name.
-     *
-     * <p>the cluster name only the arabic numerals, letters and endashes are allowed.
-     *
-     * @throws IllegalArgumentException the cluster name is null, or the cluster name is
-     *                                  illegal
-     */
-    public void checkClusterNameFormat() {
-        if (!StringUtils.isEmpty(clusterName) && !clusterName.matches(CLUSTER_NAME_SYNTAX)) {
-            throw new IllegalArgumentException("cluster name can only have these characters: 0-9a-zA-Z-, current: " + clusterName);
-        }
-    }
     
 }

api/src/main/java/com/alibaba/nacos/api/naming/utils/NamingUtils.java

@@ -21,6 +21,11 @@ import com.alibaba.nacos.api.exception.NacosException;
 import com.alibaba.nacos.api.naming.pojo.Instance;
 import com.alibaba.nacos.api.utils.StringUtils;
 
+import java.util.regex.Pattern;
+
+import static com.alibaba.nacos.api.common.Constants.CLUSTER_NAME_PATTERN_STRING;
+import static com.alibaba.nacos.api.common.Constants.NUMBER_PATTERN_STRING;
+
 /**
  * NamingUtils.
  *
@@ -29,6 +34,10 @@ import com.alibaba.nacos.api.utils.StringUtils;
  */
 public class NamingUtils {
     
+    private static final Pattern CLUSTER_NAME_PATTERN = Pattern.compile(CLUSTER_NAME_PATTERN_STRING);
+    
+    private static final Pattern NUMBER_PATTERN = Pattern.compile(NUMBER_PATTERN_STRING);
+    
     /**
      * Returns a combined string with serviceName and groupName. serviceName can not be nil.
      *
@@ -129,5 +138,20 @@ public class NamingUtils {
             throw new NacosException(NacosException.INVALID_PARAM,
                     "Instance 'heart beat interval' must less than 'heart beat timeout' and 'ip delete timeout'.");
         }
+        if (!StringUtils.isEmpty(instance.getClusterName()) && !CLUSTER_NAME_PATTERN.matcher(instance.getClusterName()).matches()) {
+            throw new NacosException(NacosException.INVALID_PARAM,
+                    String.format("Instance 'clusterName' should be characters with only 0-9a-zA-Z-. (current: %s)",
+                            instance.getClusterName()));
+        }
+    }
+    
+    /**
+     * Check string is a number or not.
+     *
+     * @param str a string of digits
+     * @return if it is a string of digits, return true
+     */
+    public static boolean isNumber(String str) {
+        return !StringUtils.isEmpty(str) && NUMBER_PATTERN.matcher(str).matches();
     }
 }

api/src/test/java/com/alibaba/nacos/api/naming/pojo/InstanceTest.java

@@ -26,10 +26,5 @@ public class InstanceTest extends TestCase {
         Instance instance = new Instance();
         instance.setClusterName("demo");
         assertEquals("demo", instance.getClusterName());
-        try {
-            instance.setClusterName("demo,demo1,demo2");
-        } catch (Exception e) {
-            assertEquals("cluster name can only have these characters: 0-9a-zA-Z-, current: demo,demo1,demo2", e.getMessage());
-        }
     }
 }

api/src/test/java/com/alibaba/nacos/api/naming/utils/NamingUtilsTest.java

@@ -16,10 +16,17 @@
 
 package com.alibaba.nacos.api.naming.utils;
 
+import com.alibaba.nacos.api.exception.NacosException;
+import com.alibaba.nacos.api.naming.PreservedMetadataKeys;
+import com.alibaba.nacos.api.naming.pojo.Instance;
 import com.alibaba.nacos.api.utils.StringUtils;
 import org.junit.Test;
 
+import java.util.HashMap;
+import java.util.Map;
+
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class NamingUtilsTest {
     
@@ -34,4 +41,55 @@ public class NamingUtilsTest {
         String groupNameAndServiceName = NamingUtils.getGroupedNameOptional("serviceA", "groupA");
         assertEquals("groupA@@serviceA", groupNameAndServiceName);
     }
+    
+    @Test
+    public void testCheckInstanceIsLegal() throws NacosException {
+        // check invalid clusterName
+        Instance instance = new Instance();
+        instance.setClusterName("cluster1,cluster2");
+        try {
+            NamingUtils.checkInstanceIsLegal(instance);
+            assertTrue(false);
+        } catch (Exception e) {
+            assertTrue(NacosException.class.equals(e.getClass()));
+            assertEquals(
+                    "Instance 'clusterName' should be characters with only 0-9a-zA-Z-. (current: cluster1,cluster2)",
+                    e.getMessage());
+        }
+        
+        // valid clusterName
+        instance.setClusterName("cluster1");
+        NamingUtils.checkInstanceIsLegal(instance);
+        assertTrue(true);
+    
+        // check heartBeatTimeout, heartBeatInterval, ipDeleteTimeout
+        Map<String, String> meta = new HashMap<>();
+        meta.put(PreservedMetadataKeys.HEART_BEAT_TIMEOUT, "1");
+        meta.put(PreservedMetadataKeys.HEART_BEAT_INTERVAL, "2");
+        meta.put(PreservedMetadataKeys.IP_DELETE_TIMEOUT, "1");
+        instance.setMetadata(meta);
+        try {
+            NamingUtils.checkInstanceIsLegal(instance);
+            assertTrue(false);
+        } catch (Exception e) {
+            assertTrue(NacosException.class.equals(e.getClass()));
+            assertEquals(
+                    "Instance 'heart beat interval' must less than 'heart beat timeout' and 'ip delete timeout'.",
+                    e.getMessage());
+        }
+        meta.put(PreservedMetadataKeys.HEART_BEAT_TIMEOUT, "3");
+        meta.put(PreservedMetadataKeys.HEART_BEAT_INTERVAL, "2");
+        meta.put(PreservedMetadataKeys.IP_DELETE_TIMEOUT, "3");
+        NamingUtils.checkInstanceIsLegal(instance);
+        assertTrue(true);
+    }
+    
+    @Test
+    public void testIsNumber() {
+        String str1 = "abc";
+        assertTrue(!NamingUtils.isNumber(str1));
+    
+        String str2 = "123456";
+        assertTrue(NamingUtils.isNumber(str2));
+    }
 }

client/src/test/java/com/alibaba/nacos/client/naming/NacosNamingServiceTest.java

@@ -31,7 +31,9 @@ import com.alibaba.nacos.client.naming.remote.NamingClientProxy;
 import com.alibaba.nacos.client.naming.remote.http.NamingHttpClientProxy;
 import org.junit.Assert;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.mockito.ArgumentMatcher;
 
 import java.lang.reflect.Field;
@@ -52,6 +54,9 @@ import static org.mockito.Mockito.when;
 
 public class NacosNamingServiceTest {
     
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+    
     private NacosNamingService client;
     
     private NamingClientProxy proxy;
@@ -183,6 +188,21 @@ public class NacosNamingServiceTest {
         verify(proxy, times(1)).registerService(serviceName, groupName, instance);
     }
     
+    @Test
+    public void testRegisterInstance7() throws NacosException {
+        expectedException.expect(NacosException.class);
+        expectedException.expectMessage(
+                "Instance 'clusterName' should be characters with only 0-9a-zA-Z-. (current: cluster1,cluster2)");
+        
+        //given
+        String serviceName = "service1";
+        String groupName = "group1";
+        Instance instance = new Instance();
+        instance.setClusterName("cluster1,cluster2");
+        //when
+        client.registerInstance(serviceName, groupName, instance);
+    }
+    
     @Test
     public void testDeregisterInstance1() throws NacosException {
         //given