azurerm_machine_learning_datastore_*: support skip_validation property

[wuxu92]

This property is in the query string and used to skip server side access validation. This PR is to address some internal incident tickets.

swagger: https://github.com/Azure/azure-rest-api-specs/blob/30d711afc12628f35811e9f6eedeb7246e53264f/specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/stable/2023-04-01/mfe.json#L5948

--- PASS: TestAccMachineLearningDataStoreFileShare_accountKeySkipValidation (421.39s)
--- PASS: TestAccMachineLearningDataStoreDataLakeGen2_skipValidation (423.02s)
--- PASS: TestAccMachineLearningDataStoreBlobStorage_accountKeySkipValidation (428.67s)
PASS

[tombuildsstuff]

Since this is a QueryString option it's not returned from the API - meaning this'll flag as a diff at import time for users (which is being worked around in the tests through the ignore) - as such what's the benefit to making this user-configurable, rather than defaulting to true?

[wuxu92]

The flag should only be set in specific scenarios, such as when your workspace is in a virtual network. I'm uncertain if it is wise to have it set to true by default.

[mybayern1974]

A user is reporting issues failing to create some Machine Learning resources, and the RP team's recommendation is users can notify RP not to do any validation to address that, which can be done by setting this param in the query string. So RP team expects TF to expose that option to users.

[tombuildsstuff]

@mybayern1974 @wuxu92

Whilst I understand the Service Team have requested that we expose this field on the resource, this would run contrary to other resources in the provider - however from your description this sounds like an API bug, so I'm wondering if there's an issue tracking that?

From Terraform's side we disable validation where it's problematic (example) and don't expose additional configuration values for this as it's an implementation detail (how do users know when to enable it vs when not too, so why not enable it all the time?).

Given this field is being used to workaround a bug in the API (and not control a behavioural feature), we shouldn't expose a user-configurable flag for this field - either on the resource or within the features block - and should instead hard-code this to true where it's needed within the Provider (at least until the API issue is resolved). This allows the resource to "just work" for users, rather than requiring users to have an understanding of how the API works internally - and so when this field should/shouldn't be set.

As such, can we remove this toggle on the resource and hard-code this to true - and also add a comment with a summary of/link to the issue so we can track when this is fixed?

[tawan0109]

@tombuildsstuff thanks for reviewing this PR. My team owns the API mentioned in this PR and we are working with @mybayern1974 and @wuxu92 to add this "skipValidation" support.

I want to clarify that "skipValidation" is not a workaround for a bug, it's a feature that Azure Machine Learning (AzureML) customers need. When receiving the request of creating new datastore AzureML service will try to validate the request by accessing the underlying storage account, and reject the request if the validation fails. However, for some scenarios (eg. datastore with network isolation) technically it's impossible for AzureML service to access underlying storage account and validation might fail. In these cases customer needs to set skipValidation=True to bypass validation so request of creating new datastore can succeed.

Hope above can explain why we need to create this PR. Please let me know if you have any other questions.

[tombuildsstuff]

@tawan0109 thanks for the additional context, but the guidance above remains valid here - this shouldn't be exposed as a user configurable toggle in Terraform and this should be hard-coded to true.

Whilst I understand the scenario here, since we're referencing the Storage Account ID within the Machine Learning resource, in Terraform that's almost certainly an interpolated value, meaning that the Storage Account is virtually guaranteed to be created before the Machine Learning Workspace/Data Stores etc.

As such whilst I can understand why the Azure API/CLI/ARM Templates are exposing this field - Terraform's dependency graph virtually guarantees that the Storage Account ID being specified for the Machine Learning resources exists - as such from our side this ends up being an implementation detail, and thus shouldn't be exposed as a user configurable toggle, and should instead be hard-coded to true.

As such @wuxu92 can you remove this field from the resource and hard-code this field, as requested above?

Thanks!

[tawan0109]

@tombuildsstuff understand that Terraform's dependency graph can guarantees storage account exists, please note that credentials of storage account are also included in the request of creating datastore, besides checking the existence of storage account, service also needs to validate if the credentials are correct and paths on storage account exist.

By default "skipValidation" is set to false on service side which means if customer does not specify this field in Terraform then validation always happen, which is what we want. As I mentioned above, in some cases validation cannot be applied so customer needs to have an option to skip the validation.

[tombuildsstuff]

@tawan0109

Whilst I understand that, unfortunately this still isn't something we would expose as a user-configurable value - and as such this needs to be hard-coded to true.

There's both behavioural reasons for this (this would differ from other resources in the Provider [example]) and technical reasons for this (for example this would cause issues via import, as is being ignored in the tests here, which would be problematic for users).

It's worth noting that any requirements should be listed in the documentation for the Service, so (presuming those are documented) we can link to that as required from the documentation, as we do elsewhere.

[wuxu92]

Thanks @tawan0109 's investigation, I have updated this PR to hard-coded skip_validation parmeter to true. Thanks a lot to @tombuildsstuff for the great review!

[stephybun]

Thanks @wuxu92 LGTM 👍

[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.