Check the ports passed on an ACL update before the stored ones

On update, take the source ports from the command first and fall back to
the rule's stored ports only when none was passed and the protocol is
given. This reads in the order the values apply and does not depend on
transferDataToNetworkAclRulePojo() having already copied the command's
ports onto the rule. Behaviour is unchanged: that copy runs first, so the
rule already held the command's ports when they were passed.

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

Author: bhouse-nexthop

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

@@ -708,15 +708,19 @@ 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.
+     * On update the ports passed in the command are checked first. The ports a rule already carries are
+     * only used for any not passed, and only 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());
+        Integer sourcePortStart = updateNetworkACLItemCmd.getSourcePortStart();
+        Integer sourcePortEnd = updateNetworkACLItemCmd.getSourcePortEnd();
+        if (StringUtils.isNotBlank(updateNetworkACLItemCmd.getProtocol())) {
+            sourcePortStart = ObjectUtils.defaultIfNull(sourcePortStart, networkACLItemVo.getSourcePortStart());
+            sourcePortEnd = ObjectUtils.defaultIfNull(sourcePortEnd, networkACLItemVo.getSourcePortEnd());
+        }
+        validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(), sourcePortStart, sourcePortEnd);
     }
 
     @Override

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

@@ -584,6 +584,16 @@ public void validatePortsAreUsableWithProtocolOnUpdateTestProtocolChangedOntoSto
         networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
     }
 
+    @Test(expected = InvalidParameterValueException.class)
+    public void validatePortsAreUsableWithProtocolOnUpdateTestPortsPassedWithProtocol() {
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("47");
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(22);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(22);
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("47");
+
+        networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
+    }
+
     @Test
     public void validateSourceCidrListTestEmptySourceCirdList() {
         Mockito.when(networkAclItemVoMock.getSourceCidrList()).thenReturn(new ArrayList<>());