Adding Advanced Threat Protection and Vulnerability Assessment cmdlets on Managed Instance #8227

talhers · 2019-01-03T12:05:08Z

Description

These cmdlets are meant for working with managed instance, and they are the same as the Advanced Threat Protection and Vulnerability Assessment cmdlets on regular Azure db.

Design review for Advanced Threat Protection and Vulnerability Assessment cmdlets on regular Azure db can be found at: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/78

PR for Advanced Threat Protection and Vulnerability Assessment cmdlets on regular Azure db can be found at: #6653

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review (Microsoft internal partners only)
- the cmdlet markdown files were generated using the platyPS module

talhers · 2019-01-03T12:05:55Z

@yoavfr @yaakoviyun

talhers · 2019-01-03T12:07:54Z

@jaredmoo

src/ResourceManager/Profile/Commands.Profile/AzureRmAlias/Mappings.json

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/Common.ps1

....Sql/AdvancedThreatProtection/Cmdlet/SqlManagedInstanceAdvancedThreatProtectionCmdletBase.cs

jaredmoo · 2019-01-04T01:17:15Z

...nds.Sql/AdvancedThreatProtection/Model/ManagedInstanceAdvancedThreatProtectionPolicyModel.cs

+        /// <summary>
+        /// Gets or sets the managed instance name
+        /// </summary>
+        public string ManagedInstanceName { get; set; }


InstanceName

In AzureSqlManagedInstanceModel, the managed instance name property is also called ManagedInstanceName. Do you still want me to rename to InstanceName?

jaredmoo · 2019-01-04T01:17:52Z

...eManager/Sql/Commands.Sql/ThreatDetection/Model/ManagedInstanceThreatDetectionPolicyModel.cs

+
+namespace Microsoft.Azure.Commands.Sql.ThreatDetection.Model
+{
+    public class ManagedInstanceThreatDetectionPolicyModel : BaseThreatDetectionPolicyModel


InstanceThreatDetectionPolicyModel

The managed instance model name is AzureSqlManagedInstanceModel. Do you still want me to rename to InstanceThreatDetectionPolicyModel?

...uleBaseline/ManagedDatabase/GetAzureSqlManagedDatabaseVulnerabilityAssessmentRuleBaseline.cs

jaredmoo · 2019-01-04T01:22:35Z

Please review the existing managed instance cmdlets and make yours consistent with those.

src/ResourceManager/Profile/Commands.Profile/AzureRmAlias/Mappings.json

...ager/Sql/Commands.Sql/help/Clear-AzSqlInstanceDatabaseVulnerabilityAssessmentRuleBaseline.md

MiYanni · 2019-01-08T20:38:20Z

...ourceManager/Sql/Commands.Sql/help/Clear-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md

@@ -20,7 +20,7 @@ Clear-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline [-InputObject <AzureSqlDa

 ## DESCRIPTION
 The **Clear-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline** cmdlet removes the vulnerability assessment rule baseline.
-Note that you need to run *Enable-AzSqlServerAdvancedThreatProtection* and *Set-AzSqlDatabaseVulnerabilityAssessmentSettings* cmdlet as a prerequisite for using this cmdlets.
+Note that you need to run *Enable-AzSqlServerAdvancedThreatProtection* and *Update-AzSqlDatabaseVulnerabilityAssessmentSettings* cmdlet as a prerequisite for using this cmdlets.


Does Set-AzSqlDatabaseVulnerabilityAssessmentSettings still exist? Because we cannot remove it outside of a breaking change release. And, I didn't see a cmdlet alias on Update-AzSqlDatabaseVulnerabilityAssessmentSettings

Set-AzSqlDatabaseVulnerabilityAssessmentSettings never existed. It's an error in the documentation that I noticed and fixed

...eManager/Sql/Commands.Sql/help/Get-AzSqlInstanceDatabaseVulnerabilityAssessmentScanRecord.md

MiYanni · 2019-01-08T21:02:54Z

@talhers We have moved our projects to a shallower depth. You have merge conflicts. Basically, your changes to:

src/ResourceManager/Sql/Commands.Sql.Test/Commands.Sql.Test.Netcore.csproj
src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/Common.ps1
src/ResourceManager/Sql/Commands.Sql/AdvancedThreatProtection/Services/SqlAdvancedThreatProtectionAdapter.cs
src/ResourceManager/Sql/Commands.Sql/Az.Sql.psd1
src/ResourceManager/Sql/Commands.Sql/Commands.Sql.Netcore.csproj
src/ResourceManager/Sql/Commands.Sql/Properties/Resources.Designer.cs
src/ResourceManager/Sql/Commands.Sql/Properties/Resources.resx
src/ResourceManager/Sql/Commands.Sql/ThreatDetection/Services/SqlThreatDetectionAdapter.cs
src/ResourceManager/Sql/Commands.Sql/ThreatDetection/Services/ThreatDetectionEndpointsCommunicator.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Cmdlet/SqlDatabaseVulnerabilityAssessmentAtpCmdletBase.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Cmdlet/SqlDatabaseVulnerabilityAssessmentOperationCmdletBase.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Cmdlet/VulnerabilityAssessmentRuleBaseline/SqlDatabaseVulnerabilityAssessmentRuleBaselineCmdletBase.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Services/BaseSqlVulnerabilityAssessmentAdapter.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentAdapter.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentRuleBaselineAdapter.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentScanAdapter.cs
src/ResourceManager/Sql/Commands.Sql/VulnerabilityAssessment/Services/VulnerabilityAssessmentEndpointsCommunicator.cs
src/ResourceManager/Sql/Commands.Sql/help/Clear-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/ResourceManager/Sql/Commands.Sql/help/Convert-AzSqlDatabaseVulnerabilityAssessmentScan.md
src/ResourceManager/Sql/Commands.Sql/help/Get-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/ResourceManager/Sql/Commands.Sql/help/Get-AzSqlDatabaseVulnerabilityAssessmentScanRecord.md
src/ResourceManager/Sql/Commands.Sql/help/Set-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/ResourceManager/Sql/Commands.Sql/help/Start-AzSqlDatabaseVulnerabilityAssessmentScan.md

Now need to be made to:

src/Sql/Sql.Test/Commands.Sql.Test.Netcore.csproj
src/Sql/Sql.Test/ScenarioTests/Common.ps1
src/Sql/Sql/AdvancedThreatProtection/Services/SqlAdvancedThreatProtectionAdapter.cs
src/Sql/Sql/Az.Sql.psd1
src/Sql/Sql/Commands.Sql.Netcore.csproj
src/Sql/Sql/Properties/Resources.Designer.cs
src/Sql/Sql/Properties/Resources.resx
src/Sql/Sql/ThreatDetection/Services/SqlThreatDetectionAdapter.cs
src/Sql/Sql/ThreatDetection/Services/ThreatDetectionEndpointsCommunicator.cs
src/Sql/Sql/VulnerabilityAssessment/Cmdlet/SqlDatabaseVulnerabilityAssessmentAtpCmdletBase.cs
src/Sql/Sql/VulnerabilityAssessment/Cmdlet/SqlDatabaseVulnerabilityAssessmentOperationCmdletBase.cs
src/Sql/Sql/VulnerabilityAssessment/Cmdlet/VulnerabilityAssessmentRuleBaseline/SqlDatabaseVulnerabilityAssessmentRuleBaselineCmdletBase.cs
src/Sql/Sql/VulnerabilityAssessment/Services/BaseSqlVulnerabilityAssessmentAdapter.cs
src/Sql/Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentAdapter.cs
src/Sql/Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentRuleBaselineAdapter.cs
src/Sql/Sql/VulnerabilityAssessment/Services/SqlVulnerabilityAssessmentScanAdapter.cs
src/Sql/Sql/VulnerabilityAssessment/Services/VulnerabilityAssessmentEndpointsCommunicator.cs
src/Sql/Sql/help/Clear-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/Sql/Sql/help/Convert-AzSqlDatabaseVulnerabilityAssessmentScan.md
src/Sql/Sql/help/Get-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/Sql/Sql/help/Get-AzSqlDatabaseVulnerabilityAssessmentScanRecord.md
src/Sql/Sql/help/Set-AzSqlDatabaseVulnerabilityAssessmentRuleBaseline.md
src/Sql/Sql/help/Start-AzSqlDatabaseVulnerabilityAssessmentScan.md

Please update those accordingly and resolve the merge conflicts by deleting the files at the old path.

…s on Managed Instance

jaredmoo · 2019-01-11T02:02:31Z

Tal, you are seeing weird merge problems because you are making changes directly in talhers/master instead of a feature branch. You should always develop in a feature branch to avoid this.

Here is how we can fix this. I already ran the following commands:

# Make sure all branches are up to date
[master ≡]> git fetch --all

# Make a new branch named talhersFixed that is at the same commit as azure/master
[master ≡]> git checkout -b talhersFixed azure/master

# Merge changes from talhers/master into this branch, squashed into a single commit in order to simplify history
[talhersFixed ≡]> git merge --squash talhers/master
[talhersFixed ≡ +56 ~37 -1 ~]> git commit -m "Adding Advanced Threat Protection and Vulnerability Assessment cmdlets on Managed Instance"
[talhersFixed ↑]> git push --set-upstream jaredmoo talhersFixed

As a result, now I have a branch jaredmoo/talhersFixed that your PR should use. You can forcefully change the PR to have the same commit as my jaredmoo/talhersFixed branch by doing this:

# Add jaredmoo as known repo
> git remote add jaredmoo https://github.com/jaredmoo/azure-powershell

# Get all commits from all known repos
> git fetch --all

# Force reset master branch to same commit as jaredmoo/talhersFixed
> git checkout master
> git reset --hard jaredmoo/talhersFixed
> git push --force

talhers · 2019-01-13T09:32:57Z

Thanks for you help!
@jaredmoo , @MiYanni - can you take another look and merge the PR if everything is fine?
Thanks!

markcowl · 2019-01-18T19:46:55Z

@talhers - there are some static analysis failures causing the ci build to fial - at least some of them (the places outlined in SignatureIssues.csv where you have cmdlets with mutiple parameters sets but no default parameter set) will need to be fixed, and you will need to go over thene potential breakign changes outlined in BreakingChangeIssues.csv to see if the new types replacing the current types have the same public property signature. Look at the log for detailed errors, and this document describes how to process atatic analysis errors:

markcowl · 2019-01-18T19:47:46Z

@maddieclayton Can you work with @tahlers to ensure this is ready for the next release?

talhers · 2019-01-22T14:42:19Z

@markcowl - thanks for checking my PR,
I've gone through both breaking changes and signature issues:

breaking changes - static analysis reported a change in input object - this is because previously the VA database cmdlets would receive the DatabaseVulnerabilityAssessmentSettingsModel object, and now they can receive the base class object - VulnerabilityAssessmentSettingsModel. This will not break any existing scripts as DatabaseVulnerabilityAssessmentSettingsModel object is still accepted. (same explanation applies on all input objects reported)

signature issues - I have defined default parameters in the required places.
As to "Consider using a singular noun for the cmdlet name" comment - I didn't change the names because I have named the additional cmdlets on managed database in the same manner as the cmdlets on regular database in order to be consistent with them.

@markcowl , @maddieclayton - can you take another look?
Thanks!

MiYanni · 2019-01-22T18:02:16Z

@talhers You have to update the suppression files or the build will continue to fail. For SQL, they are in tools/StaticAnalysis/Exceptions/Az.Sql. Add the entries to the appropriate files there.

MiYanni

LGTM

Comments were addressed.

azuresdkci added the needs-review label Jan 3, 2019

jaredmoo previously requested changes Jan 3, 2019

View reviewed changes

src/ResourceManager/Profile/Commands.Profile/AzureRmAlias/Mappings.json Outdated Show resolved Hide resolved

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/Common.ps1 Outdated Show resolved Hide resolved

jaredmoo reviewed Jan 4, 2019

View reviewed changes

....Sql/AdvancedThreatProtection/Cmdlet/SqlManagedInstanceAdvancedThreatProtectionCmdletBase.cs Outdated Show resolved Hide resolved

jaredmoo reviewed Jan 4, 2019

View reviewed changes

...uleBaseline/ManagedDatabase/GetAzureSqlManagedDatabaseVulnerabilityAssessmentRuleBaseline.cs Outdated Show resolved Hide resolved

jaredmoo reviewed Jan 4, 2019

View reviewed changes

...uleBaseline/ManagedDatabase/GetAzureSqlManagedDatabaseVulnerabilityAssessmentRuleBaseline.cs Outdated Show resolved Hide resolved

maddieclayton added Merge Conflicts needs-revision and removed needs-review labels Jan 8, 2019

maddieclayton assigned talhers Jan 8, 2019

MiYanni suggested changes Jan 8, 2019

View reviewed changes

Adding Advanced Threat Protection and Vulnerability Assessment cmdlet…

0b03e06

…s on Managed Instance

talhers and others added 2 commits January 13, 2019 16:58

moving from Advanced Threat Protection to Advanced Data Security

9ce6ca2

Update VulnerabilityAssessmentMiTests.cs

8a8a6dc

cormacpayne removed the Merge Conflicts label Jan 14, 2019

markcowl assigned maddieclayton Jan 18, 2019

talhers added 3 commits January 21, 2019 17:13

Fix signature issues

cb68e2c

Merge branch 'master' of https://github.com/talhers/azure-powershell

c9eb44b

Update help files

7457ba1

MiYanni added Sprint Candidate Merge Conflicts and removed Sprint Candidate labels Jan 22, 2019

talhers added 4 commits January 23, 2019 09:52

Merge branch 'master' into master

e12a085

Update suppression files

b1df348

Update suppression files

6aa3d07

Suppress false positive test session records

f0f4127

MiYanni removed the Merge Conflicts label Jan 23, 2019

MiYanni approved these changes Jan 23, 2019

View reviewed changes

MiYanni merged commit 84039d9 into Azure:master Jan 23, 2019

talhers mentioned this pull request Jan 27, 2019

Adding Vulnerability Assessment cmdlets on Server and Managed Instance #8413

Merged

8 tasks

Adding Advanced Threat Protection and Vulnerability Assessment cmdlets on Managed Instance #8227

Adding Advanced Threat Protection and Vulnerability Assessment cmdlets on Managed Instance #8227

Uh oh!

Conversation

talhers commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

talhers commented Jan 3, 2019

Uh oh!

talhers commented Jan 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredmoo Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

talhers Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

jaredmoo Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

talhers Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaredmoo commented Jan 4, 2019

Uh oh!

Uh oh!

Uh oh!

MiYanni Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

talhers Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MiYanni commented Jan 8, 2019

Uh oh!

jaredmoo commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talhers commented Jan 13, 2019

Uh oh!

markcowl commented Jan 18, 2019

Uh oh!

markcowl commented Jan 18, 2019

Uh oh!

talhers commented Jan 22, 2019

Uh oh!

MiYanni commented Jan 22, 2019

Uh oh!

MiYanni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

talhers commented Jan 3, 2019 •

edited

Loading

jaredmoo commented Jan 11, 2019 •

edited

Loading