storage: upgrade giovanni SDK and support AAD auth #24798

manicminer · 2024-02-06T21:33:29Z

Storage Data Plane upgrades and support for AAD authentication

Use latest tombuildsstuff/giovanni which has switched to the go-azure-sdk base layer
Support for sdk-level retries for known eventually consistent scenarios
Support for AAD authentication for all data plane APIs (where the API supports it).
- azurerm_storage_share and azurerm_storage_table will continue to use shared key authentication due to API limitations (official docs)
Refactor data plane client helpers to be operation-aware, so that supported authentication methods can be determined not just
by endpoint but also by the capabilities of the operation (enables more granular support for preferred authentication methods).
Adopt data plane resource IDs from tombuildsstuff/giovanni- Some tidying of provider package - move helper functions into own source file
Some tidying of services/storage/client package - move data plane client helpers into own source file and add support for parsing out a storage account endpoint for constructing a data plane ID
internal/common: adopt client.BaseClient interface from go-azure-sdk to support more SDK base clients
internal/provider: move helper functions into own source file

CHANGELOG

dependencies: updating to v0.25.1 of github.com/tombuildsstuff/giovanni
data.azurerm_storage_table_entities - support for AAD authentication
data.azurerm_storage_table_entity - support for AAD authentication
azurerm_storage_share_file - support for AAD authentication
azurerm_storage_share_directory - support for AAD authentication, deprecate share_name and storage_account_name in favor of storage_share_id
azurerm_storage_table_entity - support for AAD authentication, deprecate share_name and storage_account_name in favor of storage_table_id
azurerm_storage_table_entity - support for AAD authentication

Depends on:

Closes: #22467

katbyte · 2024-02-16T01:08:58Z

does this close #24213? and unblock #22583?

manicminer · 2024-02-19T11:08:03Z

@katbyte Yes and yes 👍

tombuildsstuff

Left some comments from a partial review inline

tombuildsstuff · 2024-03-12T16:42:38Z

internal/services/containers/kubernetes_flux_configuration_resource.go

@@ -244,7 +243,7 @@ func (r KubernetesFluxConfigurationResource) Arguments() map[string]*pluginsdk.S
 					"container_id": {
 						Type:         pluginsdk.TypeString,
 						Required:     true,
-						ValidateFunc: storageValidate.StorageContainerDataPlaneID,
+						ValidateFunc: validation.IsURLWithPath, // note: storage domain suffix cannot be determined at validation time, so just make sure it's a well-formed URL


Whilst we don't have access to the Environment that doesn't mean we couldn't necessarily do an opt-in enhanced validation here? e.g. expose a global variable for StorageDomainSuffix that's set in a similar manner as we do for Locations and then conditionally validate the domain name, else just check the Path matches?

I couldn't see a race-free way to achieve this, since the environment (cloud) can be set in provider configuration as well as via environment variable - building the provider also occurs post-validation so we still can't determine the appropriate domain?

Seems to work with populating a global variable! Validation function is updated accordingly :)

tombuildsstuff · 2024-03-12T16:43:44Z

internal/services/containers/kubernetes_flux_configuration_resource.go

-		output.ContainerName = &id.Name
-		output.Url = pointer.To(strings.TrimSuffix(input.ContainerID, "/"+id.Name))
+		output.ContainerName = &id.ContainerName
+		output.Url = pointer.To(strings.TrimSuffix(input.ContainerID, "/"+id.ContainerName))


perhaps pedantic but couldn't we make this:

Suggested change

output.Url = pointer.To(strings.TrimSuffix(input.ContainerID, "/"+id.ContainerName))

output.Url = pointer.To(fmt.Sprintf("https://%s", id.Domain))

(or add a function to return this value?)

That'd allow the usage of this Resource ID to be better highlighted, to help us determine the impact of a change?

Yeah this can be a lot better. I've reworked so that presumptions about the URL structure are removed from the resource and instead it leans on the ID() method of the underlying AccountId (in both the flatten and expand functions).

tombuildsstuff · 2024-03-12T16:45:58Z

internal/services/storage/client/data_plane.go

+	}
+}
+
+func (client Client) ConfigureDataPlane(ctx context.Context, baseUri, clientName string, baseClient client.BaseClient, account accountDetails, operation DataPlaneOperation) error {


does this need to be public/exported?

It does not 👍

katbyte

Aside from a couple comments and the ones Tom left this LGTM 💾

katbyte · 2024-03-12T21:16:48Z

internal/services/storage/storage_data_lake_gen2_filesystem_resource.go

 	if err != nil {
-		return fmt.Errorf("parsing ace list: %s", err)
+		return fmt.Errorf("retrieving Account %q for Data Lake Filesystem %q: %v", accountResourceManagerId.StorageAccountName, filesystemName, err)


should these all be noted as Gen2?

Suggested change

return fmt.Errorf("retrieving Account %q for Data Lake Filesystem %q: %v", accountResourceManagerId.StorageAccountName, filesystemName, err)

return fmt.Errorf("retrieving Account %q for Data Lake Gen2 Filesystem %q: %v", accountResourceManagerId.StorageAccountName, filesystemName, err)

katbyte · 2024-03-12T21:17:14Z

internal/services/storage/storage_data_lake_gen2_filesystem_resource.go

-		if utils.ResponseWasNotFound(storageAccount.Response) {
-			return fmt.Errorf("%s was not found", storageID)
-		}
+		return fmt.Errorf("building Data Lake Filesystems Client: %v", err)


etc

Suggested change

return fmt.Errorf("building Data Lake Filesystems Client: %v", err)

return fmt.Errorf("building Data Lake Gen2 Filesystems Client: %v", err)

…ux configuration resources

…ctions

manicminer · 2024-03-14T11:02:10Z

Latest test results

<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.95.0" to "3.96.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.96.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.96.0
ENHANCEMENTS:

* dependencies: updating to `v0.20240314.1083835` of `github.com/hashicorp/go-azure-sdk` ([#25255](https://github.com/hashicorp/terraform-provider-azurerm/issues/25255))
* dependencies: updating to `v0.25.1` of `github.com/tombuildsstuff/giovanni` ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* dependencies: updating to `v1.33.0` of `google.golang.org/protobuf` ([#25243](https://github.com/hashicorp/terraform-provider-azurerm/issues/25243))
* `storage`: updating the data plane resources to use the transport layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* Data Source: `azurerm_storage_table_entities` - support for AAD authentication ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* Data Source: `azurerm_storage_table_entity` - support for AAD authentication ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* `azurerm_kusto_cluster` - support `None` pattern for the `virtual_network_configuration` block ([#24733](https://github.com/hashicorp/terraform-provider-azurerm/issues/24733))
* `azurerm_linux_function_app` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))
* `azurerm_linux_function_app_slot` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))
* `azurerm_stack_hci_cluster` - support the `identity`, `cloud_id`, `service_endpoint` and `resource_provider_object_id` properties [GH-25031]
* `azurerm_storage_share_file` - support for AAD authentication ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* `azurerm_storage_share_directory` - support for AAD authentication, deprecate `share_name` and `storage_account_name` in favor of `storage_share_id` ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* `azurerm_storage_table_entity` - support for AAD authentication, deprecate `share_name` and `storage_account_name` in favor of `storage_table_id` ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* `azurerm_storage_table_entity` - support for AAD authentication ([#24798](https://github.com/hashicorp/terraform-provider-azurerm/issues/24798))
* `azurerm_windows_function_app` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))
* `azurerm_windows_function_app_slot` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))
* `azurerm_windows_web_app` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))
* `azurerm_windows_web_app_slot` - support for the Node `20` runtime ([#24073](https://github.com/hashicorp/terraform-provider-azurerm/issues/24073))

BUG FIXES:

* `azurerm_container_app_custom_domain` - fix resource ID parsing bug preventing import ([#25192](https://github.com/hashicorp/terraform-provider-azurerm/issues/25192))
* `azurerm_windows_web_app` - fix incorrect warning message when checking name availability ([#25214](https://github.com/hashicorp/terraform-provider-azurerm/issues/25214))
* `azurerm_virtual_machine_run_command` - prevent a bug during updates ([#25186](https://github.com/hashicorp/terraform-provider-azurerm/issues/25186))
* Data Source: `azurerm_storage_table_entities` - Fix `items.x.properties` truncating to one entry ([#25211](https://github.com/hashicorp/terraform-provider-azurerm/issues/25211))</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/52/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>

github-actions · 2024-04-18T02:06:51Z

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.

github-actions bot added dependencies service/storage state-migration size/XL labels Feb 6, 2024

manicminer force-pushed the feature/storage-aad-auth branch from 5f859e4 to d61675f Compare February 20, 2024 12:32

github-actions bot added the documentation label Feb 20, 2024

manicminer force-pushed the feature/storage-aad-auth branch 3 times, most recently from ab0d29a to 82a7b7c Compare February 20, 2024 12:46

github-actions bot added the waiting-response label Feb 20, 2024

manicminer force-pushed the feature/storage-aad-auth branch from 82a7b7c to b76ce67 Compare February 20, 2024 12:51

github-actions bot added waiting-response and removed waiting-response labels Feb 20, 2024

manicminer force-pushed the feature/storage-aad-auth branch from b76ce67 to 256ec27 Compare February 20, 2024 13:25

github-actions bot added waiting-response and removed waiting-response labels Feb 20, 2024

manicminer force-pushed the feature/storage-aad-auth branch from 256ec27 to d238691 Compare February 20, 2024 13:30

manicminer marked this pull request as ready for review February 20, 2024 13:30

github-actions bot added waiting-response and removed waiting-response labels Feb 20, 2024

katbyte mentioned this pull request Feb 20, 2024

internal: Upgrade giovanni to 0.22.0 #24213

Closed

manicminer force-pushed the feature/storage-aad-auth branch from d238691 to e01fadf Compare February 24, 2024 02:08

github-actions bot removed the waiting-response label Feb 24, 2024

manicminer force-pushed the feature/storage-aad-auth branch from e01fadf to 170d597 Compare February 24, 2024 02:09

github-actions bot added the waiting-response label Feb 24, 2024

manicminer force-pushed the feature/storage-aad-auth branch from 170d597 to 28a803d Compare February 24, 2024 02:14

github-actions bot added waiting-response and removed waiting-response labels Feb 24, 2024

github-actions bot added waiting-response and removed waiting-response labels Mar 1, 2024

azurerm_virtual_machine: ensure testcheck func context has a deadline

049c93d

github-actions bot added waiting-response and removed waiting-response labels Mar 1, 2024

tombuildsstuff reviewed Mar 12, 2024

View reviewed changes

katbyte approved these changes Mar 12, 2024

View reviewed changes

manicminer added 2 commits March 13, 2024 11:08

Merge branch 'main' into feature/storage-aad-auth

b63ceb0

address review, improve storage account URL handling in kubernetes fl…

e3f0cae

…ux configuration resources

github-actions bot added waiting-response and removed waiting-response labels Mar 13, 2024

linting

a7b8108

github-actions bot added waiting-response and removed waiting-response labels Mar 13, 2024

populate global variable for storage domain, to use in validation fun…

3d4058d

…ctions

github-actions bot added waiting-response and removed waiting-response labels Mar 13, 2024

github-actions bot removed the waiting-response label Mar 14, 2024

Merge branch 'main' into feature/storage-aad-auth

ccdb7fa

manicminer merged commit 5c9b4dd into main Mar 14, 2024
28 of 29 checks passed

manicminer deleted the feature/storage-aad-auth branch March 14, 2024 11:03

github-actions bot added the waiting-response label Mar 14, 2024

github-actions bot added this to the v3.96.0 milestone Mar 14, 2024

manicminer added a commit that referenced this pull request Mar 14, 2024

Changelog for #24798

db7448a

reastyn pushed a commit to reastyn/terraform-provider-azurerm that referenced this pull request Mar 14, 2024

Changelog for hashicorp#24798

761054d

NickG158 mentioned this pull request Mar 15, 2024

Unable to use the data source azurerm_storage_table_entity #25267

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: upgrade giovanni SDK and support AAD auth #24798

storage: upgrade giovanni SDK and support AAD auth #24798

manicminer commented Feb 6, 2024 •

edited

Loading

katbyte commented Feb 16, 2024

manicminer commented Feb 19, 2024

tombuildsstuff left a comment

tombuildsstuff Mar 12, 2024

manicminer Mar 13, 2024 •

edited

Loading

manicminer Mar 13, 2024

tombuildsstuff Mar 12, 2024

manicminer Mar 13, 2024

tombuildsstuff Mar 12, 2024

manicminer Mar 13, 2024

katbyte left a comment

katbyte Mar 12, 2024

katbyte Mar 12, 2024

manicminer commented Mar 14, 2024

github-actions bot commented Apr 18, 2024

	output.Url = pointer.To(strings.TrimSuffix(input.ContainerID, "/"+id.ContainerName))
	output.Url = pointer.To(fmt.Sprintf("https://%s", id.Domain))

	return fmt.Errorf("retrieving Account %q for Data Lake Filesystem %q: %v", accountResourceManagerId.StorageAccountName, filesystemName, err)
	return fmt.Errorf("retrieving Account %q for Data Lake Gen2 Filesystem %q: %v", accountResourceManagerId.StorageAccountName, filesystemName, err)

	return fmt.Errorf("building Data Lake Filesystems Client: %v", err)
	return fmt.Errorf("building Data Lake Gen2 Filesystems Client: %v", err)

storage: upgrade giovanni SDK and support AAD auth #24798

storage: upgrade giovanni SDK and support AAD auth #24798

Conversation

manicminer commented Feb 6, 2024 • edited Loading

Storage Data Plane upgrades and support for AAD authentication

katbyte commented Feb 16, 2024

manicminer commented Feb 19, 2024

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manicminer Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manicminer commented Mar 14, 2024

github-actions bot commented Apr 18, 2024

manicminer commented Feb 6, 2024 •

edited

Loading

manicminer Mar 13, 2024 •

edited

Loading