Adding azurerm_dns_ds_record and azurerm_dns_tlsa_record

[alexwilcox9]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Adding two new resources and data sources for the creation of DS and TLSA DNS records
This includes a bump to the DNS API version which is currently using a local override until the newer API version is available in the Go SDK (waiting on hashicorp/pandora#4583)

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[stephybun]

Thanks for this PR @alexwilcox9!

A few things of note here:

• This PR contains too many changes, we will be able to review this quicker if you can split this PR out into it's constituent parts e.g. A PR for updating the hashicorp/go-azure-sdk, a PR for updating the version used by the dns service package, a PR for adding the new data source and resource
• We don't accept untyped resources or data sources in the provider anymore, there is a section in the contributor guidelines that explains what untyped and typed resources are. The guides on adding a new resource/data source are for typed resources and can help you out there

[alexwilcox9]

Hi @stephybun, thanks for the review

I appreciate there's a lot going on in this commit but as the SDK doesn't have the required API version yet I can't really split that out into it's own PR. Once the SDK does have the appropriate version I'll absolutely split the changes out and then rebase this branch. I have split all of the changes into sensible commits, it might be more digestable to review each commit on its own

I hadn't realised that only typed resources were being accepted, my apologies. And just a heads up, the documentation still advises not using typed resources

I've re-written the resources/data sources as suggested, I'd appreciate your feedback as it's my first time using the new typed method.

You'll also see another commit for ValidateRecordTypeID. The existing DNS record resources not only parse the ID on import to make sure it's the correct format but also check the record is the right type. I thought it would be a shame to lose that in the new typed setup so made this function. If this should be somewhere else do let me know

I also note that every DNS record resource has a set of common attributes/arguments (name, resource_group_name, zone_name, ttl, fqdn and tags). Would it be appropriate to have those common schema/model components set once in a helper package and then extended with the unique aspects?

[alexwilcox9]

An example of what a shared schema could look like:
alexwilcox9@3cd216f

[stephybun]

Thanks for rewriting these as typed resources @alexwilcox9 and for pointing out the outdated information in the sdk folder! I will open a PR to remove these since they're no longer valid.

Overall this looks good, I only reviewed the DS record data source and resource in depth since it looks like the same comments that I left for those apply to the TLSA record data source and resource. I'd appreciate if you could propagate any changes you make to the DS record to the TLSA record.

I also note that every DNS record resource has a set of common attributes/arguments (name, resource_group_name, zone_name, ttl, fqdn and tags). Would it be appropriate to have those common schema/model components set once in a helper package and then extended with the unique aspects?

The typed SDK offers the ability to have resources share a common schema. We have some implementations of this in the provider today e.g. the cost management export resources for billing, resource groups and subscription that all "inherit" parts of their schema from a base resource. This is not an insignificant amount of effort to setup and to really reap the benefits of this all the resources that share parts of a schema need to be typed resources, so in this particular case I'm not convinced it's worth doing for these two typed resources.

A helper function that can be referenced in multiple places might seem helpful, but I find these actually end up making reviews more difficult since the reviewer doesn't have all the information for the resource at first glance (having to scroll down to a different file to view a helper and relate it back to where it's supposed to be is a personal gripe that I and other's in the team have) and when any changes are made to it in future we need to be cognisant of each resource that it will affect which complicates reviews.