Managed Disk Resource: Added support for network_access_policy

[cmendible]

Fixes: #9796

[martinbjorgan]

@cmendible I was also working on this, you beat me to it :)
I think we also should add the disk_access_id parameter to the managed disk resource since that is required if AllowPrivate is used? And also add azurerm_disk_access as a new resource?

[cmendible]

Hi! That's the reason I added the WIP prefix. We need to be sure what else is needed for that specific case...

[martinbjorgan]

@cmendible Missed that! Sorry :)

[cmendible]

@martinbjorgan confirmed here that disk_access_id is required when the NeworkAccessPolicy is set to AllowPrivate and also that the field is supported in update operations: Disks - Create Or Update.

Can you open an issue to add azurerm_disk_access as a new resource so you can tackle that one?

[martinbjorgan]

@cmendible Sure, I'll give it a whirl

[cmendible]

PR #9889 (adds the azurerm_disk_access resource) must be merged so we can add a test for network_access_policy = AllowPrivate which requires a disk_access_id

[martinbjorgan]

@cmendible I'm almost done with tests for the resource, data source is done.
Hopefully I'm done with those after the 24th

[cmendible]

PR #9889 (adds the azurerm_disk_access resource) must be merged so we can add a test for network_access_policy = AllowPrivate which requires a disk_access_id

PR #9889 was merged and test for network_access_policy = AllowPrivate are in place.

@katbyte can you please review this one?

[katbyte]

Thanks for the PR @cmendible - overall this looks great! i've just left some comments inline to address before merge

[katbyte]

Thanks @cmendible - but i think this change is going to break existing configs as tests are now failing with:

An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_managed_disk.test will be updated in-place
          ~ resource "azurerm_managed_disk" "test" {
                id                    = "/subscriptions/*******/resourceGroups/acctestRG-210415031958219440/providers/Microsoft.Compute/disks/acctestd-210415031958219440"
                name                  = "acctestd-210415031958219440"
              - network_access_policy = "AllowAll" -> null
                tags                  = {
                    "cost-center" = "ops"
                    "environment" = "acctest"
                }
                # (7 unchanged attributes hidden)
        
                # (1 unchanged block hidden)
            }

maybe we need to mark that as computed?

[cmendible]

Thanks @cmendible - but i think this change is going to break existing configs as tests are now failing with:

An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_managed_disk.test will be updated in-place
          ~ resource "azurerm_managed_disk" "test" {
                id                    = "/subscriptions/*******/resourceGroups/acctestRG-210415031958219440/providers/Microsoft.Compute/disks/acctestd-210415031958219440"
                name                  = "acctestd-210415031958219440"
              - network_access_policy = "AllowAll" -> null
                tags                  = {
                    "cost-center" = "ops"
                    "environment" = "acctest"
                }
                # (7 unchanged attributes hidden)
        
                # (1 unchanged block hidden)
            }

maybe we need to mark that as computed?

@katbyte I ran some tests and it should be fixed.

[katbyte]

Thanks @cmendible - overall looks good and i've left some comments inline

[katbyte]

Thanks @cmendible - aside from the tests failing with:

------- Stdout: -------
=== RUN   TestAccAzureRMManagedDisk_networkPolicy
=== PAUSE TestAccAzureRMManagedDisk_networkPolicy
=== CONT  TestAccAzureRMManagedDisk_networkPolicy
    testing.go:620: Step 1/2 error: Error running apply: exit status 1
        
        Error: [ERROR] disk_iops_read_write and disk_mbps_read_write are only available for UltraSSD disks
        
          on terraform_plugin_test.tf line 11, in resource "azurerm_managed_disk" "test":
          11: resource "azurerm_managed_disk" "test" {
        
--- FAIL: TestAccAzureRMManagedDisk_networkPolicy (104.83s)
FAIL

------- Stderr: -------
2021/04/28 06:39:36 [DEBUG] not using binary driver name, it's no longer needed
2021/04/28 06:39:36 [DEBUG] not using binary driver name, it's no longer needed

this is looking good to go!

[cmendible]

@katbyte fixed the tests in: d64af57

[katbyte]

@cmendible - looks like we have another casing issue 😪

------- Stdout: -------
=== RUN   TestAccAzureRMManagedDisk_networkPolicy_create_withAllowPrivate
=== PAUSE TestAccAzureRMManagedDisk_networkPolicy_create_withAllowPrivate
=== CONT  TestAccAzureRMManagedDisk_networkPolicy_create_withAllowPrivate
    testing.go:620: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_managed_disk.test will be updated in-place
          ~ resource "azurerm_managed_disk" "test" {
              ~ disk_access_id        = "/subscriptions/*******/resourceGroups/ACCTESTRG-210428201256204039/providers/Microsoft.Compute/diskAccesses/accda210428201256204039" -> "/subscriptions/*******/resourceGroups/acctestRG-210428201256204039/providers/Microsoft.Compute/diskAccesses/accda210428201256204039"
                id                    = "/subscriptions/*******/resourceGroups/acctestRG-210428201256204039/providers/Microsoft.Compute/disks/acctestd-210428201256204039"
                name                  = "acctestd-210428201256204039"
                tags                  = {
                    "cost-center" = "ops"
                    "environment" = "acctest"
                }
                # (9 unchanged attributes hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccAzureRMManagedDisk_networkPolicy_create_withAllowPrivate (189.25s)
FAIL

------- Stderr: -------
2021/04/28 20:12:54 [DEBUG] not using binary driver name, it's no longer needed
2021/04/28 20:12:55 [DEBUG] not using binary driver name, it's no longer needed

could we open an issue on the SDK describing this issue and link to it on the property? which i believe we'll need to diffsuppress case

[cmendible]

Opened issue to the Azure SDK team here: Azure/azure-rest-api-specs#14192

And suppress.CaseDifference added to the code here: f0cc2de

[katbyte]

Thanks @cmendible - LGTM 👍

[ghost]

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.