diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java index 9e7e1b5b8542..bc39a57403c4 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java +++ b/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 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); } // /////////////////////////////////////////////////// diff --git a/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java b/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java index 925515f6f4ab..5f3a802d79a9 100644 --- a/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java +++ b/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; } diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index f99ea6b9b8c7..d7ce6801d262 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -875,6 +875,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. */ @@ -888,6 +890,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) { @@ -950,8 +956,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); } } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index b24136972ad8..9c6539824ad7 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/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 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