Add ansible support for teams 1 by r-2st · Pull Request #39 · ns1/ns1-ansible-modules

r-2st · 2021-04-15T01:48:51Z

Hey there NS1 team,

Please review the following pull request to add Ansible support for NS1 portal teams. Tried to mimic how code is structured in the zone module but odds are there are still somethings you will see that could be improved to make things fit with the existing repo arch.

Both --diff & --check are currently supported.

Feel free to let me know if there are any question.

-Ross

ANSIBLE PLAYBOOK TEST RUN


PLAY [localhost] ***************************************************************

TASK [Gathering Facts] *********************************************************
ok: [localhost]

TASK [setup] *******************************************************************
ok: [localhost]

TASK [Verify setup] ************************************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Test team creation] ******************************************************
--- before
+++ after
@@ -1 +1,36 @@
-{}
+{
+    "ip_whitelist": [],
+    "name": "skipper",
+    "permissions": {
+        "account": {
+            "manage_account_settings": false,
+            "manage_apikeys": false,
+            "manage_payment_methods": false,
+            "manage_plan": false,
+            "manage_teams": false,
+            "manage_users": false,
+            "view_activity_log": false,
+            "view_invoices": false
+        },
+        "data": {
+            "manage_datafeeds": false,
+            "manage_datasources": false,
+            "push_to_datafeeds": false
+        },
+        "dns": {
+            "manage_zones": false,
+            "view_zones": false,
+            "zones_allow": [],
+            "zones_allow_by_default": false,
+            "zones_deny": []
+        },
+        "monitoring": {
+            "manage_jobs": false,
+            "manage_lists": false,
+            "view_jobs": false
+        },
+        "security": {
+            "manage_global_2fa": false
+        }
+    }
+}

changed: [localhost]

TASK [Verify team creation] ****************************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Test team ip whitelist] **************************************************
--- before
+++ after
@@ -1,5 +1,19 @@
 {
-    "ip_whitelist": [],
+    "ip_whitelist": [
+        {
+            "name": "Home Whitelist",
+            "values": [
+                "104.20.48.182"
+            ]
+        },
+        {
+            "name": "Multi Whitelist",
+            "values": [
+                "104.20.49.0/24",
+                "104.20.50.1"
+            ]
+        }
+    ],
     "name": "skipper",
     "permissions": {
         "account": {

changed: [localhost]

TASK [Verify team ip whitelist] ************************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Add all monitoring permissions] ******************************************
--- before
+++ after
@@ -1,19 +1,5 @@
 {
-    "ip_whitelist": [
-        {
-            "name": "Home Whitelist",
-            "values": [
-                "104.20.48.182"
-            ]
-        },
-        {
-            "name": "Multi Whitelist",
-            "values": [
-                "104.20.49.0/24",
-                "104.20.50.1"
-            ]
-        }
-    ],
+    "ip_whitelist": [],
     "name": "skipper",
     "permissions": {
         "account": {
@@ -39,9 +25,9 @@
             "zones_deny": []
         },
         "monitoring": {
-            "manage_jobs": false,
-            "manage_lists": false,
-            "view_jobs": false
+            "manage_jobs": true,
+            "manage_lists": true,
+            "view_jobs": true
         },
         "security": {
             "manage_global_2fa": false

changed: [localhost]

TASK [Verify permissions are set] **********************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Add all account permissions] *********************************************
--- before
+++ after
@@ -3,14 +3,15 @@
     "name": "skipper",
     "permissions": {
         "account": {
-            "manage_account_settings": false,
-            "manage_apikeys": false,
-            "manage_payment_methods": false,
-            "manage_plan": false,
-            "manage_teams": false,
-            "manage_users": false,
-            "view_activity_log": false,
-            "view_invoices": false
+            "manage_account_settings": true,
+            "manage_apikeys": true,
+            "manage_ip_whitelist": true,
+            "manage_payment_methods": true,
+            "manage_plan": true,
+            "manage_teams": true,
+            "manage_users": true,
+            "view_activity_log": true,
+            "view_invoices": true
         },
         "data": {
             "manage_datafeeds": false,
@@ -25,9 +26,9 @@
             "zones_deny": []
         },
         "monitoring": {
-            "manage_jobs": true,
-            "manage_lists": true,
-            "view_jobs": true
+            "manage_jobs": false,
+            "manage_lists": false,
+            "view_jobs": false
         },
         "security": {
             "manage_global_2fa": false

changed: [localhost]

TASK [Verify permissions are set] **********************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Add all data permissions] ************************************************
--- before
+++ after
@@ -3,20 +3,19 @@
     "name": "skipper",
     "permissions": {
         "account": {
-            "manage_account_settings": true,
-            "manage_apikeys": true,
-            "manage_ip_whitelist": true,
-            "manage_payment_methods": true,
-            "manage_plan": true,
-            "manage_teams": true,
-            "manage_users": true,
-            "view_activity_log": true,
-            "view_invoices": true
+            "manage_account_settings": false,
+            "manage_apikeys": false,
+            "manage_payment_methods": false,
+            "manage_plan": false,
+            "manage_teams": false,
+            "manage_users": false,
+            "view_activity_log": false,
+            "view_invoices": false
         },
         "data": {
-            "manage_datafeeds": false,
-            "manage_datasources": false,
-            "push_to_datafeeds": false
+            "manage_datafeeds": true,
+            "manage_datasources": true,
+            "push_to_datafeeds": true
         },
         "dns": {
             "manage_zones": false,

changed: [localhost]

TASK [Verify permissions are set] **********************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Add all security permissions] ********************************************
--- before
+++ after
@@ -13,9 +13,9 @@
             "view_invoices": false
         },
         "data": {
-            "manage_datafeeds": true,
-            "manage_datasources": true,
-            "push_to_datafeeds": true
+            "manage_datafeeds": false,
+            "manage_datasources": false,
+            "push_to_datafeeds": false
         },
         "dns": {
             "manage_zones": false,
@@ -30,7 +30,7 @@
             "view_jobs": false
         },
         "security": {
-            "manage_global_2fa": false
+            "manage_global_2fa": true
         }
     }
 }

changed: [localhost]

TASK [Verify permissions are set] **********************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Add all dns permissions] *************************************************
--- before
+++ after
@@ -18,11 +18,15 @@
             "push_to_datafeeds": false
         },
         "dns": {
-            "manage_zones": false,
-            "view_zones": false,
-            "zones_allow": [],
-            "zones_allow_by_default": false,
-            "zones_deny": []
+            "manage_zones": true,
+            "view_zones": true,
+            "zones_allow": [
+                "primary-pharmakom.net,linked-pharmakom.net"
+            ],
+            "zones_allow_by_default": true,
+            "zones_deny": [
+                "secondary-pharmakom.net"
+            ]
         },
         "monitoring": {
             "manage_jobs": false,
@@ -30,7 +34,7 @@
             "view_jobs": false
         },
         "security": {
-            "manage_global_2fa": true
+            "manage_global_2fa": false
         }
     }
 }

changed: [localhost]

TASK [Verify permissions are set] **********************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [Test team deletion] ******************************************************
--- before
+++ after
@@ -1,40 +1 @@
-{
-    "ip_whitelist": [],
-    "name": "skipper",
-    "permissions": {
-        "account": {
-            "manage_account_settings": false,
-            "manage_apikeys": false,
-            "manage_payment_methods": false,
-            "manage_plan": false,
-            "manage_teams": false,
-            "manage_users": false,
-            "view_activity_log": false,
-            "view_invoices": false
-        },
-        "data": {
-            "manage_datafeeds": false,
-            "manage_datasources": false,
-            "push_to_datafeeds": false
-        },
-        "dns": {
-            "manage_zones": true,
-            "view_zones": true,
-            "zones_allow": [
-                "primary-pharmakom.net,linked-pharmakom.net"
-            ],
-            "zones_allow_by_default": true,
-            "zones_deny": [
-                "secondary-pharmakom.net"
-            ]
-        },
-        "monitoring": {
-            "manage_jobs": false,
-            "manage_lists": false,
-            "view_jobs": false
-        },
-        "security": {
-            "manage_global_2fa": false
-        }
-    }
-}
+{}

changed: [localhost]

TASK [Verify team deletion] ****************************************************
ok: [localhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

PLAY RECAP *********************************************************************
localhost                  : ok=19   changed=8    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

mburtless · 2021-04-16T19:19:47Z

Hi there! Thanks so much for the contribution, I'll work to get a review on this shortly.

Minor correction.

mburtless

This is looking good. Structurally, odds are any permissions logic used in Teams will be resused if we add users or apikeys support, so it would be great to pull the schema and methods relating specifically to permissions out and put them in a shared place in module_utils. However, I don't think that should prevent this PR from moving forward, that can be done when the next module is created imo.

A more immediate issue is that permissions are different between the two products that our SDKs and ansible modules support: DDI and managed DNS. Some subset of the permissions are incompatible and, if no permissions are included, the py sdk has separate defaults that it uses for each product.

For this reason, I think the ansible module will need to provide a way for the user to set which product it is working with. We'll need to add a ddi param to this module. This param should be inspected in the _build_ns1 method of NS1ModuleBase and, if true, should should set self.config["ddi"] = self.module.params["ddi"].

library/ns1_team.py

mburtless · 2021-04-20T20:43:36Z

library/ns1_team.py

+                team = self.create(built_changes)
+            else:
+                team = self.update(team_id, built_changes)
+        if team != before:


Will this comparison work as expected? IP whitelist is really a set, from the API's perspective, not a list, as order in not respected. The order in the returned team may differ from before during an update, but as long as the contents are the same, that should not constitute a change.

mburtless · 2021-04-20T20:49:49Z

library/ns1_team.py

+            if team_id is None:
+                team = self.create(built_changes)
+            else:
+                team = self.update(team_id, built_changes)


It seems like this will trigger an update regardless of whether or not there is an actual change to be made. Is that intentional? This method already knows what before looks like, so it should be able to detect if it actually needs to make an update, right?

library/ns1_team.py

mburtless · 2021-04-20T21:01:21Z

tests/ns1_team.yaml

+      assert:
+        that:
+          - team_ip_whitelist is changed
+          - team_ip_whitelist.diff.after.ip_whitelist | length != 0


Would be good to assert on the values of the whitelist, just knowing len > 0 doesn't ensure the values were interpolated correctly by our module.

mburtless · 2021-04-20T21:02:40Z

tests/ns1_team.yaml

+    - name: Verify permissions are set
+      assert:
+        that:
+          - team_monitoring is changed


Ditto, would be good to assert on the values that were updated to ensure the correct changes were made.

library/ns1_team.py

r-2st added 2 commits April 15, 2021 11:38

Adding team files.

4acd190

Cleaning up test file.

a4d1b8f

Update ns1_team.py

a8e4785

Minor correction.

mburtless suggested changes Apr 20, 2021

View reviewed changes

r-2st commented May 4, 2021

View reviewed changes

library/ns1_team.py Show resolved Hide resolved

r-2st commented May 4, 2021

View reviewed changes

library/ns1_team.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ansible support for teams 1#39

Add ansible support for teams 1#39
r-2st wants to merge 3 commits intons1:masterfrom
r-2st:add-ansible-support-for-teams-1

r-2st commented Apr 15, 2021

Uh oh!

mburtless commented Apr 16, 2021

Uh oh!

mburtless left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mburtless Apr 20, 2021

Uh oh!

mburtless Apr 20, 2021

Uh oh!

Uh oh!

mburtless Apr 20, 2021

Uh oh!

mburtless Apr 20, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

r-2st commented Apr 15, 2021

Uh oh!

mburtless commented Apr 16, 2021

Uh oh!

mburtless left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mburtless Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

mburtless Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mburtless Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

mburtless Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants