From 4313c3def7edb456110254f271d9ca60c563ed7d Mon Sep 17 00:00:00 2001 From: SadiJr Date: Tue, 19 Apr 2022 18:15:15 -0300 Subject: [PATCH] Allow users to view reserved System VM IPs, if they're already allocated to user (#5902) * Allow users to view reserved system VM IPs, if this IPs are already allocated to any user VM * Fix checkstyle * Address reviews * Address reviews * Apply @weizhouapache changes Credits to @weizhouapache, and my sincere thanks for the help. Co-authored-by: SadiJr Co-authored-by: SadiJr <17a0db2854@firemailbox.club> --- .../cloud/network/IpAddressManagerImpl.java | 6 +- .../cloud/server/ManagementServerImpl.java | 7 +- .../server/ManagementServerImplTest.java | 114 ++++++++++++++++++ 3 files changed, 124 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index b1186148578..dcd89fa5698 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -306,7 +306,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage private static final ConfigKey SystemVmPublicIpReservationModeStrictness = new ConfigKey("Advanced", Boolean.class, "system.vm.public.ip.reservation.mode.strictness", "false", - "If enabled, the use of System VMs public IP reservation is strict, preferred if not.", false, ConfigKey.Scope.Global); + "If enabled, the use of System VMs public IP reservation is strict, preferred if not.", true, ConfigKey.Scope.Global); private Random rand = new Random(System.currentTimeMillis()); @@ -2306,4 +2306,8 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage NetworkDetailVO networkDetail = _networkDetailsDao.findDetail(networkId, Network.hideIpAddressUsage); return networkDetail != null && "true".equals(networkDetail.getValue()); } + + public static ConfigKey getSystemvmpublicipreservationmodestrictness() { + return SystemVmPublicIpReservationModeStrictness; + } } diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index f2e70e67b91..26d11eafb9b 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -666,6 +666,7 @@ import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; import com.cloud.info.ConsoleProxyInfo; import com.cloud.network.IpAddress; import com.cloud.network.IpAddressManager; +import com.cloud.network.IpAddressManagerImpl; import com.cloud.network.Network; import com.cloud.network.NetworkModel; import com.cloud.network.dao.IPAddressDao; @@ -2395,7 +2396,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe } } - private void setParameters(SearchCriteria sc, final ListPublicIpAddressesCmd cmd, VlanType vlanType, Boolean isAllocated) { + protected void setParameters(SearchCriteria sc, final ListPublicIpAddressesCmd cmd, VlanType vlanType, Boolean isAllocated) { final Object keyword = cmd.getKeyword(); final Long physicalNetworkId = cmd.getPhysicalNetworkId(); final Long sourceNetworkId = cmd.getNetworkId(); @@ -2467,7 +2468,9 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe sc.setParameters("state", IpAddress.State.Allocated); } - sc.setParameters( "forsystemvms", false); + if (IpAddressManagerImpl.getSystemvmpublicipreservationmodestrictness().value() && IpAddress.State.Free.name().equalsIgnoreCase(state)) { + sc.setParameters("forsystemvms", false); + } } @Override diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index 488c5f5717a..2d6969fe966 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -21,23 +21,35 @@ import static org.mockito.Mockito.any; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; +import org.apache.cloudstack.api.command.user.address.ListPublicIpAddressesCmd; import org.apache.cloudstack.api.command.user.ssh.RegisterSSHKeyPairCmd; +import org.apache.cloudstack.framework.config.ConfigKey; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; +import org.powermock.reflect.Whitebox; +import com.cloud.dc.Vlan.VlanType; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.network.IpAddress; +import com.cloud.network.IpAddressManagerImpl; +import com.cloud.network.dao.IPAddressVO; import com.cloud.user.Account; import com.cloud.user.SSHKeyPair; import com.cloud.user.SSHKeyPairVO; import com.cloud.user.dao.SSHKeyPairDao; +import com.cloud.utils.db.SearchCriteria; @RunWith(MockitoJUnitRunner.class) public class ManagementServerImplTest { + @Mock + SearchCriteria sc; + @Mock RegisterSSHKeyPairCmd regCmd; @@ -54,9 +66,20 @@ public class ManagementServerImplTest { @Mock SSHKeyPair sshKeyPair; + @Mock + IpAddressManagerImpl ipAddressManagerImpl; + @Spy ManagementServerImpl spy; + ConfigKey mockConfig; + + @Before + public void setup() { + mockConfig = Mockito.mock(ConfigKey.class); + Whitebox.setInternalState(ipAddressManagerImpl.getClass(), "SystemVmPublicIpReservationModeStrictness", mockConfig); + } + @Test(expected = InvalidParameterValueException.class) public void testDuplicateRegistraitons(){ String accountName = "account"; @@ -107,4 +130,95 @@ public class ManagementServerImplTest { spy.registerSSHKeyPair(regCmd); Mockito.verify(spy, Mockito.times(3)).getPublicKeyFromKeyKeyMaterial(anyString()); } + + @Test + public void setParametersTestWhenStateIsFreeAndSystemVmPublicIsTrue() throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException { + Mockito.when(mockConfig.value()).thenReturn(Boolean.TRUE); + + ListPublicIpAddressesCmd cmd = Mockito.mock(ListPublicIpAddressesCmd.class); + Mockito.when(cmd.getNetworkId()).thenReturn(10L); + Mockito.when(cmd.getZoneId()).thenReturn(null); + Mockito.when(cmd.getIpAddress()).thenReturn(null); + Mockito.when(cmd.getPhysicalNetworkId()).thenReturn(null); + Mockito.when(cmd.getVlanId()).thenReturn(null); + Mockito.when(cmd.getId()).thenReturn(null); + Mockito.when(cmd.isSourceNat()).thenReturn(null); + Mockito.when(cmd.isStaticNat()).thenReturn(null); + Mockito.when(cmd.getState()).thenReturn(IpAddress.State.Free.name()); + Mockito.when(cmd.getTags()).thenReturn(null); + spy.setParameters(sc, cmd, VlanType.VirtualNetwork, Boolean.FALSE); + + Mockito.verify(sc, Mockito.times(1)).setJoinParameters("vlanSearch", "vlanType", VlanType.VirtualNetwork); + Mockito.verify(sc, Mockito.times(1)).setParameters("display", false); + Mockito.verify(sc, Mockito.times(1)).setParameters("sourceNetworkId", 10L); + Mockito.verify(sc, Mockito.times(1)).setParameters("state", "Free"); + Mockito.verify(sc, Mockito.times(1)).setParameters("forsystemvms", false); + } + + @Test + public void setParametersTestWhenStateIsFreeAndSystemVmPublicIsFalse() { + Mockito.when(mockConfig.value()).thenReturn(Boolean.FALSE); + ListPublicIpAddressesCmd cmd = Mockito.mock(ListPublicIpAddressesCmd.class); + Mockito.when(cmd.getNetworkId()).thenReturn(10L); + Mockito.when(cmd.getZoneId()).thenReturn(null); + Mockito.when(cmd.getIpAddress()).thenReturn(null); + Mockito.when(cmd.getPhysicalNetworkId()).thenReturn(null); + Mockito.when(cmd.getVlanId()).thenReturn(null); + Mockito.when(cmd.getId()).thenReturn(null); + Mockito.when(cmd.isSourceNat()).thenReturn(null); + Mockito.when(cmd.isStaticNat()).thenReturn(null); + Mockito.when(cmd.getState()).thenReturn(IpAddress.State.Free.name()); + Mockito.when(cmd.getTags()).thenReturn(null); + spy.setParameters(sc, cmd, VlanType.VirtualNetwork, Boolean.FALSE); + + Mockito.verify(sc, Mockito.times(1)).setJoinParameters("vlanSearch", "vlanType", VlanType.VirtualNetwork); + Mockito.verify(sc, Mockito.times(1)).setParameters("display", false); + Mockito.verify(sc, Mockito.times(1)).setParameters("sourceNetworkId", 10L); + Mockito.verify(sc, Mockito.times(1)).setParameters("state", "Free"); + Mockito.verify(sc, Mockito.never()).setParameters("forsystemvms", false); + } + + @Test + public void setParametersTestWhenStateIsNullAndSystemVmPublicIsFalse() { + Mockito.when(mockConfig.value()).thenReturn(Boolean.FALSE); + ListPublicIpAddressesCmd cmd = Mockito.mock(ListPublicIpAddressesCmd.class); + Mockito.when(cmd.getNetworkId()).thenReturn(10L); + Mockito.when(cmd.getZoneId()).thenReturn(null); + Mockito.when(cmd.getIpAddress()).thenReturn(null); + Mockito.when(cmd.getPhysicalNetworkId()).thenReturn(null); + Mockito.when(cmd.getVlanId()).thenReturn(null); + Mockito.when(cmd.getId()).thenReturn(null); + Mockito.when(cmd.isSourceNat()).thenReturn(null); + Mockito.when(cmd.isStaticNat()).thenReturn(null); + Mockito.when(cmd.getState()).thenReturn(null); + Mockito.when(cmd.getTags()).thenReturn(null); + spy.setParameters(sc, cmd, VlanType.VirtualNetwork, Boolean.TRUE); + + Mockito.verify(sc, Mockito.times(1)).setJoinParameters("vlanSearch", "vlanType", VlanType.VirtualNetwork); + Mockito.verify(sc, Mockito.times(1)).setParameters("display", false); + Mockito.verify(sc, Mockito.times(1)).setParameters("sourceNetworkId", 10L); + Mockito.verify(sc, Mockito.never()).setParameters("forsystemvms", false); + } + + @Test + public void setParametersTestWhenStateIsNullAndSystemVmPublicIsTrue() { + Mockito.when(mockConfig.value()).thenReturn(Boolean.TRUE); + ListPublicIpAddressesCmd cmd = Mockito.mock(ListPublicIpAddressesCmd.class); + Mockito.when(cmd.getNetworkId()).thenReturn(10L); + Mockito.when(cmd.getZoneId()).thenReturn(null); + Mockito.when(cmd.getIpAddress()).thenReturn(null); + Mockito.when(cmd.getPhysicalNetworkId()).thenReturn(null); + Mockito.when(cmd.getVlanId()).thenReturn(null); + Mockito.when(cmd.getId()).thenReturn(null); + Mockito.when(cmd.isSourceNat()).thenReturn(null); + Mockito.when(cmd.isStaticNat()).thenReturn(null); + Mockito.when(cmd.getState()).thenReturn(null); + Mockito.when(cmd.getTags()).thenReturn(null); + spy.setParameters(sc, cmd, VlanType.VirtualNetwork, Boolean.TRUE); + + Mockito.verify(sc, Mockito.times(1)).setJoinParameters("vlanSearch", "vlanType", VlanType.VirtualNetwork); + Mockito.verify(sc, Mockito.times(1)).setParameters("display", false); + Mockito.verify(sc, Mockito.times(1)).setParameters("sourceNetworkId", 10L); + Mockito.verify(sc, Mockito.never()).setParameters("forsystemvms", false); + } }