Move the protocol number helpers to NetUtils

createNetworkACL and updateNetworkACLItem each turned protocol number 1
into icmp with their own copy of the same code; they now share
NetUtils.icmpProtocolNumberAsName. The TCP and UDP protocol numbers move
there too, next to ICMP_PROTO_NUMBER, in place of the private copies in
NetworkACLServiceImpl and SetNetworkACLCommand.

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java

@@ -31,7 +31,6 @@
 import org.apache.cloudstack.api.response.NetworkACLResponse;
 import org.apache.cloudstack.api.response.NetworkResponse;
 import org.apache.cloudstack.context.CallContext;
-import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.event.EventTypes;
 import com.cloud.exception.InvalidParameterValueException;
@@ -102,15 +101,7 @@ public boolean isDisplay() {
     }
 
     public String getProtocol() {
-        String p = protocol.trim();
-        // Deal with ICMP(protocol number 1) specially because it need to be paired with icmp type and code
-        if (StringUtils.isNumeric(p)) {
-            int protoNumber = Integer.parseInt(p);
-            if (protoNumber == 1) {
-                p = "icmp";
-            }
-        }
-        return p;
+        return NetUtils.icmpProtocolNumberAsName(protocol);
     }
 
     public void setProtocol(String protocol) {

api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java

@@ -26,7 +26,6 @@
 import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.response.NetworkACLItemResponse;
 import org.apache.cloudstack.context.CallContext;
-import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.event.EventTypes;
 import com.cloud.exception.ResourceUnavailableException;
@@ -101,15 +100,7 @@ public Long getId() {
     }
 
     public String getProtocol() {
-        if (protocol == null) {
-            return null;
-        }
-        String p = protocol.trim();
-        // As on create, protocol number 1 is ICMP so that it keeps its icmp type and code
-        if (StringUtils.isNumeric(p) && Integer.parseInt(p) == NetUtils.ICMP_PROTO_NUMBER) {
-            p = NetUtils.ICMP_PROTO;
-        }
-        return p;
+        return NetUtils.icmpProtocolNumberAsName(protocol);
     }
 
     public List<String> getSourceCidrList() {

core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java

@@ -33,8 +33,6 @@
 
 public class SetNetworkACLCommand extends NetworkElementCommand {
     public static final String RULE_DETAIL_SEPARATOR = ";";
-    private static final int TCP_PROTO_NUMBER = 6;
-    private static final int UDP_PROTO_NUMBER = 17;
 
     NetworkACLTO[] rules;
     NicTO nic;
@@ -128,10 +126,10 @@ protected static String normalizeProtocol(String protocol, boolean hasPorts) {
         if (number == NetUtils.ICMP_PROTO_NUMBER) {
             return NetUtils.ICMP_PROTO;
         }
-        if (hasPorts && number == TCP_PROTO_NUMBER) {
+        if (hasPorts && number == NetUtils.TCP_PROTO_NUMBER) {
             return NetUtils.TCP_PROTO;
         }
-        if (hasPorts && number == UDP_PROTO_NUMBER) {
+        if (hasPorts && number == NetUtils.UDP_PROTO_NUMBER) {
             return NetUtils.UDP_PROTO;
         }
         return String.valueOf(number);

server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java

@@ -117,8 +117,6 @@ public class NetworkACLServiceImpl extends ManagerBase implements NetworkACLServ
     private VpcManager vpcManager;
 
     private String supportedProtocolsForAclRules = "tcp,udp,icmp,all";
-    private static final int TCP_PROTO_NUMBER = 6;
-    private static final int UDP_PROTO_NUMBER = 17;
 
     @Override
     public NetworkACL createNetworkACL(final String name, final String description, final long vpcId, final Boolean forDisplay) {
@@ -703,9 +701,9 @@ protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourc
             return;
         }
         int protoNumber = NumberUtils.toInt(protocol, -1);
-        if (protoNumber != TCP_PROTO_NUMBER && protoNumber != UDP_PROTO_NUMBER) {
+        if (protoNumber != NetUtils.TCP_PROTO_NUMBER && protoNumber != NetUtils.UDP_PROTO_NUMBER) {
             throw new InvalidParameterValueException(String.format("Start and end port can only be given for TCP or UDP (protocol number %d or %d), not for protocol number [%s]",
-                    TCP_PROTO_NUMBER, UDP_PROTO_NUMBER, protocol));
+                    NetUtils.TCP_PROTO_NUMBER, NetUtils.UDP_PROTO_NUMBER, protocol));
         }
     }
 

utils/src/main/java/com/cloud/utils/net/NetUtils.java

@@ -48,6 +48,7 @@
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.SystemUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.math.NumberUtils;
 import org.apache.commons.net.util.SubnetUtils;
 import org.apache.commons.validator.routines.InetAddressValidator;
 import org.apache.commons.validator.routines.RegexValidator;
@@ -82,6 +83,8 @@ public class NetUtils {
     public final static String ICMP_PROTO = "icmp";
     public static final String ICMP6_PROTO = "icmp6";
     public final static int ICMP_PROTO_NUMBER = 1;
+    public final static int TCP_PROTO_NUMBER = 6;
+    public final static int UDP_PROTO_NUMBER = 17;
     public final static String ALL_PROTO = "all";
     public final static String HTTP_PROTO = "http";
     public final static String HTTPS_PROTO = "https";
@@ -1013,6 +1016,21 @@ public static boolean isValidSecurityGroupProto(final String p) {
         return proto.equals(TCP_PROTO) || proto.equals(UDP_PROTO) || proto.equals(ICMP_PROTO) || proto.equals(ALL_PROTO);
     }
 
+    /**
+     * Gives a rule's protocol trimmed, with protocol number 1 as icmp, so that the rule keeps its ICMP
+     * type and code.
+     */
+    public static String icmpProtocolNumberAsName(final String protocol) {
+        if (protocol == null) {
+            return null;
+        }
+        final String p = protocol.trim();
+        if (StringUtils.isNumeric(p) && NumberUtils.toInt(p, -1) == ICMP_PROTO_NUMBER) {
+            return ICMP_PROTO;
+        }
+        return p;
+    }
+
     public static boolean isValidAlgorithm(final String p) {
         final String algo = p.toLowerCase();
         return algo.equals("roundrobin") || algo.equals("leastconn") || algo.equals("source");

utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java

@@ -955,4 +955,19 @@ public void testVpnIpRange() {
         String cidr = NetUtils.transformCidr(startIp + "/" + cidrSize);
         Assert.assertEquals("10.1.2.0/28", cidr);
     }
+
+    @Test
+    public void icmpProtocolNumberAsNameTurnsProtocolNumberOneIntoIcmp() {
+        assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName("1"));
+        assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName(" 1 "));
+        assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName("01"));
+    }
+
+    @Test
+    public void icmpProtocolNumberAsNameLeavesOtherProtocolsTrimmed() {
+        assertEquals("tcp", NetUtils.icmpProtocolNumberAsName(" tcp "));
+        assertEquals("47", NetUtils.icmpProtocolNumberAsName("47"));
+        assertEquals("99999999999", NetUtils.icmpProtocolNumberAsName("99999999999"));
+        Assert.assertNull(NetUtils.icmpProtocolNumberAsName(null));
+    }
 }