Refuse an incomplete full update of a network ACL rule, and keep an ICMP type given with another protocol

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

@@ -26,8 +26,10 @@
 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.InvalidParameterValueException;
 import com.cloud.exception.ResourceUnavailableException;
 import com.cloud.network.vpc.NetworkACLItem;
 import com.cloud.user.Account;
@@ -62,7 +64,7 @@ public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd {
     @Parameter(name = ApiConstants.ICMP_CODE, type = CommandType.INTEGER, description = "Error code for this ICMP message")
     private Integer icmpCode;
 
-    @Parameter(name = ApiConstants.TRAFFIC_TYPE, type = CommandType.STRING, description = "The traffic type for the ACL, can be Ingress or Egress, defaulted to Ingress if not specified")
+    @Parameter(name = ApiConstants.TRAFFIC_TYPE, type = CommandType.STRING, description = "The traffic type for the ACL, can be Ingress or Egress. Required when partialupgrade is false")
     private String trafficType;
 
     @Parameter(name = ApiConstants.NUMBER, type = CommandType.INTEGER, description = "The Network of the Instance the ACL will be created for")
@@ -78,7 +80,7 @@ public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd {
     @Parameter(name = ApiConstants.ACL_REASON, type = CommandType.STRING, description = "A description indicating why the ACL rule is required.")
     private String reason;
 
-    @Parameter(name = ApiConstants.ACL_RULE_PARTIAL_UPGRADE, type = CommandType.BOOLEAN, required = false, description = "Indicates if the ACL rule is to be updated partially (merging the parameters sent with current configuration) or completely (disconsidering all of the current configurations). The default value is 'true'.")
+    @Parameter(name = ApiConstants.ACL_RULE_PARTIAL_UPGRADE, type = CommandType.BOOLEAN, required = false, description = "Indicates if the ACL rule is to be updated partially (merging the parameters sent with current configuration) or completely (disconsidering all of the current configurations). The default value is 'true'. When it is 'false', protocol, cidrlist and traffictype are required.")
     private boolean partialUpgrade = true;
 
     // ///////////////////////////////////////////////////
@@ -111,14 +113,15 @@ public List<String> getSourceCidrList() {
     }
 
     public NetworkACLItem.TrafficType getTrafficType() {
-        if (trafficType != null) {
-            for (NetworkACLItem.TrafficType type : NetworkACLItem.TrafficType.values()) {
-                if (type.toString().equalsIgnoreCase(trafficType)) {
-                    return type;
-                }
+        if (StringUtils.isBlank(trafficType)) {
+            return null;
+        }
+        for (NetworkACLItem.TrafficType type : NetworkACLItem.TrafficType.values()) {
+            if (type.toString().equalsIgnoreCase(trafficType.trim())) {
+                return type;
             }
         }
-        return null;
+        throw new InvalidParameterValueException("Invalid traffic type " + trafficType);
     }
 
     // ///////////////////////////////////////////////////

engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java

@@ -92,8 +92,14 @@ public NetworkACLItemVO findById(Long id) {
 
     @Override
     public boolean update(Long id, NetworkACLItemVO item) {
+        // the rule and its CIDRs are written together, so that a failure on the CIDRs does not leave the rule half updated
+        TransactionLegacy txn = TransactionLegacy.currentTxn();
+        txn.start();
+
         boolean result = super.update(id, item);
         _networkACLItemCidrsDao.updateCidrs(item.getId(), item.getSourceCidrList());
+
+        txn.commit();
         return result;
     }
 

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

@@ -885,6 +885,8 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork
      *  All of the parameters in {@link UpdateNetworkACLItemCmd} that are not null will be set to their corresponding fields in {@link NetworkACLItemVO}.
      *  If the parameter {@link UpdateNetworkACLItemCmd#isPartialUpgrade()} returns false, we will use null parameters, which will allow us to completely update the ACL rule.
      *  However, the number and custom Uuid will never be set to null. Therefore, if it is not a partial upgrade, these values will remain the same.
+     *  A complete update requires the protocol, the CIDR list and the traffic type: a rule cannot be stored without them, and defaulting them could widen the rule
+     *  or change its direction without the caller noticing.
      *
      *  We use {@link #validateAndCreateNetworkAclRuleAction(String)} when converting an action as {@link String} to its Enum corresponding value.
      */
@@ -898,6 +900,10 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe
             networkACLItemVo.setNumber(number);
         }
         boolean isPartialUpgrade = updateNetworkACLItemCmd.isPartialUpgrade();
+        if (!isPartialUpgrade && (StringUtils.isBlank(updateNetworkACLItemCmd.getProtocol()) || updateNetworkACLItemCmd.getSourceCidrList() == null
+                || updateNetworkACLItemCmd.getTrafficType() == null)) {
+            throw new InvalidParameterValueException("The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false");
+        }
 
         Integer sourcePortStart = updateNetworkACLItemCmd.getSourcePortStart();
         if (!isPartialUpgrade || sourcePortStart != null) {
@@ -960,8 +966,9 @@ private void updateIcmpCodeAndTypeFullUpgrade (Integer icmpCode, Integer icmpTyp
             networkACLItemVo.setIcmpCode(icmpCode != null ? icmpCode : -1);
             networkACLItemVo.setIcmpType(icmpType != null ? icmpType : -1);
         } else {
-            networkACLItemVo.setIcmpCode(null);
-            networkACLItemVo.setIcmpType(null);
+            // keep what was given, so that validateProtocol() refuses an ICMP type or code on another protocol, as on create
+            networkACLItemVo.setIcmpCode(icmpCode);
+            networkACLItemVo.setIcmpType(icmpType);
         }
     }
 

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

@@ -814,29 +814,73 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
     public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull() {
         Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false);
 
+        Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
+
+        InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class,
+                () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock));
+
+        Assert.assertEquals("The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false", e.getMessage());
+        Mockito.verify(networkAclItemVoMock, Mockito.never()).setProtocol(nullable(String.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.never()).setSourceCidrList(nullable(List.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.never()).setTrafficType(nullable(TrafficType.class));
+    }
+
+    @Test
+    public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeWithoutCidrListOrTrafficType() {
+        Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false);
+        Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp");
+
+        String expected = "The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false";
+
+        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
+        InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class,
+                () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock));
+        Assert.assertEquals(expected, e.getMessage());
+
+        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(List.of("10.1.1.0/24"));
+        Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null);
+        e = Assert.assertThrows(InvalidParameterValueException.class,
+                () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock));
+        Assert.assertEquals(expected, e.getMessage());
+        Mockito.verify(updateNetworkACLItemCmdMock).getTrafficType();
+
+        Mockito.verify(networkAclItemVoMock, Mockito.never()).setSourceCidrList(nullable(List.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.never()).setTrafficType(nullable(TrafficType.class));
+    }
+
+    @Test
+    public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeRequiredValuesOnly() {
+        Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false);
+
+        List<String> cidrs = List.of("10.1.1.0/24");
         Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
-        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
-        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(cidrs);
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp");
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn(null);
-        Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Egress);
         Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn(null);
 
         Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(false);
-        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("");
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("tcp");
 
         networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock);
 
         Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setNumber(nullable(Integer.class));
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortStart(nullable(Integer.class));
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(nullable(Integer.class));
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(nullable(List.class));
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol(nullable(String.class));
-        Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(nullable(Action.class));
-        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(nullable(TrafficType.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortStart(null);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(null);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(cidrs);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol("tcp");
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpCode(null);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpType(null);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(Action.Allow);
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(TrafficType.Egress);
         Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(nullable(String.class));
         Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setReason(nullable(String.class));
         Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setDisplay(nullable(Boolean.class));
@@ -944,8 +988,67 @@ public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNotNullCodeAndType(
 
         networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);
 
-        Mockito.verify(networkAclItemVoMock).setIcmpCode(null);
-        Mockito.verify(networkAclItemVoMock).setIcmpType(null);
+        Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
+        Mockito.verify(networkAclItemVoMock).setIcmpType(5);
+    }
+
+    private void setUpNotPartialUpgradeToTcp() {
+        Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false);
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp");
+        Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(List.of("10.1.1.0/24"));
+        Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Ingress);
+    }
+
+    private TrafficType trafficTypeOf(String trafficType) {
+        UpdateNetworkACLItemCmd cmd = new UpdateNetworkACLItemCmd();
+        org.springframework.test.util.ReflectionTestUtils.setField(cmd, "trafficType", trafficType);
+        return cmd.getTrafficType();
+    }
+
+    @Test
+    public void updateNetworkACLItemCmdGetTrafficTypeTest() {
+        Assert.assertNull(trafficTypeOf(null));
+        Assert.assertNull(trafficTypeOf(""));
+        Assert.assertEquals(TrafficType.Egress, trafficTypeOf("egress"));
+        Assert.assertEquals(TrafficType.Ingress, trafficTypeOf("Ingress"));
+        InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, () -> trafficTypeOf("egres"));
+        Assert.assertEquals("Invalid traffic type egres", e.getMessage());
+    }
+
+    @Test
+    public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeTcpWithIcmpTypeIsRefused() {
+        NetworkACLItemVO rule = new NetworkACLItemVO(null, null, NetUtils.ICMP_PROTO, networkAclMockId, List.of("10.1.1.0/24"), 0, 8,
+                TrafficType.Ingress, Action.Allow, 10, null);
+        setUpNotPartialUpgradeToTcp();
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(8);
+        Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true);
+
+        networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, rule, networkAclMock);
+
+        InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, () -> networkAclServiceImpl.validateNetworkACLItem(rule));
+        Assert.assertEquals("Can specify icmpCode and icmpType for ICMP protocol only", e.getMessage());
+    }
+
+    @Test
+    public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeIcmpToTcpClearsIcmpFields() {
+        NetworkACLItemVO rule = new NetworkACLItemVO(null, null, NetUtils.ICMP_PROTO, networkAclMockId, List.of("10.1.1.0/24"), 0, 8,
+                TrafficType.Ingress, Action.Allow, 10, null);
+        setUpNotPartialUpgradeToTcp();
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true);
+
+        networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, rule, networkAclMock);
+        networkAclServiceImpl.validateNetworkACLItem(rule);
+
+        Assert.assertEquals("tcp", rule.getProtocol());
+        Assert.assertNull(rule.getIcmpType());
+        Assert.assertNull(rule.getIcmpCode());
+        Assert.assertEquals(List.of("10.1.1.0/24"), rule.getSourceCidrList());
+        Assert.assertEquals(TrafficType.Ingress, rule.getTrafficType());
     }
 
     @Test