Merge pull request #304 from rjeffman/fix_forwardzone_issues

Fix forwardzone issues

README-dnsforwardzone.md

@@ -49,7 +49,7 @@ Example playbook to ensure presence of a forwardzone to ipa DNS:
   tasks:
   - name: ensure presence of forwardzone for DNS requests for example.com to 8.8.8.8
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
@@ -59,13 +59,13 @@ Example playbook to ensure presence of a forwardzone to ipa DNS:
 
   - name: ensure the forward zone is disabled
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       state: disabled
 
   - name: ensure presence of multiple upstream DNS servers for example.com
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
@@ -74,7 +74,7 @@ Example playbook to ensure presence of a forwardzone to ipa DNS:
 
   - name: ensure presence of another forwarder to any existing ones for example.com
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
@@ -83,7 +83,7 @@ Example playbook to ensure presence of a forwardzone to ipa DNS:
 
   - name: ensure the forwarder for example.com does not exists (delete it if needed)
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       state: absent
 ```
@@ -99,9 +99,12 @@ Variable | Description | Required
 `ipaadmin_principal` | The admin principal is a string and defaults to `admin` | no
 `ipaadmin_password` | The admin password is a string and is required if there is no admin ticket available on the node | no
 `name` \| `cn` | Zone name (FQDN). | yes if `state` == `present`
-`forwarders` \| `idnsforwarders` |  Per-zone conditional forwarding policy. Possible values are `only`, `first`, `none`) | no
+`forwarders` \| `idnsforwarders` |  Per-zone forwarders. A custom port can be specified for each forwarder. Options | no
-`forwardpolicy` \| `idnsforwardpolicy` | Per-zone conditional forwarding policy. Set to "none" to disable forwarding to global forwarder for this zone. In that case, conditional zone forwarders are disregarded. | no
+  | `ip_address`: The forwarder IP address. | yes
+  | `port`: The forwarder IP port. | no
+`forwardpolicy` \| `idnsforwardpolicy` | Per-zone conditional forwarding policy. Possible values are `only`, `first`, `none`. Set to "none" to disable forwarding to global forwarder for this zone. In that case, conditional zone forwarders are disregarded. | no
 `skip_overlap_check` | Force DNS zone creation even if it will overlap with an existing zone. Defaults to False. | no
+`permission` | Allow DNS Forward Zone to be managed. (bool) | no
 `action` | Work on group or member level. It can be on of `member` or `dnsforwardzone` and defaults to `dnsforwardzone`. | no
 `state` | The state to ensure. It can be one of `present`, `absent`, `enabled` or `disabled`, default: `present`. | yes
 

plugins/modules/ipadnsforwardzone.py

@@ -54,9 +54,16 @@ options:
   forwarders:
     description:
     - List of the DNS servers to forward to
-    required: true
-    type: list
     aliases: ["idnsforwarders"]
+    options:
+      ip_address:
+        description: Forwarder IP address (either IPv4 or IPv6).
+        required: false
+        type: string
+      port:
+        description: Forwarder port.
+        required: false
+        type: int
   forwardpolicy:
     description: Per-zone conditional forwarding policy
     required: false
@@ -68,6 +75,11 @@ options:
     - Force DNS zone creation even if it will overlap with an existing zone.
     required: false
     default: false
+  permission:
+    description:
+    - Allow DNS Forward Zone to be managed.
+    required: false
+    type: bool
 '''
 
 EXAMPLES = '''
@@ -128,20 +140,41 @@ def gen_args(forwarders, forwardpolicy, skip_overlap_check):
     return _args
 
 
+def forwarder_list(forwarders):
+    """Convert the forwarder dict into a list compatible with IPA API."""
+    if forwarders is None:
+        return None
+    fwd_list = []
+    for forwarder in forwarders:
+        if forwarder.get('port', None) is not None:
+            formatter = "{ip_address} port {port}"
+        else:
+            formatter = "{ip_address}"
+        fwd_list.append(formatter.format(**forwarder))
+    return fwd_list
+
+
 def main():
     ansible_module = AnsibleModule(
         argument_spec=dict(
             # general
             ipaadmin_principal=dict(type="str", default="admin"),
             ipaadmin_password=dict(type="str", required=False, no_log=True),
-            name=dict(type="str", aliases=["cn"], default=None,
+            name=dict(type="list", aliases=["cn"], default=None,
                       required=True),
-            forwarders=dict(type='list', aliases=["idnsforwarders"],
+            forwarders=dict(type="list", default=None, required=False,
-                            required=False),
+                            aliases=["idnsforwarders"], elements='dict',
+                            options=dict(
+                                 ip_address=dict(type='str', required=True),
+                                 port=dict(type='int', required=False,
+                                           default=None),
+                            )),
             forwardpolicy=dict(type='str', aliases=["idnsforwardpolicy"],
                                required=False,
                                choices=['only', 'first', 'none']),
             skip_overlap_check=dict(type='bool', required=False),
+            permission=dict(type='bool', required=False,
+                            aliases=['managedby']),
             action=dict(type="str", default="dnsforwardzone",
                         choices=["member", "dnsforwardzone"]),
             # state
@@ -158,14 +191,22 @@ def main():
                                            "ipaadmin_principal")
     ipaadmin_password = module_params_get(ansible_module,
                                           "ipaadmin_password")
-    name = module_params_get(ansible_module, "name")
+    names = module_params_get(ansible_module, "name")
     action = module_params_get(ansible_module, "action")
-    forwarders = module_params_get(ansible_module, "forwarders")
+    forwarders = forwarder_list(
+        module_params_get(ansible_module, "forwarders"))
     forwardpolicy = module_params_get(ansible_module, "forwardpolicy")
     skip_overlap_check = module_params_get(ansible_module,
                                            "skip_overlap_check")
+    permission = module_params_get(ansible_module, "permission")
     state = module_params_get(ansible_module, "state")
 
+    if state == 'present' and len(names) != 1:
+        ansible_module.fail_json(
+            msg="Only one dnsforwardzone can be added at a time.")
+    if state == 'absent' and len(names) < 1:
+        ansible_module.fail_json(msg="No name given.")
+
     # absent stae means delete if the action is NOT member but update if it is
     # if action is member then update an exisiting resource
     # and if action is not member then create a resource
@@ -176,18 +217,30 @@ def main():
     else:
         operation = "add"
 
-    if state == "disabled":
+    if state in ["enabled", "disabled"]:
-        wants_enable = False
+        if action == "member":
-    else:
+            ansible_module.fail_json(
-        wants_enable = True
+                msg="Action `member` cannot be used with state `%s`"
-
+                    % (state))
-    if operation == "del":
+        invalid = [
-        invalid = ["forwarders", "forwardpolicy", "skip_overlap_check"]
+            "forwarders", "forwardpolicy", "skip_overlap_check", "permission"
+        ]
         for x in invalid:
             if vars()[x] is not None:
                 ansible_module.fail_json(
                     msg="Argument '%s' can not be used with action "
-                    "'%s'" % (x, action))
+                    "'%s', state `%s`" % (x, action, state))
+        wants_enable = (state == "enabled")
+
+    if operation == "del":
+        invalid = [
+            "forwarders", "forwardpolicy", "skip_overlap_check", "permission"
+        ]
+        for x in invalid:
+            if vars()[x] is not None:
+                ansible_module.fail_json(
+                    msg="Argument '%s' can not be used with action "
+                    "'%s', state `%s`" % (x, action, state))
 
     changed = False
     exit_args = {}
@@ -207,60 +260,20 @@ def main():
                                                  ipaadmin_password)
         api_connect()
 
+        for name in names:
+            commands = []
+            command = None
+
             # Make sure forwardzone exists
             existing_resource = find_dnsforwardzone(ansible_module, name)
 
-        if existing_resource is None and operation == "update":
+            # validate parameters
-            # does not exist and is updating
+            if state == 'present':
-            # trying to update something that doesn't exist, so error
+                if existing_resource is None and not forwarders:
-            ansible_module.fail_json(msg="""dnsforwardzone '%s' is not
+                    ansible_module.fail_json(msg='No forwarders specified.')
-                                                     valid""" % (name))
-        elif existing_resource is None and operation == "del":
-            # does not exists and should be absent
-            # set command
-            command = None
-            # enabled or disabled?
-            is_enabled = "IGNORE"
-        elif existing_resource is not None and operation == "del":
-            # exists but should be absent
-            # set command
-            command = "dnsforwardzone_del"
-            # enabled or disabled?
-            is_enabled = "IGNORE"
-        elif forwarders is None:
-            # forwarders are not defined its not a delete, update state?
-            # set command
-            command = None
-            # enabled or disabled?
-            if existing_resource is not None:
-                is_enabled = existing_resource["idnszoneactive"][0]
-            else:
-                is_enabled = "IGNORE"
-        elif existing_resource is not None and operation == "update":
-            # exists and is updating
-            # calculate the new forwarders and mod
-            # determine args
-            if state != "absent":
-                forwarders = list(set(existing_resource["idnsforwarders"]
-                                      + forwarders))
-            else:
-                forwarders = list(set(existing_resource["idnsforwarders"])
-                                  - set(forwarders))
-            args = gen_args(forwarders, forwardpolicy,
-                            skip_overlap_check)
-            if skip_overlap_check is not None:
-                del args['skip_overlap_check']
 
-            # command
+            if existing_resource is None:
-            if not compare_args_ipa(ansible_module, args, existing_resource):
+                if operation == "add":
-                command = "dnsforwardzone_mod"
-            else:
-                command = None
-
-            # enabled or disabled?
-            is_enabled = existing_resource["idnszoneactive"][0]
-
-        elif existing_resource is None and operation == "add":
                     # does not exist but should be present
                     # determine args
                     args = gen_args(forwarders, forwardpolicy,
@@ -268,38 +281,95 @@ def main():
                     # set command
                     command = "dnsforwardzone_add"
                     # enabled or disabled?
-            is_enabled = "TRUE"
 
-        elif existing_resource is not None and operation == "add":
+                elif operation == "update":
+                    # does not exist and is updating
+                    # trying to update something that doesn't exist, so error
+                    ansible_module.fail_json(
+                        msg="dnsforwardzone '%s' not found." % (name))
+
+                elif operation == "del":
+                    # there's nothnig to do.
+                    continue
+
+            else:   # existing_resource is not None
+                if state != "absent":
+                    if forwarders:
+                        forwarders = list(
+                            set(existing_resource["idnsforwarders"]
+                                + forwarders))
+                else:
+                    if forwarders:
+                        forwarders = list(
+                            set(existing_resource["idnsforwarders"])
+                            - set(forwarders))
+
+                if operation == "add":
                     # exists and should be present, has it changed?
                     # determine args
-            args = gen_args(forwarders, forwardpolicy, skip_overlap_check)
+                    args = gen_args(
-            if skip_overlap_check is not None:
+                        forwarders, forwardpolicy, skip_overlap_check)
+                    if 'skip_overlap_check' in args:
                         del args['skip_overlap_check']
 
                     # set command
-            if not compare_args_ipa(ansible_module, args, existing_resource):
+                    if not compare_args_ipa(
+                            ansible_module, args, existing_resource):
                         command = "dnsforwardzone_mod"
-            else:
-                command = None
 
-            # enabled or disabled?
+                elif operation == "del":
+                    # exists but should be absent
+                    # set command
+                    command = "dnsforwardzone_del"
+                    args = {}
+
+                elif operation == "update":
+                    # exists and is updating
+                    # calculate the new forwarders and mod
+                    args = gen_args(
+                        forwarders, forwardpolicy, skip_overlap_check)
+                    if "skip_overlap_check" in args:
+                        del args['skip_overlap_check']
+
+                    # command
+                    if not compare_args_ipa(
+                            ansible_module, args, existing_resource):
+                        command = "dnsforwardzone_mod"
+
+            if state in ['enabled', 'disabled']:
+                if existing_resource is not None:
                     is_enabled = existing_resource["idnszoneactive"][0]
-
+                else:
-        # if command is set then run it with the args
+                    ansible_module.fail_json(
-        if command is not None:
+                        msg="dnsforwardzone '%s' not found." % (name))
-            api_command(ansible_module, command, name, args)
-            changed = True
 
             # does the enabled state match what we want (if we care)
             if is_enabled != "IGNORE":
                 if wants_enable and is_enabled != "TRUE":
-                api_command(ansible_module, "dnsforwardzone_enable",
+                    commands.append([name, "dnsforwardzone_enable", {}])
-                            name, {})
-                changed = True
                 elif not wants_enable and is_enabled != "FALSE":
-                api_command(ansible_module, "dnsforwardzone_disable",
+                    commands.append([name, "dnsforwardzone_disable", {}])
-                            name, {})
+
+            # if command is set...
+            if command is not None:
+                commands.append([name, command, args])
+
+            if permission is not None:
+                if existing_resource is None:
+                    managedby = None
+                else:
+                    managedby = existing_resource.get('managedby', None)
+                if permission and managedby is None:
+                    commands.append(
+                        [name, 'dnsforwardzone_add_permission', {}]
+                    )
+                elif not permission and managedby is not None:
+                    commands.append(
+                        [name, 'dnsforwardzone_remove_permission', {}]
+                    )
+
+            for name, command, args in commands:
+                api_command(ansible_module, command, name, args)
                 changed = True
 
     except Exception as e:

tests/dnsforwardzone/test_dnsforwardzone.yml

@@ -5,19 +5,21 @@
   gather_facts: false
 
   tasks:
-  - name: ensure forwardzone example.com is absent - prep
+  - name: ensure test forwardzones are absent
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
-      name: example.com
+      name:
+      - example.com
+      - newfailzone.com
       state: absent
 
   - name: ensure forwardzone example.com is created
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
       forwardpolicy: first
       skip_overlap_check: true
     register: result
@@ -25,11 +27,11 @@
 
   - name: ensure forwardzone example.com is present again
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
       forwardpolicy: first
       skip_overlap_check: true
     register: result
@@ -37,12 +39,13 @@
 
   - name: ensure forwardzone example.com has two forwarders
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
-        - 4.4.4.4
+        - ip_address: 4.4.4.4
+          port: 8053
       forwardpolicy: first
       skip_overlap_check: true
     register: result
@@ -50,165 +53,246 @@
 
   - name: ensure forwardzone example.com has one forwarder again
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
       forwardpolicy: first
       skip_overlap_check: true
       state: present
     register: result
-    failed_when: not result.changed
+    failed_when: result.changed
 
   - name: skip_overlap_check can only be set on creation so change nothing
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
       forwardpolicy: first
       skip_overlap_check: false
       state: present
     register: result
     failed_when: result.changed
 
-  - name: change all the things at once
+  - name: ensure forwardzone example.com is absent.
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
-      state: present
       name: example.com
-      forwarders:
+      state: absent
-        - 8.8.8.8
-        - 4.4.4.4
-      forwardpolicy: only
-      skip_overlap_check: false
     register: result
     failed_when: not result.changed
 
-  - name: ensure forwardzone example.com is absent for next testset
+  - name: ensure forwardzone example.com is absent, again.
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      state: absent
+    register: result
+    failed_when: result.changed
+
+  - name: change all the things at once
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      state: present
+      name: example.com
+      forwarders:
+        - ip_address: 8.8.8.8
+        - ip_address: 4.4.4.4
+          port: 8053
+      forwardpolicy: only
+      skip_overlap_check: true
+      permission: yes
+    register: result
+    failed_when: not result.changed
+
+  - name: change zone forward policy
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      forwardpolicy: first
+    register: result
+    failed_when: not result.changed
+
+  - name: change zone forward policy, again
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      forwardpolicy: first
+    register: result
+    failed_when: result.changed
+
+  - name: ensure forwardzone example.com is absent.
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       state: absent
 
   - name: ensure forwardzone example.com is created with minimal args
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       skip_overlap_check: true
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
     register: result
     failed_when: not result.changed
 
   - name: add a forwarder to any existing ones
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 4.4.4.4
+        - ip_address: 4.4.4.4
+          port: 8053
       action: member
     register: result
     failed_when: not result.changed
 
   - name: check the list of forwarders is what we expect
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 4.4.4.4
+        - ip_address: 4.4.4.4
-        - 8.8.8.8
+          port: 8053
+        - ip_address: 8.8.8.8
       action: member
     register: result
     failed_when: result.changed
 
   - name: remove a single forwarder
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: absent
       name: example.com
       forwarders:
-        - 8.8.8.8
+        - ip_address: 8.8.8.8
       action: member
     register: result
     failed_when: not result.changed
 
   - name: check the list of forwarders is what we expect now
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       state: present
       name: example.com
       forwarders:
-        - 4.4.4.4
+        - ip_address: 4.4.4.4
+          port: 8053
       action: member
     register: result
     failed_when: result.changed
 
-  - name: ensure forwardzone example.com is absent again
+  - name: Add a permission for per-forward zone access delegation.
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
-      state: absent
+      permission: yes
-
-  - name: try to create a new forwarder with action=member
-    ipadnsforwardzone:
-      ipaadmin_password: password01
-      state: present
-      name: example.com
-      forwarders:
-        - 4.4.4.4
       action: member
-      skip_overlap_check: true
-    register: result
-    failed_when: result.changed
-
-  - name: ensure forwardzone example.com is absent - tidy up
-    ipadnsforwardzone:
-      ipaadmin_password: password01
-      name: example.com
-      state: absent
-
-  - name: try to create a new forwarder is disabled state
-    ipadnsforwardzone:
-      ipaadmin_password: password01
-      state: disabled
-      name: example.com
-      forwarders:
-        - 4.4.4.4
-      skip_overlap_check: true
     register: result
     failed_when: not result.changed
 
-  - name: enable the forwarder
+  - name: Add a permission for per-forward zone access delegation, again.
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
       name: example.com
-      state: enabled
+      permission: yes
+      action: member
+    register: result
+    failed_when: result.changed
+
+  - name: Remove a permission for per-forward zone access delegation.
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      permission: no
+      action: member
+    register: result
+    failed_when: not result.changed
+
+  - name: Remove a permission for per-forward zone access delegation, again.
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      permission: no
+      action: member
+    register: result
+    failed_when: result.changed
+
+  - name: disable the forwarder
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      state: disabled
     register: result
     failed_when: not result.changed
 
   - name: disable the forwarder again
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
-      name: example.com
-      state: disabled
-      action: member
-    register: result
-    failed_when: not result.changed
-
-  - name: ensure it stays disabled
-    ipadnsforwardzone:
-      ipaadmin_password: password01
       name: example.com
       state: disabled
     register: result
     failed_when: result.changed
 
-  - name: ensure forwardzone example.com is absent - tidy up
+  - name: enable the forwarder
     ipadnsforwardzone:
-      ipaadmin_password: password01
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      state: enabled
+    register: result
+    failed_when: not result.changed
+
+  - name: enable the forwarder, again
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      state: enabled
+    register: result
+    failed_when: result.changed
+
+  - name: ensure forwardzone example.com is absent again
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
       name: example.com
       state: absent
+
+  - name: try to create a new forwarder with action=member
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      state: present
+      name: example.com
+      forwarders:
+        - ip_address: 4.4.4.4
+          port: 8053
+      action: member
+      skip_overlap_check: true
+    register: result
+    failed_when: not result.failed or "not found" not in result.msg
+
+  - name: try to create a new forwarder with disabled state
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: example.com
+      state: disabled
+    register: result
+    failed_when: not result.failed or "not found" not in result.msg
+
+  - name: Ensure forwardzone is not added without forwarders, with correct message.
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name: newfailzone.com
+    register: result
+    failed_when: not result.failed or "No forwarders specified" not in result.msg
+
+  - name: ensure forwardzone example.com is absent - tidy up
+    ipadnsforwardzone:
+      ipaadmin_password: SomeADMINpassword
+      name:
+      - example.com
+      - newfailzone.com
+      state: absent