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 <sadi@scclouds.com.br>
Co-authored-by: SadiJr <17a0db2854@firemailbox.club>

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

@@ -306,7 +306,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
 
     private static final ConfigKey<Boolean> SystemVmPublicIpReservationModeStrictness = new ConfigKey<Boolean>("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<Boolean> getSystemvmpublicipreservationmodestrictness() {
+        return SystemVmPublicIpReservationModeStrictness;
+    }
 }

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<IPAddressVO> sc, final ListPublicIpAddressesCmd cmd, VlanType vlanType, Boolean isAllocated) {
+    protected void setParameters(SearchCriteria<IPAddressVO> 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

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<IPAddressVO> 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);
+    }
 }