Support one to many A records in nsupdate module #25620

dcritch · Jun 12, 2017

Updating the nsupdate module to accept a list for 'value' instead
of a string. This is to allow manipulating 1:many DNS records.

A string can still be supplied so it should be backwards compatible.

Addresses issue #25554

SUMMARY

There are certain use cases where one would want a one to many mapping of A records. For instance, in a kubernetes or OpenShift environment with a load balanced router, a wildcard DNS entry is required.

# host foo.paas.ocp.example.com 192.168.1.211
Using domain server:
Name: 192.168.1.211
Address: 192.168.1.211#53
Aliases: 

foo.paas.ocp.example.com has address 192.168.1.172
foo.paas.ocp.example.com has address 192.168.1.171
foo.paas.ocp.example.com has address 192.168.1.171

The current version of the module will overwrite entries rather than manage this type of record.

Fixes #25554

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

nsupdate

ANSIBLE VERSION

ansible 2.3.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.5 (default, Aug  2 2016, 04:20:16) [GCC 4.8.5 20150623 (Red Hat 4.8.5-4)]

ADDITIONAL INFORMATION

I've tested the module successfully. An example play would be:

---
- hosts: localhost
  gather_facts: false
  tasks:
  - name: update wildcard DNS entry
    nsupdate:
      state: present
      key_name: "rndc-key"
      key_secret: "r/SECRETKEEEEEEYYYY"
      server: "192.168.1.211"
      zone: "ocp.example.com"
      record: "*.paas"
      value: ["192.168.1.171", "192.168.1.172"]
    delegate_to: localhost
    run_once: true

@nerzhul

cc @nerzhul
click here for bot help

ansibot · Jun 12, 2017

cc @nerzhul
click here for bot help

First of all, I'm all for the value parameter being a list, it mapping much better to the DNS concept of RRsets.

That said; I think the implementation could benefit from some improvement, given how it now repeats itself network wise.

Assuming the following task.

- nsupdate:
    key_name: corrino-halleck
    key_secret: ...
    server: halleck.arrakis.se.
    zone: vag.arrakis.se.
    record: p3t
    state: present
    value: ['127.0.53.2', '127.0.53.3', '127.0.53.4']

This is what the DNS master sees.

key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.3
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.3
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.4

Depending on timing factors, how the DNS master is configured, etc this also has the potential of triggering multiple zone transfers.

andreaso · Jul 7, 2017

First of all, I'm all for the value parameter being a list, it mapping much better to the DNS concept of RRsets.

That said; I think the implementation could benefit from some improvement, given how it now repeats itself network wise.

Assuming the following task.

- nsupdate:
    key_name: corrino-halleck
    key_secret: ...
    server: halleck.arrakis.se.
    zone: vag.arrakis.se.
    record: p3t
    state: present
    value: ['127.0.53.2', '127.0.53.3', '127.0.53.4']

This is what the DNS master sees.

key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: updating zone 'vag.arrakis.se/IN': update unsuccessful: p3t.vag.arrakis.se/A: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.3
key corrino-halleck: signer "corrino-halleck" approved
key corrino-halleck: updating zone 'vag.arrakis.se/IN': deleting rrset at 'p3t.vag.arrakis.se' A
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.2
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.3
key corrino-halleck: updating zone 'vag.arrakis.se/IN': adding an RR at 'p3t.vag.arrakis.se' A 127.0.53.4

Depending on timing factors, how the DNS master is configured, etc this also has the potential of triggering multiple zone transfers.

Nicely done!
In the EXAMPLES & RETURN variables, the value should be set as a list.

rafi · Jul 11, 2017

Nicely done!
In the EXAMPLES & RETURN variables, the value should be set as a list.

@andreaso

Thanks!

I've updated the EXAMPLES and RETURNS section.

@andreaso any suggestions on how I could improve it and not trigger all the updates?

dcritch · Jul 13, 2017

Thanks!

I've updated the EXAMPLES and RETURNS section.

@andreaso any suggestions on how I could improve it and not trigger all the updates?

sounds interesting. Does the regular behaviour using a string instead of a list works or does this converts myvalue to [m,y,v,a,l,u,e] ?

nerzhul · Jul 15, 2017

sounds interesting. Does the regular behaviour using a string instead of a list works or does this converts myvalue to [m,y,v,a,l,u,e] ?

Yes, the regular behaviour is still there so if you give it just one string, its just a one element list.

dcritch · Jul 16, 2017

Yes, the regular behaviour is still there so if you give it just one string, its just a one element list.

andreaso · Aug 15, 2017

andreaso reviewed Aug 15, 2017

View changes

andreaso · Aug 15, 2017

lib/ansible/modules/net_tools/nsupdate.py

+                self.module.fail_json(msg='value needed when state=present')
+            except dns.exception.SyntaxError:
+                self.module.fail_json(msg='Invalid/malformed value')
+            response = self.__do_update(update)


@dcritch

Here we have the cause of the repetition problem I complained about in a previous comment.

It's all about the indention level. You want to trigger the update outside the loop, after the full changeset has been generated. Just like it's being done in the create_record method, which does work as intended.

Sorry about taking so long in getting back to you.

@andreaso

@andreaso thanks! I see it and have made the change. And my apologies for the delay on my side, was on vacation for a bit :)

dcritch · Aug 30, 2017

@andreaso thanks! I see it and have made the change. And my apologies for the delay on my side, was on vacation for a bit :)

@dcritch

@dcritch Yepp, that fixed that issue!

Taking a closer look at the patch I see that the same indentation fix also should be applied to these two lines, taking them out of the for loop.

https://github.com/dcritch/ansible/blob/ae7a64e/lib/ansible/modules/net_tools/nsupdate.py#L324-L325

andreaso · Aug 31, 2017

@dcritch Yepp, that fixed that issue!

Taking a closer look at the patch I see that the same indentation fix also should be applied to these two lines, taking them out of the for loop.

https://github.com/dcritch/ansible/blob/ae7a64e/lib/ansible/modules/net_tools/nsupdate.py#L324-L325

@andreaso

@andreaso we missed the 2.4 window merge, sorry for the delay i was a little bit too busy these days to make this PR merged :( (and i need it too)

nerzhul · Aug 31, 2017

@andreaso we missed the 2.4 window merge, sorry for the delay i was a little bit too busy these days to make this PR merged :( (and i need it too)

Seems like all of us involved in this ticket have been a bit busy/slow :-)

andreaso · Aug 31, 2017

Seems like all of us involved in this ticket have been a bit busy/slow :-)

@andreaso

@andreaso yup... my understanding of the python dns module was a little naive, but it all makes sense now. Change has been made. Thanks!

dcritch · Aug 31, 2017

@andreaso yup... my understanding of the python dns module was a little naive, but it all makes sense now. Change has been made. Thanks!

shipit

nerzhul · Sep 13, 2017

shipit

nerzhul · Sep 13, 2017

nerzhul approved these changes Sep 13, 2017

View changes

Tested on Debian 9 works great, nice job

test cases:

new record
record + 1 entry
record - 1 entry
record + 2 entries
modify entry 2
record - 2 entries

rebuild_merge

abadger · Sep 13, 2017

rebuild_merge

Awesome. Thanks everyone for the feedback and getting it merged!

dcritch · Sep 14, 2017

Awesome. Thanks everyone for the feedback and getting it merged!

No problem i will try to be more reactive especially before a RC feature freeze :p

nerzhul · Sep 14, 2017

No problem i will try to be more reactive especially before a RC feature freeze :p

Updating the nsupdate module to accept a list for 'value' instead
of a string. This is to allow manipulating 1:many DNS records. A string can still be supplied so it should be backwards compatible. Addresses issue #25554

f805596

ansibot added affects_2.4 community_review feature_pull_request module needs_triage net_tools labels Jun 12, 2017

bcoca removed the needs_triage label Jun 12, 2017

ansibot added stale_ci support:community labels Jun 23, 2017

ansibot added support:core and removed community_review module labels Jul 7, 2017

Update nsupdate.py

83512e0

ansibot added community_review module and removed stale_ci labels Jul 13, 2017

ansibot added the stale_ci label Jul 24, 2017

ansibot removed the support:core label Aug 23, 2017

Update nsupdate.py

ae7a64e

ansibot removed the stale_ci label Aug 30, 2017

Update nsupdate.py

9081577

ansibot added the stale_ci label Sep 9, 2017

andreaso referenced this pull request Sep 10, 2017
Open
Add `append` as state to nsupdate module #28993

ansibot removed the stale_ci label Sep 13, 2017

ansibot merged commit e462e3b into ansible:devel Sep 13, 2017
1 check passed

1 check passed

Shippable Run 37202 status is SUCCESS.
Details

ansibot added feature and removed feature_pull_request labels Mar 5, 2018

ansible/ansible

Join GitHub today

Support one to many A records in nsupdate module #25620

Conversation

dcritch commented Jun 12, 2017 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

This comment has been minimized.

ansibot commented Jun 12, 2017

ansibot added affects_2.4 community_review feature_pull_request module needs_triage net_tools labels Jun 12, 2017

bcoca removed the needs_triage label Jun 12, 2017

ansibot added stale_ci support:community labels Jun 23, 2017

This comment has been minimized.

andreaso commented Jul 7, 2017 • edited

ansibot added support:core and removed community_review module labels Jul 7, 2017

This comment has been minimized.

rafi commented Jul 11, 2017

This comment has been minimized.

dcritch commented Jul 13, 2017

ansibot added community_review module and removed stale_ci labels Jul 13, 2017

This comment has been minimized.

nerzhul commented Jul 15, 2017

This comment has been minimized.

dcritch commented Jul 16, 2017

ansibot added the stale_ci label Jul 24, 2017

andreaso reviewed Aug 15, 2017 View changes

This comment has been minimized.

ansibot removed the support:core label Aug 23, 2017

This comment has been minimized.

dcritch commented Aug 30, 2017

ansibot removed the stale_ci label Aug 30, 2017

This comment has been minimized.

andreaso commented Aug 31, 2017

This comment has been minimized.

nerzhul commented Aug 31, 2017 • edited

This comment has been minimized.

andreaso commented Aug 31, 2017 • edited

This comment has been minimized.

dcritch commented Aug 31, 2017

ansibot added the stale_ci label Sep 9, 2017

andreaso referenced this pull request Sep 10, 2017

This comment has been minimized.

nerzhul commented Sep 13, 2017

nerzhul approved these changes Sep 13, 2017 View changes

This comment has been minimized.

abadger commented Sep 13, 2017

ansibot removed the stale_ci label Sep 13, 2017

Hide details View details ansibot merged commit e462e3b into ansible:devel Sep 13, 2017 1 check passed

1 check passed

This comment has been minimized.

dcritch commented Sep 14, 2017

This comment has been minimized.

nerzhul commented Sep 14, 2017

prasadkatti pushed a commit to prasadkatti/ansible that referenced this pull request Oct 1, 2017

BondAnthony added a commit to BondAnthony/ansible that referenced this pull request Oct 5, 2017

sayap pushed a commit to KMK-ONLINE/ansible that referenced this pull request Oct 6, 2017

ansibot added feature and removed feature_pull_request labels Mar 5, 2018

dcritch commented Jun 12, 2017 •

edited

andreaso commented Jul 7, 2017 •

edited

andreaso reviewed Aug 15, 2017

View changes

nerzhul commented Aug 31, 2017 •

edited

andreaso commented Aug 31, 2017 •

edited

nerzhul approved these changes Sep 13, 2017

View changes

ansibot merged commit `e462e3b` into ansible:devel Sep 13, 2017
1 check passed