Skip to content

Conversation

@isra-fel
Copy link
Member

@isra-fel isra-fel commented May 6, 2020

Description

(Creating PR on behalf of @tejasshah7 as the original PR #11807 contains to many conflicts.)

These changes are to add support for DNS Proxy options to Azure Firewall command. 3 new parameters are added.

  • DNSServers: Array of strings
  • DNSEnableProxy: This can betrue/false with default being false

Allowed:
New-AzFirewall -Name "azFw" -ResourceGroupName $rgName -Location centralus -VirtualNetwork $vnet -PublicIpAddress $pip -DNSEnableProxy true -DNSServers @("10.10.10.1", "20.20.20.2")

Allowed only if no Network Rules have FQDNs:
New-AzFirewall -Name "azFw" -ResourceGroupName $rgName -Location centralus -VirtualNetwork $vnet -PublicIpAddress $pip -DNSRequireProxyForNetworkRules false

Only validation required is

  • If DNSRequireProxyForNetworkRules is explicitly set to False with DNSProxy disabled too, then no Network Rules can have FQDNs. Default value is true

Checklist

  • [x ] I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@adxsdkps
Copy link
Collaborator

adxsdkps commented May 6, 2020

Can one of the admins verify this patch?

@isra-fel
Copy link
Member Author

isra-fel commented May 6, 2020

Also please fix the test.
Besides, we prefer "Dns" to "DNS" in our cmdlet interfaces, for example, Set-AzNetworkInterface -DnsServer

@anton-evseev
Copy link
Contributor

Please use Network SDK version 19.21.0-preview (see #11814)

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch from e675f94 to 530a864 Compare May 6, 2020 19:19
@tejasshah7
Copy link
Contributor

Also please fix the test.
Besides, we prefer "Dns" to "DNS" in our cmdlet interfaces, for example, Set-AzNetworkInterface -DnsServer

Changed as per recommendation

@tejasshah7 tejasshah7 closed this May 6, 2020
@tejasshah7 tejasshah7 reopened this May 6, 2020
@tejasshah7
Copy link
Contributor

Also please fix the test.
Besides, we prefer "Dns" to "DNS" in our cmdlet interfaces, for example, Set-AzNetworkInterface -DnsServer

Changed as per recommendation

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch from 9f99f38 to 01998aa Compare May 7, 2020 03:53
@tejasshah7
Copy link
Contributor

tejasshah7 commented May 7, 2020

@isra-fel

The test recording is NOT updated because local build no longer works for network-april branch after the SDK update. So the test recording here needs to be updated. If the branch itself could be fixed, that would be great.

I found a way to fix the build and test. I will upload the new test recording soon. Thx.

@isra-fel updated test checked in.

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch from 01998aa to c1923de Compare May 7, 2020 16:58
@isra-fel
Copy link
Member Author

isra-fel commented May 8, 2020

@tejasshah7 there are still cases failing.
Besides, please resolve the conflict in ChangeLog.md, as another network PR was merged.

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch 2 times, most recently from ecde7fe to 6f53959 Compare May 8, 2020 05:38
@anton-evseev anton-evseev force-pushed the tesha/AzureFirewallSupportDNSProxy branch from 6f53959 to 536a63e Compare May 8, 2020 08:11
@anton-evseev anton-evseev changed the base branch from network-april to master May 8, 2020 08:12
@anton-evseev
Copy link
Contributor

network-april was merged into master, so retargetting this PR too

@anton-evseev
Copy link
Contributor

@tejasshah7 there are still issues with Firewall tests, could you please pull this rebased branch and resolve them?

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch from 536a63e to 2ef6250 Compare May 9, 2020 03:31
@tejasshah7
Copy link
Contributor

@tejasshah7 there are still issues with Firewall tests, could you please pull this rebased branch and resolve them?

@number213 Rebased and fixed

@tejasshah7 tejasshah7 force-pushed the tesha/AzureFirewallSupportDNSProxy branch from 2ef6250 to 5d08af6 Compare May 11, 2020 02:35
@isra-fel isra-fel requested a review from anton-evseev as a code owner May 11, 2020 02:35
@isra-fel
Copy link
Member Author

Looks good to me. @wyunchi-ms could you help merge it? Thanks

@wyunchi-ms wyunchi-ms merged commit 1df9935 into master May 11, 2020
@isra-fel isra-fel deleted the tesha/AzureFirewallSupportDNSProxy branch May 11, 2020 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants