Updated Admin CLI commands to support tenants. #620

msterin · 2016-10-14T01:14:48Z

Defines the syntax for the following vmdkops_admin.py commands:

tenant {create, rm , ls} - manipulates tenants
tenant vm {add, rm, ls}  - manipulates VMs for a tenant
tenant access {add, set, rm, ls} - manipulates datastore access right for a tenant

Note that the actual commands implementation is still TBD - this change only defines
new command syntax and keywords. When invoked with correct arguments, the commands simply print Not implemented.

//CC @andrewjstone @lipingxue @marksoper

Note: doc update is tracked in #622

TESTED: manually , by asking for --help. Log below

[root@localhost:~] 
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py  status 
Version: 0.7.3eb1146-0.0.1
Status: Running
Pid: 51662
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log
LogLevel: DEBUG
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant
usage: vmdkops_admin.py tenant [-h] {access,rm,create,ls,vm} ...
vmdkops_admin.py tenant: error: too few arguments
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant create --help 
usage: vmdkops_admin.py tenant create [-h] [--vm-list vm1, vm2, ...] name

positional arguments:
  name                  The name of the tenant

optional arguments:
  -h, --help            show this help message and exit
  --vm-list vm1, vm2, ...
                        A list of VM names to place in this Tenant
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant rm --help 
usage: vmdkops_admin.py tenant rm [-h] [--remove-volumes REMOVE_VOLUMES] name

positional arguments:
  name                  The name of the tenant

optional arguments:
  -h, --help            show this help message and exit
  --remove-volumes REMOVE_VOLUMES
                        BE CAREFUL - Removes this tenant volumes when removing
                        a tenant
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls --help 
usage: vmdkops_admin.py tenant ls [-h]

optional arguments:
  -h, --help  show this help message and exit
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant vm --help 
usage: vmdkops_admin.py tenant vm [-h] {rm,add,ls} ...

positional arguments:
  {rm,add,ls}
    rm         Remove VM(s) from a tenant
    add        Add a VM(s) to a tenant
    ls         list VMs in a tenant

optional arguments:
  -h, --help   show this help message and exit
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant vm add --help 
usage: vmdkops_admin.py tenant vm add [-h] --name NAME --vm-list VM_LIST

optional arguments:
  -h, --help         show this help message and exit
  --name NAME        Tenant to add the VM to
  --vm-list VM_LIST  A list of VM names to add to this Tenant
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant vm rm --help 
usage: vmdkops_admin.py tenant vm rm [-h] --name NAME --vm-list VM_LIST

optional arguments:
  -h, --help         show this help message and exit
  --name NAME        Tenant to remove the VM from
  --vm-list VM_LIST  A list of VM names to rm from this Tenant
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant vm ls --help 
usage: vmdkops_admin.py tenant vm ls [-h] [--name NAME]

optional arguments:
  -h, --help   show this help message and exit
  --name NAME  Tenant to list the VMs for
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access --help 
usage: vmdkops_admin.py tenant access [-h] {rm,add,set,ls} ...

positional arguments:
  {rm,add,set,ls}
    rm             Remove all access to a datastore for a tenant
    add            Add a datastore access for a tenant
    set            Modify datastore access for a tenant
    ls             List all access info for a tenant

optional arguments:
  -h, --help       show this help message and exit
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access add --help 
usage: vmdkops_admin.py tenant access add [-h] --name NAME
                                          [--volume-maxsize Num{MB,GB,TB} - e.g. 2TB]
                                          --datastore DATASTORE
                                          [--rights create,delete,mount]

optional arguments:
  -h, --help            show this help message and exit
  --name NAME           The name of the tenant
  --volume-maxsize Num{MB,GB,TB} - e.g. 2TB
                        Maximum size of the volume that can be created
  --datastore DATASTORE
                        Datastore which access is controlled
  --rights create,delete,mount
                        Datastore access Permissions granted: Choices =
                        ['create', 'delete', 'mount', 'all']
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access rm  --help 
usage: vmdkops_admin.py tenant access rm [-h] --name NAME --datastore
                                         DATASTORE

optional arguments:
  -h, --help            show this help message and exit
  --name NAME           The name of the tenant
  --datastore DATASTORE
                        Datstore which access is controlled
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access set --help 
usage: vmdkops_admin.py tenant access set [-h]
                                          [--rm-rights create,delete,mount]
                                          --name NAME
                                          [--volume-maxsize Num{MB,GB,TB} - e.g. 2TB]
                                          [--add-rights create,delete,mount]
                                          --datastore DATASTORE

optional arguments:
  -h, --help            show this help message and exit
  --rm-rights create,delete,mount
                        Datastore access Permissions granted: Choices =
                        ['create', 'delete', 'mount', 'all']
  --name NAME           Tenant name
  --volume-maxsize Num{MB,GB,TB} - e.g. 2TB
                        Maximum size of the volume that can be created
  --add-rights create,delete,mount
                        Datastore access Permissions granted: Choices =
                        ['create', 'delete', 'mount', 'all']
  --datastore DATASTORE
                        Datastore name
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access ls --help 
usage: vmdkops_admin.py tenant access ls [-h] --name NAME

optional arguments:
  -h, --help   show this help message and exit
  --name NAME  The name of the tenant

pdhamdhere · 2016-10-14T15:04:44Z

@msterin I think you need to fix tests to remove "role" subcommand. For DS access, I only see maxVolSize. Does underlying API support maxNumVols or quota-Per-Datastore?

msterin · 2016-10-14T16:58:57Z

@pdhamdhere - yes, that's what happens in a rush :-). Thanks for the review, I will fix tests/ add numvol/quota and refresh the PR

govint · 2016-10-16T05:49:44Z

docs/misc/vmdkops-admin-cli-spec.md

@@ -11,7 +11,9 @@ All output from the admin cli defaults to human readable formats. It will be mad
 The majority of testing will be automated. We can ensure that parsing calls the right callbacks with
 the right information by generating representative input and mocking the callbacks to assert that
 the right information is parsed and delivered correctly. Additionally, and specifically for testing
-roles, we can create roles and then test that they act as expected by calling `role get`. Unit
+access control, we can create access controk definition (tenants and privileges) 


nit - "control"

govint · 2016-10-16T05:50:21Z

docs/misc/vmdkops-admin-cli-spec.md

@@ -11,7 +11,9 @@ All output from the admin cli defaults to human readable formats. It will be mad
 The majority of testing will be automated. We can ensure that parsing calls the right callbacks with
 the right information by generating representative input and mocking the callbacks to assert that
 the right information is parsed and delivered correctly. Additionally, and specifically for testing
-roles, we can create roles and then test that they act as expected by calling `role get`. Unit
+access control, we can create access controk definition (tenants and privileges) 
+and then test that they act as expected by invoking vmdk_ops commmands on behalf of a fake VM.


Can suggest using the dummy VM created with vim-cmd and say how that can be created.

govint · 2016-10-16T06:59:17Z

esx_service/cli/vmdkops_admin.py

             the subcommand. Subcommands can have further subcommands, but currently there is only
             one level of subcommands in this specification. Each subcommand can contain the same
             attributes as top level commands: (func, help, args, cmds). These attributes have
             identical usage to the top-level keys, except they only apply when the subcommand is
-             part of the command. For example the `--matches-vm` argument only applies to `role
-             create` or `role set` commands. It will be invalid in any other context.
+             part of the command. For example the `--matches-vm` argument only applies to `tenant


The --matches-vm option seems to be removed. Remove it from here as well.

govint · 2016-10-16T07:00:00Z

esx_service/cli/vmdkops_admin.py

@@ -121,6 +121,9 @@ def commands():
                                'attached-to', 'policy', 'capacity', 'used',
                                'fstype', 'access', 'attach-as'],
                    'metavar': 'Col1,Col2,...'
+                },
+                '--tenant' : {
+                    'help': 'Displays only VMs for a given tenant'


Are there any entities to display besides VMs, if not then can reword as "Displays VMs for a given tenant".

govint · 2016-10-16T07:03:34Z

esx_service/cli/vmdkops_admin.py

@@ -121,6 +121,9 @@ def commands():
                                'attached-to', 'policy', 'capacity', 'used',
                                'fstype', 'access', 'attach-as'],
                    'metavar': 'Col1,Col2,...'
+                },
+                '--tenant' : {
+                    'help': 'Displays only VMs for a given tenant'


Should --tenant be a part of the ls command? It seems to be listing volumes by default.

Instead we could have like:

vmdkops_admin with commands like
a. volume - ls, create, rm, set
b. tenant - ls, create, rm, set
c.

Which is how most of the admin cli commands are laid out?
which matches with esxcli type command options where we have broad categories within which the same CRUD options are available (list, create, delete, ....)

this command is supposed to do LS on volumes , filtered by a tenant.

govint · 2016-10-16T11:40:13Z

esx_service/cli/vmdkops_admin.py

+                      },
+                      '--remove-volumes': {
+                        'help': 'BE CAREFUL -  Removes this tenant volumes when removing a tenant'
+                      }


Or something like "WARNING: - Removes the named tenant's volumes from all datastores the tenant has access to.

I don't think it's much better

govint · 2016-10-16T11:40:40Z

esx_service/cli/vmdkops_admin.py

-                    'func': role_rm,
-                    'help': 'Delete a role',
+                    'func': tenant_rm,
+                    'help': 'Delete a tenant',


"Remove a tenant" as its tenant_rm vs. tenant_delete?

yes, all commands are "rm"

govint · 2016-10-16T11:51:32Z

esx_service/cli/vmdkops_admin.py

+                                    'metavar': 'create,delete,mount'
+                                }, 
+                                '--rm-rights': {
+                                    'help': 'Datastore access Permissions granted',


"Datastore access permissions removed"

Also, rm-rights must allow a way to specify a list of datastores or "all" datastores in one go.

govint · 2016-10-16T11:52:33Z

esx_service/cli/vmdkops_admin.py

+                                    'required': True
+                                },
+                                '--datastore': {
+                                    'help': "Datastore name",


This should really be a list, else users need to run the command for each and every datastore which is an effort.

I agree but I am not sure how to configure it. @andrewjstone - any hints here ?

I like the concept of accepting datastore list. However, that can be separate PR/improvement.

govint · 2016-10-16T12:57:07Z

esx_service/cli/vmdkops_admin.py

+                            'args': {
+                                '--name': {
+                                    'help': 'The name of the tenant',
+                                    'required': True


This should be a list, a use case would be user wants to retire a datastore. User should be able to get all tenants to stop using the datastore.

govint · 2016-10-16T13:16:23Z

esx_service/cli/vmdkops_admin.py

+                            'args': {
+                                '--name': {
+                                    'help': 'The name of the tenant',
+                                    'required': True


Again, list for named tenants or make required as "False" and when not provided list access for all tenants.

govint · 2016-10-16T13:17:40Z

esx_service/cli/vmdkops_admin.py

@@ -324,7 +430,7 @@ def make_list_of_values(allowed):
    Take a list of allowed values for an option and return a function that can be
    used to typecheck a string of given values and ensure they match the allowed
    values.  This is required to support options that take comma seperated lists
-    such as --rights in 'role set --rights=create,delete,mount'
+    such as --rights in 'tenant set --rights=create,delete,mount'


Reword this to mention --add-rights or --rm-rights for tenent access set command.

that is just an example in comments, it does not need to enumerate all usages

govint · 2016-10-16T13:18:21Z

esx_service/cli/vmdkops_admin.py

@@ -352,6 +458,8 @@ def ls(args):
    else:
        header = all_ls_headers()
        rows = generate_ls_rows()
+    if args.tenant:
+        print("TBD: print only VMS for the tenant ", args.tenant)  # TODO


"Print VMs included for the tenant."

govint

Comments on changes to options and command descriptions.

Note that the actual commands implementation is still TBD - this change only defines new command syntax and keywords

msterin · 2016-10-16T14:50:44Z

@govint - thanks for the review ! I replied to some of the comments and fixed the rest. Everything not replied to is fixed.

pdhamdhere

LGTM.

pdhamdhere · 2016-10-17T17:49:26Z

esx_service/cli/vmdkops_admin.py

+                                    'required': True
+                                },
+                                '--datastore': {
+                                    'help': "Datastore name",


I like the concept of accepting datastore list. However, that can be separate PR/improvement.

pdhamdhere · 2016-10-17T17:56:53Z

esx_service/cli/vmdkops_admin.py

-                    'func': role_rm,
-                    'help': 'Delete a role',
+                    'func': tenant_rm,
+                    'help': 'Delete a tenant',


Delete an "empty"? tenant?

pdhamdhere · 2016-10-19T17:18:34Z

CC/ @lipingxue Can you please review?

* master: (25 commits) Update new ESX IP added forgotten .so file Install sqlite3 py libs on ESX and load for Python2 Added py code and binaries for sqlite3 python libs Update drone security Removed accidental .pyc files Handle byte to string conversions for status command. Auth configuration and operation admission check (Auth.liping) (vmware-archive#603) Revert "Cli auth.liping" Cli auth.liping (vmware-archive#640) Handle missing or invalid fs type on mount. (vmware-archive#639) Updated Admin CLI commands to support tenants. (vmware-archive#620) Workaround older versions of e2fsprogs (vmware-archive#631) Add auth proposal Made handing of missing metafile less harsh. (vmware-archive#627) Fixed ACLs in payload bin dir (vmware-archive#624) Fixed error handling for set command. (vmware-archive#610) Use new error variables when rolling back volume creation to avoid nil reassignment. (vmware-archive#617) Change wording Fix broken link ...

vmwclabot added the cla-not-required label Oct 14, 2016

msterin force-pushed the cli-auth.msterin branch 2 times, most recently from be031a4 to 94106c3 Compare October 14, 2016 20:05

govint reviewed Oct 16, 2016

View reviewed changes

Mark Sterin added 2 commits October 16, 2016 15:34

Updated Admin CLI commands to support tenants.

bdb62ba

Note that the actual commands implementation is still TBD - this change only defines new command syntax and keywords

Addressed @pdhamdhere comments

c7845ef

Fixes to adress @govint comments

a6ca62f

msterin force-pushed the cli-auth.msterin branch from b0379ee to a6ca62f Compare October 16, 2016 14:51

pdhamdhere approved these changes Oct 17, 2016

View reviewed changes

govint approved these changes Oct 19, 2016

View reviewed changes

lipingxue mentioned this pull request Oct 21, 2016

Cli auth.liping #640

Merged

msterin merged commit 153cf67 into master Oct 21, 2016

msterin deleted the cli-auth.msterin branch October 22, 2016 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Admin CLI commands to support tenants. #620

Updated Admin CLI commands to support tenants. #620

msterin commented Oct 14, 2016 •

edited

Loading

pdhamdhere commented Oct 14, 2016

msterin commented Oct 14, 2016 •

edited

Loading

govint Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016 •

edited

Loading

msterin Oct 16, 2016

govint Oct 16, 2016 •

edited

Loading

msterin Oct 16, 2016

govint Oct 16, 2016

msterin Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016

msterin Oct 16, 2016

pdhamdhere Oct 17, 2016

govint Oct 16, 2016

govint Oct 16, 2016

govint Oct 16, 2016

msterin Oct 16, 2016

govint Oct 16, 2016

govint left a comment

msterin commented Oct 16, 2016 •

edited

Loading

pdhamdhere left a comment

pdhamdhere Oct 17, 2016

pdhamdhere Oct 17, 2016

pdhamdhere commented Oct 19, 2016

Updated Admin CLI commands to support tenants. #620

Updated Admin CLI commands to support tenants. #620

Conversation

msterin commented Oct 14, 2016 • edited Loading

pdhamdhere commented Oct 14, 2016

msterin commented Oct 14, 2016 • edited Loading

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

govint Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint Oct 16, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

msterin commented Oct 16, 2016 • edited Loading

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdhamdhere commented Oct 19, 2016

msterin commented Oct 14, 2016 •

edited

Loading

msterin commented Oct 14, 2016 •

edited

Loading

govint Oct 16, 2016 •

edited

Loading

govint Oct 16, 2016 •

edited

Loading

msterin commented Oct 16, 2016 •

edited

Loading