Add ThreatIntelWhitelist to AzFirewall commands #10390

b564518128 · 2019-10-25T01:16:56Z

Description

Checklist

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 · 2019-10-25T01:17:06Z

Can one of the admins verify this patch?

anton-evseev · 2019-10-25T01:24:07Z

Depends on #10386

wyunchi-ms · 2019-10-25T15:26:35Z

Hi @b564518128 @number213 could you please take a look at this PR? Because there are too many changed files even after I merged the network-october. I don't think this make sense.

This reverts commit e5fda3d.

anton-evseev

Please also consider renaming TestAzureFirewallAdditionalPropertyCRUD. It made sense in Andrey's PR, but with ThreatIntel cmdlet it's confusing

src/Accounts/Accounts/AzureRmAlias/Mappings.json

...rk/Network/AzureFirewall/ThreatIntelWhitelist/NewAzureFirewallThreatIntelWhitelistCommand.cs

src/Network/Network/ChangeLog.md

b564518128 · 2019-10-25T19:40:51Z

Please also consider renaming TestAzureFirewallAdditionalPropertyCRUD. It made sense in Andrey's PR, but with ThreatIntel cmdlet it's confusing

Changed the test name

b564518128 · 2019-10-25T21:37:51Z

hi @wyunchi-ms , I updated the PR and removed the unnecessary helper md. Please complete the PR

isra-fel · 2019-10-28T02:10:20Z

Azure/azure-powershell-cmdlet-review-pr/issues/410

src/Network/Network/help/Az.Network.md

src/Network/Network/AzureFirewall/NewAzureFirewallCommand.cs

isra-fel · 2019-10-29T01:28:23Z

...rk/Network/AzureFirewall/ThreatIntelWhitelist/NewAzureFirewallThreatIntelWhitelistCommand.cs

+
+namespace Microsoft.Azure.Commands.Network
+{
+    [Cmdlet(VerbsCommon.New, ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "FirewallThreatIntelWhitelist", SupportsShouldProcess = true), OutputType(typeof(PSAzureFirewallThreatIntelWhitelist))]


No need for shouldprocess cause this cmdlet is creating an in-memory object, not modifying anything on Azure

Suggested change

[Cmdlet(VerbsCommon.New, ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "FirewallThreatIntelWhitelist", SupportsShouldProcess = true), OutputType(typeof(PSAzureFirewallThreatIntelWhitelist))]

[Cmdlet(VerbsCommon.New, ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "FirewallThreatIntelWhitelist"), OutputType(typeof(PSAzureFirewallThreatIntelWhitelist))]

@isra-fel After I removed this, the check is emitting this error:
"New-AzFirewallThreatIntelWhitelist Does not support ShouldProcess but the cmdlet verb New indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue"
Could you please take a loo?

src/Network/Network/Models/AzureFirewall/PSAzureFirewall.cs

src/Network/Network/Models/AzureFirewall/PSAzureFirewallThreatIntelWhitelist.cs

src/Network/Network/help/Get-AzFirewall.md

src/Network/Network/help/New-AzFirewall.md

src/Network/Network/help/New-AzFirewallThreatIntelWhitelist.md

isra-fel · 2019-10-29T01:51:27Z

src/Network/Network.Test/ScenarioTests/AzureFirewallTests.ps1

+
+        # Verify
+        $getAzureFirewall = Get-AzFirewall -Name $azureFirewallName -ResourceGroupName $rgname
+        Assert-AreEqual true (-not (Compare-Object $threatIntelWhitelist1.FQDNs $getAzureFirewall.ThreatIntelWhitelist.FQDNs))


Suggested change

Assert-AreEqual true (-not (Compare-Object $threatIntelWhitelist1.FQDNs $getAzureFirewall.ThreatIntelWhitelist.FQDNs))

Assert-AreEqualArray $threatIntelWhitelist1.FQDNs $getAzureFirewall.ThreatIntelWhitelist.FQDNs

Same suggestion for the below assertions

This is not changed

isra-fel · 2019-10-29T06:57:06Z

Unresolved one conversation.
And for the static analysis failure, please refer here to suppress it:
https://github.com/Azure/azure-powershell/blob/master/documentation/Debugging-StaticAnalysis-Errors.md#signature-issues

b564518128 added 5 commits October 24, 2019 14:17

create threat intel whitelist object

7c91cbf

update threat intel whitelist class definition

47d194e

create New-AzFirewallThreatIntelWhitelist cmdlet

1c1bd55

add ThreatIntelWhitelist to PSAzureFirewall and change mapping

e5fda3d

add test

45268b9

b564518128 changed the title ~~Merge network~~ Add ThreatIntelWhitelist to AzFirewall commands Oct 25, 2019

update change log

bd6177b

azuresdkci added the needs-review label Oct 25, 2019

wyunchi-ms self-assigned this Oct 25, 2019

Revert "add ThreatIntelWhitelist to PSAzureFirewall and change mapping"

da7f18e

This reverts commit e5fda3d.

anton-evseev suggested changes Oct 25, 2019

View reviewed changes

b564518128 added 4 commits October 25, 2019 11:49

re-add function changes

bdc54c0

modifying existing cmdlet helper md

6554603

helper md for new cmdlet

bb54124

update changelog per comments, update helper md

768c85c

anton-evseev previously approved these changes Oct 25, 2019

View reviewed changes

change data structure in PSAzureFirewallWhitelist to string[]

c3014a3

b564518128 dismissed anton-evseev’s stale review via c3014a3 October 25, 2019 21:23

anton-evseev previously approved these changes Oct 25, 2019

View reviewed changes

add validation to parameter

f04c251

b564518128 dismissed anton-evseev’s stale review via f04c251 October 25, 2019 23:19

anton-evseev previously approved these changes Oct 25, 2019

View reviewed changes

markcowl added Do Not Merge 🚫 Cmdlet Review Required ⚠️ and removed needs-review labels Oct 25, 2019

markcowl removed Cmdlet Review Required ⚠️ Do Not Merge 🚫 labels Oct 26, 2019

fix test

05db17a

b564518128 dismissed anton-evseev’s stale review via 05db17a October 28, 2019 21:39

fix test

107ee10

isra-fel requested changes Oct 29, 2019

View reviewed changes

isra-fel assigned b564518128 and unassigned wyunchi-ms Oct 29, 2019

isra-fel added the needs-revision label Oct 29, 2019

b564518128 added 2 commits October 28, 2019 22:37

address comment

e89edad

more comment fix

c16eaa2

andreycpp mentioned this pull request Oct 29, 2019

Add ThreatIntelWhitelist to AzFirewall commands #10414

Merged

8 tasks

isra-fel merged commit c16eaa2 into Azure:master Oct 29, 2019

	[Cmdlet(VerbsCommon.New, ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "FirewallThreatIntelWhitelist", SupportsShouldProcess = true), OutputType(typeof(PSAzureFirewallThreatIntelWhitelist))]
	[Cmdlet(VerbsCommon.New, ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "FirewallThreatIntelWhitelist"), OutputType(typeof(PSAzureFirewallThreatIntelWhitelist))]

	Assert-AreEqual true (-not (Compare-Object $threatIntelWhitelist1.FQDNs $getAzureFirewall.ThreatIntelWhitelist.FQDNs))
	Assert-AreEqualArray $threatIntelWhitelist1.FQDNs $getAzureFirewall.ThreatIntelWhitelist.FQDNs

Add ThreatIntelWhitelist to AzFirewall commands #10390

Add ThreatIntelWhitelist to AzFirewall commands #10390

Uh oh!

Conversation

b564518128 commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

adxsdkps commented Oct 25, 2019

Uh oh!

anton-evseev commented Oct 25, 2019

Uh oh!

wyunchi-ms commented Oct 25, 2019

Uh oh!

anton-evseev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

b564518128 commented Oct 25, 2019

Uh oh!

b564518128 commented Oct 25, 2019

Uh oh!

isra-fel commented Oct 28, 2019

Uh oh!

Uh oh!

Uh oh!

isra-fel Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

b564518128 Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

b564518128 Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

b564518128 Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

isra-fel Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

b564518128 commented Oct 25, 2019 •

edited

Loading

isra-fel commented Oct 29, 2019 •

edited

Loading