[Az.RecoveryServices.Backup] Customer Managed Key Encryption for recovery services vault #13593

hiaga · 2020-11-26T09:47:22Z

Description

CMK for V1 vaults

Checklist

[Yes] I have read the Submitting Changes section of CONTRIBUTING.md
[Yes] The title of the PR is clear and informative
[Yes] 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
[Yes] The PR does not introduce breaking changes
[N/A] If applicable, the changes made in the PR have proper test coverage
[Yes] For public API changes to cmdlets:
- [Yes] a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
  - https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/793
- [Yes] the markdown help files have been regenerated using the commands listed here

hiaga · 2020-12-22T04:09:37Z

@isra-fel : can you pls merge the cmdlet review PR - https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/793

src/RecoveryServices/RecoveryServices/help/Get-AzRecoveryServicesVaultProperty.md

src/RecoveryServices/RecoveryServices/help/Set-AzRecoveryServicesVaultProperty.md

isra-fel · 2020-12-23T06:44:54Z

...ecoveryServices/RecoveryServices.Backup/Cmdlets/Jobs/WaitAzureRmRecoveryServicesBackupJob.cs

@@ -139,7 +140,7 @@ public override void ExecuteCmdlet()
                    }

                    // sleep for 30 seconds before checking again
-                    TestMockSupport.Delay(30 * 1000);
+                    Thread.Sleep(30000);


For such long sleep, we suggest check if it's in a playback test and skip sleep if so. This could save you some time when playback tests.

For example
https://github.com/Azure/azure-powershell/blob/master/src/Accounts/Authentication/Factories/CancelRetryHandler.cs#L66

isra-fel · 2020-12-23T07:49:02Z

src/RecoveryServices/RecoveryServices/help/Get-AzRecoveryServicesVault.md

@@ -51,10 +51,11 @@ Get the list of vault in resource group in selected subscription.
 ### Example 3

 ```powershell
-PS C:\> Get-AzRecoveryServicesVault -ResourceGroupName "resourceGroup" -Name "vaultName"
+PS C:\> $vault = Get-AzRecoveryServicesVault -ResourceGroupName "resourceGroup" -Name "vaultName"
+PS C:\> $vault.Identity 


Could use a sample output here, for example

Suggested change

PS C:\> $vault.Identity

PS C:\> $vault.Identity

IdentityType: SystemAssigned

ObjectId: xxx

isra-fel · 2020-12-23T07:52:19Z

src/RecoveryServices/RecoveryServices/help/Set-AzRecoveryServicesVaultProperty.md

 ```
 Set-AzRecoveryServicesVaultProperty -SoftDeleteFeatureState <String> [-VaultId <String>]
 [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]
 ```

+### AzureRSVaultCMKParameterSet
+```
+Set-AzRecoveryServicesVaultProperty -EncryptionKeyID <string> -KeyVaultSubscriptionId <string> [-InfrastructureEncryption] [-VaultId <String>]


We talked about specifying the key by vault name + key name + (optional) key version, which should be a third parameter set. But it's OK if you choose to add that in the next sprint

We are interested in first trying it this way, we can add it next CMK update. Also we aren't sure of hardcoding the Key url because it might use different format in service. is there a way we can construct the exact key.Id with these three values?

It's basically $"https://{KeyVaultName}.{KeyVaultDnsSuffix}/keys/{key-name}" with optional "/{key-version}" at the end.

You can get {KeyVaultDnsSuffix} by DefaultContext.Environment.GetEndpoint(AzureEnvironment.Endpoint.AzureKeyVaultDnsSuffix)

Great we would want to add this parameter set later.

src/RecoveryServices/RecoveryServices.Test/RecoveryServices.Test.csproj

fixing static analysis fixing casing skipping tests for upcoming release resolving review comments

hiaga · 2020-12-23T12:03:10Z

@isra-fel: added the changes. pls review.

hiaga · 2020-12-23T16:39:43Z

...ecoveryServices/RecoveryServices.Backup/Cmdlets/Jobs/WaitAzureRmRecoveryServicesBackupJob.cs

@@ -139,7 +140,12 @@ public override void ExecuteCmdlet()
                    }

                    // sleep for 30 seconds before checking again
-                    TestMockSupport.Delay(30 * 1000);
+                    string testMode = Environment.GetEnvironmentVariable("AZURE_TEST_MODE");


@isra-fel added this check to skip waiting in case Playback mode. seems like tests are still stuck. can you pls let me know the environment variable for /azp and the value it takes.

Sorry I was wrong. We didn't set a envrionment when running tests. However there's a static field TestMockSupport.RunningMocked you can check.

In fact, I don't know why you change all the TestMockSupport.Delay() , that should work perfectly fine here

Besides, not only here, but all the "System.Threading.Sleep()should be wrapped in anIf` clause, otherwise the tests will still time out

@isra-fel : included "TestMockSupport.RunningMocked" and skipped two failed tests for now. I changed TestMockSupport.Delay() to Thread.Sleep() because while running the tests in record mode this was not putting a delay and was flooding our service with many calls. However it was working fine while running the cmdlets.

isra-fel · 2020-12-23T17:21:25Z

Also, I tried to run the tests locally, there seem to be 2 cases still failling, needing to be recorded.

TestAzureVMSetVaultProperty in RecoveryServices.Backup.Test/ScenarioTests/IaasVm/ItemTests.cs
TestRecoveryServicesVaultCRUD in RecoveryServices.Test/ScenarioTests/RecoveryServicesTests.cs

Please either record them or skip.

isra-fel · 2020-12-24T04:54:44Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2020-12-24T04:54:53Z

Azure Pipelines successfully started running 1 pipeline(s).

isra-fel · 2020-12-24T06:39:01Z

CI stuck at generation test on MacOS. Last run was sucessful. It's OK to force merge.

hiaga added the Do Not Merge 🚫 label Nov 26, 2020

hiaga self-assigned this Nov 26, 2020

hiaga force-pushed the hiaga/CMK branch 5 times, most recently from 1f3b2d8 to 60b4dbd Compare December 21, 2020 08:54

hiaga added needs-review and removed Do Not Merge 🚫 labels Dec 21, 2020

hiaga changed the title ~~[Do Not Merge] [Az.RecoveryServices] Customer Managed Key Encryption for V1 vaults~~ [Az.RecoveryServices.Backup] Customer Managed Key Encryption for recovery services vault Dec 21, 2020

hiaga assigned isra-fel and wyunchi-ms and unassigned hiaga Dec 21, 2020

hiaga force-pushed the hiaga/CMK branch 3 times, most recently from 8efd182 to deaca5d Compare December 22, 2020 01:39

isra-fel added the Cmdlet Review Required ⚠️ label Dec 22, 2020

isra-fel added this to the S180 (2020-12-29) milestone Dec 22, 2020

hiaga force-pushed the hiaga/CMK branch 2 times, most recently from 046e7bc to d6f0cb9 Compare December 22, 2020 06:37

isra-fel added Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ labels Dec 22, 2020

isra-fel requested changes Dec 22, 2020

View reviewed changes

src/RecoveryServices/RecoveryServices/help/Get-AzRecoveryServicesVaultProperty.md Outdated Show resolved Hide resolved

src/RecoveryServices/RecoveryServices/help/Set-AzRecoveryServicesVaultProperty.md Outdated Show resolved Hide resolved

hiaga force-pushed the hiaga/CMK branch from d6f0cb9 to 2028098 Compare December 22, 2020 08:22

isra-fel changed the base branch from master to release-2020-12-29 December 22, 2020 09:26

hiaga force-pushed the hiaga/CMK branch 3 times, most recently from 8c7fe5d to 8833168 Compare December 23, 2020 04:54

isra-fel reviewed Dec 23, 2020

View reviewed changes

src/RecoveryServices/RecoveryServices.Test/RecoveryServices.Test.csproj Outdated Show resolved Hide resolved

Customer Managed Key Encryption

9c927df

fixing static analysis fixing casing skipping tests for upcoming release resolving review comments

hiaga force-pushed the hiaga/CMK branch from 8833168 to 9c927df Compare December 23, 2020 10:33

hiaga commented Dec 23, 2020

View reviewed changes

fixing azp

73512f9

isra-fel approved these changes Dec 24, 2020

View reviewed changes

dolauli merged commit 4a33f21 into Azure:release-2020-12-29 Dec 24, 2020

[Az.RecoveryServices.Backup] Customer Managed Key Encryption for recovery services vault #13593

[Az.RecoveryServices.Backup] Customer Managed Key Encryption for recovery services vault #13593

Uh oh!

Conversation

hiaga commented Nov 26, 2020 • edited by isra-fel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

hiaga commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hiaga commented Dec 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Dec 23, 2020

Uh oh!

isra-fel commented Dec 24, 2020

Uh oh!

azure-pipelines bot commented Dec 24, 2020

Uh oh!

isra-fel commented Dec 24, 2020

Uh oh!

Uh oh!

hiaga commented Nov 26, 2020 •

edited by isra-fel

Loading

hiaga commented Dec 22, 2020 •

edited

Loading