Issue with [StorageSync] / API Version [2020-03-01]

[magodo]

Is there an existing issue for this?

• I have searched the existing issues

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Service Used

StorageSync

API Versions Used

2020-03-01

Description

Since migrating storage_sync resources to hashicorp/go-azure-sdk, the acctests for storage sync and the alikes keep failing, e.g.:

------- Stdout: -------
=== RUN   TestAccStorageSync_update
=== PAUSE TestAccStorageSync_update
=== CONT  TestAccStorageSync_update
    testcase.go:113: Step 5/6 error: Error running apply: exit status 1
        Error: creating Storage Sync Service (Subscription: "*******"
        Resource Group Name: "acctestRG-ss-230725005007008353"
        Storage Sync Service Name: "acctest-SS-230725005007008353"): polling after StorageSyncServicesCreate: `result.Status` was nil/empty - `op.Status` was "finishNewStorageSyncService" / `op.Properties.ProvisioningState` was ""
          with azurerm_storage_sync.test,
          on terraform_plugin_test.tf line 24, in resource "azurerm_storage_sync" "test":
          24: resource "azurerm_storage_sync" "test" {
--- FAIL: TestAccStorageSync_update (403.32s)
FAIL

The reason is as seen in the above error message, that the service API returns some unknown status, which doesn't belong to any of the status expected by longRunningOperationPoller:

https://github.com/magodo/terraform-provider-azurerm/blob/main/vendor/github.com/hashicorp/go-azure-sdk/sdk/client/resourcemanager/poller_lro.go#L152-L171.

In contrast, the Track1 SDK has below logic for polling:

https://github.com/magodo/terraform-provider-azurerm/blob/d6bc225707d78a51f7db65824bb84aab197aa536/vendor/github.com/Azure/go-autorest/autorest/azure/async.go#L570-L575

		if pt.resp.StatusCode == http.StatusAccepted {
			pt.State = operationInProgress
		} else if provStateApl {
			if ps := pt.getProvisioningState(); ps != nil {
				pt.State = *ps
			} else {
				pt.State = operationSucceeded
			}
		} else {
			return autorest.NewError("pollingTrackerBase", "updatePollingState", "the response from the async operation has an invalid status code")
		}

In this case, any unknown status will be regarded as succeeded as long as the response status code is successful. This somehow hides the issue, whilst I don't think it is the right way.

So my question is do I need to extend the "known" status for the longRunningOperationPoller to include the reported status?

The problem is that, from the test failures, so far we have following status for storage sync resource:

• finishNewStorageSyncService
• validateInput
• newPrivateDnsEntries

And we have following status for the storage sync cloud endpoint resource:

• newReplicaGroup

The swagger doesn't define the full list of status, where the status is defined as a plain string.

References

No response

[tombuildsstuff]

@magodo

So my question is do I need to extend the "known" status for the longRunningOperationPoller to include the reported status?

In short, yes - this list is added based on API responses: https://github.com/hashicorp/go-azure-sdk/blob/main/sdk/client/resourcemanager/poller_lro.go#L152-L171

In this case it's unfortunate that these aren't documented within the Swagger since they are in some of the other API's - newer API's limit the provisioningState values to 5 defined values (succeeded etc) and instead use another field for this "sub-status" (kusto/aks/hdinsight do this iirc).

FWIW we're intentionally not interpreting an unexpected value as pending to ensure we're handling these correctly/to avoid polling indefinitely - since these should all be caught by the acctests/during development of these resources - these unknown provisioningState's shouldn't be shipped to users (and whilst I can understand the legacy base layer using this approach, we're intentionally not doing that to ensure we're avoiding infinite polling issues etc).

Hope that helps?