Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new resource: azurerm_private_dns_resolver #18473

Merged
merged 11 commits into from
Oct 22, 2022

Conversation

ms-henglu
Copy link
Contributor

@ms-henglu ms-henglu force-pushed the issue-16956-support-dns-resolver branch from 4037610 to f191943 Compare September 21, 2022 05:25
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may need to configure what regions to run this service in:

Test Failed

------- Stdout: -------
=== RUN   TestAccDNSResolverDnsResolver_requiresImport
=== PAUSE TestAccDNSResolverDnsResolver_requiresImport
=== CONT  TestAccDNSResolverDnsResolver_requiresImport
=== CONT  TestAccDNSResolverDnsResolver_requiresImport
    testcase.go:110: Step 1/2 error: Error running apply: exit status 1
        
        Error: creating Dns Resolver (Subscription: "*******"
        Resource Group Name: "acctest-rg-220922165907733233"
        Dns Resolver Name: "acctest-dr-220922165907733233"): performing CreateOrUpdate: dnsresolvers.DnsResolversClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="BadRequest" Message="The provided location 'westus2' is not available for Microsoft.Network/dnsResolvers and Microsoft.Network/dnsForwardingRulesets resources." Target="" Details=[{}] InnerError={}
        
          with azurerm_dns_resolver.test,
          on terraform_plugin_test.tf line 20, in resource "azurerm_dns_resolver" "test":
          20: resource "azurerm_dns_resolver" "test" {
        
        creating Dns Resolver (Subscription: "*******"
        Resource Group Name: "acctest-rg-220922165907733233"
        Dns Resolver Name: "acctest-dr-220922165907733233"): performing
        CreateOrUpdate: dnsresolvers.DnsResolversClient#CreateOrUpdate: Failure
        sending request: StatusCode=0 -- Original Error: Code="BadRequest"
        Message="The provided location 'westus2' is not available for
        Microsoft.Network/dnsResolvers and Microsoft.Network/dnsForwardingRulesets
        resources." Target="" Details=[{}] InnerError={}
--- FAIL: TestAccDNSResolverDnsResolver_requiresImport (70.39s)

@ms-henglu
Copy link
Contributor Author

Hi @katbyte , maybe disable the ARM_PROVIDER_DYNAMIC_TEST ?

@katbyte
Copy link
Collaborator

katbyte commented Sep 27, 2022

No, we can configure this service to only run in certain regions grep for:

// Server is only available in certain locations
        "analysisservices" to testConfiguration(locationOverride = LocationConfiguration("westus", "northeurope", "southcentralus", true)),

@ms-henglu
Copy link
Contributor Author

Hi @katbyte , thanks for the suggestion, I've added it into test configs. Please take another look, thanks!

@sebader
Copy link
Contributor

sebader commented Sep 30, 2022

The service itself is very specifically called "Azure DNS Private Resolver" (not just DNS resolver). I'd highly recommend to align the name of the terraform resource, too. Or is there a reason to only call it azurerm_dns_resolver?

https://learn.microsoft.com/en-us/azure/dns/dns-private-resolver-overview

@ms-henglu
Copy link
Contributor Author

Hi @sebader , thanks for the suggestion! I've confirmed with the service's PM, he prefers azurerm_dns_resolver, which aligns with azure cli and powershell.

@tombuildsstuff
Copy link
Contributor

@ms-henglu are there plans for this service to also support the Public DNS resources? If not this'd make more sense as Private DNS since it's scoped at that?

@katbyte
Copy link
Collaborator

katbyte commented Oct 3, 2022

@ms-henglu - i agree with @sebader, we should match the correct name and change this to azurerm_dns_private_resolver

@sfiguemsft
Copy link

@katbyte @sebader we're looking for consistency with naming which is already in place for PSH and CLI clients

@ms-henglu
Copy link
Contributor Author

Hi @katbyte , WDYT about

we're looking for consistency with naming which is already in place for PSH and CLI clients

@katbyte
Copy link
Collaborator

katbyte commented Oct 13, 2022

@ms-henglu - are there plans to support public DNS with this service? because if not this the name is misleading and conflicts with the existing nomenclature in the provider with azurerm_private_dns_* for private DNS, and azurerm_dns for public DNS

@sfiguemsft
Copy link

@ms-henglu - are there plans to support public DNS with this service? because if not this the name is misleading and conflicts with the existing nomenclature in the provider with azurerm_private_dns_* for private DNS, and azurerm_dns for public DNS

@katbyte yes, public version will still be under this same service

@katbyte katbyte self-assigned this Oct 18, 2022
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both - with public going into the same service this LGTM now 🙌

@JannoTjarks
Copy link

Thanks both - with public going into the same service this LGTM now 🙌

Hi @katbyte, I think there is a misunderstanding for this resource.
I think, there will never be something like a azure public dns resolver, because there is no need.
Microsoft has two possible dns zones: Public DNS und Private DNS.
Public DNS can be resolved from the internet.
Private DNS Zones can only be resolved from a virtual network in your specific azure tenant. So if you want to resolve a private dns record from your on-premise network, you need a dns resolver in azure for the private dns records.
The Azure Private DNS Resolver is Microsoft SaaS Solution for this.

As I mentioned, public dns can resolved from everyone from anywhere. There is no SaaS from Microsoft needed.

So I would recommend to use the correct name azure dns private resolver.

@desweil
Copy link

desweil commented Oct 19, 2022

@JannoTjarks It is probably true that there never will be a "public resolver SAAS Service".

Still I think the name is fine, because it is "THE" Azure DNS Resolver and it already is named like this in PSH (see for example) and AZ CLI (see for example) documentation from MS.

@sfiguemsft
Copy link

Thanks both - with public going into the same service this LGTM now 🙌

Hi @katbyte, I think there is a misunderstanding for this resource. I think, there will never be something like a azure public dns resolver, because there is no need. Microsoft has two possible dns zones: Public DNS und Private DNS. Public DNS can be resolved from the internet. Private DNS Zones can only be resolved from a virtual network in your specific azure tenant. So if you want to resolve a private dns record from your on-premise network, you need a dns resolver in azure for the private dns records. The Azure Private DNS Resolver is Microsoft SaaS Solution for this.

As I mentioned, public dns can resolved from everyone from anywhere. There is no SaaS from Microsoft needed.

So I would recommend to use the correct name azure dns private resolver.

@JannoTjarks as product owner of the services mentioned above, I prefer not to disclose any information on this PR related to roadmap and/or directional strategy of Azure DNS. Lets keep consistency with CLI/PSH were possible given that this is the only DNS resolver service from Azure

@JannoTjarks
Copy link

Hi @sfiguemsft thanks for the clarification 👍🏻 I understand that the product owner has more insights :D

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on responses here and how the naming is in the portal/docs, the name should not be azurerm_dns_resolver and instead be azurerm_private_dns_resolver. We should keep it consistent with the other dns resources in Terraform and not psh and cli.

@sfiguemsft
Copy link

Based on responses here and how the naming is in the portal/docs, the name should not be azurerm_dns_resolver and instead be azurerm_dns_private_resolver. We should keep it consistent with the other dns resources in Terraform and not psh and cli.

@mbfrahry I'm asking the name to be azurerm_dns_resolver and this is what I would like customers to see in the TF modules - if this needs to be formalized through other channel please let me know.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfiguemsft - given this is for private DNS only i also agree this should be azurerm_private_dns_resolver - our resources names often do not 1:1 match service names and generally are chosen to create the least confusion for our users. Primarily we look to the portal for guidance on what to pick as that is where most people will be using the service/coming to terraform from. Given the existing private dns resources all start azurerm_private_dns_* and that the portal also calls it Private resolver users are going to expect/look for "azure private resolver" not "azure dns resolver"

Because of this i concur with everyone else here the name should be azurerm_private_dns_resolver to match the portal's chosen name for these resources and match existing resource in terraform. As long as that is its name in the portal (our primary reference for names - not APIs) i don't see us being able to call it something else.

@ms-henglu can we update the name as requested to at the very least get this in for users? we can always add a resource alias later on for a different name if required

.github/labeler-pull-request-triage.yml Outdated Show resolved Hide resolved
.teamcity/components/generated/services.kt Outdated Show resolved Hide resolved
.teamcity/components/settings.kt Outdated Show resolved Hide resolved
internal/clients/client.go Outdated Show resolved Hide resolved
internal/clients/client.go Outdated Show resolved Hide resolved
internal/services/dnsresolver/client/client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ms-henglu - LGTM 🧮

@katbyte katbyte changed the title new resource: azurerm_dns_resolver new resource: azurerm_private_dns_resolver Oct 22, 2022
@katbyte katbyte merged commit 74361ca into hashicorp:main Oct 22, 2022
katbyte added a commit that referenced this pull request Oct 22, 2022
@github-actions github-actions bot added this to the v3.28.0 milestone Oct 22, 2022
@github-actions
Copy link

This functionality has been released in v3.28.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@DevopsMercenary
Copy link

Don't see any docs in v3.28.0 for azurerm_private_dns_resolver nor in the release notes.

@ms-henglu
Copy link
Contributor Author

Hi @DevopsMercenary ,

I guess it was labeled v3.28.0 by mistake, but these resources have been released in v3.29.0, please check: https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.29.0

@github-actions
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants