New Cmdlets for Management.Sql that supports Managed instance key and… #9222

nivimsft · 2019-05-15T23:15:23Z

… managed instance TDE protector management

Description

New Cmdlets for Management.Sql to allow customers to add TDE keys and set TDE protector for managed instances

Add-AzureRmSqlManagedInstanceKeyVaultKey
Get-AzureRmSqlManagedInstanceKeyVaultKey
Remove-AzureRmSqlManagedInstanceKeyVaultKey
Get-AzureRmSqlManagedInstanceTransparentDataEncryptionProtector
Set-AzureRmSqlManagedInstanceTransparentDataEncryptionProtector

cmdlet design review

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

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceProtectorTests.ps1

MiYanni · 2019-05-16T22:06:58Z

@nivimsft There are merge conflicts to resolve.

src/Sql/Sql/ChangeLog.md

src/Sql/Sql/help/Add-AzSqlInstanceKeyVaultKey.md

markcowl · 2019-05-20T17:51:00Z

@nivimsft There are merge conflicts, please pull the latest, fix the conflicts, and update the PR

… managed instance TDE protector management

niander · 2019-05-30T18:34:07Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2019-05-30T18:34:32Z

Azure Pipelines successfully started running 1 pipeline(s).

Reviving old server tests

markcowl · 2019-05-31T01:40:10Z

/azp run azure-powershell - windows-powershell

azure-pipelines · 2019-05-31T01:40:26Z

Azure Pipelines successfully started running 1 pipeline(s).

markcowl · 2019-06-03T21:24:32Z

@jaredmoo @petrajkogit can you sign off on the requested changes in your previous review?

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

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceKeyVaultKeyTests.cs

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceProtectorTests.cs

src/Sql/Sql.Test/ScenarioTests/ServerKeyVaultKeyTests.ps1

- clean up commented logic. - remove irrelavent comments.

markcowl · 2019-06-04T18:02:56Z

@jaredmoo Please sign off on the review changes

src/Sql/Sql/TransparentDataEncryption/Model/AzureRmSqlManagedInstanceKeyVaultKeyModel.cs

src/Sql/Sql/TransparentDataEncryption/Model/ManagedInstanceKeyHelper.cs

markcowl

An issue and a question

markcowl · 2019-06-05T19:02:04Z

...rentDataEncryption/Cmdlet/AzureSqlRmManagedInstanceTransparentDataEncryptionProtectorBase.cs

+            Position = 1,
+            HelpMessage = "The instance name")]
+        [ValidateNotNullOrEmpty]
+        public string InstanceName { get; set; }


This probably needs to be called Name - if the object this names matches the cmdlet noun (i.e ManagedServiceInstance, then it should be called 'Name' - it is OK to have an alias to this

I think Name could be confusing in this context, it could mean ManagedInstanceTransparentDataEncryptionProtector Name or ManagedInstance Name. Hence I think it is important to use InstanceName instead of just Name

markcowl · 2019-06-05T19:03:44Z

src/Sql/Sql/TransparentDataEncryption/Cmdlet/RemoveAzureRmSqlManagedInstanceKeyVaultKey.cs

+    /// Defines the Remove-AzureRmSqlManagedInstanceKeyVaultKey cmdlet
+    /// </summary>
+    [Cmdlet("Remove", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "SqlInstanceKeyVaultKey", SupportsShouldProcess = true, DefaultParameterSetName = DefaultParameterSet)]
+    [OutputType(typeof(AzureRmSqlManagedInstanceKeyVaultKeyModel))]


Generally, remove cmdlets return nothing if successful. Is this pattern followed in other Remove cmdlets in Sql?

Yes, I have followed the pattern in the released cmdlet for sql server as we want to keep the user experience same for sql server and managed instance. The corresponding cmdlet for sql server key vault key returns the deleted key and remove key vault key cmdlet also returns deleted key.

markcowl · 2019-06-05T19:07:01Z

@nivimsft Please address comments/questions

nivimsft · 2019-06-05T19:22:11Z

@nivimsft Please address comments/questions

Hi @markcowl, I have addressed your comments. Please take a look :)

nivimsft requested a review from jaredmoo May 15, 2019 23:19

azuresdkci added the needs-review label May 16, 2019

jaredmoo reviewed May 16, 2019

View reviewed changes

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceProtectorTests.ps1 Outdated Show resolved Hide resolved

MiYanni added the Merge Conflicts label May 16, 2019

petrajkogit reviewed May 17, 2019

View reviewed changes

src/Sql/Sql/ChangeLog.md Outdated Show resolved Hide resolved

petrajkogit reviewed May 17, 2019

View reviewed changes

src/Sql/Sql/help/Add-AzSqlInstanceKeyVaultKey.md Outdated Show resolved Hide resolved

markcowl assigned nivimsft May 20, 2019

markcowl added needs-revision and removed needs-review labels May 20, 2019

nivimsft added 2 commits May 23, 2019 10:57

New Cmdlets for Management.Sql that supports Managed instance key and…

195ab40

… managed instance TDE protector management

adding test session records

6f00db9

nivimsft force-pushed the tdemiPreviewToRelease branch from 3c8bae5 to 6f00db9 Compare May 23, 2019 18:03

niander unassigned nivimsft May 30, 2019

niander added needs-review and removed Merge Conflicts needs-revision labels May 30, 2019

nivimsft added 2 commits May 30, 2019 12:29

Updating tests to create MI in test setup

f8dae11

Reviving old server tests

Adding test session records.

83c0f04

Adding reference to keyvault module

ab62de1

markcowl assigned jaredmoo and petrajkogit Jun 3, 2019

jaredmoo reviewed Jun 3, 2019

View reviewed changes

Addressed PR comments

c2ff3bd

- clean up commented logic. - remove irrelavent comments.

jaredmoo previously approved these changes Jun 5, 2019

View reviewed changes

src/Sql/Sql/TransparentDataEncryption/Model/AzureRmSqlManagedInstanceKeyVaultKeyModel.cs Show resolved Hide resolved

src/Sql/Sql/TransparentDataEncryption/Model/ManagedInstanceKeyHelper.cs Outdated Show resolved Hide resolved

Addressed PR comments

fb9886c

nivimsft dismissed jaredmoo’s stale review via fb9886c June 5, 2019 02:09

jaredmoo approved these changes Jun 5, 2019

View reviewed changes

markcowl requested changes Jun 5, 2019

View reviewed changes

markcowl assigned nivimsft and unassigned jaredmoo and petrajkogit Jun 5, 2019

markcowl added needs-revision and removed needs-review labels Jun 5, 2019

markcowl approved these changes Jun 6, 2019

View reviewed changes

markcowl removed the needs-revision label Jun 6, 2019

markcowl unassigned nivimsft Jun 6, 2019

markcowl merged commit 567b700 into Azure:master Jun 6, 2019

New Cmdlets for Management.Sql that supports Managed instance key and… #9222

New Cmdlets for Management.Sql that supports Managed instance key and… #9222

Uh oh!

Conversation

nivimsft commented May 15, 2019

Description

Checklist

Uh oh!

Uh oh!

MiYanni commented May 16, 2019

Uh oh!

Uh oh!

Uh oh!

markcowl commented May 20, 2019

Uh oh!

niander commented May 30, 2019

Uh oh!

azure-pipelines bot commented May 30, 2019

Uh oh!

markcowl commented May 31, 2019

Uh oh!

azure-pipelines bot commented May 31, 2019

Uh oh!

markcowl commented Jun 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markcowl commented Jun 4, 2019

Uh oh!

Uh oh!

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

markcowl Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

nivimsft Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

nivimsft Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl commented Jun 5, 2019

Uh oh!

nivimsft commented Jun 5, 2019

Uh oh!

Uh oh!