Keep protocol 6 and 17 without ports as the whole protocol

Sending 6 or 17 to the VR as tcp or udp also when the rule has no ports
made it a tcp or udp rule with the port range 0:0, which the VR's
iptables path applies as --dport 0: a rule meant for all of TCP or UDP
matched port 0 only. Map them only when the rule has ports; without, the
bare number is what matches the whole protocol, as before.

Also read the protocol as a number, so that "006" is treated as 6, as the
API's own validation does, and check the ports a rule already carries
when an update changes its protocol, so that switching a tcp rule to
another protocol number cannot leave its ports on it.

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

Author: bhouse-nexthop

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

@@ -25,12 +25,16 @@
 import java.util.List;
 import java.util.Locale;
 
+import org.apache.commons.lang3.StringUtils;
+
 import com.cloud.agent.api.to.NetworkACLTO;
 import com.cloud.agent.api.to.NicTO;
 import com.cloud.utils.net.NetUtils;
 
 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;
@@ -71,7 +75,7 @@ public String[][] generateFwRules() {
 
             List<String> cidr;
             final StringBuilder sb = new StringBuilder();
-            final String protocol = normalizeProtocol(aclTO.getProtocol());
+            final String protocol = normalizeProtocol(aclTO.getProtocol(), aclTO.getSrcPortRange() != null);
             sb.append(aclTO.getTrafficType().toString()).append(RULE_DETAIL_SEPARATOR).append(protocol).append(RULE_DETAIL_SEPARATOR);
             if (NetUtils.ICMP_PROTO.equals(protocol)) {
                 sb.append(icmpTypeOrCode(aclTO.getIcmpType())).append(RULE_DETAIL_SEPARATOR).append(icmpTypeOrCode(aclTO.getIcmpCode())).append(RULE_DETAIL_SEPARATOR);
@@ -100,26 +104,36 @@ public String[][] generateFwRules() {
     }
 
     /**
-     * The VR applies ports only to a rule it is told is tcp or udp, and an ICMP type and code only to
-     * one it is told is icmp; any other protocol goes to it as a bare number. Name those three by their
-     * protocol number too, and whatever the case they were stored in, so their ports or ICMP type are
-     * not dropped.
+     * The VR applies ports only to a rule it is sent as tcp or udp, and an ICMP type and code only to one
+     * sent as icmp; any other protocol goes to it as a bare number, which matches the whole protocol. So
+     * send protocol number 1 as icmp, and 6 and 17 as tcp and udp when the rule has ports, whatever form
+     * or case they were stored in. Without ports 6 and 17 stay bare numbers: that is what matches all of
+     * TCP or UDP, where a tcp or udp rule without ports would match port 0 only.
      */
-    protected static String normalizeProtocol(String protocol) {
+    protected static String normalizeProtocol(String protocol, boolean hasPorts) {
         if (protocol == null) {
             return null;
         }
         String p = protocol.trim().toLowerCase(Locale.ROOT);
-        switch (p) {
-            case "1":
-                return NetUtils.ICMP_PROTO;
-            case "6":
-                return NetUtils.TCP_PROTO;
-            case "17":
-                return NetUtils.UDP_PROTO;
-            default:
-                return p;
+        if (!StringUtils.isNumeric(p)) {
+            return p;
+        }
+        int number;
+        try {
+            number = Integer.parseInt(p);
+        } catch (NumberFormatException e) {
+            return p;
+        }
+        if (number == NetUtils.ICMP_PROTO_NUMBER) {
+            return NetUtils.ICMP_PROTO;
+        }
+        if (hasPorts && number == TCP_PROTO_NUMBER) {
+            return NetUtils.TCP_PROTO;
+        }
+        if (hasPorts && number == UDP_PROTO_NUMBER) {
+            return NetUtils.UDP_PROTO;
         }
+        return String.valueOf(number);
     }
 
     private static int icmpTypeOrCode(Integer value) {

core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java

@@ -64,6 +64,20 @@ public void testTcpAndUdpByProtocolNumberKeepTheirPorts() {
         assertEquals("Ingress;udp;53;53;10.0.0.0/24;ACCEPT;", generateRule("17", 53, 53, null, null));
     }
 
+    @Test
+    public void testTcpAndUdpByProtocolNumberWithoutPortsStayWholeProtocol() {
+        // as tcp or udp with no ports the VR would match port 0 only
+        assertEquals("Ingress;6;0;0;10.0.0.0/24;ACCEPT;", generateRule("6", null, null, null, null));
+        assertEquals("Ingress;17;0;0;10.0.0.0/24;ACCEPT;", generateRule("17", null, null, null, null));
+    }
+
+    @Test
+    public void testProtocolNumberInAnotherFormIsReadAsANumber() {
+        assertEquals("Ingress;tcp;22;22;10.0.0.0/24;ACCEPT;", generateRule("006", 22, 22, null, null));
+        assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule(" 01 ", null, null, 8, 0));
+        assertEquals("Ingress;47;0;0;10.0.0.0/24;ACCEPT;", generateRule("047", null, null, null, null));
+    }
+
     @Test
     public void testIcmpByProtocolNumberKeepsItsTypeAndCode() {
         assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("1", null, null, 8, 0));
@@ -72,9 +86,8 @@ public void testIcmpByProtocolNumberKeepsItsTypeAndCode() {
     }
 
     @Test
-    public void testProtocolNameIsMatchedWhateverItsCase() {
+    public void testIcmpIsMatchedWhateverItsCase() {
         assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("ICMP", null, null, 8, 0));
-        assertEquals("Ingress;tcp;22;22;10.0.0.0/24;ACCEPT;", generateRule("TCP", 22, 22, null, null));
     }
 
     @Test

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

@@ -709,6 +709,18 @@ protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourc
         }
     }
 
+    /**
+     * On update the ports a rule already carries are only checked when the protocol is given too, so
+     * that a rule stored with ports on another protocol number can still be edited otherwise, while a
+     * protocol change cannot leave them on it.
+     */
+    protected void validatePortsAreUsableWithProtocolOnUpdate(UpdateNetworkACLItemCmd updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) {
+        boolean protocolGiven = StringUtils.isNotBlank(updateNetworkACLItemCmd.getProtocol());
+        validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(),
+                protocolGiven ? networkACLItemVo.getSourcePortStart() : updateNetworkACLItemCmd.getSourcePortStart(),
+                protocolGiven ? networkACLItemVo.getSourcePortEnd() : updateNetworkACLItemCmd.getSourcePortEnd());
+    }
+
     @Override
     public NetworkACLItem getNetworkACLItem(final long ruleId) {
         return _networkAclMgr.getNetworkACLItem(ruleId);
@@ -895,8 +907,7 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork
 
         transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl);
         validateNetworkACLItem(networkACLItemVo);
-        // only the ports given now: older rules may already carry ports, and must stay editable
-        validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(), updateNetworkACLItemCmd.getSourcePortStart(), updateNetworkACLItemCmd.getSourcePortEnd());
+        validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmd, networkACLItemVo);
         return _networkAclMgr.updateNetworkACLItem(networkACLItemVo);
     }
 

server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java

@@ -564,6 +564,26 @@ public void validatePortsAreUsableWithProtocolTestOtherProtocolNumberWithStartPo
         networkAclServiceImpl.validatePortsAreUsableWithProtocol("47", 22, null);
     }
 
+    @Test
+    public void validatePortsAreUsableWithProtocolOnUpdateTestStoredPortsKeptWhenProtocolNotGiven() {
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("47");
+
+        networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void validatePortsAreUsableWithProtocolOnUpdateTestProtocolChangedOntoStoredPorts() {
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("47");
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("47");
+        Mockito.when(networkAclItemVoMock.getSourcePortStart()).thenReturn(22);
+        Mockito.when(networkAclItemVoMock.getSourcePortEnd()).thenReturn(22);
+
+        networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
+    }
+
     @Test
     public void validateSourceCidrListTestEmptySourceCirdList() {
         Mockito.when(networkAclItemVoMock.getSourceCidrList()).thenReturn(new ArrayList<>());