new CS module cs_role_permission #37065

dpassante · 2018-03-06T11:21:29Z

SUMMARY

Add a new CloudStack module to manage role permissions.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

ANSIBLE VERSION

$ ansible --version
ansible 2.6.0 (devel c3ee2188a9) last updated 2018/03/06 12:05:56 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/dpassante/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /mnt/filer/Github/ansible/lib/ansible
  executable location = /mnt/filer/Github/ansible/bin/ansible
  python version = 2.7.6 (default, Nov 23 2017, 15:49:48) [GCC 4.8.4]

ADDITIONAL INFORMATION

This module is working with Cloudstack 4.11. Some integration tests have been commented out to be compliant with the Cloudstack 4.9.2 integration environment.
Everything seem's to work with Cloudstack 4.9.2 except the dynamic role permission rules update.

ansibot · 2018-03-06T11:27:50Z

cc @mattclay @resmo
click here for bot help

resmo · 2018-03-06T11:31:25Z

test/sanity/validate-modules/ignore.txt

@@ -413,6 +413,7 @@ lib/ansible/modules/cloud/cloudstack/cs_project.py E325
 lib/ansible/modules/cloud/cloudstack/cs_region.py E324
 lib/ansible/modules/cloud/cloudstack/cs_resourcelimit.py E324
 lib/ansible/modules/cloud/cloudstack/cs_role.py E324
+lib/ansible/modules/cloud/cloudstack/cs_role_permission.py E324


this is ok for now, I am working on a fix for E324

resmo · 2018-03-06T11:33:48Z

@dpassante thanks for this new module. would you like to join me in maintaining the cloudstack namespace?

dpassante · 2018-03-06T12:38:48Z

@resmo Yes! I would be happy to help maintaining this namespace.

ansibot · 2018-03-07T14:04:54Z

@dpassante this PR contains the following merge commits:

25d8231

Please rebase your branch to remove these commits.

click here for bot help

ansibot · 2018-03-07T15:28:06Z

@DazWorrall @jeffersongirao @marcaurele @netservers

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

resmo

Thanks for this. Looks pretty good. Hadn't had the chance to test it (especially the comp with 4.9 issue), can you explain a bit what and how it fails? Any suggestion to make it "not fail" for 4.9?

resmo · 2018-03-07T16:05:33Z

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

+        super(AnsibleCloudStackRolePermission, self).__init__(module)
+        self.returns = {
+            'id': 'id',
+            'roleid': 'roleid',


i would prefer role_id here

would you mind change this to the following?

'roleid': 'role_id',

Sorry! Done.

dpassante · 2018-03-07T22:02:14Z

The ability to dynamically update permission of existing rules has been introduced in the 4.11 release by this issue.
Cloudstack 4.9 only supports updating rules order.

Trying to update a permission (e.g. from "allow" to "deny") in 4.9 now returns the following as 'ruleorder' argument is required by the 4.9 'updateRolePermission' API method:

TASK [cs_role_permission : test update role permission] *****************************************************************************************************
fatal: [testhost]: FAILED! => {"api_http_method": "get", "api_key": "ClwInp7F5FC0IFNJGGf5u9QlqecLvsK5vhs6sV9jGtYr8JOy_zxAwEUxJ6y_hW4hVUWSkqlAR9hiJfFG_8T2jw", "api_region": "cloudstack", "api_timeout": "60", "api_url": "http://localhost:8888/client/api", "changed": true, "msg": "CloudStackException: ('HTTP 431 response from CloudStack', <Response [431]>, {'uuidList': [], 'errorcode': 431, 'cserrorcode': 9999, 'errortext': 'Unable to execute API command updaterolepermission due to missing parameter ruleorder'})"}

The 'ruleid' / 'permission' couple and 'ruleorder' are mutually exclusive in Cloudstack 4.11.

The options I have in mind to avoid any issue with 4.9 would be:

Catch the Cloudstack response and return an error like ‘Updating rule permission is not supported on cs <= 4.11’ for 4.9.
Remove the ability to update rule permission and never do any update except if 'parent' argument is passed

On the first case, the behavior of the module would be different depending on the version of Cloudstack but the new 4.11 feature would be supported.
On the second, we would have the choice to return that nothing has been changed or to return an error when 'permission' argument of the playbook doesn't match the actual rule permission.

dpassante · 2018-03-09T14:57:46Z

Another option is to return a 'skipped' result with an explicit message when the permission update fail.
The task doesn't fail and the user is aware that the permission wasn't updated.
The behavior of the module would be something like this: dpassante@d25484e

Or outright replace the existing rule by a new one with the updated permission for Cloudstack versions that don't natively support it ? :)

resmo · 2018-04-25T06:34:38Z

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

+    name: "createVPC"
+    permission: "deny"
+
+# Updade rules order. move the rule at the top of list


resmo · 2018-04-25T06:38:14Z

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

+
+        if self._get_role_perm():
+            for _rule in self._get_role_perm():
+                if rule == _rule['rule'] or rule in _rule['id']:


shouldn't this be like the following?

if rule == _rule['rule'] or rule == _rule['id']:

Yes, it seems to be a more appropriate syntax

resmo · 2018-04-25T06:43:05Z

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

+            'name',
+            'permission',
+        ]
+        self.module.fail_on_missing_params(required_params=required_params)


IMHO this check is not required, because name is always required and permission is defaulted to something (not None). Any thoughts?

You're right. I'll remove this check.

resmo · 2018-04-25T06:49:30Z

lib/ansible/modules/cloud/cloudstack/cs_role_permission.py

+    description:
+      - The rule permission, allow or deny. Defaulted to deny.
+    choices: [ allow, deny ]
+    required: false


not a blocker but required: false is the default, can be omitted in the docs (was needed in the past)

resmo · 2018-04-27T11:39:33Z

Thanks for the update, waiting for the build to finish before merge

dpassante · 2018-04-27T11:40:33Z

@resmo Thanks for review!

resmo · 2018-04-27T11:47:53Z

;) btw, I think about to update the cloudstack version for integration testing to 4.11 (once 4.11.1 is released) any concerns? Or other preferences?

dpassante · 2018-04-27T12:16:17Z

I think it's a good idea to base integration testing to the latest LTS major release.
The ideal would be to have the 4.11 as default but keeping the ability to run tests on the 4.9.2 to check backward compatibility. But this can already be done by managing docker images manually...

resmo · 2018-04-27T13:38:52Z

shipit

resmo reviewed Mar 6, 2018

View reviewed changes

resmo self-assigned this Mar 6, 2018

ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 6, 2018

resmo mentioned this pull request Mar 6, 2018

botmeta: new cloudstack maintainer #37081

Merged

dpassante force-pushed the devel branch from 25d8231 to b278f20 Compare March 7, 2018 15:12

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 7, 2018

resmo reviewed Mar 7, 2018

View reviewed changes

resmo reviewed Apr 25, 2018

View reviewed changes

dpassante added 3 commits April 27, 2018 11:17

new CS module cs_role_permission

c78c4f7

make cs_role_permission work with Cloudstack <= 4.11

c499ae5

normalise return

ac56174

dpassante force-pushed the devel branch from f4853fd to ac56174 Compare April 27, 2018 09:19

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 27, 2018

dpassante added 2 commits April 27, 2018 11:23

cs_role_permission: fix documentation

576210b

cs_role_permission: remove unneeded params checking

fbda0e3

ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 27, 2018

resmo merged commit 200a0bc into ansible:devel Apr 27, 2018

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 12, 2018

cloudstack: new module cs_role_permission (ansible#37065)

6c1c77d

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018

cloudstack: new module cs_role_permission (ansible#37065)

0d44875

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018

cloudstack: new module cs_role_permission (ansible#37065)

679d71e

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018

cloudstack: new module cs_role_permission (ansible#37065)

27302ea

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018

cloudstack: new module cs_role_permission (ansible#37065)

66cff67

ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018

cloudstack: new module cs_role_permission (ansible#37065)

ead53d7

ansible locked and limited conversation to collaborators Apr 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new CS module cs_role_permission #37065

new CS module cs_role_permission #37065

dpassante commented Mar 6, 2018

ansibot commented Mar 6, 2018

resmo Mar 6, 2018

resmo commented Mar 6, 2018

dpassante commented Mar 6, 2018

ansibot commented Mar 7, 2018

ansibot commented Mar 7, 2018

resmo left a comment

resmo Mar 7, 2018

resmo Apr 4, 2018

dpassante Apr 4, 2018

dpassante commented Mar 7, 2018

dpassante commented Mar 9, 2018

resmo Apr 25, 2018

resmo Apr 25, 2018

dpassante Apr 27, 2018

resmo Apr 25, 2018

dpassante Apr 27, 2018

resmo Apr 25, 2018

resmo commented Apr 27, 2018

dpassante commented Apr 27, 2018

resmo commented Apr 27, 2018

dpassante commented Apr 27, 2018

resmo commented Apr 27, 2018

new CS module cs_role_permission #37065

new CS module cs_role_permission #37065

Conversation

dpassante commented Mar 6, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Mar 6, 2018

Choose a reason for hiding this comment

resmo commented Mar 6, 2018

dpassante commented Mar 6, 2018

ansibot commented Mar 7, 2018

ansibot commented Mar 7, 2018

resmo 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

dpassante commented Mar 7, 2018

dpassante commented Mar 9, 2018

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

resmo commented Apr 27, 2018

dpassante commented Apr 27, 2018

resmo commented Apr 27, 2018

dpassante commented Apr 27, 2018

resmo commented Apr 27, 2018