Skip to content

Commit 4a0ca20

Browse files
gpordeusgpordeus
authored
An ICMP ACL rule should not be able to have code and type null (#8464)
1 parent 187f17c commit 4a0ca20

File tree

2 files changed

+113
-20
lines changed

2 files changed

+113
-20
lines changed

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

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -860,14 +860,7 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe
860860
if (!isPartialUpgrade || StringUtils.isNotBlank(protocol)) {
861861
networkACLItemVo.setProtocol(protocol);
862862
}
863-
Integer icmpCode = updateNetworkACLItemCmd.getIcmpCode();
864-
if (!isPartialUpgrade || icmpCode != null) {
865-
networkACLItemVo.setIcmpCode(icmpCode);
866-
}
867-
Integer icmpType = updateNetworkACLItemCmd.getIcmpType();
868-
if (!isPartialUpgrade || icmpType != null) {
869-
networkACLItemVo.setIcmpType(icmpType);
870-
}
863+
updateIcmpCodeAndType(isPartialUpgrade, updateNetworkACLItemCmd, networkACLItemVo);
871864
String action = updateNetworkACLItemCmd.getAction();
872865
if (!isPartialUpgrade || StringUtils.isNotBlank(action)) {
873866
Action aclRuleAction = validateAndCreateNetworkAclRuleAction(action);
@@ -891,6 +884,32 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe
891884
}
892885
}
893886

887+
protected void updateIcmpCodeAndType (boolean isPartialUpgrade, UpdateNetworkACLItemCmd updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) {
888+
Integer icmpCode = updateNetworkACLItemCmd.getIcmpCode();
889+
Integer icmpType = updateNetworkACLItemCmd.getIcmpType();
890+
891+
if (!isPartialUpgrade) {
892+
updateIcmpCodeAndTypeFullUpgrade(icmpCode, icmpType, networkACLItemVo);
893+
return;
894+
}
895+
if (icmpCode != null) {
896+
networkACLItemVo.setIcmpCode(icmpCode);
897+
}
898+
if (icmpType != null) {
899+
networkACLItemVo.setIcmpType(icmpType);
900+
}
901+
}
902+
903+
private void updateIcmpCodeAndTypeFullUpgrade (Integer icmpCode, Integer icmpType, NetworkACLItemVO networkACLItemVo) {
904+
if (networkACLItemVo.getProtocol().equalsIgnoreCase(NetUtils.ICMP_PROTO)) {
905+
networkACLItemVo.setIcmpCode(icmpCode != null ? icmpCode : -1);
906+
networkACLItemVo.setIcmpType(icmpType != null ? icmpType : -1);
907+
} else {
908+
networkACLItemVo.setIcmpCode(null);
909+
networkACLItemVo.setIcmpType(null);
910+
}
911+
}
912+
894913
/**
895914
* We validate the network ACL rule ID provided. If not ACL rule is found with the given Id an {@link InvalidParameterValueException} is thrown.
896915
* If an ACL rule is found, we return the clone of the rule to avoid messing up with CGlib enhanced objects that might be linked to database entries.

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

Lines changed: 86 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
import com.cloud.exception.PermissionDeniedException;
3333
import com.cloud.network.vpc.dao.VpcDao;
34+
import com.cloud.utils.net.NetUtils;
3435
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
3536
import org.apache.cloudstack.api.ServerApiException;
3637
import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd;
@@ -772,8 +773,6 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
772773
Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
773774
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
774775
Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
775-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null);
776-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null);
777776
Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn(null);
778777
Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null);
779778
Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn(null);
@@ -789,8 +788,7 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
789788
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setSourcePortEnd(Mockito.anyInt());
790789
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setSourceCidrList(Mockito.anyList());
791790
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setProtocol(Mockito.anyString());
792-
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setIcmpCode(Mockito.anyInt());
793-
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setIcmpType(Mockito.anyInt());
791+
Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
794792
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setAction(Mockito.any(Action.class));
795793
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setTrafficType(Mockito.any(TrafficType.class));
796794
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(Mockito.anyString());
@@ -808,14 +806,13 @@ public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull()
808806
Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
809807
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
810808
Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
811-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null);
812-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null);
813809
Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn(null);
814810
Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null);
815811
Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn(null);
816812
Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn(null);
817813

818814
Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(false);
815+
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("");
819816

820817
networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock);
821818

@@ -824,8 +821,7 @@ public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull()
824821
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(nullable(Integer.class));
825822
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(nullable(List.class));
826823
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol(nullable(String.class));
827-
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpCode(nullable(Integer.class));
828-
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpType(nullable(Integer.class));
824+
Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
829825
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(nullable(Action.class));
830826
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(nullable(TrafficType.class));
831827
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(nullable(String.class));
@@ -845,14 +841,13 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
845841
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(cidrsList);
846842

847843
Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("all");
848-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(5);
849-
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(6);
850844
Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn("deny");
851845
Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Egress);
852846
Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn("customUuid");
853847
Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn("reason");
854848

855849
Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true);
850+
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("");
856851

857852
networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock);
858853

@@ -861,8 +856,7 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
861856
Mockito.verify(networkAclItemVoMock).setSourcePortEnd(24);
862857
Mockito.verify(networkAclItemVoMock).setSourceCidrList(cidrsList);
863858
Mockito.verify(networkAclItemVoMock).setProtocol("all");
864-
Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
865-
Mockito.verify(networkAclItemVoMock).setIcmpType(6);
859+
Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
866860
Mockito.verify(networkAclItemVoMock).setAction(Action.Deny);
867861
Mockito.verify(networkAclItemVoMock).setTrafficType(TrafficType.Egress);
868862
Mockito.verify(networkAclItemVoMock).setUuid("customUuid");
@@ -871,6 +865,86 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
871865
Mockito.verify(networkAclServiceImpl).validateAndCreateNetworkAclRuleAction("deny");
872866
}
873867

868+
private void setUpdateICMPCodeAndTypeTest(String protocol, Integer icmpCode, Integer icmpType) {
869+
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn(protocol);
870+
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(icmpCode);
871+
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(icmpType);
872+
}
873+
874+
@Test
875+
public void updateICMPCodeAndTypeTestIsPartialUpgradeIsICMPNotNullCodeAndType() {
876+
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, 5, 5);
877+
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);
878+
879+
Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
880+
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
881+
}
882+
883+
@Test
884+
public void updateICMPCodeAndTypeTestIsPartialUpgradeIsICMPNullCodeAndType() {
885+
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, null, null);
886+
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);
887+
888+
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpCode(nullable(Integer.class));
889+
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpType(nullable(Integer.class));
890+
}
891+
892+
@Test
893+
public void updateICMPCodeAndTypeTestIsPartialUpgradeNotICMPNotNullCodeAndType() {
894+
setUpdateICMPCodeAndTypeTest("", 5, 5);
895+
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);
896+
897+
Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
898+
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
899+
}
900+
901+
@Test
902+
public void updateICMPCodeAndTypeTestIsPartialUpgradeNotICMPNullCodeAndType() {
903+
setUpdateICMPCodeAndTypeTest("", null, null);
904+
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);
905+
906+
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpCode(Mockito.any());
907+
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpType(Mockito.any());
908+
}
909+
910+
@Test
911+
public void updateICMPCodeAndTypeTestNotPartialUpgradeIsICMPNotNullCodeAndType() {
912+
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, 5, 5);
913+
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);
914+
915+
Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
916+
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
917+
}
918+
919+
@Test
920+
public void updateICMPCodeAndTypeTestNotPartialUpgradeIsICMPNullCodeAndType() {
921+
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, null, null);
922+
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);
923+
924+
Mockito.verify(networkAclItemVoMock).setIcmpCode(-1);
925+
Mockito.verify(networkAclItemVoMock).setIcmpType(-1);
926+
}
927+
928+
@Test
929+
public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNotNullCodeAndType() {
930+
setUpdateICMPCodeAndTypeTest("", 5, 5);
931+
932+
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);
933+
934+
Mockito.verify(networkAclItemVoMock).setIcmpCode(null);
935+
Mockito.verify(networkAclItemVoMock).setIcmpType(null);
936+
}
937+
938+
@Test
939+
public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNullCodeAndType() {
940+
setUpdateICMPCodeAndTypeTest("", null, null);
941+
942+
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);
943+
944+
Mockito.verify(networkAclItemVoMock).setIcmpCode(null);
945+
Mockito.verify(networkAclItemVoMock).setIcmpType(null);
946+
}
947+
874948
@Test
875949
public void updateNetworkACLTestParametersNotNull() {
876950
String name = "name";

0 commit comments

Comments
 (0)