Allow Netbox device modification (#53631)

* netbox_device: Allow device modification

* Add ability to update and existing device
* Allow check_mode
* Fail when device name is missing
* Fail when cannot resolve ID instead of taking ID 1 by default

* netbox_device: Add diff output

* netbox: Some refactoring

* Add diff output and check_mode to netbox_ip_address
* Deduplicate redundant code into netbox_utils

* netbox_utils: A few unit tests

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py

@@ -237,7 +237,7 @@ def update_netbox_object(nb_obj, data, check_mode):
 
         if not check_mode:
             nb_obj.update(data)
-            udpated_obj = nb_obj.serialize()
+            updated_obj = nb_obj.serialize()
 
         diff = _build_diff(before=data_before, after=data_after)
         return updated_obj, diff
@@ -312,7 +312,7 @@ def find_ids(nb, data):
                 try:
                     query_id = nb_endpoint.get(**{QUERY_TYPES.get(k, "q"): search})
                 except ValueError:
-                    return ValueError(
+                    raise ValueError(
                         "Multiple results found while searching for key: %s" % (k)
                     )
 
@@ -320,6 +320,8 @@ def find_ids(nb, data):
                 data[k] = id_list
             elif query_id:
                 data[k] = query_id.id
+            elif k in NO_DEFAULT_ID:
+                pass
             else:
                 raise ValueError("Could not resolve id of %s: %s" % (k, v))
 

lib/ansible/modules/net_tools/netbox/netbox_device.py

@@ -14,9 +14,9 @@ ANSIBLE_METADATA = {'metadata_version': '1.1',
 DOCUMENTATION = r'''
 ---
 module: netbox_device
-short_description: Create or delete devices within Netbox
+short_description: Create, update or delete devices within Netbox
 description:
-  - Creates or removes devices from Netbox
+  - Creates, updates or removes devices from Netbox
 notes:
   - Tags should be defined as a YAML list
   - This should be ran with connection C(local) and hosts C(localhost)
@@ -42,12 +42,13 @@ options:
       name:
         description:
           - The name of the device
+        required: true
       device_type:
         description:
-          - Required if I(state=present)
+          - Required if I(state=present) and the device does not exist yet
       device_role:
         description:
-          - Required if I(state=present)
+          - Required if I(state=present) and the device does not exist yet
       tenant:
         description:
           - The tenant that the device will be assigned to
@@ -62,7 +63,7 @@ options:
           - Asset tag that is associated to the device
       site:
         description:
-          - Required if I(state=present)
+          - Required if I(state=present) and the device does not exist yet
       rack:
         description:
           - The name of the rack to assign the device to
@@ -119,7 +120,7 @@ EXAMPLES = r'''
         netbox_url: http://netbox.local
         netbox_token: thisIsMyToken
         data:
-          name: Test (not really required, but helpful)
+          name: Test Device
           device_type: C9410R
           device_role: Core Switch
           site: Main
@@ -130,7 +131,7 @@ EXAMPLES = r'''
         netbox_url: http://netbox.local
         netbox_token: thisIsMyToken
         data:
-          name: Test
+          name: Test Device
         state: absent
 
     - name: Create device with tags
@@ -138,7 +139,7 @@ EXAMPLES = r'''
         netbox_url: http://netbox.local
         netbox_token: thisIsMyToken
         data:
-          name: Test
+          name: Another Test Device
           device_type: C9410R
           device_role: Core Switch
           site: Main
@@ -146,24 +147,22 @@ EXAMPLES = r'''
             - Schnozzberry
         state: present
 
-    - name: Create device and assign to rack and position
+    - name: Update the rack and position of an existing device
       netbox_device:
         netbox_url: http://netbox.local
         netbox_token: thisIsMyToken
         data:
-          name: Test
+          name: Test Device
-          device_type: C9410R
-          device_role: Core Switch
-          site: Main
           rack: Test Rack
           position: 10
           face: Front
+        state: present
 '''
 
 RETURN = r'''
 device:
   description: Serialized object as created or already existent within Netbox
-  returned: on creation
+  returned: success (when I(state=present))
   type: dict
 msg:
   description: Message indicating failure or info about what has been achieved
@@ -175,7 +174,15 @@ import json
 import traceback
 
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
-from ansible.module_utils.net_tools.netbox.netbox_utils import find_ids, normalize_data, DEVICE_STATUS, FACE_ID
+from ansible.module_utils.net_tools.netbox.netbox_utils import (
+    find_ids,
+    normalize_data,
+    create_netbox_object,
+    delete_netbox_object,
+    update_netbox_object,
+    DEVICE_STATUS,
+    FACE_ID
+)
 
 PYNETBOX_IMP_ERR = None
 try:
@@ -198,13 +205,18 @@ def main():
         validate_certs=dict(type="bool", default=True)
     )
 
+    global module
     module = AnsibleModule(argument_spec=argument_spec,
-                           supports_check_mode=False)
+                           supports_check_mode=True)
 
     # Fail module if pynetbox is not installed
     if not HAS_PYNETBOX:
         module.fail_json(msg=missing_required_lib('pynetbox'), exception=PYNETBOX_IMP_ERR)
 
+    # Fail if device name is not given
+    if not module.params["data"].get("name"):
+        module.fail_json(msg="missing device name")
+
     # Assign variables to be used with module
     app = 'dcim'
     endpoint = 'devices'
@@ -228,59 +240,72 @@ def main():
     norm_data = normalize_data(data)
     try:
         if 'present' in state:
-            return module.exit_json(
+            result = ensure_device_present(nb, nb_endpoint, norm_data)
-                **ensure_device_present(nb, nb_endpoint, norm_data)
-            )
         else:
-            return module.exit_json(
+            result = ensure_device_absent(nb_endpoint, norm_data)
-                **ensure_device_absent(nb_endpoint, norm_data)
+        return module.exit_json(**result)
-            )
     except pynetbox.RequestError as e:
         return module.fail_json(msg=json.loads(e.error))
+    except ValueError as e:
+        return module.fail_json(msg=str(e))
 
 
-def ensure_device_present(nb, nb_endpoint, data):
+def _find_ids(nb, data):
+    if data.get("status"):
+        data["status"] = DEVICE_STATUS.get(data["status"].lower())
+    if data.get("face"):
+        data["face"] = FACE_ID.get(data["face"].lower())
+    return find_ids(nb, data)
+
+
+def ensure_device_present(nb, nb_endpoint, normalized_data):
     '''
-    :returns dict(device, msg, changed): dictionary resulting of the request,
+    :returns dict(device, msg, changed, diff): dictionary resulting of the request,
         where `device` is the serialized device fetched or newly created in
         Netbox
     '''
+    data = _find_ids(nb, normalized_data)
     nb_device = nb_endpoint.get(name=data["name"])
+    result = {}
     if not nb_device:
-        device = _netbox_create_device(nb, nb_endpoint, data).serialize()
+        device, diff = create_netbox_object(nb_endpoint, data, module.check_mode)
-        changed = True
         msg = "Device %s created" % (data["name"])
+        changed = True
+        result["diff"] = diff
     else:
-        msg = "Device %s already exists" % (data["name"])
+        device, diff = update_netbox_object(nb_device, data, module.check_mode)
-        device = nb_device.serialize()
+        if device is False:
-        changed = False
+            module.fail_json(
-
+                msg="Request failed, couldn't update device: %s" % data["name"]
-    return {"device": device, "msg": msg, "changed": changed}
+            )
-
+        if diff:
-
+            msg = "Device %s updated" % (data["name"])
-def _netbox_create_device(nb, nb_endpoint, data):
+            changed = True
-    if data.get("status"):
+            result["diff"] = diff
-        data["status"] = DEVICE_STATUS.get(data["status"].lower(), 0)
+        else:
-    if data.get("face"):
+            msg = "Device %s already exists" % (data["name"])
-        data["face"] = FACE_ID.get(data["face"].lower(), 0)
+            changed = False
-    data = find_ids(nb, data)
+    result.update({"device": device, "changed": changed, "msg": msg})
-    return nb_endpoint.create(data)
+    return result
 
 
 def ensure_device_absent(nb_endpoint, data):
     '''
-    :returns dict(msg, changed)
+    :returns dict(msg, changed, diff)
     '''
-    device = nb_endpoint.get(name=data["name"])
+    nb_device = nb_endpoint.get(name=data["name"])
-    if device:
+    result = {}
-        device.delete()
+    if nb_device:
+        dummy, diff = delete_netbox_object(nb_device, module.check_mode)
         msg = 'Device %s deleted' % (data["name"])
         changed = True
+        result["diff"] = diff
     else:
         msg = 'Device %s already absent' % (data["name"])
         changed = False
 
-    return {"msg": msg, "changed": changed}
+    result.update({"changed": changed, "msg": msg})
+    return result
 
 
 if __name__ == "__main__":

lib/ansible/modules/net_tools/netbox/netbox_ip_address.py

@@ -223,7 +223,14 @@ import json
 import traceback
 
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
-from ansible.module_utils.net_tools.netbox.netbox_utils import find_ids, normalize_data, IP_ADDRESS_ROLE, IP_ADDRESS_STATUS
+from ansible.module_utils.net_tools.netbox.netbox_utils import (
+    find_ids,
+    normalize_data,
+    create_netbox_object,
+    delete_netbox_object,
+    IP_ADDRESS_ROLE,
+    IP_ADDRESS_STATUS
+)
 from ansible.module_utils.compat import ipaddress
 from ansible.module_utils._text import to_text
 
@@ -249,8 +256,9 @@ def main():
         validate_certs=dict(type="bool", default=True)
     )
 
+    global module
     module = AnsibleModule(argument_spec=argument_spec,
-                           supports_check_mode=False)
+                           supports_check_mode=True)
 
     # Fail module if pynetbox is not installed
     if not HAS_PYNETBOX:
@@ -383,11 +391,11 @@ def create_ip_address(nb_endpoint, data):
         changed = False
         return {"msg": data, "changed": changed}
 
-    ip_addr = nb_endpoint.create(data).serialize()
+    ip_addr, diff = create_netbox_object(nb_endpoint, data, module.check_mode)
     changed = True
     msg = "IP Addresses %s created" % (data["address"])
 
-    return {"ip_address": ip_addr, "msg": msg, "changed": changed}
+    return {"ip_address": ip_addr, "msg": msg, "changed": changed, "diff": diff}
 
 
 def ensure_ip_in_prefix_present_on_netif(nb_app, nb_endpoint, data):
@@ -424,21 +432,24 @@ def get_new_available_ip_address(nb_app, data):
     if data.get("vrf"):
         prefix_query["vrf_id"] = data["vrf"]
 
+    result = {}
     prefix = nb_app.prefixes.get(**prefix_query)
     if not prefix:
         changed = False
         msg = "%s does not exist - please create first" % (data["prefix"])
         return {"msg": msg, "changed": changed}
     elif prefix.available_ips.list():
-        ip_addr = prefix.available_ips.create(data)
+        ip_addr, diff = create_netbox_object(prefix.available_ips, data, module.check_mode)
         changed = True
         msg = "IP Addresses %s created" % (ip_addr["address"])
+        result["diff"] = diff
     else:
         changed = False
         msg = "No available IPs available within %s" % (data['prefix'])
         return {"msg": msg, "changed": changed}
 
-    return {"ip_address": ip_addr, "msg": msg, "changed": changed}
+    result.update({"ip_address": ip_addr, "msg": msg, "changed": changed})
+    return result
 
 
 def _get_prefix_id(nb_app, prefix, vrf_id=None):
@@ -476,15 +487,18 @@ def ensure_ip_address_absent(nb_endpoint, data):
     except ValueError:
         return _error_multiple_ip_results(data)
 
+    result = {}
     if ip_addr:
-        ip_addr.delete()
+        dummy, diff = delete_netbox_object(ip_addr, module.check_mode)
         changed = True
         msg = "IP Address %s deleted" % (data["address"])
+        result["diff"] = diff
     else:
         changed = False
         msg = "IP Address %s already absent" % (data["address"])
 
-    return {"msg": msg, "changed": changed}
+    result.update({"msg": msg, "changed": changed})
+    return result
 
 
 if __name__ == "__main__":

test/units/module_utils/net_tools/test_netbox.py

@@ -0,0 +1,152 @@
+# -*- coding: utf-8 -*-
+# Copyright: (c) 2019, Bruno Inec (@sweenu) <bruno@inec.fr>
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+import pytest
+
+from ansible.module_utils.net_tools.netbox.netbox_utils import (
+    QUERY_TYPES,
+    _build_diff,
+    create_netbox_object,
+    delete_netbox_object,
+    update_netbox_object,
+    normalize_data,
+)
+
+
+def test_normalize_data():
+    assert "name" not in QUERY_TYPES
+    assert QUERY_TYPES.get("rack") == "slug"
+    assert QUERY_TYPES.get("primary_ip") != "slug"
+
+    raw_data = {
+        "name": "Some name",
+        "primary_ip": "10.3.72.74/31",
+        "rack": "Some rack",
+    }
+    normalized_data = raw_data.copy()
+    normalized_data["rack"] = "some-rack"
+
+    assert normalize_data(raw_data) == normalized_data
+
+
+def test_build_diff():
+    before = "The state before"
+    after = {"A": "more", "complicated": "state"}
+    diff = _build_diff(before=before, after=after)
+    assert diff == {"before": before, "after": after}
+
+
+@pytest.fixture
+def nb_obj_mock(mocker):
+    serialized_object = {"The serialized": "object"}
+    nb_obj = mocker.Mock(name="nb_obj_mock")
+    nb_obj.delete.return_value = True
+    nb_obj.update.return_value = True
+    nb_obj.update.side_effect = serialized_object.update
+    nb_obj.serialize.return_value = serialized_object
+
+    return nb_obj
+
+
+@pytest.fixture
+def endpoint_mock(mocker, nb_obj_mock):
+    endpoint = mocker.Mock(name="endpoint_mock")
+    endpoint.create.return_value = nb_obj_mock
+
+    return endpoint
+
+
+@pytest.fixture
+def on_creation_diff():
+    return _build_diff(before={"state": "absent"}, after={"state": "present"})
+
+
+@pytest.fixture
+def on_deletion_diff():
+    return _build_diff(before={"state": "present"}, after={"state": "absent"})
+
+
+@pytest.fixture
+def data():
+    return {"name": "Some Netbox object name"}
+
+
+def test_create_netbox_object(endpoint_mock, data, on_creation_diff):
+    return_value = endpoint_mock.create().serialize()
+
+    serialized_obj, diff = create_netbox_object(
+        endpoint_mock, data, check_mode=False
+    )
+    assert endpoint_mock.create.called_once_with(data)
+    assert serialized_obj == return_value
+    assert diff == on_creation_diff
+
+
+def test_create_netbox_object_in_check_mode(endpoint_mock, data, on_creation_diff):
+    serialized_obj, diff = create_netbox_object(
+        endpoint_mock, data, check_mode=True
+    )
+    assert endpoint_mock.create.not_called()
+    assert serialized_obj == data
+    assert diff == on_creation_diff
+
+
+def test_delete_netbox_object(nb_obj_mock, on_deletion_diff):
+    serialized_obj, diff = delete_netbox_object(nb_obj_mock, check_mode=False)
+    assert nb_obj_mock.delete.called_once()
+    assert serialized_obj == nb_obj_mock.serialize()
+    assert diff == on_deletion_diff
+
+
+def test_delete_netbox_object_in_check_mode(nb_obj_mock, on_deletion_diff):
+    serialized_obj, diff = delete_netbox_object(nb_obj_mock, check_mode=True)
+    assert nb_obj_mock.delete.not_called()
+    assert serialized_obj == nb_obj_mock.serialize()
+    assert diff == on_deletion_diff
+
+
+def test_update_netbox_object_no_changes(nb_obj_mock):
+    unchanged_data = nb_obj_mock.serialize()
+    serialized_obj, diff = update_netbox_object(nb_obj_mock, unchanged_data, check_mode=True)
+    assert nb_obj_mock.update.not_called()
+    assert serialized_obj == unchanged_data
+    assert diff is None
+
+
+@pytest.fixture
+def changed_serialized_obj(nb_obj_mock):
+    changed_serialized_obj = nb_obj_mock.serialize().copy()
+    changed_serialized_obj[list(changed_serialized_obj.keys())[0]] += " (modified)"
+
+    return changed_serialized_obj
+
+
+@pytest.fixture
+def on_update_diff(nb_obj_mock, changed_serialized_obj):
+    return _build_diff(before=nb_obj_mock.serialize().copy(), after=changed_serialized_obj)
+
+
+def test_update_netbox_object_with_changes(
+    nb_obj_mock, changed_serialized_obj, on_update_diff
+):
+    serialized_obj, diff = update_netbox_object(
+        nb_obj_mock, changed_serialized_obj, check_mode=False
+    )
+    assert nb_obj_mock.update.called_once_with(changed_serialized_obj)
+    assert serialized_obj == nb_obj_mock.serialize()
+    assert diff == on_update_diff
+
+
+def test_update_netbox_object_with_changes_in_check_mode(
+    nb_obj_mock, changed_serialized_obj, on_update_diff
+):
+    updated_serialized_obj = nb_obj_mock.serialize().copy()
+    updated_serialized_obj.update(changed_serialized_obj)
+
+    serialized_obj, diff = update_netbox_object(
+        nb_obj_mock, changed_serialized_obj, check_mode=True
+    )
+    assert nb_obj_mock.update.not_called()
+
+    assert serialized_obj == updated_serialized_obj
+    assert diff == on_update_diff