azurerm_dns_cname_record: try normalize target_resource_id with RecordTypeID before set to state

[wuxu92]

fixes: #24138.

There is a nother API casing issue related to this resource: Azure/azure-rest-api-specs#6641 so change the acctest resource group name to lower case to make the acc test work.

[stephybun]

We're only calling this once so it doesn't need to be a separate function. Can you please in-line the ID parsing above. We should also be raising the error from the parser instead of silently ignoring.

[wuxu92]

Sure! Updated.

[stephybun]

@wuxu92 We still need to return the error here since we're expecting a record type ID. If the service suddenly begins returning an ID of a completely different type we would never know.

Suggested change

	if parsedID, err := recordsets.ParseRecordTypeIDInsensitively(targetResourceId); err == nil {
	targetResourceId = parsedID.ID()
	}
	parsedID, err := recordsets.ParseRecordTypeIDInsensitively(targetResourceId)
	if err != nil {
	return err
	}
	targetResourceId = parsedID.ID()

[wuxu92]

There is no document stating that the target resource ID must be a DNS record type ID. As far as I understand, it can refer to other Azure resources.

[jackofallops]

Hi @wuxu92 - I don't think this is the correct way to handle the reported issue. The target ref is intended for only a handful of resource types:

I think the user should be using record not target_resource_id in the configuration? Can you try a repro that way to see if the issue is resolved that way? Since the record property is supposed to be another DNS record, this would be a more stable place to work with any casing mitigations if they are still required.

[wuxu92]

Thanks @jackofallops I have repro with record field and it works well! FWIW should we still normalize the target_resource_id as we can with all these possible resource types?

[stephybun]

Thanks @wuxu92. This is almost good to go, could you please take a look at the latest comment?

[stephybun]

We're still silently ignoring the case where the returned ID is none of those resource types. We should error if the resource type isn't any of these.

[wuxu92]

If the resource type is not listed, we should directly use the ID from the API response. This could indicate an API bug or a new resource type supported by the API. Neither of these scenarios should disrupt client behavior, as far as I understand it.

[stephybun]

I needed to check internally but you're right @wuxu92, apologies!

I did push a commit to add a comment above this logic linking to a piece of work we're doing on dynamic resource IDs - once that functionality is available we should be able to update this.

[tombuildsstuff]

x-ref to the recaser in: hashicorp/go-azure-helpers#189

[stephybun]

Thanks @wuxu92 LGTM 👍

[stephybun]

I needed to check internally but you're right @wuxu92, apologies!

I did push a commit to add a comment above this logic linking to a piece of work we're doing on dynamic resource IDs - once that functionality is available we should be able to update this.

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