New resource azurerm_managed_disk_sas_token to manage disk exports #15558

harshavmb · 2022-02-22T20:09:57Z

New resource azurerm_disk_export to manage disk exports.
This PR is inspired from azurerm_storage_account_blob_container_sas datasource

Few details about disk access grants here :: https://docs.microsoft.com/en-us/rest/api/compute/disks/grant-access
https://docs.microsoft.com/en-us/rest/api/compute/disks/revoke-access

With the help of this disk export sas token, we could copy data from disks onto storage blobs or elsewhere without the need of azcopy

Looking forward to hear from you.

katbyte

Thanks @harshavmb - it looks like there are some issues with the test:

[](https://ci-oss.hashicorp.engineering/viewLog.html?buildId=260323&buildTypeId=TerraformOpenSource_TerraformProviders_AzureRMPublic_AZURERM_SERVICE_PUBLIC_COMPUTE#)------- Stdout: -------
=== RUN   TestAccDataSourceDiskExportSas_basic
=== PAUSE TestAccDataSourceDiskExportSas_basic
=== CONT  TestAccDataSourceDiskExportSas_basic
    testcase.go:110: Step 1/1 error: Check failed: Check 1/4 error: data.azurerm_managed_disk_export.test: Attribute 'managed_disk_id' expected "/subscriptions/42cbb0b8a331-abaf-4e69-8d8f-14b86a40/resourceGroups/disksrg/providers/Microsoft.Compute/disks/disk1", got "/subscriptions/*******/resourceGroups/acctestRG-disk-220223000403491059/providers/Microsoft.Compute/disks/acctestsadsq8vx2"
    testing_new.go:70: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: deleting Managed Disk "acctestsadsq8vx2" (Resource Group "acctestRG-disk-220223000403491059"): compute.DisksClient#Delete: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="OperationNotAllowed" Message="There is an active shared access signature outstanding for disk acctestsadsq8vx2. Call EndGetAccess before attaching or deleting the disk. Learn more here: aka.ms/revokeaccessapi."
        
--- FAIL: TestAccDataSourceDiskExportSas_basic (45.57s)
FAIL

…t sas token post disk operations

harshavmb · 2022-02-23T11:41:15Z

Thanks @harshavmb - it looks like there are some issues with the test:

[](https://ci-oss.hashicorp.engineering/viewLog.html?buildId=260323&buildTypeId=TerraformOpenSource_TerraformProviders_AzureRMPublic_AZURERM_SERVICE_PUBLIC_COMPUTE#)------- Stdout: -------
=== RUN   TestAccDataSourceDiskExportSas_basic
=== PAUSE TestAccDataSourceDiskExportSas_basic
=== CONT  TestAccDataSourceDiskExportSas_basic
    testcase.go:110: Step 1/1 error: Check failed: Check 1/4 error: data.azurerm_managed_disk_export.test: Attribute 'managed_disk_id' expected "/subscriptions/42cbb0b8a331-abaf-4e69-8d8f-14b86a40/resourceGroups/disksrg/providers/Microsoft.Compute/disks/disk1", got "/subscriptions/*******/resourceGroups/acctestRG-disk-220223000403491059/providers/Microsoft.Compute/disks/acctestsadsq8vx2"
    testing_new.go:70: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: deleting Managed Disk "acctestsadsq8vx2" (Resource Group "acctestRG-disk-220223000403491059"): compute.DisksClient#Delete: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="OperationNotAllowed" Message="There is an active shared access signature outstanding for disk acctestsadsq8vx2. Call EndGetAccess before attaching or deleting the disk. Learn more here: aka.ms/revokeaccessapi."
        
--- FAIL: TestAccDataSourceDiskExportSas_basic (45.57s)
FAIL

Hi @katbyte ,

Thanks for the review. Indeed, it was something I missed. I added another data source to revoke the granted access on the disk.
Tested locally, it worked. Please rereview this..

tombuildsstuff

hi @harshavmb

Thanks for this PR

Taking a look through here since these are creating and subsequently revoking resources on the Azure side these want to be a single Resource rather than two Data Sources here - so can we update this to be a single Resource rather than two Data Sources?

Thanks!

…ta sources

harshavmb · 2022-02-25T18:33:36Z

hi @harshavmb

Thanks for this PR

Taking a look through here since these are creating and subsequently revoking resources on the Azure side these want to be a single Resource rather than two Data Sources here - so can we update this to be a single Resource rather than two Data Sources?

Thanks!

Hi @tombuildsstuff

I've added a new resource as you suggested. Changes LGTM, tested locally.

Please re-review.

tombuildsstuff

hi @harshavmb

Thanks for this PR - apologies for the delayed re-review here.

I've taken a look through and left some more comments inline, in particular I can't see this defined as a "Disk Export" within Azure, since this is a SAS Token for a Managed Disk, I'm thinking that this resource would make more sense with that name -WDYT?

Thanks!

internal/services/compute/disk_export_resource.go

tombuildsstuff · 2022-04-21T16:53:47Z

internal/services/compute/disk_export_resource.go

+	diskName := parsedManagedDiskId.DiskName
+	resourceGroupName := parsedManagedDiskId.ResourceGroup
+
+	grantAccessData := compute.GrantAccessData{
+		Access:            compute.AccessLevel(access),
+		DurationInSeconds: &durationInSeconds,
+	}


we're missing a Requires Import check here, we need to check if the Disk already has an Export and if so, raise a "this should be imported" error, (as below) it appears that this can be accessed by retrieving the disk and checking if it has a DiskAccess URI

Hi @tombuildsstuff ,

I think disk access is more related to private links. I did check the API again here. diskAccessId existed only for disks with private endpoints.

Having said that, I found diskState parameter which will be ActiveSAS when the disk export is active. I'll check this parameter.

internal/services/compute/disk_export_resource.go

internal/services/compute/disk_export_resource_test.go

internal/services/compute/registration.go

… PR review & other changes

…t function as tests are failing

katbyte

Thanks, just some minor comments i've left inline and this should be good!

website/docs/r/disk_export.html.markdown

internal/services/compute/disk_sas_token_resource.go

internal/services/compute/disk_sas_token_resource_test.go

website/docs/r/disk_export.html.markdown

katbyte · 2022-05-06T00:04:31Z

website/docs/r/disk_export.html.markdown

+
+* `create` - (Defaults to 30 minutes) Used when creating the Disk.
+* `read` - (Defaults to 5 minutes) Used when retrieving the Disk.
+* `delete` - (Defaults to 30 minutes) Used when deleting the Disk.


we should include how to import the resource here?

The SAS tokens are ephemeral, I don't know how useful it would be to import the disk here.

I'm just importing the managed disk resource id. Hope that's okay..

harshavmb · 2022-05-06T09:17:57Z

Thanks, just some minor comments i've left inline and this should be good!

Thank you, I've made changes as requested. Let me know if there is something I need to change

katbyte

Thanks @harshavmb ! LGTM 🦀

github-actions · 2022-05-17T05:49:48Z

This functionality has been released in v3.6.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2022-06-17T02:32:35Z

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.

New datasource azurerm_managed_disk_export for managed disks export

268cff5

github-actions bot added documentation size/L labels Feb 22, 2022

Formatting tf and website files to meet lint needs

2c87c9e

katbyte requested changes Feb 23, 2022

View reviewed changes

Adding azurerm_managed_disk_export_revoke data source to revoke expor…

77d0084

…t sas token post disk operations

github-actions bot added size/XL and removed size/L labels Feb 23, 2022

harshavmb changed the title ~~New datasource azurerm_managed_disk_export for managed disk exports~~ New datasources azurerm_managed_disk_export and azurerm_managed_disk_export_revoke for managed disk exports Feb 23, 2022

Adding err return value for diskRevokeFuture.WaitForCompletionRef func

c7c011d

harshavmb requested a review from katbyte February 23, 2022 11:39

Merge branch 'hashicorp:main' into main

0234202

tombuildsstuff suggested changes Feb 25, 2022

View reviewed changes

tombuildsstuff added the waiting-response label Feb 25, 2022

New resource azurerm_disk_export to manage disk exports instead of da…

da3b6f3

…ta sources

github-actions bot added size/L and removed size/XL labels Feb 25, 2022

harshavmb changed the title ~~New datasources azurerm_managed_disk_export and azurerm_managed_disk_export_revoke for managed disk exports~~ New resource azurerm_disk_export to manage disk exports Feb 25, 2022

harshavmb requested a review from tombuildsstuff February 25, 2022 18:32

github-actions bot removed the waiting-response label Feb 25, 2022

harshavmb added 2 commits February 28, 2022 08:53

Merge branch 'hashicorp:main' into main

0e7be9b

Merge branch 'hashicorp:main' into main

03653e5

This comment was marked as off-topic.

Sign in to view

tombuildsstuff suggested changes Apr 22, 2022

View reviewed changes

harshavmb added 2 commits April 22, 2022 19:16

Merge branch 'hashicorp:main' into main

a7cd2be

Renaming azurerm_disk_export to azurerm_managed_disk_sas_token as per…

3cfc731

… PR review & other changes

github-actions bot added size/XL and removed size/L labels Apr 24, 2022

Only min condition while validating duration_in_seconds & using impor…

9eb76d6

…t function as tests are failing

harshavmb requested a review from tombuildsstuff May 3, 2022 10:18

katbyte requested changes May 6, 2022

View reviewed changes

Changes as request in PR review

192f970

harshavmb requested a review from katbyte May 6, 2022 09:17

katbyte changed the title ~~New resource azurerm_disk_export to manage disk exports~~ New resource azurerm_managed_disk_sas_token to manage disk exports May 8, 2022

katbyte approved these changes May 9, 2022

View reviewed changes

katbyte merged commit 0b4c283 into hashicorp:main May 9, 2022

github-actions bot added this to the v3.6.0 milestone May 9, 2022

katbyte added a commit that referenced this pull request May 9, 2022

CHANGELOG #15558

c83ea1c

github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource azurerm_managed_disk_sas_token to manage disk exports #15558

New resource azurerm_managed_disk_sas_token to manage disk exports #15558

harshavmb commented Feb 22, 2022 •

edited

Loading

katbyte left a comment

harshavmb commented Feb 23, 2022

tombuildsstuff left a comment

harshavmb commented Feb 25, 2022

This comment was marked as off-topic.

tombuildsstuff left a comment

tombuildsstuff Apr 21, 2022

harshavmb Apr 22, 2022

katbyte left a comment

katbyte May 6, 2022

harshavmb May 6, 2022 •

edited

Loading

harshavmb commented May 6, 2022

katbyte left a comment

github-actions bot commented May 17, 2022

github-actions bot commented Jun 17, 2022

New resource azurerm_managed_disk_sas_token to manage disk exports #15558

New resource azurerm_managed_disk_sas_token to manage disk exports #15558

Conversation

harshavmb commented Feb 22, 2022 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

harshavmb commented Feb 23, 2022

tombuildsstuff left a comment

Choose a reason for hiding this comment

harshavmb commented Feb 25, 2022

This comment was marked as off-topic.

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Apr 21, 2022

Choose a reason for hiding this comment

harshavmb Apr 22, 2022

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

katbyte May 6, 2022

Choose a reason for hiding this comment

harshavmb May 6, 2022 • edited Loading

Choose a reason for hiding this comment

harshavmb commented May 6, 2022

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented May 17, 2022

github-actions bot commented Jun 17, 2022

harshavmb commented Feb 22, 2022 •

edited

Loading

harshavmb May 6, 2022 •

edited

Loading