Backport improvements to recordset module

- General refactoring of module
- Move recordset specific tests from the dns role to new recordset role
- Adds additional tests to recordset role

Note that this is not a clean cherry pick due to sdk changes

Change-Id: If8fda40780050d271c9d869d8959ef569644fd88
(cherry picked from commit 97b05533f1)

ci/roles/dns/tasks/main.yml

@@ -18,53 +18,6 @@
 
 - debug: var=updated_dns_zone
 
-- name: Create a recordset
-  openstack.cloud.recordset:
-    cloud: "{{ cloud }}"
-    zone: "{{ updated_dns_zone.zone.name }}"
-    name: "{{ recordset_name }}"
-    recordset_type: "a"
-    records: "{{ records }}"
-  register: recordset
-
-- name: Verify recordset info
-  assert:
-    that:
-      - recordset["recordset"].name == recordset_name
-      - recordset["recordset"].zone_name ==  dns_zone.zone.name
-      - recordset["recordset"].records == records
-
-- name: Update a recordset
-  openstack.cloud.recordset:
-    cloud: "{{ cloud }}"
-    zone: "{{ updated_dns_zone.zone.name }}"
-    name: "{{ recordset_name }}"
-    recordset_type: "a"
-    records: "{{ updated_records }}"
-    description: "new test recordset"
-  register: updated_recordset
-
-- name: Verify recordset info
-  assert:
-    that:
-      - updated_recordset["recordset"].zone_name == dns_zone.zone.name
-      - updated_recordset["recordset"].name == recordset_name
-      - updated_recordset["recordset"].records == updated_records
-
-- name: Delete recordset
-  openstack.cloud.recordset:
-    cloud: "{{ cloud }}"
-    zone: "{{ updated_dns_zone.zone.name }}"
-    name: "{{ recordset.recordset.name }}"
-    state: absent
-  register: deleted_recordset
-
-- name: Verify recordset deletion
-  assert:
-    that:
-    - deleted_recordset is successful
-    - deleted_recordset is changed
-
 - name: Delete dns zone
   openstack.cloud.dns_zone:
     cloud: "{{ cloud }}"

ci/roles/recordset/defaults/main.yml

@@ -0,0 +1,19 @@
+dns_zone_name: test.dns.zone.
+recordset_name: testrecordset.test.dns.zone.
+records: ['10.0.0.0']
+updated_records: ['10.1.1.1']
+
+recordset_fields:
+  - action
+  - created_at
+  - description
+  - id
+  - links
+  - name
+  - project_id
+  - records
+  - status
+  - ttl
+  - type
+  - zone_id
+  - zone_name

ci/roles/recordset/tasks/main.yml

@@ -0,0 +1,112 @@
+- name: Ensure DNS zone not present before tests
+  openstack.cloud.dns_zone:
+    cloud: "{{ cloud }}"
+    name: "{{ dns_zone_name }}"
+    zone_type: "primary"
+    email: test@example.net
+    state: absent
+
+- name: Ensure dns zone
+  openstack.cloud.dns_zone:
+    cloud: "{{ cloud }}"
+    name: "{{ dns_zone_name }}"
+    zone_type: "primary"
+    email: test@example.net
+  register: dns_zone
+
+- name: Create a recordset
+  openstack.cloud.recordset:
+    cloud: "{{ cloud }}"
+    zone: "{{ dns_zone.zone.name }}"
+    name: "{{ recordset_name }}"
+    recordset_type: "a"
+    records: "{{ records }}"
+  register: recordset
+
+- name: Verify recordset info
+  assert:
+    that:
+      - recordset is changed
+      - recordset["recordset"].name == recordset_name
+      - recordset["recordset"].zone_name ==  dns_zone.zone.name
+      - recordset["recordset"].records == records
+
+- name: Assert recordset fields
+  assert:
+    that: item in recordset.recordset
+  loop: "{{ recordset_fields }}"
+
+- name: Create identical recordset
+  openstack.cloud.recordset:
+    cloud: "{{ cloud }}"
+    zone: "{{ dns_zone.zone.name }}"
+    name: "{{ recordset_name }}"
+    recordset_type: "a"
+    records: "{{ records }}"
+  register: recordset
+
+- name: Assert recordset not changed
+  assert:
+    that:
+      - recordset is not changed
+
+- name: Assert recordset fields
+  assert:
+    that: item in recordset.recordset
+  loop: "{{ recordset_fields }}"
+
+- name: Update a recordset
+  openstack.cloud.recordset:
+    cloud: "{{ cloud }}"
+    zone: "{{ dns_zone.zone.name }}"
+    name: "{{ recordset_name }}"
+    recordset_type: "a"
+    records: "{{ updated_records }}"
+    description: "new test recordset"
+  register: recordset
+
+- name: Verify recordset info
+  assert:
+    that:
+      - recordset is changed
+      - recordset["recordset"].zone_name == dns_zone.zone.name
+      - recordset["recordset"].name == recordset_name
+      - recordset["recordset"].records == updated_records
+
+- name: Assert recordset fields
+  assert:
+    that: item in recordset.recordset
+  loop: "{{ recordset_fields }}"
+
+- name: Delete recordset
+  openstack.cloud.recordset:
+    cloud: "{{ cloud }}"
+    zone: "{{ dns_zone.zone.name }}"
+    name: "{{ recordset.recordset.name }}"
+    state: absent
+  register: deleted_recordset
+
+- name: Verify recordset deletion
+  assert:
+    that:
+    - deleted_recordset is successful
+    - deleted_recordset is changed
+
+- name: Delete unexistent recordset
+  openstack.cloud.recordset:
+    cloud: "{{ cloud }}"
+    zone: "{{ dns_zone.zone.name }}"
+    name: "{{ recordset.recordset.name }}"
+    state: absent
+  register: deleted_recordset
+
+- name: Verify recordset deletion
+  assert:
+    that:
+    - deleted_recordset is not changed
+
+- name: Delete dns zone
+  openstack.cloud.dns_zone:
+    cloud: "{{ cloud }}"
+    name: "{{ dns_zone.zone.name }}"
+    state: absent

ci/run-collection.yml

@@ -49,6 +49,7 @@
     - { role: object, tags: object }
     - { role: port, tags: port }
     - { role: project, tags: project }
+    - { role: recordset, tags: recordset }
     - { role: router, tags: router }
     - { role: security_group, tags: security_group }
     - { role: server, tags: server }

plugins/modules/recordset.py

@@ -12,42 +12,42 @@ description:
       updated. Only the I(records), I(description), and I(ttl) values
       can be updated.
 options:
-   zone:
    description:
-        - Zone managing the recordset
+     description:
-     required: true
+        - Description of the recordset
      type: str
    name:
      description:
         - Name of the recordset. It must be ended with name of dns zone.
      required: true
      type: str
-   recordset_type:
-     description:
-        - Recordset type
-        - Required when I(state=present).
-     choices: ['a', 'aaaa', 'mx', 'cname', 'txt', 'ns', 'srv', 'ptr', 'caa']
-     type: str
    records:
      description:
         - List of recordset definitions.
         - Required when I(state=present).
      type: list
      elements: str
+   recordset_type:
      description:
-     description:
+        - Recordset type
-        - Description of the recordset
+        - Required when I(state=present).
+     choices: ['a', 'aaaa', 'mx', 'cname', 'txt', 'ns', 'srv', 'ptr', 'caa']
      type: str
-   ttl:
-     description:
-        -  TTL (Time To Live) value in seconds
-     type: int
    state:
      description:
        - Should the resource be present or absent.
      choices: [present, absent]
      default: present
      type: str
+   ttl:
+     description:
+        -  TTL (Time To Live) value in seconds
+     type: int
+   zone:
+     description:
+        - Name or ID of the zone which manages the recordset
+     required: true
+     type: str
 requirements:
     - "python >= 3.6"
     - "openstacksdk"
@@ -90,36 +90,73 @@ RETURN = '''
 recordset:
     description: Dictionary describing the recordset.
     returned: On success when I(state) is 'present'.
-    type: complex
+    type: dict
     contains:
-        id:
+        action:
-            description: Unique recordset ID
+            description: Current action in progress on the resource
             type: str
-            sample: "c1c530a3-3619-46f3-b0f6-236927b2618c"
+            returned: always
-        name:
+        created_at:
-            description: Recordset name
+            description: Timestamp when the zone was created
             type: str
-            sample: "www.example.net."
+            returned: always
-        zone_id:
-            description: Zone id
-            type: str
-            sample: 9508e177-41d8-434e-962c-6fe6ca880af7
-        type:
-            description: Recordset type
-            type: str
-            sample: "A"
         description:
             description: Recordset description
             type: str
             sample: "Test description"
-        ttl:
+            returned: always
-            description: Zone TTL value
+        id:
-            type: int
+            description: Unique recordset ID
-            sample: 3600
+            type: str
+            sample: "c1c530a3-3619-46f3-b0f6-236927b2618c"
+        links:
+            description: Links related to the resource
+            type: dict
+            returned: always
+        name:
+            description: Recordset name
+            type: str
+            sample: "www.example.net."
+            returned: always
+        project_id:
+            description: ID of the proect to which the recordset belongs
+            type: str
+            returned: always
         records:
             description: Recordset records
             type: list
             sample: ['10.0.0.1']
+            returned: always
+        status:
+            description:
+                - Recordset status
+                - Valid values include `PENDING_CREATE`, `ACTIVE`,`PENDING_DELETE`,
+                  `ERROR`
+            type: str
+            returned: always
+        ttl:
+            description: Zone TTL value
+            type: int
+            sample: 3600
+            returned: always
+        type:
+            description:
+                - Recordset type
+                - Valid values include `A`, `AAAA`, `MX`, `CNAME`, `TXT`, `NS`,
+                  `SSHFP`, `SPF`, `SRV`, `PTR`
+            type: str
+            sample: "A"
+            returned: always
+        zone_id:
+            description: The id of the Zone which this recordset belongs to
+            type: str
+            sample: 9508e177-41d8-434e-962c-6fe6ca880af7
+            returned: always
+        zone_name:
+            description: The name of the Zone which this recordset belongs to
+            type: str
+            sample: "example.com."
+            returned: always
 '''
 
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import OpenStackModule
@@ -127,13 +164,13 @@ from ansible_collections.openstack.cloud.plugins.module_utils.openstack import O
 
 class DnsRecordsetModule(OpenStackModule):
     argument_spec = dict(
-        zone=dict(required=True),
-        name=dict(required=True),
-        recordset_type=dict(required=False, choices=['a', 'aaaa', 'mx', 'cname', 'txt', 'ns', 'srv', 'ptr', 'caa']),
-        records=dict(required=False, type='list', elements='str'),
         description=dict(required=False, default=None),
-        ttl=dict(required=False, type='int'),
+        name=dict(required=True),
+        records=dict(required=False, type='list', elements='str'),
+        recordset_type=dict(required=False, choices=['a', 'aaaa', 'mx', 'cname', 'txt', 'ns', 'srv', 'ptr', 'caa']),
         state=dict(default='present', choices=['absent', 'present']),
+        ttl=dict(required=False, type='int'),
+        zone=dict(required=True),
     )
 
     module_kwargs = dict(
@@ -145,86 +182,71 @@ class DnsRecordsetModule(OpenStackModule):
 
     module_min_sdk_version = '0.28.0'
 
-    def _system_state_change(self, state, records, description, ttl, recordset):
+    def _needs_update(self, params, recordset):
+        for k in ('description', 'records', 'ttl'):
+            if k not in params:
+                continue
+            if params[k] is not None and params[k] != recordset[k]:
+                return True
+        return False
+
+    def _system_state_change(self, state, recordset):
         if state == 'present':
             if recordset is None:
                 return True
-            if records is not None and recordset['records'] != records:
+            kwargs = self._build_params()
-                return True
+            return self._needs_update(kwargs, recordset)
-            if description is not None and recordset['description'] != description:
-                return True
-            if ttl is not None and recordset['ttl'] != ttl:
-                return True
         if state == 'absent' and recordset:
             return True
         return False
 
+    def _build_params(self):
+        recordset_type = self.params['recordset_type']
+        records = self.params['records']
+        description = self.params['description']
+        ttl = self.params['ttl']
+        params = {
+            'description': description,
+            'records': records,
+            'type': recordset_type.upper(),
+            'ttl': ttl,
+        }
+        return {k: v for k, v in params.items() if v is not None}
+
     def run(self):
         zone = self.params.get('zone')
         name = self.params.get('name')
         state = self.params.get('state')
+        ttl = self.params.get('ttl')
 
         recordsets = self.conn.search_recordsets(zone, name_or_id=name)
 
+        recordset = None
         if recordsets:
             recordset = recordsets[0]
-            try:
-                recordset_id = recordset['id']
-            except KeyError as e:
-                self.fail_json(msg=str(e))
-        else:
-            # recordsets is filtered by type and should never be more than 1 return
-            recordset = None
-
-        if state == 'present':
-            recordset_type = self.params.get('recordset_type').upper()
-            records = self.params.get('records')
-            description = self.params.get('description')
-            ttl = self.params.get('ttl')
-
-            kwargs = {}
-            if description:
-                kwargs['description'] = description
-            kwargs['records'] = records
 
         if self.ansible.check_mode:
-                self.exit_json(
+            self.exit_json(changed=self._system_state_change(state, recordset))
-                    changed=self._system_state_change(
-                        state, records, description, ttl, recordset))
 
+        changed = False
+        if state == 'present':
+            kwargs = self._build_params()
             if recordset is None:
-                if ttl:
+                kwargs['ttl'] = ttl or 300
-                    kwargs['ttl'] = ttl
+                type = kwargs.pop('type', None)
-                else:
+                if type is not None:
-                    kwargs['ttl'] = 300
+                    kwargs['recordset_type'] = type
-
+                recordset = self.conn.create_recordset(zone=zone, name=name,
-                recordset = self.conn.create_recordset(
+                                                       **kwargs)
-                    zone=zone, name=name, recordset_type=recordset_type,
+                changed = True
+            elif self._needs_update(kwargs, recordset):
+                type = kwargs.pop('type', None)
+                recordset = self.conn.update_recordset(zone, recordset['id'],
                                                        **kwargs)
                 changed = True
-            else:
-
-                if ttl:
-                    kwargs['ttl'] = ttl
-
-                pre_update_recordset = recordset
-                changed = self._system_state_change(
-                    state, records, description, ttl, pre_update_recordset)
-                if changed:
-                    recordset = self.conn.update_recordset(
-                        zone=zone, name_or_id=recordset_id, **kwargs)
-
             self.exit_json(changed=changed, recordset=recordset)
-
+        elif state == 'absent' and recordset is not None:
-        elif state == 'absent':
+            self.conn.delete_recordset(zone, recordset['id'])
-            if self.ansible.check_mode:
-                self.exit_json(changed=self._system_state_change(
-                    state, None, None, None, recordset))
-
-            if recordset is None:
-                changed = False
-            else:
-                self.conn.delete_recordset(zone, recordset_id)
             changed = True
         self.exit_json(changed=changed)