[DevTestLabs] Exposing commands to manage secrets in the lab #2691

vishrutshah · 2017-03-31T00:16:37Z

Users should be able to create new secrets
Users should be able to view existing secrets
Users should be able to delete an existing secrets
Users should be able to update an existing secrets

Reference: https://azure.microsoft.com/en-us/updates/azure-devtest-labs-keep-your-secrets-safe-and-easy-to-use-with-the-new-personal-secret-store/

NOTE: I'll be sending new PRs on DTL work but it's not necessary for us to ship them with this release so feel free to label it as team prefers

codecov-io · 2017-03-31T00:34:22Z

Codecov Report

Merging #2691 into master will increase coverage by 0.02%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master    #2691      +/-   ##
==========================================
+ Coverage   62.82%   62.85%   +0.02%     
==========================================
  Files         480      480              
  Lines       25784    25802      +18     
  Branches     3905     3905              
==========================================
+ Hits        16199    16217      +18     
  Misses       8572     8572              
  Partials     1013     1013

Impacted Files	Coverage Δ
...-cli-lab/azure/cli/command_modules/lab/commands.py	`100% <100%> (ø)`	⬆️
...zure-cli-lab/azure/cli/command_modules/lab/help.py	`100% <100%> (ø)`	⬆️
...b/azure/cli/command_modules/lab/_client_factory.py	`68.18% <50%> (-1.82%)`	⬇️
...li-lab/azure/cli/command_modules/lab/validators.py	`25.52% <50%> (+0.17%)`	⬆️
...re-cli-lab/azure/cli/command_modules/lab/params.py	`97.95% <83.33%> (-2.05%)`	⬇️
...azure-cli-core/azure/cli/core/commands/__init__.py	`80.62% <0%> (+0.87%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8d8cbc...7758106. Read the comment docs.

derekbekoe · 2017-03-31T16:34:43Z

src/command_modules/azure-cli-lab/azure/cli/command_modules/lab/validators.py

@@ -80,6 +80,10 @@ def validate_lab_vm_list(namespace):
        namespace.filters = "Properties/ownerObjectId eq '{}'".format(object_id)


+def validate_user_name(namespace):
+    namespace.user_name = "@me"


What's this for?

REST endpoint exposes user_name parameter but from the cli context we are hiding them to enable scenario that it's the user's context. so that default value for that parameter will be @me.

For that reason I've created validator to default. Let me know if that is not the way we should be defaulting.

If this command is reflecting on the SDK, then this is perfectly acceptable. If it calls into a custom command, then you can just omit that parameter in the signature and plug in "@me" where appropriate.

Yup all the secret commands are now just reflecting on SDK

tjprescott

Need to rethink have identical create/update commands and there should be a tests to cover these new commands. Also, please show the -h output for the create and update commands.

tjprescott · 2017-03-31T16:35:59Z

src/command_modules/azure-cli-lab/azure/cli/command_modules/lab/custom.py

+                                            lab_name=lab_name,
+                                            user_name=user_name,
+                                            name=name,
+                                            secret=Secret(value=value))


Why do we have a custom command that literally just calls the API when we could just reflect on it? Why do we have two commands that call the exact same method? They should have a functional difference. Our "create" commands are typically "create or update" commands (though we don't advertise them as such. "Update" commands have patch or pseudo-patch behavior. If that doesn't apply, then "set" is a candidate.

You are right, we can definitely reflect and get the create / update of secrets. If we do that then based on the current model for the Secret class, I have to ignore bunch of parameters that doesn't make sense to user like provisioning_state, unique_identifier, id, type, location, tags because they are not required from the portal experience. I'd agree with you that that's the swagger modeling issue.

Let me update the command based on direct reflection and ignore unnecessary param :)

You don't need to expand the Secret class. You can just use a validator to transform the "secret string" into a Secret(value=value) object. This is one of the use cases for validators.

As per your suggestion, I've updated to use type instead of expand. Thanks for helping me understand type and its usage :)

vishrutshah · 2017-04-03T17:18:22Z

Some helps on commands usage. [Removed Global Argument section of help ]

(env) vishrut@mac azure-cli (lab-secrets) $ az lab secret -h

Group
    az lab secret: Commands to manage secrets of a DevTest Lab.

Commands:
    create: Create a secret in the DevTest Lab.
    delete: Delete secret.
    list  : List secrets in a given user profile.
    show  : Get secret.
    update: Update a secret in the DevTest Lab.

(env) vishrut@mac azure-cli (lab-secrets) $ az lab secret create -h

Command
    az lab secret create: Create a secret in the DevTest Lab.

Arguments
    --lab-name          [Required]: Name of the Lab.
    --resource-group -g [Required]: Name of resource group. You can configure the default group
                                    using 'az configure --defaults group=<name>'.
    --name -n                     : Name of the secret.
    --value                       : Value of the secret.

(env) vishrut@mac azure-cli (lab-secrets) $ az lab secret show -h

Command
    az lab secret show: Get secret.

Arguments
    --lab-name          [Required]: The name of the lab.
    --name -n           [Required]: The name of the secret.
    --resource-group -g [Required]: Name of resource group. You can configure the default group
                                    using 'az configure --defaults group=<name>'.
    --expand                      : Specify the $expand query. Example: 'properties($select=value)'.

(env) vishrut@mac azure-cli (lab-secrets) $ az lab secret update -h

Command
    az lab secret update: Update a secret in the DevTest Lab.

Arguments
    --lab-name          [Required]: Name of the Lab.
    --resource-group -g [Required]: Name of resource group. You can configure the default group
                                    using 'az configure --defaults group=<name>'.
    --name -n                     : Name of the secret.
    --value                       : Value of the secret.

Generic Update Arguments
    --add                         : Add an object to a list of objects by specifying a path and key
                                    value pairs.  Example: --add property.listProperty <key=value,
                                    string or JSON string>.
    --remove                      : Remove a property or an element from a list.  Example: --remove
                                    property.list <indexToRemove> OR --remove propertyToRemove.
    --set                         : Update an object by specifying a property path and value to set.
                                    Example: --set property1.property2=<value>.

tjprescott · 2017-04-03T17:27:24Z

src/command_modules/azure-cli-lab/azure/cli/command_modules/lab/params.py

+
+with ParametersContext(command='lab secret update') as c:
+    from .sdk.devtestlabs.models.secret import Secret
+    c.expand('secret', Secret)


Yes, you shouldn't use expand here. Use a validator to convert the parameter string to a Secret object (exactly like your custom command does)

I think I didn't quite get the comment. Can you please clarify or I'll stop by

tjprescott · 2017-04-03T23:53:53Z

src/command_modules/azure-cli-lab/azure/cli/command_modules/lab/help.py

@@ -170,6 +170,6 @@
                  short-summary: Name of the Lab
                - name: --name -n
                  short-summary: Name of the secret
-                - name: --value
+                - name: --secret


The alias of the dest parameter 'secret' to --value is a logical (and I'd argue good) one.

tjprescott · 2017-04-04T19:58:04Z

src/command_modules/azure-cli-lab/azure/cli/command_modules/lab/commands.py

+        c.command('show', 'get_resource')
+        c.command('list', 'list')
+        c.command('delete', 'delete_resource')
+        c.generic_update_command('update', 'get_resource', 'create_or_update_resource')


This breaks the contract of generic update. Generic update commands are supposed to guarantee that in the event of a conflict between a convenience argument and a generic argument, the generic argument wins. Using the custom command as the setter inverts this.

@tjprescott Seems like there is a confusion. I am not using any custom commands here. It seems like I missed to remove create_or_update_secret which i was using it before. Let me know if that is the thing you are referring.

vishrutshah · 2017-04-04T22:40:20Z

@tjprescott Thanks for the review but CI seems filing on tests from webapp @ https://travis-ci.org/Azure/azure-cli/jobs/218650474#L2620. Do we know whether it is a known issues? If so i'll wait for the CI to be fixed.

tjprescott · 2017-04-04T22:52:11Z

Yes, #2757. I have a PR which will unblock this. It's crawling its sad way through the CI right now.

tjprescott

Blah one more thing--please add this to the HISTORY for the DTL module.

vishrutshah · 2017-04-05T02:33:46Z

@tjprescott Just curios, do we want to keep updating history every time we make changes or is it like only when we release because I see version associated with each entry (at least in vm that i am looking at). Let me know what you think.

tjprescott · 2017-04-05T16:15:04Z

@vishrutshah you add the relevant entry for each PR so that when a release happens, in theory the change history is already there.

vishrutshah · 2017-04-05T18:00:11Z

@tjprescott Updated the history. Let me know how it looks. Thanks!

tjprescott · 2017-04-05T18:51:29Z

src/command_modules/azure-cli-lab/HISTORY.rst

@@ -3,6 +3,10 @@
 Release History
 ===============

+++++++++++++++++++++
+
+* Adding commands to manage secrets in the Azure DevTest Lab.


Since these are the release notes FOR DevTestLab (instead of the CLI in general), I would recommend something like:

Add commands to manage secrets within a lab.

tjprescott · 2017-04-05T18:53:09Z

@vishrutshah I suggested a wording change for your release notes entry, but LGTM. I'll leave merging up to you in case you want to go-ahead from other reviewers.

vishrutshah · 2017-04-05T20:20:39Z

Thanks for the review!!

)

* Enable delay-load of descriptions for commands (speed up az) * Update find indexing commands to accept callables for description. * Command load time in progress * - Moved previously dead command filter from parser to application configuration. - Removed unused configuration object/argv on application create. * Remove unused argument (pylint) * Remove dummy parameter * Fix for python 2.7 * Fix yet incorrect passage of parameters * Fix up additional pylint complaints * Update tests * Update tests * Fix up more tests * Fix up more core tests * Enable delay-load of descriptions for commands (speed up az) * Update find indexing commands to accept callables for description. * [Network] Remove nulls from VPN connection show/list output (#2748) * Fix #1615. * Code review feedback. * Update test docs for running individual test and all tests in mod (#2763) * Update test docs for running individual test and all tests in mod * Made feedback changes * Make argument parameters match up. (#2717) Make lock command parameter aliases match up with resource commands. * [DevTestLabs] Adding scenario test to create simple Linux + Windows VM in lab (#2767) * WIP create linux + Windows vm in lab * Adding recording * Add some more error checking/handling. (#2768) Add more validation to resolve "lock level" for lock commands. * Fix doc references to azure.cli.commands (#2740) * Fix doc references to azure.cli.commands This module has moved to azure.cli.core.commands * Fix PyLint * Add clearer guidelines on modifying changelog (#2739) * Add clearer guidelines on modifying changelog * A few smaller changes * another small format change * Code review changes * [DevTestLabs] Exposing commands to manage secrets in the lab (#2691) * ACS Update: nulling out the windows profile so that there isn't a validation fail… (#2764) * nulling out the windows profile so that there isn't a valdiation failure for missing password ACS doesn't return a password on GET. az acs scale command does a GET then PUT, but since ACS doesn't return the password the verification is failing before the PUT is sent to ACS. There is a bug in ACS this exposes. So this shouldn't be merged until after the ACS rollout finishes. Should be about start of next week. * updating history * updating version in history * removing white space added by editor * [Compute] Fix issues with VMSS and VM availability set update. (#2773) * Fix issues with VMSS and VM availability set update. * Update help. Fix #2762. * Error out if you try to list resources for a group that doesn't exist. (#2769) * Minor text fixes (#2776) * Add docs for az lock update. (#2702) * [DevTestLab] Explicitly enable usage of saved secrets while lab vm creation (#2686) * Explicitly enable usage of saved secrets for vm creation * Better error message with not overriding competing paramters * Adding export-artifacts commands on formula (#2707) * core: apply configured defaults on optional arg (#2770) * Core:apply configured defaults on optional argument * add a test * add tests * update history doc * address review feedback * [Network] Support active-active VNet gateways (#2751) * Start active-active test scenario. * Add active-active parameter. * Active-active scenario test 1 (cross premise) * Add second active-active scenario (vnet-to-vnet) * Refine active-active gateway configuration. * Pylint... * Code review feedback * Packaged release notes and changes for 0.2.4 (#2735) * Modify HISTORY.md * Update Dockerfile * Update debian also * Add pip dependencies also * Command load time in progress * - Moved previously dead command filter from parser to application configuration. - Removed unused configuration object/argv on application create. * Remove unused argument (pylint) * Remove dummy parameter * Fix for python 2.7 * Fix yet incorrect passage of parameters * Fix up additional pylint complaints * Update tests * Update tests * Fix up more tests * Fix up more core tests * Improve load time of custom.py for profile, find and configure (speeds up raw az command) * Pylint + flake8 fixes * Fix new vm tests that failed due to perf refactoring * Update redis tests that was broken due to perf refactoring * Delay-load msrest for command executions that don't need it * Fix flake8 issues * Fixing/improving detection of pageable class * flake8 fixes * Fix broken merge from upstream/master * Fix broken merge (again) * flake8 fixes * Fix up even more merge errors from last upstream merge * Flake8 fixes (wrong number of newlines) * Fix delay load of storage assembly for az * Update history to reference improved performance

msftclas added the cla-already-signed label Mar 31, 2017

vishrutshah requested review from troydai, tjprescott, ericame and derekbekoe March 31, 2017 00:17

vishrutshah added the Devtestlab az lab label Mar 31, 2017

tjprescott added the New Command(s) label Mar 31, 2017

derekbekoe approved these changes Mar 31, 2017

View reviewed changes

tjprescott suggested changes Mar 31, 2017

View reviewed changes

tjprescott reviewed Apr 3, 2017

View reviewed changes

tjprescott approved these changes Apr 4, 2017

View reviewed changes

tjprescott suggested changes Apr 4, 2017

View reviewed changes

tjprescott approved these changes Apr 4, 2017

View reviewed changes

tjprescott suggested changes Apr 5, 2017

View reviewed changes

vishrutshah added 8 commits April 5, 2017 10:29

Exposing commands to manage secrets in the lab

f3166e4

Adding new line at end of help file

0627b05

Using reflection to generate create/update secret commands

ccd8e8a

WIP

d9b0469

Using type argument instead of expanding on Secret

4ef36cd

Aliasing secret to value of the secret

e513451

Merging create/update into set as only one value is being set

b630586

Rebasing master and updating history

6499776

vishrutshah force-pushed the lab-secrets branch from 9cc7854 to 6499776 Compare April 5, 2017 17:30

tjprescott reviewed Apr 5, 2017

View reviewed changes

tjprescott approved these changes Apr 5, 2017

View reviewed changes

Updating message

7758106

vishrutshah merged commit 6186044 into Azure:master Apr 5, 2017

vishrutshah deleted the lab-secrets branch April 5, 2017 20:21

johanste pushed a commit to johanste/azure-cli that referenced this pull request Apr 7, 2017

[DevTestLabs] Exposing commands to manage secrets in the lab (Azure#2691

ff69644

)

haroldrandom added cla-already-signed DevTestLabs New Command(s) labels Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DevTestLabs] Exposing commands to manage secrets in the lab #2691

[DevTestLabs] Exposing commands to manage secrets in the lab #2691

vishrutshah commented Mar 31, 2017

codecov-io commented Mar 31, 2017 •

edited

Loading

derekbekoe Mar 31, 2017

vishrutshah Apr 3, 2017

tjprescott Apr 3, 2017

vishrutshah Apr 4, 2017

tjprescott left a comment

tjprescott Mar 31, 2017

vishrutshah Apr 3, 2017

tjprescott Apr 3, 2017

vishrutshah Apr 4, 2017

vishrutshah commented Apr 3, 2017

tjprescott Apr 3, 2017

vishrutshah Apr 3, 2017

tjprescott Apr 3, 2017

tjprescott Apr 4, 2017 •

edited

Loading

vishrutshah Apr 4, 2017

vishrutshah commented Apr 4, 2017

tjprescott commented Apr 4, 2017

tjprescott left a comment

vishrutshah commented Apr 5, 2017

tjprescott commented Apr 5, 2017

vishrutshah commented Apr 5, 2017

tjprescott Apr 5, 2017

tjprescott commented Apr 5, 2017

vishrutshah commented Apr 5, 2017

[DevTestLabs] Exposing commands to manage secrets in the lab #2691

[DevTestLabs] Exposing commands to manage secrets in the lab #2691

Conversation

vishrutshah commented Mar 31, 2017

codecov-io commented Mar 31, 2017 • edited Loading

Codecov Report

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

tjprescott left a 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

vishrutshah commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishrutshah commented Apr 4, 2017

tjprescott commented Apr 4, 2017

tjprescott left a comment

Choose a reason for hiding this comment

vishrutshah commented Apr 5, 2017

tjprescott commented Apr 5, 2017

vishrutshah commented Apr 5, 2017

Choose a reason for hiding this comment

tjprescott commented Apr 5, 2017

vishrutshah commented Apr 5, 2017

codecov-io commented Mar 31, 2017 •

edited

Loading

tjprescott Apr 4, 2017 •

edited

Loading