From 8335ef1cbbbad9cc56bc7249c93b50746786daf2 Mon Sep 17 00:00:00 2001 From: Shahzeb Patel Date: Fri, 10 Mar 2017 16:05:12 -0800 Subject: [PATCH] Handling tenant vm add, delete, replace corner cases (#992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handling corner cases to add,remove, replace vms in a tenant 1. Ensuring VM already in a tenant cannot be added to another tenant through tenant_create, tenant_vm_add and tenant_vm_replace methods 2. Ensuring VM list in tenant_vm_add, tenant_vm_replace, tenant_vm_rm cannot be empty and doesn’t contain duplicates 3. Ensuring existing VM in a tenant cannot be replaced back into same tenant 4. Sequenced parameter processing and error message simplification Resolves: #930, #994 --- esx_service/cli/vmdkops_admin_test.py | 43 ++++++++ esx_service/utils/auth_api.py | 143 ++++++++++++++++++++------ esx_service/utils/error_code.py | 10 +- 3 files changed, 162 insertions(+), 34 deletions(-) diff --git a/esx_service/cli/vmdkops_admin_test.py b/esx_service/cli/vmdkops_admin_test.py index 80dececeb..b09f61dd5 100644 --- a/esx_service/cli/vmdkops_admin_test.py +++ b/esx_service/cli/vmdkops_admin_test.py @@ -28,6 +28,7 @@ import log_config import logging import convert +import error_code # Number of expected columns in ADMIN_CLI ls EXPECTED_COLUMN_COUNT = 12 @@ -639,6 +640,15 @@ def test_tenant_vm(self): actual_output = rows self.assertEqual(expected_output, actual_output) + # Trying to create tenant with duplicate vm names + error_info, tenant_dup = auth_api._tenant_create( + name="tenant_add_dup_vms", + description="Tenant with duplicate VMs", + vm_list=[self.vm1_name, self.vm1_name], + privileges=[]) + + self.assertEqual(error_code.ErrorCode.VM_DUPLICATE, error_info.code) + # tenant vm add to add two VMs to the tenant error_info = auth_api._tenant_vm_add( name=self.tenant1_name, @@ -648,6 +658,39 @@ def test_tenant_vm(self): error_info, vms = auth_api._tenant_vm_ls(self.tenant1_name) self.assertEqual(None, error_info) + # create tenant2 with vm1 a part of it. Should fail as VM can be a part + # of just one tenant + error_info, tenant2 = auth_api._tenant_create( + name="Test_tenant2", + description="Test_tenant2", + vm_list=[self.vm1_name], + privileges=[]) + self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code) + + # create tenant3 and then try to add vm1 to it which is a part of + # another tenant. Should fail as VM can be a part of just one tenant + error_info, tenant3 = auth_api._tenant_create( + name="Test_tenant3", + description="Test_tenant3", + vm_list=[], + privileges=[]) + self.assertEqual(None, error_info) + + error_info = auth_api._tenant_vm_add( + name=tenant3.name, + vm_list=[self.vm1_name]) + self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code) + + # Replace should fail since vm1 is already a part of tenant1 + error_info = auth_api._tenant_vm_replace( + name=tenant3.name, + vm_list=[self.vm1_name]) + self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code) + + # remove the tenant3 + error_info = auth_api._tenant_rm(name=tenant3.name) + self.assertEqual(None, error_info) + # There are 2 columns for each row, the name of the columns are # "Uuid", "Name" # Sample output of a row: diff --git a/esx_service/utils/auth_api.py b/esx_service/utils/auth_api.py index ffec641f1..a9ac67097 100644 --- a/esx_service/utils/auth_api.py +++ b/esx_service/utils/auth_api.py @@ -276,6 +276,18 @@ def is_tenant_name_valid(name): else: return False +def is_vm_duplicate(vm_list): + """ + Check if vm names in vm_list contain duplicates + """ + + if len(vm_list) != len(set(vm_list)): + error_info = error_code.generate_error_info(ErrorCode.VM_DUPLICATE, vm_list) + logging.error(error_info.msg) + return error_info + + return None + def _tenant_create(name, description="", vm_list=None, privileges=None): """ API to create a tenant """ logging.debug("_tenant_create: name=%s description=%s vm_list=%s privileges=%s", name, description, vm_list, privileges) @@ -283,18 +295,30 @@ def _tenant_create(name, description="", vm_list=None, privileges=None): error_info = error_code.generate_error_info(ErrorCode.TENANT_NAME_INVALID, name, VALID_TENANT_NAME_REGEXP) return error_info, None - error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list) - if error_msg: - not_found_vm_list = ",".join(not_found_vms) - error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) - return error_info, None - # if param "description" is not set by caller, the default value is empty string if not description: description = "" - logging.debug("_tenant_create: vms=%s", vms) - vms_uuid_list = [(vm_id) for (vm_id, vm_name) in vms] + # VM list can be empty during tenant create. Validate only if it exists + vms_uuid_list = [] + if vm_list: + error_info = is_vm_duplicate(vm_list) + if error_info: + return error_info, None + + error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list) + if error_msg: + not_found_vm_list = ",".join(not_found_vms) + error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) + return error_info, None + + error_info = vm_in_any_tenant(vms) + if error_info: + return error_info, None + + logging.debug("_tenant_create: vms=%s", vms) + vms_uuid_list = [(vm_id) for (vm_id, vm_name) in vms] + error_info, tenant = create_tenant_in_db( name=name, description=description, @@ -384,26 +408,57 @@ def _tenant_ls(name=None): error_info, tenant_list = get_tenant_list_from_db(name) return error_info, tenant_list -def any_vm_already_exist(existing_vms, vms): +def vm_already_in_tenant(name, vms): """ - Check whehter any vm in @param "vms" already exist in @param "existing_vms" + Check whether any vm in @param "vms" already exists in tenant @param "name" """ - for vm in vms: - if vm[0] in existing_vms: - return True + error_info, existing_vms = _tenant_vm_ls(name) + if error_info: + return error_info - return False + for vm_id, vm_name in vms: + if vm_id in existing_vms: + error_info = error_code.generate_error_info(ErrorCode.VM_ALREADY_IN_TENANT, + vm_name, name) + logging.error(error_info.msg) + return error_info + + return None -def any_vm_not_exist(existing_vms, vms): +def vm_not_exist(name, vms): """ - Check whehter any vm in @param "vms" does not exist in @param "existing_vms" + Check whether any vm in @param "vms" does not exist in tenant @param "name" """ - for vm in vms: - if not vm[0] in existing_vms: - return True + error_info, existing_vms = _tenant_vm_ls(name) + if error_info: + return error_info + + for vm_id, vm_name in vms: + if not vm_id in existing_vms: + error_info = error_code.generate_error_info(ErrorCode.VM_NOT_IN_TENANT, vm_name, name) + logging.error(error_info.msg) + return error_info + + return None - return False +def vm_in_any_tenant(vms): + """ + Check if any vm in @param "vms" is a part of another tenant + """ + error_info, tenant_list = get_tenant_list_from_db() + if error_info: + return error_info + + for tenant in tenant_list: + for vm_id, vm_name in vms: + if vm_id in tenant.vms: + error_info = error_code.generate_error_info(ErrorCode.VM_IN_ANOTHER_TENANT, + vm_name, tenant.name) + logging.error(error_info.msg) + return error_info + + return None def _tenant_vm_add(name, vm_list): """ API to add vms for a tenant """ @@ -416,18 +471,26 @@ def _tenant_vm_add(name, vm_list): error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) return error_info + if not vm_list: + error_info = error_code.generate_error_info(ErrorCode.VM_LIST_EMPTY) + return error_info + + error_info = is_vm_duplicate(vm_list) + if error_info: + return error_info + error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list) if error_msg: not_found_vm_list = ",".join(not_found_vms) error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) return error_info - error_info, existing_vms = _tenant_vm_ls(name) + error_info = vm_already_in_tenant(name, vms) if error_info: return error_info - if any_vm_already_exist(existing_vms, vms): - error_info = error_code.generate_error_info(ErrorCode.VM_ALREADY_IN_TENANT, vm_list, name) + error_info = vm_in_any_tenant(vms) + if error_info: return error_info error_info, auth_mgr = get_auth_mgr_object() @@ -453,6 +516,14 @@ def _tenant_vm_rm(name, vm_list): error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) return error_info + if not vm_list: + error_info = error_code.generate_error_info(ErrorCode.VM_LIST_EMPTY) + return error_info + + error_info = is_vm_duplicate(vm_list) + if error_info: + return error_info + error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list) if error_msg: not_found_vm_list = ",".join(not_found_vms) @@ -461,14 +532,10 @@ def _tenant_vm_rm(name, vm_list): logging.debug("_tenant_vm_rm: vms=%s", vms) - error_info, existing_vms = _tenant_vm_ls(name) + error_info = vm_not_exist(name, vms) if error_info: return error_info - if any_vm_not_exist(existing_vms, vms): - error_info = error_code.generate_error_info(ErrorCode.VM_NOT_IN_TENANT, vm_list, name) - return error_info - error_info, auth_mgr = get_auth_mgr_object() if error_info: return error_info @@ -496,10 +563,6 @@ def _tenant_vm_ls(name): def _tenant_vm_replace(name, vm_list): """ API to replace vms for a tenant """ logging.debug("_tenant_vm_replace: name=%s vm_list=%s", name, vm_list) - if not vm_list: - error_info = error_code.generate_error_info(ErrorCode.REPLACE_VM_EMPTY) - return error_info - error_info, tenant = get_tenant_from_db(name) if error_info: return error_info @@ -508,6 +571,14 @@ def _tenant_vm_replace(name, vm_list): error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) return error_info + if not vm_list: + error_info = error_code.generate_error_info(ErrorCode.REPLACE_VM_EMPTY) + return error_info + + error_info = is_vm_duplicate(vm_list) + if error_info: + return error_info + error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list) if error_msg: @@ -515,6 +586,14 @@ def _tenant_vm_replace(name, vm_list): error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) return error_info + error_info = vm_already_in_tenant(name, vms) + if error_info: + return error_info + + error_info = vm_in_any_tenant(vms) + if error_info: + return error_info + logging.debug("_tenant_vm_replace: vms=%s", vms) error_info, auth_mgr = get_auth_mgr_object() if error_info: diff --git a/esx_service/utils/error_code.py b/esx_service/utils/error_code.py index a55a6cfe1..0159b1e0d 100644 --- a/esx_service/utils/error_code.py +++ b/esx_service/utils/error_code.py @@ -30,6 +30,9 @@ class ErrorCode: REPLACE_VM_EMPTY = 102 VM_ALREADY_IN_TENANT = 103 VM_NOT_IN_TENANT = 104 + VM_IN_ANOTHER_TENANT = 105 + VM_LIST_EMPTY = 106 + VM_DUPLICATE = 107 # VM related error code end # Privilege related error code start @@ -67,8 +70,11 @@ class ErrorCode: ErrorCode.VM_NOT_FOUND : "Cannot find vm {0}", ErrorCode.REPLACE_VM_EMPTY : "Replace VM cannot be empty", - ErrorCode.VM_ALREADY_IN_TENANT : "Some VMs in {0} have already been associated with tenant '{1}', cannot add them again", - ErrorCode.VM_NOT_IN_TENANT : "Some VMs in {0} have not been associated with tenant '{1}', cannot remove them", + ErrorCode.VM_ALREADY_IN_TENANT : "VM '{0}' has already been associated with tenant '{1}', cannot add it again", + ErrorCode.VM_NOT_IN_TENANT : "VM '{0}' has not been associated with tenant '{1}', cannot remove it", + ErrorCode.VM_IN_ANOTHER_TENANT : "VM '{0}' has already been associated with tenant '{1}', can't add it", + ErrorCode.VM_LIST_EMPTY : "VM list cannot be empty", + ErrorCode.VM_DUPLICATE : "VMs {0} contain duplicates, they should be unique", ErrorCode.PRIVILEGE_NOT_FOUND : "No privilege exists for ({0}, {1})", ErrorCode.PRIVILEGE_ALREADY_EXIST : "Privilege for ({0}, {1}) already exists",