Skip to content

Fix ICMP ACL rule edge case #8464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 1 commit into from
Feb 14, 2024
Merged

Fix ICMP ACL rule edge case #8464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,14 +862,7 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe
if (!isPartialUpgrade || StringUtils.isNotBlank(protocol)) {
networkACLItemVo.setProtocol(protocol);
}
Integer icmpCode = updateNetworkACLItemCmd.getIcmpCode();
if (!isPartialUpgrade || icmpCode != null) {
networkACLItemVo.setIcmpCode(icmpCode);
}
Integer icmpType = updateNetworkACLItemCmd.getIcmpType();
if (!isPartialUpgrade || icmpType != null) {
networkACLItemVo.setIcmpType(icmpType);
}
updateIcmpCodeAndType(isPartialUpgrade, updateNetworkACLItemCmd, networkACLItemVo);
String action = updateNetworkACLItemCmd.getAction();
if (!isPartialUpgrade || StringUtils.isNotBlank(action)) {
Action aclRuleAction = validateAndCreateNetworkAclRuleAction(action);
Expand All @@ -893,6 +886,32 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe
}
}

protected void updateIcmpCodeAndType (boolean isPartialUpgrade, UpdateNetworkACLItemCmd updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) {
Integer icmpCode = updateNetworkACLItemCmd.getIcmpCode();
Integer icmpType = updateNetworkACLItemCmd.getIcmpType();

if (!isPartialUpgrade) {
updateIcmpCodeAndTypeFullUpgrade(icmpCode, icmpType, networkACLItemVo);
return;
}
if (icmpCode != null) {
networkACLItemVo.setIcmpCode(icmpCode);
}
if (icmpType != null) {
networkACLItemVo.setIcmpType(icmpType);
}
}

private void updateIcmpCodeAndTypeFullUpgrade (Integer icmpCode, Integer icmpType, NetworkACLItemVO networkACLItemVo) {
if (networkACLItemVo.getProtocol().equalsIgnoreCase(NetUtils.ICMP_PROTO)) {
networkACLItemVo.setIcmpCode(icmpCode != null ? icmpCode : -1);
networkACLItemVo.setIcmpType(icmpType != null ? icmpType : -1);
} else {
networkACLItemVo.setIcmpCode(null);
networkACLItemVo.setIcmpType(null);
}
}

/**
* We validate the network ACL rule ID provided. If not ACL rule is found with the given Id an {@link InvalidParameterValueException} is thrown.
* 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.
Expand Down
98 changes: 86 additions & 12 deletions server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

import com.cloud.exception.PermissionDeniedException;
import com.cloud.network.vpc.dao.VpcDao;
import com.cloud.utils.net.NetUtils;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd;
Expand Down Expand Up @@ -772,8 +773,6 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
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.getCustomId()).thenReturn(null);
Expand All @@ -789,8 +788,7 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setSourcePortEnd(Mockito.anyInt());
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setSourceCidrList(Mockito.anyListOf(String.class));
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setProtocol(Mockito.anyString());
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setIcmpCode(Mockito.anyInt());
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setIcmpType(Mockito.anyInt());
Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setAction(Mockito.any(Action.class));
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setTrafficType(Mockito.any(TrafficType.class));
Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(Mockito.anyString());
Expand All @@ -808,14 +806,13 @@ public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull()
Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
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.getCustomId()).thenReturn(null);
Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn(null);

Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(false);
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("");

networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock);

Expand All @@ -824,8 +821,7 @@ public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull()
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(networkAclItemVoMock, Mockito.times(1)).setIcmpCode(nullable(Integer.class));
Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpType(nullable(Integer.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(0)).setUuid(nullable(String.class));
Expand All @@ -845,14 +841,13 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(cidrsList);

Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("all");
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(5);
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(6);
Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn("deny");
Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Egress);
Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn("customUuid");
Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn("reason");

Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true);
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("");

networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock);

Expand All @@ -861,8 +856,7 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
Mockito.verify(networkAclItemVoMock).setSourcePortEnd(24);
Mockito.verify(networkAclItemVoMock).setSourceCidrList(cidrsList);
Mockito.verify(networkAclItemVoMock).setProtocol("all");
Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
Mockito.verify(networkAclItemVoMock).setIcmpType(6);
Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any());
Mockito.verify(networkAclItemVoMock).setAction(Action.Deny);
Mockito.verify(networkAclItemVoMock).setTrafficType(TrafficType.Egress);
Mockito.verify(networkAclItemVoMock).setUuid("customUuid");
Expand All @@ -871,6 +865,86 @@ public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
Mockito.verify(networkAclServiceImpl).validateAndCreateNetworkAclRuleAction("deny");
}

private void setUpdateICMPCodeAndTypeTest(String protocol, Integer icmpCode, Integer icmpType) {
Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn(protocol);
Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(icmpCode);
Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(icmpType);
}

@Test
public void updateICMPCodeAndTypeTestIsPartialUpgradeIsICMPNotNullCodeAndType() {
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, 5, 5);
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
}

@Test
public void updateICMPCodeAndTypeTestIsPartialUpgradeIsICMPNullCodeAndType() {
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, null, null);
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpCode(nullable(Integer.class));
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpType(nullable(Integer.class));
}

@Test
public void updateICMPCodeAndTypeTestIsPartialUpgradeNotICMPNotNullCodeAndType() {
setUpdateICMPCodeAndTypeTest("", 5, 5);
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
}

@Test
public void updateICMPCodeAndTypeTestIsPartialUpgradeNotICMPNullCodeAndType() {
setUpdateICMPCodeAndTypeTest("", null, null);
networkAclServiceImpl.updateIcmpCodeAndType(true, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpCode(Mockito.any());
Mockito.verify(networkAclItemVoMock, Mockito.never()).setIcmpType(Mockito.any());
}

@Test
public void updateICMPCodeAndTypeTestNotPartialUpgradeIsICMPNotNullCodeAndType() {
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, 5, 5);
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(5);
Mockito.verify(networkAclItemVoMock).setIcmpType(5);
}

@Test
public void updateICMPCodeAndTypeTestNotPartialUpgradeIsICMPNullCodeAndType() {
setUpdateICMPCodeAndTypeTest(NetUtils.ICMP_PROTO, null, null);
networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(-1);
Mockito.verify(networkAclItemVoMock).setIcmpType(-1);
}

@Test
public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNotNullCodeAndType() {
setUpdateICMPCodeAndTypeTest("", 5, 5);

networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(null);
Mockito.verify(networkAclItemVoMock).setIcmpType(null);
}

@Test
public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNullCodeAndType() {
setUpdateICMPCodeAndTypeTest("", null, null);

networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock);

Mockito.verify(networkAclItemVoMock).setIcmpCode(null);
Mockito.verify(networkAclItemVoMock).setIcmpType(null);
}

@Test
public void updateNetworkACLTestParametersNotNull() {
String name = "name";
Expand Down