Merge branch 'develop' into feature/apiddubny_bug_155_public_ip_attri…

…bute_is_not_effecting_the_vm

drivers/azure_shell/driver.py

@@ -38,7 +38,7 @@ def PrepareConnectivity(self, context, request):
         return self.azure_shell.prepare_connectivity(context, request)
 
     def CleanupConnectivity(self, context, request):
-        self.azure_shell.cleanup_connectivity(command_context=context)
+        return self.azure_shell.cleanup_connectivity(command_context=context)
 
     def GetApplicationPorts(self, context, ports):
         pass

package/cloudshell/cp/azure/azure_shell.py

@@ -35,7 +35,7 @@ def __init__(self):
         self.tags_service = TagService()
         self.vm_credentials_service = VMCredentialsService()
         self.key_pair_service = KeyPairService(storage_service=self.storage_service)
-        self.security_group_service = SecurityGroupService()
+        self.security_group_service = SecurityGroupService(self.network_service)
         self.access_key_operation = AccessKeyOperation(self.key_pair_service)
 
         self.prepare_connectivity_operation = PrepareConnectivityOperation(
@@ -62,7 +62,8 @@ def __init__(self):
         self.delete_azure_vm_operation = DeleteAzureVMOperation(
             vm_service=self.vm_service,
             network_service=self.network_service,
-            tags_service=self.tags_service)
+            tags_service=self.tags_service,
+            security_group_service=self.security_group_service)
 
     def deploy_azure_vm(self, command_context, deployment_request):
         """Will deploy Azure Image on the cloud provider
@@ -141,18 +142,14 @@ def cleanup_connectivity(self, command_context):
                 azure_clients = AzureClientsManager(cloud_provider_model)
                 resource_group_name = command_context.reservation.reservation_id
 
-                self.delete_azure_vm_operation.remove_nsg_from_subnet(network_client=azure_clients.network_client,
-                                                                      resource_group_name=resource_group_name,
-                                                                      cloud_provider_model=cloud_provider_model)
-
-                self.delete_azure_vm_operation.delete_sandbox_subnet(
+                result = self.delete_azure_vm_operation.cleanup_connectivity(
                     network_client=azure_clients.network_client,
+                    resource_client=azure_clients.resource_client,
                     cloud_provider_model=cloud_provider_model,
-                    resource_group_name=resource_group_name)
+                    resource_group_name=resource_group_name,
+                    logger=logger)
 
-                self.delete_azure_vm_operation.delete_resource_group(
-                    resource_client=azure_clients.resource_client,
-                    group_name=resource_group_name)
+                return self.command_result_parser.set_command_result({'driverResponse': {'actionResults': result}})
 
     def delete_azure_vm(self, command_context):
         with LoggingSessionContext(command_context) as logger:

package/cloudshell/cp/azure/domain/services/network_service.py

@@ -285,6 +285,16 @@ def get_public_ip(self, network_client, group_name, ip_name):
 
         return network_client.public_ip_addresses.get(group_name, ip_name)
 
+    def get_private_ip(self, network_client, group_name, vm_name):
+        """
+
+        :param network_client:
+        :param group_name:
+        :param vm_name:
+        """
+        nic = network_client.network_interfaces.get(group_name, vm_name)
+        return nic.ip_configurations[0].private_ip_address
+
     def delete_nic(self, network_client, group_name, interface_name):
         """
 

package/cloudshell/cp/azure/domain/services/security_group.py

@@ -8,8 +8,9 @@ class SecurityGroupService(object):
     RULE_DEFAULT_PRIORITY = 1000
     RULE_PRIORITY_INCREASE_STEP = 5
 
-    def __init__(self):
+    def __init__(self, network_service):
         self._lock = Lock()
+        self.network_service = network_service
 
     def _rule_priority_generator(self, existing_rules, start_from=None):
         """Endless priority generator for NSG rules
@@ -137,6 +138,20 @@ def create_network_security_group_custom_rule(self, network_client, group_name,
 
             return operation_poller.result()
 
+    def get_network_security_group(self, network_client, group_name):
+        network_security_groups = self.list_network_security_group(
+            network_client=network_client,
+            group_name=group_name)
+        self._validate_network_security_group_is_single_per_group(network_security_groups, group_name)
+        return network_security_groups[0]
+
+    @staticmethod
+    def _validate_network_security_group_is_single_per_group(resources_list, group_name):
+        if len(resources_list) > 1:
+            raise Exception("The resource group {} contains more than one network security group.".format(group_name))
+        if len(resources_list) == 0:
+            raise Exception("The resource group {} does not contain a network security group.".format(group_name))
+
     def create_network_security_group_rules(self, network_client, group_name, security_group_name,
                                             inbound_rules, destination_addr, start_from=None):
         """Create NSG inbound rules on the Azure
@@ -163,3 +178,37 @@ def create_network_security_group_rules(self, network_client, group_name, securi
                     rule_data=rule_data,
                     destination_addr=destination_addr,
                     priority=next(priority_generator))
+
+    def delete_security_rules(self, network_client, resource_group_name, vm_name):
+        """
+        removes NSG inbound rules for virtual machine (based on private ip address)
+
+        :param vm_name:
+        :param network_client: azure.mgmt.network.NetworkManagementClient instance
+        :param resource_group_name: resource group name (reservation id)
+
+        :return: None
+        """
+
+        private_ip_address = self.network_service.get_private_ip(network_client=network_client,
+                                                                 group_name=resource_group_name,
+                                                                 vm_name=vm_name)
+        # NetworkSecurityGroup
+        security_group = self.get_network_security_group(network_client=network_client, group_name=resource_group_name)
+
+        if security_group is None:
+            raise Exception("Could not find NetworkSecurityGroup in '{}'".format(resource_group_name))
+
+        rules = security_group.security_rules
+
+        vm_rules = [rule for rule in rules if rule.destination_address_prefix == private_ip_address]
+
+        if vm_rules is None or len(vm_rules) == 0:
+            return
+
+        for vm_rule in vm_rules:
+            network_client.security_rules.delete(
+                resource_group_name=resource_group_name,
+                network_security_group_name=security_group.name,
+                security_rule_name=vm_rule.name
+            )

package/cloudshell/cp/azure/domain/vm_management/operations/delete_operation.py

@@ -6,18 +6,46 @@ class DeleteAzureVMOperation(object):
     def __init__(self,
                  vm_service,
                  network_service,
-                 tags_service):
+                 tags_service,
+                 security_group_service):
         """
-
         :param cloudshell.cp.azure.domain.services.virtual_machine_service.VirtualMachineService vm_service:
         :param cloudshell.cp.azure.domain.services.network_service.NetworkService network_service:
         :param cloudshell.cp.azure.domain.services.tags.TagService tags_service:
+        :param cloudshell.cp.azure.domain.services.security_group.SecurityGroupService security_group_service:
         :return:
         """
 
         self.vm_service = vm_service
         self.network_service = network_service
         self.tags_service = tags_service
+        self.security_group_service = security_group_service
+
+    def cleanup_connectivity(self, network_client, resource_client, cloud_provider_model, resource_group_name, logger):
+        """
+        :param logger:
+        :param network_client:
+        :param resource_client:
+        :param cloud_provider_model:
+        :param resource_group_name:
+        """
+        result = {'success': True}
+
+        try:
+            self.remove_nsg_from_subnet(network_client=network_client, cloud_provider_model=cloud_provider_model,
+                                        resource_group_name=resource_group_name)
+
+            self.delete_sandbox_subnet(network_client=network_client, cloud_provider_model=cloud_provider_model,
+                                       resource_group_name=resource_group_name)
+
+            self.delete_resource_group(resource_client=resource_client, group_name=resource_group_name)
+
+        except Exception as ex:
+            logger.error("Error in cleanup connectivity. Error: {0}".format(ex.message))
+            result['success'] = False
+            result['errorMessage'] = 'CleanupConnectivity ended with the error: {0}'.format(ex.message)
+
+        return result
 
     def remove_nsg_from_subnet(self, network_client, resource_group_name, cloud_provider_model):
         management_group_name = cloud_provider_model.management_group_name
@@ -67,6 +95,10 @@ def delete(self, compute_client, network_client, group_name, vm_name, logger):
         """
         try:
 
+            self.security_group_service.delete_security_rules(network_client=network_client,
+                                                              resource_group_name=group_name,
+                                                              vm_name=vm_name)
+
             self.vm_service.delete_vm(compute_management_client=compute_client,
                                       group_name=group_name,
                                       vm_name=vm_name)

package/cloudshell/cp/azure/domain/vm_management/operations/deploy_operation.py

@@ -50,14 +50,10 @@ def _process_nsg_rules(self, network_client, group_name, azure_vm_deployment_mod
             inbound_rules = RulesAttributeParser.parse_port_group_attribute(
                 ports_attribute=azure_vm_deployment_model.inbound_ports)
 
-            network_security_groups = self.security_group_service.list_network_security_group(
+            network_security_group = self.security_group_service.get_network_security_group(
                 network_client=network_client,
                 group_name=group_name)
 
-            self._validate_resource_is_single_per_group(network_security_groups, group_name, "network security group")
-
-            network_security_group = network_security_groups[0]
-
             self.security_group_service.create_network_security_group_rules(
                 network_client=network_client,
                 group_name=group_name,
@@ -209,6 +205,7 @@ def deploy(self, azure_vm_deployment_model,
                             public_ip=public_ip_address,
                             resource_group=reservation_id)
 
+
     def _validate_resource_is_single_per_group(self, resources_list, group_name, resource_name):
         if len(resources_list) > 1:
             raise Exception("The resource group {} contains more than one {}.".format(group_name, resource_name))

package/tests/test_connectivity/test_cleanup_connectivity.py

@@ -1,8 +1,9 @@
 from unittest import TestCase
 
-from mock import Mock
+from mock import Mock, MagicMock
 
 from cloudshell.cp.azure.domain.services.network_service import NetworkService
+from cloudshell.cp.azure.domain.services.security_group import SecurityGroupService
 from cloudshell.cp.azure.domain.services.tags import TagService
 from cloudshell.cp.azure.domain.services.virtual_machine_service import VirtualMachineService
 from cloudshell.cp.azure.domain.vm_management.operations.delete_operation import DeleteAzureVMOperation
@@ -14,18 +15,41 @@ def setUp(self):
         self.vm_service = VirtualMachineService()
         self.network_service = NetworkService()
         self.tags_service = TagService()
+        self.security_group_service = SecurityGroupService(self.network_service)
         self.delete_operation = DeleteAzureVMOperation(vm_service=self.vm_service,
                                                        network_service=self.network_service,
-                                                       tags_service=self.tags_service)
+                                                       tags_service=self.tags_service,
+                                                       security_group_service=self.security_group_service)
+        self.logger = MagicMock()
+
+    def test_cleanup_on_error(self):
+        # Arrange
+        test_exception_message = "lalala"
+        self.delete_operation.remove_nsg_from_subnet = Mock(side_effect=Exception(test_exception_message))
+        self.delete_operation.delete_sandbox_subnet = Mock()
+
+        # Act
+        result = self.delete_operation.cleanup_connectivity(network_client=Mock(),
+                                                            resource_client=Mock(),
+                                                            cloud_provider_model=Mock(),
+                                                            resource_group_name=Mock(),
+                                                            logger=self.logger)
+
+        # Verify
+        self.assertTrue(result['success'] == False)
+        self.assertTrue(
+            result['errorMessage'] == 'CleanupConnectivity ended with the error: {0}'.format(test_exception_message))
+        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.logger.error))
 
     def test_cleanup(self):
         """
         :return:
         """
 
         # Arrange
-        self.vm_service.delete_resource_group = Mock()
-        self.vm_service.delete_sandbox_subnet = Mock()
+        self.delete_operation.remove_nsg_from_subnet = Mock()
+        self.delete_operation.delete_resource_group = Mock()
+        self.delete_operation.delete_sandbox_subnet = Mock()
         tested_group_name = "test_group"
         resource_client = Mock()
         network_client = Mock()
@@ -40,16 +64,18 @@ def test_cleanup(self):
         self.network_service.get_sandbox_virtual_network = Mock(return_value=vnet)
 
         # Act
-        self.delete_operation.delete_resource_group(resource_client=resource_client, group_name=tested_group_name)
-        self.delete_operation.delete_sandbox_subnet(network_client=network_client,
-                                                    cloud_provider_model=cloud_provider_model,
-                                                    resource_group_name=tested_group_name)
+        self.delete_operation.cleanup_connectivity(network_client=network_client,
+                                                   resource_client=resource_client,
+                                                   cloud_provider_model=cloud_provider_model,
+                                                   resource_group_name=tested_group_name,
+                                                   logger=self.logger)
 
         # Verify
-        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.vm_service.delete_resource_group))
-        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.network_service.get_sandbox_virtual_network))
-        self.vm_service.delete_resource_group.assert_called_with(resource_management_client=resource_client,
-                                                                 group_name=tested_group_name)
+        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.delete_operation.remove_nsg_from_subnet))
+        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.delete_operation.delete_sandbox_subnet))
+        self.assertTrue(TestHelper.CheckMethodCalledXTimes(self.delete_operation.delete_resource_group))
+        self.delete_operation.delete_resource_group.assert_called_with(resource_client=resource_client,
+                                                                       group_name=tested_group_name)
 
     def test_delete_sandbox_subnet_on_error(self):
         # Arrange
@@ -68,4 +94,3 @@ def test_delete_sandbox_subnet_on_error(self):
         self.assertRaises(Exception,
                           self.delete_operation.delete_sandbox_subnet,
                           )
-

package/tests/test_cp/test_azure/test_azure_shell.py

@@ -153,14 +153,12 @@ def test_cleanup_connectivity(self, error_handling_class, logging_context_class,
         error_handling.__enter__.assert_called_once_with()
         error_handling_class.assert_called_once_with(self.logger)
 
-        self.azure_shell.delete_azure_vm_operation.delete_resource_group.assert_called_once_with(
-            resource_client=azure_clients_manager.resource_client,
-            group_name=self.group_name)
-
-        self.azure_shell.delete_azure_vm_operation.delete_sandbox_subnet.assert_called_once_with(
+        self.azure_shell.delete_azure_vm_operation.cleanup_connectivity.assert_called_once_with(
             network_client=azure_clients_manager.network_client,
+            resource_client=azure_clients_manager.resource_client,
             cloud_provider_model=cloud_provider_model,
-            resource_group_name=self.group_name)
+            resource_group_name=self.group_name,
+            logger=self.logger)
 
     @mock.patch("cloudshell.cp.azure.azure_shell.AzureClientsManager")
     @mock.patch("cloudshell.cp.azure.azure_shell.LoggingSessionContext")

package/tests/test_operations/test_delete_operation.py

@@ -4,6 +4,7 @@
 from requests import Response
 
 from cloudshell.cp.azure.domain.services.network_service import NetworkService
+from cloudshell.cp.azure.domain.services.security_group import SecurityGroupService
 from cloudshell.cp.azure.domain.services.tags import TagService
 from cloudshell.cp.azure.domain.services.virtual_machine_service import VirtualMachineService
 from cloudshell.cp.azure.domain.vm_management.operations.delete_operation import DeleteAzureVMOperation
@@ -16,9 +17,11 @@ def setUp(self):
         self.vm_service = VirtualMachineService()
         self.network_service = NetworkService()
         self.tags_service = TagService()
+        self.security_group_service = SecurityGroupService(self.network_service)
         self.delete_operation = DeleteAzureVMOperation(vm_service=self.vm_service,
                                                        network_service=self.network_service,
-                                                       tags_service=self.tags_service)
+                                                       tags_service=self.tags_service,
+                                                       security_group_service=self.security_group_service)
 
     def test_delete_operation(self):
         """
@@ -30,6 +33,7 @@ def test_delete_operation(self):
         network_client = Mock()
         network_client.network_interfaces.delete = Mock()
         network_client.public_ip_addresses.delete = Mock()
+        self.delete_operation.security_group_service.delete_security_rules = Mock()
 
         # Act
         self.delete_operation.delete(compute_client=Mock(),
@@ -65,6 +69,7 @@ def test_delete_operation_on_cloud_error_not_found_no_exception(self):
         response.reason = "Not Found"
         error = CloudError(response)
         self.vm_service.delete_vm = Mock(side_effect=error)
+        self.delete_operation.security_group_service.delete_security_rules = Mock()
 
         # Act
         self.delete_operation.delete(

package/tests/test_operations/test_deploy_operation.py

@@ -169,6 +169,7 @@ def test_process_nsg_rules(self):
         security_groups_list = MagicMock()
         self.deploy_operation.security_group_service.list_network_security_group.return_value = security_groups_list
         self.deploy_operation._validate_resource_is_single_per_group = MagicMock()
+        self.deploy_operation.security_group_service.get_network_security_group.return_value = security_groups_list[0]
 
         # Act
         self.deploy_operation._process_nsg_rules(
@@ -178,12 +179,9 @@ def test_process_nsg_rules(self):
             nic=nic)
 
         # Verify
-        self.deploy_operation.security_group_service.list_network_security_group.assert_called_once_with(
-            group_name=group_name,
-            network_client=network_client)
-
-        self.deploy_operation._validate_resource_is_single_per_group.assert_called_once_with(
-            security_groups_list, group_name, 'network security group')
+        self.deploy_operation.security_group_service.get_network_security_group.assert_called_once_with(
+            network_client=network_client,
+            group_name=group_name)
 
         self.deploy_operation.security_group_service.create_network_security_group_rules.assert_called_once_with(
             destination_addr=nic.ip_configurations[0].private_ip_address,