Repository: ansible/ansible
Pull request: ipa_dnsrecord: Allow for PTR Records (supporting reverse DNS)
##### SUMMARY
This pull request allows one to manually add a pointer record to support reverse DNS.  After discussion, I would like to discuss the possibility of us supporting the ability to auto-create reverse DNS records as well, but I figured this would be a good first step towards the ideal.

Fixes #26702 

##### ISSUE TYPE
 - Feature Pull Request

##### COMPONENT NAME
ipa_dnsrecord

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
$ ansible --version
ansible 2.3.1.0
  config file =
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, May 18 2017, 22:52:22) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)]
```



Problem statement:
ipa-dnsrecord: unable to support reverse DNS
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
ipa_dnsrecord

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
$ ansible --version
ansible 2.3.1.0
  config file = /Users/fxfitz/dev/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, May 18 2017, 22:52:22) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)]
```

##### CONFIGURATION
N/A

##### OS / ENVIRONMENT
N/A

##### SUMMARY
The new ipa_dnsrecord module (slated for release in 2.4; we've backported it for the time being) does not seem to support reverse DNS records at all.  While the A and AAAA record implementation appears to work great (thanks!), there doesn't seem to be any way to add a corresponding reverse record.

##### STEPS TO REPRODUCE
N/A

##### EXPECTED RESULTS
I expected to be able to do one of the following with the `ipa_dnsrecord` module:

1. (ideal) Have an option to be able to `reverse_dns_record: present` so the module will auto-create a reverse DNS record (similar to the FreeIPA Web UI).
2. Create the PTR record manually in the proper zone.

##### ACTUAL RESULTS
1. There was no option to automatically ensure a reverse_dns_record was present/absent.
2. You are unable to create a PTR record manually; only A and AAAA are supported.

Hints from issue discussion:
cc @Nosmoht
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Adds patch to allow for PTR records

Patch to review:
diff --git a/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py b/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
index 8e3ff794af1762..7e72509d518a9c 100644
--- a/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
+++ b/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
@@ -45,10 +45,10 @@
   record_type:
     description:
     - The type of DNS record name
-    - Currently, 'A', 'AAAA' is supported
+    - Currently, 'A', 'AAAA', and 'PTR' are supported
     required: false
     default: 'A'
-    choices: ['A', 'AAAA']
+    choices: ['A', 'AAAA', 'PTR']
   record_ip:
     description:
     - Manage DNS record name with this IP address.
@@ -99,6 +99,16 @@
     record_type: 'AAAA'
     record_ip: '::1'
 
+# Ensure a PTR record is present
+- ipa_dnsrecord:
+    ipa_host: spider.example.com
+    ipa_pass: Passw0rd!
+    state: present
+    zone_name: 2.168.192.in-addr.arpa
+    record_name: 5
+    record_type: 'PTR'
+    record_ip: 'internal.ipa.example.com'
+
 # Ensure that dns record is removed
 - ipa_dnsrecord:
     name: host01
@@ -136,6 +146,8 @@ def dnsrecord_add(self, zone_name=None, record_name=None, details=None):
             item.update(a_part_ip_address=details['record_ip'])
         elif details['record_type'] == 'AAAA':
             item.update(aaaa_part_ip_address=details['record_ip'])
+        elif details['record_type'] == 'PTR':
+            item.update(ptr_part_hostname=details['record_ip'])
 
         return self._post_json(method='dnsrecord_add', name=zone_name, item=item)
 
@@ -156,6 +168,8 @@ def get_dnsrecord_dict(details=None):
         module_dnsrecord.update(arecord=details['record_ip'])
     elif details['record_type'] == 'AAAA' and details['record_ip']:
         module_dnsrecord.update(aaaarecord=details['record_ip'])
+    elif details['record_type'] == 'PTR' and details['record_ip']:
+        module_dnsrecord.update(ptrrecord=details['record_ip'])
     return module_dnsrecord
 
 
@@ -201,7 +215,7 @@ def ensure(module, client):
 
 
 def main():
-    record_types = ['A', 'AAAA']
+    record_types = ['A', 'AAAA', 'PTR']
     module = AnsibleModule(
         argument_spec=dict(
             zone_name=dict(type='str', required=True),


Code review comments:
diff_hunk: @@ -99,6 +99,16 @@
     record_type: 'AAAA'
     record_ip: '::1'
 
+# Ensure a PTR record is present
+- ipa_dnsrecord:
+    ipa_host: spider.example.com
+    ipa_pass: Passw0rd!
+    state: present
+    zone_name: 2.168.192.in-addr.arpa
+    record_name: 5
+    record_type: 'PTR'
+    record_ip: 'internal.ipa.example.com'
path: lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
text: @author:
I didn't quite like the use of `record_ip` here, since this is actually a hostname. I left it the way it is for discussion.

What would you think about changing this `record_ip` to `record_value`? Now would be the best time to do it if we were going to since 2.4 hasn't been released yet.

@user1:
+1 for `record_value`. Can you add record_value as AnsibleModule Argument? and make dependency to `record_type` 

```
--- a/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
+++ b/lib/ansible/modules/identity/ipa/ipa_dnsrecord.py
@@ -207,7 +207,8 @@ def main():
             zone_name=dict(type='str', required=True),
             record_name=dict(type='str', required=True, aliases=['name']),
             record_type=dict(type='str', required=False, default='A', choices=record_types),
-            record_ip=dict(type='str', required=True),
+            record_ip=dict(type='str', required=False),
+            record_value=dict(type='str', required=False),
             state=dict(type='str', required=False, default='present', choices=['present', 'absent']),
             ipa_prot=dict(type='str', required=False, default='https', choices=['http', 'https']),
             ipa_host=dict(type='str', required=False, default='ipa.example.com'),
@@ -217,6 +218,10 @@ def main():
             validate_certs=dict(type='bool', required=False, default=True),
         ),
         supports_check_mode=True,
+        required_if=[('record_type', 'PTR', ['record_value']),
+                     ('record_type', 'A', ['record_ip']),
+                     ('record_type', 'AAAA', ['record_ip']),
+                     ],
     )
```

And Also add documentation and example for this change.


@author:
@user1 Just so I'm clear, I was proposing renaming all instances of `record_ip` to `record_value`; `record_ip` would cease to exist.

The reason for that being is that `record_ip` makes sense when working with A and AAAA records, but when this module starts supporting all of the other record types, I think `record_value` makes more sense since it is more flexible.

As I said initially, if we were to do this, I think now is the best time since 2.4 still hasn't been released.

I wasn't thinking of having `record_ip` be applicable to A/AAAA records while `record_value` would apply to PTR records.

Are you on the same page, or should I just go ahead and add an additional `record_value`?

@user1:
Ah! I am OK with this approach as well. Go ahead with changes - `record_value` replacing `record_ip`.