From d3bdc7f96a1dbb8d315fa62747b110997f9d3cbe Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Tue, 22 Dec 2020 14:21:45 +0100 Subject: [PATCH] Fix by client side filtering protected/protectable shares --- .../backup_protected_file_share_resource.go | 56 +++-- ...ckup_protected_file_share_resource_test.go | 198 ++++++++++-------- 2 files changed, 153 insertions(+), 101 deletions(-) diff --git a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go index 16250455cda14..54327caa0e184 100644 --- a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go +++ b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go @@ -95,37 +95,55 @@ func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta i return fmt.Errorf("[ERROR] parsed source_storage_account_id '%s' doesn't contain 'storageAccounts'", storageAccountID) } - // The fileshare has a user defined name, but its system name (fileShareSystemName) is only known to Azure Backup - filter := fmt.Sprintf("backupManagementType eq 'AzureStorage' and friendlyName eq '%s'", fileShareName) + // the fileshare has a user defined name, but its system name (fileShareSystemName) is only known to Azure Backup + fileShareSystemName := "" + // @aristosvo: preferred filter would be like below but the 'and' expression seems to fail + // filter := fmt.Sprintf("backupManagementType eq 'AzureStorage' and friendlyName eq '%s'", fileShareName) + // this means which means we have to do it client side and loop over backupProtectedItems en backupProtectableItems until share is found + filter := fmt.Sprint("backupManagementType eq 'AzureStorage'") backupProtectableItemsResponse, err := protectableClient.List(ctx, vaultName, resourceGroup, filter, "") if err != nil { return fmt.Errorf("Error checking for protectable fileshares in Recovery Service Vault %q (Resource Group %q): %+v", vaultName, resourceGroup, err) } - backupProtectableItems := backupProtectableItemsResponse.Values() - backupProtectedItemsResponse, err := protectedClient.List(ctx, vaultName, resourceGroup, filter, "") - if err != nil { - return fmt.Errorf("Error checking for protected fileshares in Recovery Service Vault %q (Resource Group %q): %+v", vaultName, resourceGroup, err) - } - backupProtectedItems := backupProtectedItemsResponse.Values() + for _, protectableItem := range backupProtectableItemsResponse.Values() { + if *protectableItem.Name == "" || protectableItem.Properties == nil { + continue + } + azureFileShareProtectableItem, check := protectableItem.Properties.AsAzureFileShareProtectableItem() - if backupProtectedItems == nil && backupProtectableItems == nil { - return fmt.Errorf("[ERROR] fileshare '%s' not found in protectable or protected fileshares, make sure Storage Account %q is registered with Recovery Service Vault %q (Resource Group %q)", fileShareName, accountName, vaultName, resourceGroup) - } - if len(backupProtectableItems)+len(backupProtectedItems) > 1 { - return fmt.Errorf("[ERROR] multiple fileshares found after filtering protectable or protected fileshares where only one is expected") + // check if protected item has the same fileshare name and is from the same storage account + if check && *azureFileShareProtectableItem.FriendlyName == fileShareName && *azureFileShareProtectableItem.ParentContainerFriendlyName == accountName { + fileShareSystemName = *protectableItem.Name + break + } } - fileShareSystemName := "" - if backupProtectableItems != nil && *backupProtectableItems[0].Name != "" { - fileShareSystemName = *backupProtectableItems[0].Name + // fileShareSystemName not found? Check if already protected by this vault! + if fileShareSystemName == "" { + backupProtectedItemsResponse, err := protectedClient.List(ctx, vaultName, resourceGroup, filter, "") + if err != nil { + return fmt.Errorf("Error checking for protected fileshares in Recovery Service Vault %q (Resource Group %q): %+v", vaultName, resourceGroup, err) + } + + for _, protectedItem := range backupProtectedItemsResponse.Values() { + if *protectedItem.Name == "" || protectedItem.Properties == nil { + continue + } + azureFileShareProtectedItem, check := protectedItem.Properties.AsAzureFileshareProtectedItem() + + // check if protected item has the same fileshare name and is from the same storage account + if check && *azureFileShareProtectedItem.FriendlyName == fileShareName && strings.ToLower(*azureFileShareProtectedItem.SourceResourceID) == strings.ToLower(storageAccountID) { + fileShareSystemName = *protectedItem.Name + break + } + } } - if backupProtectedItems != nil && *backupProtectedItems[0].Name != "" { - fileShareSystemName = *backupProtectedItems[0].Name + if fileShareSystemName == "" { + return fmt.Errorf("[ERROR] fileshare '%s' not found in protectable or protected fileshares, make sure Storage Account %q is registered with Recovery Service Vault %q (Resource Group %q)", fileShareName, accountName, vaultName, resourceGroup) } containerName := fmt.Sprintf("StorageContainer;storage;%s;%s", parsedStorageAccountID.ResourceGroup, accountName) - log.Printf("[DEBUG] creating/updating Recovery Service Protected File Share %q (Container Name %q)", fileShareName, containerName) if d.IsNewResource() { diff --git a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource_test.go b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource_test.go index db840bbe2209f..94387651a1c7d 100644 --- a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource_test.go +++ b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource_test.go @@ -182,6 +182,75 @@ resource "azurerm_backup_policy_file_share" "test1" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } +func (r BackupProtectedFileShareResource) basic(data acceptance.TestData) string { + return fmt.Sprintf(` +%s + +resource "azurerm_backup_container_storage_account" "test" { + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + storage_account_id = azurerm_storage_account.test.id +} + +resource "azurerm_backup_protected_file_share" "test" { + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id + source_file_share_name = azurerm_storage_share.test.name + backup_policy_id = azurerm_backup_policy_file_share.test1.id +} +`, r.base(data)) +} + +func (r BackupProtectedFileShareResource) updatePolicy(data acceptance.TestData) string { + return fmt.Sprintf(` +%s + +resource "azurerm_backup_policy_file_share" "test2" { + name = "acctest-%d-Secondary" + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + + backup { + frequency = "Daily" + time = "23:00" + } + + retention_daily { + count = 10 + } +} + +resource "azurerm_backup_container_storage_account" "test" { + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + storage_account_id = azurerm_storage_account.test.id +} + +resource "azurerm_backup_protected_file_share" "test" { + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id + source_file_share_name = azurerm_storage_share.test.name + backup_policy_id = azurerm_backup_policy_file_share.test2.id +} +`, r.base(data), data.RandomInteger) +} + +func (r BackupProtectedFileShareResource) requiresImport(data acceptance.TestData) string { + return fmt.Sprintf(` +%s + +resource "azurerm_backup_protected_file_share" "test_import" { + resource_group_name = azurerm_resource_group.test.name + recovery_vault_name = azurerm_recovery_services_vault.test.name + source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id + source_file_share_name = azurerm_storage_share.test.name + backup_policy_id = azurerm_backup_policy_file_share.test1.id +} +`, r.basic(data)) +} + func (r BackupProtectedFileShareResource) baseMultiple(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { @@ -193,8 +262,16 @@ resource "azurerm_resource_group" "test" { location = "%[2]s" } -resource "azurerm_storage_account" "test" { - name = "acctest%[3]s" +resource "azurerm_storage_account" "test1" { + name = "acctest%[3]s1" + location = "${azurerm_resource_group.test.location}" + resource_group_name = "${azurerm_resource_group.test.name}" + account_tier = "Standard" + account_replication_type = "LRS" +} + +resource "azurerm_storage_account" "test2" { + name = "acctest%[3]s2" location = "${azurerm_resource_group.test.location}" resource_group_name = "${azurerm_resource_group.test.name}" account_tier = "Standard" @@ -203,7 +280,7 @@ resource "azurerm_storage_account" "test" { resource "azurerm_storage_share" "testshare1" { name = "acctest-ss-%[1]d-1" - storage_account_name = "${azurerm_storage_account.test.name}" + storage_account_name = "${azurerm_storage_account.test1.name}" metadata = {} lifecycle { @@ -212,14 +289,34 @@ resource "azurerm_storage_share" "testshare1" { } resource "azurerm_storage_share" "testshare2" { - name = "acctest-ss-%[1]d-2" - storage_account_name = "${azurerm_storage_account.test.name}" - metadata = {} - - lifecycle { - ignore_changes = [metadata] // Ignore changes Azure Backup makes to the metadata - } + name = "acctest-ss-%[1]d-2" + storage_account_name = "${azurerm_storage_account.test1.name}" + metadata = {} + + lifecycle { + ignore_changes = [metadata] // Ignore changes Azure Backup makes to the metadata } +} + +resource "azurerm_storage_share" "testshare3" { + name = "acctest-ss-%[1]d-1" + storage_account_name = "${azurerm_storage_account.test2.name}" + metadata = {} + + lifecycle { + ignore_changes = [metadata] // Ignore changes Azure Backup makes to the metadata + } +} + +resource "azurerm_storage_share" "testshare4" { + name = "acctest-ss-%[1]d-2" + storage_account_name = "${azurerm_storage_account.test2.name}" + metadata = {} + + lifecycle { + ignore_changes = [metadata] // Ignore changes Azure Backup makes to the metadata + } +} resource "azurerm_recovery_services_vault" "test" { name = "acctest-VAULT-%[1]d" @@ -230,7 +327,7 @@ resource "azurerm_recovery_services_vault" "test" { soft_delete_enabled = false } -resource "azurerm_backup_policy_file_share" "test1" { +resource "azurerm_backup_policy_file_share" "test" { name = "acctest-PFS-%[1]d" resource_group_name = "${azurerm_resource_group.test.name}" recovery_vault_name = "${azurerm_recovery_services_vault.test.name}" @@ -247,91 +344,28 @@ resource "azurerm_backup_policy_file_share" "test1" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } -func (r BackupProtectedFileShareResource) basic(data acceptance.TestData) string { - return fmt.Sprintf(` -%s - -resource "azurerm_backup_container_storage_account" "test" { - resource_group_name = azurerm_resource_group.test.name - recovery_vault_name = azurerm_recovery_services_vault.test.name - storage_account_id = azurerm_storage_account.test.id -} - -resource "azurerm_backup_protected_file_share" "test" { - resource_group_name = azurerm_resource_group.test.name - recovery_vault_name = azurerm_recovery_services_vault.test.name - source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id - source_file_share_name = azurerm_storage_share.test.name - backup_policy_id = azurerm_backup_policy_file_share.test1.id -} -`, r.base(data)) -} - func (r BackupProtectedFileShareResource) multiple(data acceptance.TestData) string { return fmt.Sprintf(` %s -resource "azurerm_backup_container_storage_account" "test" { - resource_group_name = azurerm_resource_group.test.name - recovery_vault_name = azurerm_recovery_services_vault.test.name - storage_account_id = azurerm_storage_account.test.id -} - -resource "azurerm_backup_protected_file_share" "test" { - resource_group_name = azurerm_resource_group.test.name - recovery_vault_name = azurerm_recovery_services_vault.test.name - source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id - source_file_share_name = azurerm_storage_share.testshare2.name - backup_policy_id = azurerm_backup_policy_file_share.test1.id -} -`, r.baseMultiple(data)) -} - -func (r BackupProtectedFileShareResource) updatePolicy(data acceptance.TestData) string { - return fmt.Sprintf(` -%s - -resource "azurerm_backup_policy_file_share" "test2" { - name = "acctest-%d-Secondary" +resource "azurerm_backup_container_storage_account" "test1" { resource_group_name = azurerm_resource_group.test.name recovery_vault_name = azurerm_recovery_services_vault.test.name - - backup { - frequency = "Daily" - time = "23:00" - } - - retention_daily { - count = 10 - } + storage_account_id = azurerm_storage_account.test1.id } -resource "azurerm_backup_container_storage_account" "test" { +resource "azurerm_backup_container_storage_account" "test2" { resource_group_name = azurerm_resource_group.test.name recovery_vault_name = azurerm_recovery_services_vault.test.name - storage_account_id = azurerm_storage_account.test.id + storage_account_id = azurerm_storage_account.test2.id } resource "azurerm_backup_protected_file_share" "test" { resource_group_name = azurerm_resource_group.test.name recovery_vault_name = azurerm_recovery_services_vault.test.name - source_storage_account_id = azurerm_backup_container_storage_account.test.storage_account_id - source_file_share_name = azurerm_storage_share.test.name - backup_policy_id = azurerm_backup_policy_file_share.test2.id + source_storage_account_id = azurerm_backup_container_storage_account.test2.storage_account_id + source_file_share_name = azurerm_storage_share.testshare3.name + backup_policy_id = azurerm_backup_policy_file_share.test.id } -`, r.base(data), data.RandomInteger) -} - -func (r BackupProtectedFileShareResource) requiresImport(data acceptance.TestData) string { - return fmt.Sprintf(` -%s - -resource "azurerm_backup_protected_file_share" "test_import" { - resource_group_name = azurerm_resource_group.test.name - recovery_vault_name = azurerm_recovery_services_vault.test.name - source_storage_account_id = azurerm_storage_account.test.id - source_file_share_name = azurerm_storage_share.test.name - backup_policy_id = azurerm_backup_policy_file_share.test1.id -} -`, r.base(data)) +`, r.baseMultiple(data)) }