Allow VPC offering creation only with active VR service offerings (#6957)

stephankruggg · web-flow · commit 26eaae787237 · 2023-01-31T08:42:57.000+01:00
diff --git a/api/src/main/java/com/cloud/network/NetworkService.java b/api/src/main/java/com/cloud/network/NetworkService.java
@@ -41,6 +41,7 @@
 import com.cloud.exception.InsufficientCapacityException;
 import com.cloud.exception.ResourceAllocationException;
 import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.network.Network.IpAddresses;
 import com.cloud.network.Network.Service;
 import com.cloud.network.Networks.TrafficType;
@@ -241,4 +242,6 @@ Network createPrivateNetwork(String networkName, String displayText, long physic
     boolean removeNetworkPermissions(RemoveNetworkPermissionsCmd removeNetworkPermissionsCmd);
 
     boolean resetNetworkPermissions(ResetNetworkPermissionsCmd resetNetworkPermissionsCmd);
+
+    void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(final Long serviceOfferingId) throws InvalidParameterValueException;
 }
diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
@@ -5885,13 +5885,7 @@ public NetworkOffering createNetworkOffering(final CreateNetworkOfferingCmd cmd)
         final Long serviceOfferingId = cmd.getServiceOfferingId();
 
         if (serviceOfferingId != null) {
-            final ServiceOfferingVO offering = _serviceOfferingDao.findById(serviceOfferingId);
-            if (offering == null) {
-                throw new InvalidParameterValueException("Cannot find specified service offering: " + serviceOfferingId);
-            }
-            if (!VirtualMachine.Type.DomainRouter.toString().equalsIgnoreCase(offering.getSystemVmType())) {
-                throw new InvalidParameterValueException("The specified service offering " + serviceOfferingId + " cannot be used by virtual router!");
-            }
+            _networkSvc.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(serviceOfferingId);
         }
 
         // configure service provider map
diff --git a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java
@@ -41,6 +41,8 @@
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
+import com.cloud.offering.ServiceOffering;
+import com.cloud.service.dao.ServiceOfferingDao;
 import org.apache.cloudstack.acl.ControlledEntity.ACLType;
 import org.apache.cloudstack.acl.SecurityChecker.AccessType;
 import org.apache.cloudstack.alert.AlertService;
@@ -247,6 +249,7 @@
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDao;
 import com.googlecode.ipv6.IPv6Address;
+import com.cloud.service.ServiceOfferingVO;
 
 /**
  * NetworkServiceImpl implements NetworkService.
@@ -395,6 +398,8 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C
     CommandSetupHelper commandSetupHelper;
     @Inject
     AgentManager agentManager;
+    @Inject
+    ServiceOfferingDao serviceOfferingDao;
 
     @Autowired
     @Qualifier("networkHelper")
@@ -4136,6 +4141,26 @@ private List<Pair<Integer, Integer>> validateVlanRange(PhysicalNetworkVO network
 
     }
 
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(final Long serviceOfferingId) {
+        s_logger.debug(String.format("Validating if service offering [%s] is active, and if system VM is of Domain Router type.", serviceOfferingId));
+        final ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(serviceOfferingId);
+
+        if (serviceOffering == null) {
+            throw new InvalidParameterValueException(String.format("Could not find specified service offering [%s].", serviceOfferingId));
+        }
+
+        if (serviceOffering.getState() == ServiceOffering.State.Inactive) {
+            throw new InvalidParameterValueException(String.format("The specified service offering [%s] is inactive.", serviceOffering));
+        }
+
+        final String virtualMachineDomainRouterType = VirtualMachine.Type.DomainRouter.toString();
+        if (!virtualMachineDomainRouterType.equalsIgnoreCase(serviceOffering.getSystemVmType())) {
+            throw new InvalidParameterValueException(String.format("The specified service offering [%s] is of type [%s]. Virtual routers can only be created with service offering "
+                    + "of type [%s].", serviceOffering, serviceOffering.getSystemVmType(), virtualMachineDomainRouterType.toLowerCase()));
+        }
+    }
+
+
     public String generateVnetString(List<String> vnetList) {
         Collections.sort(vnetList, new Comparator<String>() {
             @Override
diff --git a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java
@@ -438,6 +438,10 @@ public VpcOffering createVpcOffering(CreateVPCOfferingCmd cmd) {
             }
         }
 
+        if (serviceOfferingId != null) {
+            _ntwkSvc.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(serviceOfferingId);
+        }
+
         return createVpcOffering(vpcOfferingName, displayText, supportedServices,
                 serviceProviderList, serviceCapabilityList, internetProtocol, serviceOfferingId,
                 domainIds, zoneIds, (enable ? State.Enabled : State.Disabled));
diff --git a/server/src/test/java/com/cloud/network/NetworkServiceImplTest.java b/server/src/test/java/com/cloud/network/NetworkServiceImplTest.java
@@ -24,6 +24,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.doReturn;
 
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
@@ -61,7 +62,6 @@
 import com.cloud.dc.DataCenterVO;
 import com.cloud.dc.dao.DataCenterDao;
 import com.cloud.exception.InsufficientCapacityException;
-import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.ResourceAllocationException;
 import com.cloud.network.dao.IPAddressDao;
 import com.cloud.network.dao.IPAddressVO;
@@ -96,7 +96,10 @@
 import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.dao.DomainRouterDao;
 import com.cloud.vm.dao.NicDao;
-
+import com.cloud.offering.ServiceOffering;
+import com.cloud.service.ServiceOfferingVO;
+import com.cloud.service.dao.ServiceOfferingDao;
+import com.cloud.exception.InvalidParameterValueException;
 
 @PowerMockIgnore("javax.management.*")
 @RunWith(PowerMockRunner.class)
@@ -154,6 +157,10 @@ public class NetworkServiceImplTest {
     AccountService accountService;
     @Mock
     NetworkHelper networkHelper;
+    @Mock
+    ServiceOfferingDao serviceOfferingDaoMock;
+    @Mock
+    ServiceOfferingVO serviceOfferingVoMock;
 
     @InjectMocks
     AccountManagerImpl accountManagerImpl;
@@ -171,7 +178,8 @@ public class NetworkServiceImplTest {
     @Mock
     private Account accountMock;
     @InjectMocks
-    private NetworkServiceImpl service = new NetworkServiceImpl();
+    NetworkServiceImpl service = new NetworkServiceImpl();
+
     private static final String VLAN_ID_900 = "900";
     private static final String VLAN_ID_901 = "901";
     private static final String VLAN_ID_902 = "902";
@@ -200,6 +208,8 @@ private void registerCallContext() {
         CallContext.register(user, account);
     }
 
+    Class<InvalidParameterValueException> expectedException = InvalidParameterValueException.class;
+
     @Before
     public void setup() throws Exception {
         MockitoAnnotations.initMocks(this);
@@ -241,6 +251,7 @@ public void setup() throws Exception {
         Mockito.lenient().doNothing().when(accountMgr).checkAccess(accountMock, networkOffering, dc);
         Mockito.when(accountMgr.isRootAdmin(accountMock.getId())).thenReturn(true);
     }
+
     @Test
     public void testGetPrivateVlanPairNoVlans() {
         Pair<String, Network.PVlanType> pair = service.getPrivateVlanPair(null, null, null);
@@ -713,4 +724,50 @@ public void testCheckAndUpdateNetworkResetSuccess() {
         Assert.assertNull(networkVO.getIp6Dns1());
         Assert.assertNull(networkVO.getIp6Dns2());
     }
+    @Test
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouterTestMustThrowInvalidParameterValueExceptionWhenServiceOfferingIsNull() {
+        doReturn(null).when(serviceOfferingDaoMock).findById(anyLong());
+
+        String expectedMessage = String.format("Could not find specified service offering [%s].", 1l);
+        InvalidParameterValueException assertThrows = Assert.assertThrows(expectedException, () -> {
+            service.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(1l);
+        });
+
+        Assert.assertEquals(expectedMessage, assertThrows.getMessage());
+    }
+
+    @Test
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouterTestMustThrowInvalidParameterValueExceptionWhenServiceOfferingStateIsInactive() {
+        doReturn(serviceOfferingVoMock).when(serviceOfferingDaoMock).findById(anyLong());
+        doReturn(ServiceOffering.State.Inactive).when(serviceOfferingVoMock).getState();
+
+        String expectedMessage = String.format("The specified service offering [%s] is inactive.", serviceOfferingVoMock);
+        InvalidParameterValueException assertThrows = Assert.assertThrows(expectedException, () -> {
+            service.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(1l);
+        });
+
+        Assert.assertEquals(expectedMessage, assertThrows.getMessage());
+    }
+
+    @Test
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouterTestMustThrowInvalidParameterValueExceptionWhenSystemVmTypeIsNotDomainRouter() {
+        doReturn(serviceOfferingVoMock).when(serviceOfferingDaoMock).findById(anyLong());
+        doReturn(ServiceOffering.State.Active).when(serviceOfferingVoMock).getState();
+        doReturn(VirtualMachine.Type.ElasticLoadBalancerVm.toString()).when(serviceOfferingVoMock).getSystemVmType();
+
+        String expectedMessage = String.format("The specified service offering [%s] is of type [%s]. Virtual routers can only be created with service offering of type [%s].",
+                serviceOfferingVoMock, serviceOfferingVoMock.getSystemVmType(), VirtualMachine.Type.DomainRouter.toString().toLowerCase());
+        InvalidParameterValueException assertThrows = Assert.assertThrows(expectedException, () -> {
+            service.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(1l);
+        });
+
+        Assert.assertEquals(expectedMessage, assertThrows.getMessage());
+    }
+
+    @Test
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouterTestMustNotThrowInvalidParameterValueExceptionWhenSystemVmTypeIsDomainRouter() {
+        NetworkServiceImpl networkServiceImplMock = mock(NetworkServiceImpl.class);
+
+        networkServiceImplMock.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(1l);
+    }
 }
diff --git a/server/src/test/java/com/cloud/network/vpc/VpcManagerImplTest.java b/server/src/test/java/com/cloud/network/vpc/VpcManagerImplTest.java
@@ -41,6 +41,7 @@
 
 
 import com.cloud.alert.AlertManager;
+import com.cloud.network.NetworkService;
 import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.alert.AlertService;
 import org.apache.cloudstack.context.CallContext;
@@ -146,6 +147,8 @@ public class VpcManagerImplTest {
     NicDao nicDao;
     @Mock
     AlertManager alertManager;
+    @Mock
+    NetworkService networkServiceMock;
 
     public static final long ACCOUNT_ID = 1;
     private AccountVO account;
@@ -196,6 +199,7 @@ public void setup()
         manager._resourceLimitMgr = resourceLimitService;
         manager._vpcOffDao = vpcOfferingDao;
         manager._dcDao = dataCenterDao;
+        manager._ntwkSvc = networkServiceMock;
         CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
         registerCallContext();
     }
@@ -418,6 +422,7 @@ public void testUpdatePublicMtuToGreaterThanThreshold() {
     public void testDisabledConfigCreateIpv6VpcOffering() {
         CreateVPCOfferingCmd cmd = Mockito.mock(CreateVPCOfferingCmd.class);
         Mockito.when(cmd.getInternetProtocol()).thenReturn(NetUtils.InternetProtocol.DualStack.toString());
+        Mockito.doNothing().when(networkServiceMock).validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(Mockito.any());
         manager.createVpcOffering(cmd);
     }
 
diff --git a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java
@@ -1050,4 +1050,8 @@ public boolean removeNetworkPermissions(RemoveNetworkPermissionsCmd removeNetwor
     public boolean resetNetworkPermissions(ResetNetworkPermissionsCmd resetNetworkPermissionsCmd) {
         return false;
     }
+
+    @Override
+    public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(final Long serviceOfferingId) {
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -438,6 +438,10 @@ public VpcOffering createVpcOffering(CreateVPCOfferingCmd cmd) {`
`438`	`438`	`}`
`439`	`439`	`}`
`440`	`440`
	`441`	`+ if (serviceOfferingId != null) {`
	`442`	`+ _ntwkSvc.validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(serviceOfferingId);`
	`443`	`+ }`
	`444`	`+`
`441`	`445`	`return createVpcOffering(vpcOfferingName, displayText, supportedServices,`
`442`	`446`	`serviceProviderList, serviceCapabilityList, internetProtocol, serviceOfferingId,`
`443`	`447`	`domainIds, zoneIds, (enable ? State.Enabled : State.Disabled));`
Original file line number	Diff line number	Diff line change
`@@ -1050,4 +1050,8 @@ public boolean removeNetworkPermissions(RemoveNetworkPermissionsCmd removeNetwor`
`1050`	`1050`	`public boolean resetNetworkPermissions(ResetNetworkPermissionsCmd resetNetworkPermissionsCmd) {`
`1051`	`1051`	`return false;`
`1052`	`1052`	`}`
	`1053`	`+`
	`1054`	`+ @Override`
	`1055`	`+ public void validateIfServiceOfferingIsActiveAndSystemVmTypeIsDomainRouter(final Long serviceOfferingId) {`
	`1056`	`+ }`
`1053`	`1057`	`}`