adding support for backing up and restoring of KeyVault secrets #3787

dragav · 2017-04-17T21:22:23Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

msftclas · 2017-04-17T21:22:27Z

@dragav,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-04-17T21:25:03Z

Can one of the admins verify this patch?

markcowl · 2017-04-21T01:21:08Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/BackupAzureKeyVaultSecret.cs

+                    OutputFile = GetDefaultFileForOperation("backup", VaultName, Name);
+                }
+
+                var filePath = ResolvePathFromFilename(OutputFile, throwOnPreExisting: true, errorMessage: KeyVaultProperties.Resources.BackupSecretFileAlreadyExists);


Two things:

(1) You should be using AzureSession.DataStore when interacting with the file system (checking for existence and the like), as this is mockable in tests
(2) The normal standrad would be to prompt for overwite if the file exists, which would mena having a ShouldContinue prompt asking if the user wants to overwrite and a Force parameter. Is there a reason why you wouldn't want to allow the user to overwrite the file?

markcowl · 2017-04-21T01:22:21Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/BackupAzureKeyVaultSecret.cs

+    [Cmdlet( VerbsData.Backup, "AzureKeyVaultSecret",
+        SupportsShouldProcess = true,
+        HelpUri = Constants.KeyVaultHelpUri )]
+    [OutputType( typeof( String ) )]


Not sure the OutputType is useful. You should probably return nothing and implement a passthrough parameter so you can return true/false for success / failure

The output of this cmdlet is the file path of the newly generated backup file.

markcowl · 2017-04-21T01:23:49Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/BackupAzureKeyVaultSecret.cs

+    {
+        #region Input Parameter Definitions
+
+        /// <summary>


You should consider having an alternate parameter set that takes a KeyVaultSecret so that users can more easily pipe get-AzureKeyVaultSecret to this cmdlet.

I don't quite see the value of this scenario - the piped-in object would only serve to extract the identifier, as that's what the server expects. The service must find/load/encrypt the object before creating a backup - the client can't upload an object to be backed up.

Another thing to consider is that the secret can be retrieved by id (ie a specific version) whereas the backup acts on the entire family (head secret + all versions). I'm concerned that the user expectation (on passing in a specific secret object) will be that an individual backup of that object will be generated.

markcowl · 2017-04-21T01:24:48Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultSecret.cs

+
+        private string ResolvePath( string filePath )
+        {
+            FileInfo secretFile = new FileInfo(this.GetUnresolvedProviderPathFromPSPath(filePath));


Please use the data store, as outlined above

markcowl · 2017-04-21T01:29:19Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultSecret.cs

+    [Cmdlet( VerbsData.Restore, "AzureKeyVaultSecret",
+        SupportsShouldProcess = true,
+        HelpUri = Constants.KeyVaultHelpUri )]
+    [OutputType( typeof( Secret ) )]


It looks like you are returnign the secret FullName, isn't that a string?

Restore returns the newly reinstated secret (or key, for existing cmdlets). It is similar in that sense with the Get command.

markcowl · 2017-04-21T01:31:26Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultSecret.cs

+                   Position = 1,
+                   HelpMessage = "Input file. The input file containing the backed-up blob" )]
+        [ValidateNotNullOrEmpty]
+        public string InputFile { get; set; }


It would be nice to take a list of files in this parameter, and process each one

Will add this to the backlog for a future release :)

Sounds good, this would be a non-breaking change

cormacpayne · 2017-04-25T20:08:45Z

@dragav please pull the latest changes from the preview branch - these changes remove dll-Help.xml files from the repository, so you will no longer need to update this file in your PR

dragav · 2017-04-27T23:37:24Z

Addressed feedback and ported these changes to the corresponding preview branch. Will resubmit the PR shortly.

adding support for backing up and restoring of KeyVault secrets

be4cee2

msftclas added the cla-not-required label Apr 17, 2017

azuresdkci assigned markcowl Apr 17, 2017

markcowl changed the base branch from dev to preview April 19, 2017 17:00

markcowl requested changes Apr 21, 2017

View reviewed changes

markcowl assigned dragav and unassigned markcowl Apr 21, 2017

markcowl added the needs-revision label Apr 21, 2017

dragav requested a review from amitbapat April 22, 2017 00:49

dragav closed this Apr 27, 2017

cormacpayne mentioned this pull request Apr 28, 2017

adding support for backing up and restoring KeyVault secrets #3855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding support for backing up and restoring of KeyVault secrets #3787

adding support for backing up and restoring of KeyVault secrets #3787

dragav commented Apr 17, 2017 •

edited by azuresdkci

Loading

msftclas commented Apr 17, 2017

azuresdkci commented Apr 17, 2017

markcowl Apr 21, 2017

markcowl Apr 21, 2017

dragav Apr 25, 2017

markcowl Apr 21, 2017

dragav Apr 21, 2017

markcowl Apr 21, 2017

markcowl Apr 21, 2017

dragav Apr 21, 2017

markcowl Apr 21, 2017

dragav Apr 21, 2017

markcowl Apr 22, 2017

cormacpayne commented Apr 25, 2017

dragav commented Apr 27, 2017

adding support for backing up and restoring of KeyVault secrets #3787

adding support for backing up and restoring of KeyVault secrets #3787

Conversation

dragav commented Apr 17, 2017 • edited by azuresdkci Loading

Description

msftclas commented Apr 17, 2017

azuresdkci commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cormacpayne commented Apr 25, 2017

dragav commented Apr 27, 2017

dragav commented Apr 17, 2017 •

edited by azuresdkci

Loading