From 42f573486fe998064fb87f64b300e74341b06e82 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sat, 24 Oct 2020 11:09:57 +0200 Subject: [PATCH 1/9] fix azurerm_backup_protected_file_share --- .../backup_protected_file_share_resource.go | 53 +++++++++++++------ .../recoveryservices/client/client.go | 10 ++++ 2 files changed, 47 insertions(+), 16 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 e12f537aa225..24cfce1c5733 100644 --- a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go +++ b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go @@ -71,6 +71,7 @@ func resourceBackupProtectedFileShare() *schema.Resource { } func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta interface{}) error { + protectableClient := meta.(*clients.Client).RecoveryServices.ProtectableItemsClient client := meta.(*clients.Client).RecoveryServices.ProtectedItemsClient opClient := meta.(*clients.Client).RecoveryServices.BackupOperationStatusesClient ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) @@ -93,16 +94,36 @@ func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta i return fmt.Errorf("[ERROR] parsed source_storage_account_id '%s' doesn't contain 'storageAccounts'", storageAccountID) } - protectedItemName := fmt.Sprintf("AzureFileShare;%s", fileShareName) + // 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) + backupProtectableItemsResponse, err := protectableClient.List(ctx, vaultName, resourceGroup, filter, "") + backupProtectableItems := backupProtectableItemsResponse.Values() + + backupProtectedItemsResponse, err := protectedClient.List(ctx, vaultName, resourceGroup, filter, "") + backupProtectedItems := backupProtectedItemsResponse.Values() + + fileShareSystemName := "" + if backupProtectableItems != nil && *backupProtectableItems[0].Name != "" { + fileShareSystemName = *backupProtectableItems[0].Name + } else if backupProtectedItems != nil && *backupProtectedItems[0].Name != "" { + fileShareSystemName = *backupProtectedItems[0].Name + } else { + return fmt.Errorf("[ERROR] fileshare '%s' not found in protectable or protected fileshares, make sure ", fileShareName) + } + + if len(backupProtectableItems)+len(backupProtectedItems) > 1 { + return fmt.Errorf("[ERROR] multiple fileshares found after filtering protectable or protected fileshares where only one is expected") + } + containerName := fmt.Sprintf("StorageContainer;storage;%s;%s", parsedStorageAccountID.ResourceGroup, accountName) - log.Printf("[DEBUG] Creating/updating Recovery Service Protected File Share %q (Container Name %q)", protectedItemName, containerName) + log.Printf("[DEBUG] creating/updating Recovery Service Protected File Share %q (Container Name %q)", fileShareName, containerName) if d.IsNewResource() { - existing, err2 := client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, protectedItemName, "") + existing, err2 := client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, fileShareSystemName, "") if err2 != nil { if !utils.ResponseWasNotFound(existing.Response) { - return fmt.Errorf("Error checking for presence of existing Recovery Service Protected File Share %q (Resource Group %q): %+v", protectedItemName, resourceGroup, err2) + return fmt.Errorf("Error checking for presence of existing Recovery Service Protected File Share %q (Resource Group %q): %+v", fileShareName, resourceGroup, err2) } } @@ -121,9 +142,9 @@ func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta i }, } - resp, err := client.CreateOrUpdate(ctx, vaultName, resourceGroup, "Azure", containerName, protectedItemName, item) + resp, err := client.CreateOrUpdate(ctx, vaultName, resourceGroup, "Azure", containerName, fileShareSystemName, item) if err != nil { - return fmt.Errorf("Error creating/updating Recovery Service Protected File Share %q (Resource Group %q): %+v", protectedItemName, resourceGroup, err) + return fmt.Errorf("Error creating/updating Recovery Service Protected File Share %q (Resource Group %q): %+v", fileShareName, resourceGroup, err) } locationURL, err := resp.Response.Location() @@ -143,10 +164,10 @@ func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta i return err } - resp, err = client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, protectedItemName, "") + resp, err = client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, fileShareSystemName, "") if err != nil { - return fmt.Errorf("Error creating/udpating Azure File Share backup item %q (Vault %q): %+v", protectedItemName, vaultName, err) + return fmt.Errorf("Error creating/updating Azure File Share backup item %q (Vault %q): %+v", fileShareName, vaultName, err) } id := strings.Replace(*resp.ID, "Subscriptions", "subscriptions", 1) // This code is a workaround for this bug https://github.com/Azure/azure-sdk-for-go/issues/2824 @@ -165,21 +186,21 @@ func resourceBackupProtectedFileShareRead(d *schema.ResourceData, meta interface return err } - protectedItemName := id.Path["protectedItems"] + fileShareSystemName := id.Path["protectedItems"] vaultName := id.Path["vaults"] resourceGroup := id.ResourceGroup containerName := id.Path["protectionContainers"] - log.Printf("[DEBUG] Reading Recovery Service Protected File Share %q (resource group %q)", protectedItemName, resourceGroup) + log.Printf("[DEBUG] Reading Recovery Service Protected File Share %q (resource group %q)", fileShareSystemName, resourceGroup) - resp, err := client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, protectedItemName, "") + resp, err := client.Get(ctx, vaultName, resourceGroup, "Azure", containerName, fileShareSystemName, "") if err != nil { if utils.ResponseWasNotFound(resp.Response) { d.SetId("") return nil } - return fmt.Errorf("Error making Read request on Recovery Service Protected File Share %q (Vault %q Resource Group %q): %+v", protectedItemName, vaultName, resourceGroup, err) + return fmt.Errorf("Error making Read request on Recovery Service Protected File Share %q (Vault %q Resource Group %q): %+v", fileShareSystemName, vaultName, resourceGroup, err) } d.Set("resource_group_name", resourceGroup) @@ -211,17 +232,17 @@ func resourceBackupProtectedFileShareDelete(d *schema.ResourceData, meta interfa return err } - protectedItemName := id.Path["protectedItems"] + fileShareSystemName := id.Path["protectedItems"] resourceGroup := id.ResourceGroup vaultName := id.Path["vaults"] containerName := id.Path["protectionContainers"] - log.Printf("[DEBUG] Deleting Recovery Service Protected Item %q (resource group %q)", protectedItemName, resourceGroup) + log.Printf("[DEBUG] Deleting Recovery Service Protected Item %q (resource group %q)", fileShareSystemName, resourceGroup) - resp, err := client.Delete(ctx, vaultName, resourceGroup, "Azure", containerName, protectedItemName) + resp, err := client.Delete(ctx, vaultName, resourceGroup, "Azure", containerName, fileShareSystemName) if err != nil { if !utils.ResponseWasNotFound(resp) { - return fmt.Errorf("Error issuing delete request for Recovery Service Protected File Share %q (Resource Group %q): %+v", protectedItemName, resourceGroup, err) + return fmt.Errorf("Error issuing delete request for Recovery Service Protected File Share %q (Resource Group %q): %+v", fileShareSystemName, resourceGroup, err) } } diff --git a/azurerm/internal/services/recoveryservices/client/client.go b/azurerm/internal/services/recoveryservices/client/client.go index f4a8e0fb8c29..c757041481a9 100644 --- a/azurerm/internal/services/recoveryservices/client/client.go +++ b/azurerm/internal/services/recoveryservices/client/client.go @@ -8,7 +8,9 @@ import ( ) type Client struct { + ProtectableItemsClient *backup.ProtectableItemsClient ProtectedItemsClient *backup.ProtectedItemsClient + ProtectedItemsGroupClient *backup.ProtectedItemsGroupClient ProtectionPoliciesClient *backup.ProtectionPoliciesClient BackupProtectionContainersClient *backup.ProtectionContainersClient BackupOperationStatusesClient *backup.OperationStatusesClient @@ -29,9 +31,15 @@ func NewClient(o *common.ClientOptions) *Client { vaultsClient := recoveryservices.NewVaultsClientWithBaseURI(o.ResourceManagerEndpoint, o.SubscriptionId) o.ConfigureClient(&vaultsClient.Client, o.ResourceManagerAuthorizer) + protectableItemsClient := backup.NewProtectableItemsClientWithBaseURI(o.ResourceManagerEndpoint, o.SubscriptionId) + o.ConfigureClient(&protectableItemsClient.Client, o.ResourceManagerAuthorizer) + protectedItemsClient := backup.NewProtectedItemsClientWithBaseURI(o.ResourceManagerEndpoint, o.SubscriptionId) o.ConfigureClient(&protectedItemsClient.Client, o.ResourceManagerAuthorizer) + protectedItemsGroupClient := backup.NewProtectedItemsGroupClientWithBaseURI(o.ResourceManagerEndpoint, o.SubscriptionId) + o.ConfigureClient(&protectedItemsGroupClient.Client, o.ResourceManagerAuthorizer) + protectionPoliciesClient := backup.NewProtectionPoliciesClientWithBaseURI(o.ResourceManagerEndpoint, o.SubscriptionId) o.ConfigureClient(&protectionPoliciesClient.Client, o.ResourceManagerAuthorizer) @@ -78,7 +86,9 @@ func NewClient(o *common.ClientOptions) *Client { } return &Client{ + ProtectableItemsClient: &protectableItemsClient, ProtectedItemsClient: &protectedItemsClient, + ProtectedItemsGroupClient: &protectedItemsGroupClient, ProtectionPoliciesClient: &protectionPoliciesClient, BackupProtectionContainersClient: &backupProtectionContainersClient, BackupOperationStatusesClient: &backupOperationStatusesClient, From 60c1fe6991d3e9fd55ce0f33c87c7c345b203a6b Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sat, 24 Oct 2020 11:41:11 +0200 Subject: [PATCH 2/9] update documentation azurerm_backup_protected_file_share --- website/docs/r/backup_protected_file_share.markdown | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/website/docs/r/backup_protected_file_share.markdown b/website/docs/r/backup_protected_file_share.markdown index 27167fe715d7..89ba9028893c 100644 --- a/website/docs/r/backup_protected_file_share.markdown +++ b/website/docs/r/backup_protected_file_share.markdown @@ -10,10 +10,6 @@ description: |- Manages an Azure Backup Protected File Share to enable backups for file shares within an Azure Storage Account --> **NOTE:** Azure Backup for Azure File Shares is currently in public preview. During the preview, the service is subject to additional limitations and unsupported backup scenarios. [Read More](https://docs.microsoft.com/en-us/azure/backup/backup-azure-files#limitations-for-azure-file-share-backup-during-preview) - --> **NOTE** Azure Backup for Azure File Shares does not support Soft Delete at this time. Deleting this resource will also delete all associated backup data. Please exercise caution. Consider using [`prevent_destroy`](https://www.terraform.io/docs/configuration/resources.html#prevent_destroy) to guard against accidental deletion. - ## Example Usage ```hcl @@ -108,7 +104,7 @@ The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/d Azure Backup Protected File Shares can be imported using the `resource id`, e.g. ```shell -terraform import azurerm_backup_protected_file_share.item1 "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.RecoveryServices/vaults/example-recovery-vault/backupFabrics/Azure/protectionContainers/StorageContainer;storage;group2;example-storage-account/protectedItems/AzureFileShare;example-share" +terraform import azurerm_backup_protected_file_share.item1 "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.RecoveryServices/vaults/example-recovery-vault/backupFabrics/Azure/protectionContainers/StorageContainer;storage;group2;example-storage-account/protectedItems/AzureFileShare;3f6e3108a45793581bcbd1c61c87a3b2ceeb4ff4bc02a95ce9d1022b23722935" ``` -Note the ID requires quoting as there are semicolons +-> **NOTE** The ID requires quoting as there are semicolons. This user unfriendly ID can be found in the Deployments of the used resourcegroup, look for an Deployment which starts with `ConfigureAFSProtection-`, click then `Go to resource`. From ba2c9c78c49b4b0442d91b170ce3af969a30099d Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sat, 24 Oct 2020 12:08:27 +0200 Subject: [PATCH 3/9] fix lintrest issues --- .../backup_protected_file_share_resource.go | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 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 24cfce1c5733..6ce3299f38f4 100644 --- a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go +++ b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go @@ -97,22 +97,30 @@ func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta i // 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) 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() + 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") + } + fileShareSystemName := "" if backupProtectableItems != nil && *backupProtectableItems[0].Name != "" { fileShareSystemName = *backupProtectableItems[0].Name - } else if backupProtectedItems != nil && *backupProtectedItems[0].Name != "" { - fileShareSystemName = *backupProtectedItems[0].Name - } else { - return fmt.Errorf("[ERROR] fileshare '%s' not found in protectable or protected fileshares, make sure ", fileShareName) } - - if len(backupProtectableItems)+len(backupProtectedItems) > 1 { - return fmt.Errorf("[ERROR] multiple fileshares found after filtering protectable or protected fileshares where only one is expected") + if backupProtectedItems != nil && *backupProtectedItems[0].Name != "" { + fileShareSystemName = *backupProtectedItems[0].Name } containerName := fmt.Sprintf("StorageContainer;storage;%s;%s", parsedStorageAccountID.ResourceGroup, accountName) From ccefd10395527744ea39b67e1130f4fd29524dcd Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Mon, 21 Dec 2020 11:40:03 +0100 Subject: [PATCH 4/9] acctest for multiple shares --- GNUmakefile | 2 +- ...ckup_protected_file_share_resource_test.go | 105 ++++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/GNUmakefile b/GNUmakefile index b36b1bfb409a..5f8d74af571a 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -104,7 +104,7 @@ testacc: fmtcheck TF_ACC=1 go test $(TEST) -v $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" acctests: fmtcheck - TF_ACC=1 go test -v ./azurerm/internal/services/$(SERVICE)/tests/ $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" + TF_ACC=1 go test -v ./azurerm/internal/services/$(SERVICE)/ $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" debugacc: fmtcheck TF_ACC=1 dlv test $(TEST) --headless --listen=:2345 --api-version=2 -- -test.v $(TESTARGS) 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 046a97ecd3d9..db840bbe2209 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 @@ -38,6 +38,26 @@ func TestAccBackupProtectedFileShare_basic(t *testing.T) { }) } +func TestAccBackupProtectedFileShare_multiple(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_backup_protected_file_share", "test") + r := BackupProtectedFileShareResource{} + + data.ResourceTest(t, r, []resource.TestStep{ + { + Config: r.multiple(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("resource_group_name").Exists(), + ), + }, + data.ImportStep(), + { + // vault cannot be deleted unless we unregister all backups + Config: r.baseMultiple(data), + }, + }) +} + func TestAccBackupProtectedFileShare_requiresImport(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_backup_protected_file_share", "test") r := BackupProtectedFileShareResource{} @@ -162,6 +182,71 @@ resource "azurerm_backup_policy_file_share" "test1" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } +func (r BackupProtectedFileShareResource) baseMultiple(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-backup-%[1]d" + location = "%[2]s" +} + +resource "azurerm_storage_account" "test" { + name = "acctest%[3]s" + location = "${azurerm_resource_group.test.location}" + resource_group_name = "${azurerm_resource_group.test.name}" + account_tier = "Standard" + account_replication_type = "LRS" +} + +resource "azurerm_storage_share" "testshare1" { + name = "acctest-ss-%[1]d-1" + storage_account_name = "${azurerm_storage_account.test.name}" + metadata = {} + + lifecycle { + ignore_changes = [metadata] // Ignore changes Azure Backup makes to the metadata + } +} + +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 + } + } + +resource "azurerm_recovery_services_vault" "test" { + name = "acctest-VAULT-%[1]d" + location = "${azurerm_resource_group.test.location}" + resource_group_name = "${azurerm_resource_group.test.name}" + sku = "Standard" + + soft_delete_enabled = false +} + +resource "azurerm_backup_policy_file_share" "test1" { + name = "acctest-PFS-%[1]d" + 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 + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString) +} + func (r BackupProtectedFileShareResource) basic(data acceptance.TestData) string { return fmt.Sprintf(` %s @@ -182,6 +267,26 @@ resource "azurerm_backup_protected_file_share" "test" { `, 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 From ccaf783502445d57ea60e39550dc299a7cf875b3 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Tue, 22 Dec 2020 10:42:47 +0100 Subject: [PATCH 5/9] Fix after rebase --- .../recoveryservices/backup_protected_file_share_resource.go | 1 + 1 file changed, 1 insertion(+) 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 6ce3299f38f4..16250455cda1 100644 --- a/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go +++ b/azurerm/internal/services/recoveryservices/backup_protected_file_share_resource.go @@ -71,6 +71,7 @@ func resourceBackupProtectedFileShare() *schema.Resource { } func resourceBackupProtectedFileShareCreateUpdate(d *schema.ResourceData, meta interface{}) error { + protectedClient := meta.(*clients.Client).RecoveryServices.ProtectedItemsGroupClient protectableClient := meta.(*clients.Client).RecoveryServices.ProtectableItemsClient client := meta.(*clients.Client).RecoveryServices.ProtectedItemsClient opClient := meta.(*clients.Client).RecoveryServices.BackupOperationStatusesClient From fd5eda131025e9a218eba28644f037409340f732 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Tue, 22 Dec 2020 14:21:45 +0100 Subject: [PATCH 6/9] 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 16250455cda1..b9897fd0e5f8 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 := "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.EqualFold(*azureFileShareProtectedItem.SourceResourceID, 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 db840bbe2209..94387651a1c7 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)) } From d12e8d9169421c64b0536f87255bf05138497d74 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Tue, 22 Dec 2020 18:19:23 +0100 Subject: [PATCH 7/9] Improve test stability --- .../backup_protected_file_share_resource_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) 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 94387651a1c7..b513341d7e44 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 @@ -163,6 +163,10 @@ resource "azurerm_recovery_services_vault" "test" { sku = "Standard" soft_delete_enabled = false + + lifecycle { + ignore_changes = [soft_delete_enabled] // Ignore changes to prevent tests from flakiness + } } resource "azurerm_backup_policy_file_share" "test1" { @@ -325,6 +329,10 @@ resource "azurerm_recovery_services_vault" "test" { sku = "Standard" soft_delete_enabled = false + + lifecycle { + ignore_changes = [soft_delete_enabled] // Ignore changes to prevent tests from flakiness + } } resource "azurerm_backup_policy_file_share" "test" { From 1cb4fc889096e6b90a5c6a4a8e11126da60c4979 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Tue, 29 Dec 2020 12:46:35 +0100 Subject: [PATCH 8/9] Improve test stability 2 --- ...backup_container_storage_account_resource_test.go | 2 +- .../backup_protected_file_share_resource_test.go | 12 ++---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/azurerm/internal/services/recoveryservices/backup_container_storage_account_resource_test.go b/azurerm/internal/services/recoveryservices/backup_container_storage_account_resource_test.go index 117e3af4d29b..e6b64b8e1491 100644 --- a/azurerm/internal/services/recoveryservices/backup_container_storage_account_resource_test.go +++ b/azurerm/internal/services/recoveryservices/backup_container_storage_account_resource_test.go @@ -68,7 +68,7 @@ resource "azurerm_recovery_services_vault" "testvlt" { resource_group_name = azurerm_resource_group.test.name sku = "Standard" - soft_delete_enabled = false + soft_delete_enabled = true } resource "azurerm_storage_account" "test" { 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 b513341d7e44..9cececdf60da 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 @@ -162,11 +162,7 @@ resource "azurerm_recovery_services_vault" "test" { resource_group_name = "${azurerm_resource_group.test.name}" sku = "Standard" - soft_delete_enabled = false - - lifecycle { - ignore_changes = [soft_delete_enabled] // Ignore changes to prevent tests from flakiness - } + soft_delete_enabled = true } resource "azurerm_backup_policy_file_share" "test1" { @@ -328,11 +324,7 @@ resource "azurerm_recovery_services_vault" "test" { resource_group_name = "${azurerm_resource_group.test.name}" sku = "Standard" - soft_delete_enabled = false - - lifecycle { - ignore_changes = [soft_delete_enabled] // Ignore changes to prevent tests from flakiness - } + soft_delete_enabled = true } resource "azurerm_backup_policy_file_share" "test" { From 102382d6f69feb3e86ee40e093be15a3429625f4 Mon Sep 17 00:00:00 2001 From: kt Date: Thu, 14 Jan 2021 11:19:41 -0800 Subject: [PATCH 9/9] Update GNUmakefile --- GNUmakefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GNUmakefile b/GNUmakefile index 5f8d74af571a..b36b1bfb409a 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -104,7 +104,7 @@ testacc: fmtcheck TF_ACC=1 go test $(TEST) -v $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" acctests: fmtcheck - TF_ACC=1 go test -v ./azurerm/internal/services/$(SERVICE)/ $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" + TF_ACC=1 go test -v ./azurerm/internal/services/$(SERVICE)/tests/ $(TESTARGS) -timeout $(TESTTIMEOUT) -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" debugacc: fmtcheck TF_ACC=1 dlv test $(TEST) --headless --listen=:2345 --api-version=2 -- -test.v $(TESTARGS)