azurerm_firewall - dns_settings block to fix dns_servers, dns_proxy_enabled logic

[wuxu92]

resolves: #20500 , resolves: #9184, resolves: #12084

[github-actions]

Build failure

Found new usages of deprecated functionality:

https://github.com/hashicorp/terraform-provider-azurerm/actions/runs/4290183876

[katbyte]

we could just move this to the to level instead of creating a new block?

[wuxu92]

Thanks for your suggestion. i have tried to move it to the top level, but then it is not compatiable with what Portal does. for portal there is a separate switch to turn off/on the DNS configurations:

Before Turn On

After Turn On

[katbyte]

i don't understand why its not compatible ? you can leave the existing servers property and then add a dns_proxy_enabled property instead of creating a new block?

[wuxu92]

with a top-level dns_proxy_enabled property, no matter the value is true or false, the DNS settings will be Enabled. then how can we set the DNS Setting to Disabled?

[katbyte]

we are already passing in the custom servers?

i don't follow how it will be a problem to have it outside the block vs in? the radio option "enabling disabling" it in the portal appears to be purely cosmetic

[wuxu92]

here we set the dns_proxy_enabled as top level:

	"dns_servers": {
		Type:     pluginsdk.TypeList,
		Optional: true,
		MinItems: 1,
		Elem: &pluginsdk.Schema{
			Type:         pluginsdk.TypeString,
			ValidateFunc: validation.IsIPAddress,
		},
	},

	"dns_proxy_enabled": {
		Type:     pluginsdk.TypeBool,
		Optional: true,
		Computed: true,
	},

then with tf configuration like below will also Enable the DNS Settings and set the DNS Proxy to Disabled. it may action just the same as not enable DNS settings. but i guess it makes a diff between terraform-provider and portal and it could lead to issues down the line.

resource "azurerm_firewall" "test" {
  name                = "acctestfirewall"
  location            = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
  sku_name            = "AZFW_VNet"
  sku_tier            = "Standard"
}

i have actually implemented the desing of make dns_proxy_enabled as a top-level and have tested that it works for current version. i can push it if you are ok with it.

[katbyte]

What "enables dns settings" in the api? from the screenshot all it seems to do is allow you to configure those two options?

[wuxu92]

there is no specific field in the API to enable DNS Settings, but just set fields to a map as field AdditionalProperties as below:

AdditionalProperties["Network.DNS.EnableProxy"] = "true"
AdditionalProperties["Network.DNS.Servers"] = "1.1.1.1,2.2.2.2"

from the screenshot below and API requests in the Portal, when Disabled selected, we should not set the AdditionalProperties fields. but as previous discusstion as top-level dns_proxy_enabled property, we'll always set the AdditionalProperties["Network.DNS.EnableProxy"] field.

code of how AdditionalProperties is set:
https://github.com/hashicorp/terraform-provider-azurerm/pull/20519/files#diff-9b29e7d553e1b06cad14665d2ee767d5ffeefad32821d783e74f7372fbcde3d0R861-R865

[katbyte]

what you can do is only set it if the proxy is enabled

[wuxu92]

in my test on Azure Portal, if only set AdditionalProperties["Network.DNS.EnableProxy"] = "true" it will just set the proxy is enabled (which means DNS Servers use the Default (Azure provided)) and this is what #9184 want.

[katbyte]

test failures:

------- Stdout: -------
=== RUN   TestAccFirewall_privateRanges
=== PAUSE TestAccFirewall_privateRanges
=== CONT  TestAccFirewall_privateRanges
    testcase.go:110: Step 6/6 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
        
          map[string]string{
        - 	"dns_proxy_enabled": "false",
          }
--- FAIL: TestAccFirewall_privateRanges (1179.90s)
FAIL

<div class="rightBlock expandedDetails" style="float: right; background-color: rgba(245, 245, 245, 0.81); border-left: 5px solid rgb(255, 255, 255); border-bottom: 5px solid rgb(255, 255, 255); color: rgb(31, 35, 38); font-family: system-ui, -apple-system, &quot;system-ui&quot;, &quot;Segoe UI&quot;, Roboto, Oxygen, Ubuntu, Cantarell, &quot;Droid Sans&quot;, &quot;Helvetica Neue&quot;, Arial, sans-serif; font-size: 13px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;"><div class="relatedBuildsWrapper" style="border-right: 3px solid white; float: right;"><br class="Apple-interchange-newline">

  | First failure: | refs/pull/20519/merge | #752 | Changes (10) | 06 Apr 23 16:35
-- | -- | -- | -- | -- | --


<div id="div_iaa_187751c0fbb"></div></div></div><div class="testMetadata" id="testMetadata_tdi_187751c0fb9" style="color: rgb(31, 35, 38); font-family: system-ui, -apple-system, &quot;system-ui&quot;, &quot;Segoe UI&quot;, Roboto, Oxygen, Ubuntu, Cantarell, &quot;Droid Sans&quot;, &quot;Helvetica Neue&quot;, Arial, sans-serif; font-size: 13px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;"></div><pre class="fullStacktrace" id="fullStacktrace_388440_2930815752523330276" style="font: 400 12px / 1.2em Menlo, &quot;Bitstream Vera Sans Mono&quot;, &quot;Ubuntu Mono&quot;, Consolas, &quot;Courier New&quot;, Courier, monospace; padding: 3px 3px 0px; color: darkred; margin-top: 0.5em; margin-bottom: 0.5em; white-space: pre-wrap; overflow-wrap: break-word; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;">------- Stdout: -------
=== RUN   TestAccFirewallNetworkRuleCollection_fqdns
=== PAUSE TestAccFirewallNetworkRuleCollection_fqdns
=== CONT  TestAccFirewallNetworkRuleCollection_fqdns
    testcase.go:110: Step 1/2 error: Error running apply: exit status 1
        
        Error: waiting for creation/update of Network Rule Collection "acctestnrc" of Firewall "acctestfirewall230406170629416797" (Resource Group "acctestRG-230406170629416797"): Code="AzureFirewallDNSProxyMustBeEnabled" Message="DNS Proxy must be enabled in order to use DestinationFqdns in Network Rules. See rule rule1 in network rule collection acctestnrc" Details=[]
        
          with azurerm_firewall_network_rule_collection.test,
          on terraform_plugin_test.tf line 63, in resource "azurerm_firewall_network_rule_collection" "test":
          63: resource "azurerm_firewall_network_rule_collection" "test" {
        
--- FAIL: TestAccFirewallNetworkRuleCollection_fqdns (671.31s)
FAIL</pre>First failure: 	refs/pull/20519/merge	[#752 ](https://ci-oss.hashicorp.engineering/viewLog.html?buildId=388440&tab=buildResultsDiv&buildTypeId=TerraformOpenSource_TerraformProviders_AzureRMPublic_AZURERM_SERVICE_PUBLIC_FIREWALL) 	[Changes (10)](https://ci-oss.hashicorp.engineering/viewLog.html?buildId=388440&tab=buildChangesDiv&buildTypeId=TerraformOpenSource_TerraformProviders_AzureRMPublic_AZURERM_SERVICE_PUBLIC_FIREWALL) 	06 Apr 23 16:35
------- Stdout: -------
=== RUN   TestAccFirewallNetworkRuleCollection_fqdns
=== PAUSE TestAccFirewallNetworkRuleCollection_fqdns
=== CONT  TestAccFirewallNetworkRuleCollection_fqdns
    testcase.go:110: Step 1/2 error: Error running apply: exit status 1
        
        Error: waiting for creation/update of Network Rule Collection "acctestnrc" of Firewall "acctestfirewall230406170629416797" (Resource Group "acctestRG-230406170629416797"): Code="AzureFirewallDNSProxyMustBeEnabled" Message="DNS Proxy must be enabled in order to use DestinationFqdns in Network Rules. See rule rule1 in network rule collection acctestnrc" Details=[]
        
          with azurerm_firewall_network_rule_collection.test,
          on terraform_plugin_test.tf line 63, in resource "azurerm_firewall_network_rule_collection" "test":
          63: resource "azurerm_firewall_network_rule_collection" "test" {
        
--- FAIL: TestAccFirewallNetworkRuleCollection_fqdns (671.31s)
FAIL

[katbyte]

we have some test failures:

[katbyte]

i am still seeing a test failued:

# azurerm_firewall.test will be updated in-place
          ~ resource "azurerm_firewall" "test" {
              ~ dns_proxy_enabled   = true -> false
                id                  = "/subscriptions/*******/resourceGroups/acctestRG-230608033252412082/providers/Microsoft.Network/azureFirewalls/acctestfirewall230608033252412082"
                name                = "acctestfirewall230608033252412082"
                # (6 unchanged attributes hidden)
        
                # (1 unchanged block hidden)
            }

[wuxu92]

update the PR with an optional dns_proxy_enabled property. but this will cause an update to an existing firewall resource with dns_servers. because the current provider will set dns_proxy_enabled to true automatically. so the user have to set dns_proxy_enabled = true or add a ignore_changes = [dns_proxy_enabled] manually. WDYT @katbyte

[katbyte]

Still have a test failure:

------- Stdout: -------
=== RUN   TestAccFirewallPolicy_updatePremium
=== PAUSE TestAccFirewallPolicy_updatePremium
=== CONT  TestAccFirewallPolicy_updatePremium
    testing_new.go:86: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: deleting IP Group "acctestIpGroupForFirewallNetworkRulesDestination" (Resource Group "acctestRG-networkfw-230706183105166294"): network.IPGroupsClient#Delete: Failure sending request: StatusCode=400 -- Original Error: Code="IpGroupsHasFirewallPolicyReferences" Message="IpGroups '/subscriptions/*******/resourceGroups/acctestRG-networkfw-230706183105166294/providers/Microsoft.Network/ipGroups/acctestIpGroupForFirewallNetworkRulesDestination' cannot be deleted since there are firewall policies using this resource."
        
--- FAIL: TestAccFirewallPolicy_updatePremium (334.52s)
FAIL

for now we can make the property computed?

[katbyte]

@wuxu92 - we still see test failures:

------- Stdout: -------
=== RUN   TestAccFirewallPolicy_complete
=== PAUSE TestAccFirewallPolicy_complete
=== CONT  TestAccFirewallPolicy_complete
    testcase.go:113: Step 1/2 error: Error running apply: exit status 1
        
        Error: creating/updating Firewall Policy: (Name "acctest-networkfw-Policy-231106214108497927" / Resource Group "acctestRG-networkfw-231106214108497927"): network.FirewallPoliciesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="FirewallPolicyExplicitProxyPacFileSasUrlInvalid" Message="Firewall Policy /subscriptions/*******/resourceGroups/acctestRG-networkfw-231106214108497927/providers/Microsoft.Network/firewallPolicies/acctest-networkfw-Policy-231106214108497927 Pac file Sas Url is invalid"
        
          with azurerm_firewall_policy.test,
          on terraform_plugin_test.tf line 26, in resource "azurerm_firewall_policy" "test":
          26: resource "azurerm_firewall_policy" "test" {
        
--- FAIL: TestAccFirewallPolicy_complete (96.13s)
FAIL

multiple with that

[wuxu92]

@katbyte The test should pass now, but the TestAccFirewallPolicy_updatePremium case is not consistently successful in the main branch due to occasional failures caused by a strange Provider produced inconsistent result after apply issue. I am still investigating it in the main branch.

[katbyte]

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.