New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new module: dyn_record #33222

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@oboukili
Contributor

oboukili commented Nov 23, 2017

ISSUE TYPE
 - New Module Pull Request
COMPONENT NAME
dyn_record
ANSIBLE VERSION
ansible 2.5.0 (devel e2ccb71b18) last updated 2017/11/23 11:13:26 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/bki/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/bki/Workspaces/ansible/lib/ansible
  executable location = /home/bki/Workspaces/ansible/bin/ansible
  python version = 2.7.13 (default, May 10 2017, 20:04:28) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]
                                                    
SUMMARY
Manage DNS records on Dyn service (dyn.com)
@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 23, 2017

The test ansible-test compile --python 2.6 [?] failed with the following error:

lib/ansible/modules/web_infrastructure/dyn_record.py:362:36: SyntaxError: filteredkeys = {'record_id'}

The test ansible-test sanity --test ansible-doc --python 2.6 [?] failed with the following error:

Command "ansible-doc dyn_record" returned exit status 1.
>>> Standard Error
ERROR! module dyn_record missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 2.7 [?] failed with the following error:

Command "ansible-doc dyn_record" returned exit status 1.
>>> Standard Error
ERROR! module dyn_record missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.5 [?] failed with the following error:

Command "ansible-doc dyn_record" returned exit status 1.
>>> Standard Error
ERROR! module dyn_record missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.6 [?] failed with the following error:

Command "ansible-doc dyn_record" returned exit status 1.
>>> Standard Error
ERROR! module dyn_record missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test import --python 2.6 [?] failed with the following error:

lib/ansible/modules/web_infrastructure/dyn_record.py:362:36: SyntaxError: invalid syntax

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/web_infrastructure/dyn_record.py:262:13: E123 closing bracket does not match indentation of opening bracket's line
lib/ansible/modules/web_infrastructure/dyn_record.py:287:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/web_infrastructure/dyn_record.py:302:91: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:303:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:303:84: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:304:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:304:80: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:305:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:307:89: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:308:45: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:309:43: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:336:76: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:337:41: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:339:85: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:340:41: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:341:39: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:345:16: E714 test for object identity should be 'is not'
lib/ansible/modules/web_infrastructure/dyn_record.py:366:91: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:367:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:367:84: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:368:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:368:86: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:369:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:372:89: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:373:45: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:374:43: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:390:95: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:391:49: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:391:88: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:392:49: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:392:90: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:393:49: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:393:72: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:394:49: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:397:93: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:398:49: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:399:47: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:425:91: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:426:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:426:84: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:427:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:427:86: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:428:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:428:68: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:429:45: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:431:89: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:432:45: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:433:43: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:446:99: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:447:41: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:449:85: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:450:41: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:451:39: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:467:92: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:468:41: E127 continuation line over-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:471:85: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:472:41: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:473:39: E124 closing bracket does not match visual indentation
lib/ansible/modules/web_infrastructure/dyn_record.py:516:85: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:517:45: E128 continuation line under-indented for visual indent
lib/ansible/modules/web_infrastructure/dyn_record.py:561:49: E502 the backslash is redundant between brackets
lib/ansible/modules/web_infrastructure/dyn_record.py:564:9: E125 continuation line with same indent as next logical line

The test ansible-test sanity --test validate-modules [?] failed with the following error:

lib/ansible/modules/web_infrastructure/dyn_record.py:57:69: E302 DOCUMENTATION is not valid YAML

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 23, 2017

The test ansible-test compile --python 2.6 [?] failed with the following error:

lib/ansible/modules/web_infrastructure/dyn_record.py:359:36: SyntaxError: filteredkeys = {'record_id'}

The test ansible-test sanity --test ansible-doc --python 2.6 [?] failed with the following error:

Command "ansible-doc dyn_record" returned exit status 1.
>>> Standard Error
ERROR! module dyn_record missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test import --python 2.6 [?] failed with the following error:

lib/ansible/modules/web_infrastructure/dyn_record.py:359:36: SyntaxError: invalid syntax

click here for bot help

@oboukili oboukili force-pushed the oboukili:devel branch 2 times, most recently to 94e7a13 Nov 23, 2017

@oboukili

This comment has been minimized.

Contributor

oboukili commented Nov 23, 2017

ready_for_review

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 23, 2017

@Slezhuk @agmezr @berendt @ghjm @hogarthj @inetfuture @jhoekx @jlaska @jtyr @lekum @matburt @mattupstate @mgruener @n0trax @nerzhul @ramondelafuente @robinro @scottanderson42 @sermilrod @tarka @tastychutney @wwitzel3

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@resmo

This comment has been minimized.

Member

resmo commented Nov 25, 2017

Hi @oboukili ! Great to see a dyn module, I also started to work on it but didn't publish anything yet. So let's join forces. :)

I noticed you pass a list of records, but in the code you loop over the list to make the api call. So in my point of view, there is no much gain to pass a list and I would change it to only handle one record at a time. This reduces complexity and Ansible has loop already built in. The only "gain" would be to reduce "reauthenticating" for each record. This problem exists in many modules and is going to be addressed in a global context for several modules. Did I miss anything?

You already separated out the authentication part, I assume when more dyn modules arrive, we should reuse it and move it to module_utils to share it across the dyn modules.

Speaking of more dyn modules, what other modules are you working on currently?

Further, the path the module is currently located is not ideal. Please move it to to a subpath modules/net_tools/dynect (you have to create an emtpy init.py file).

Commenting the other improvements I see inline.

Thanks!

@resmo resmo self-requested a review Nov 25, 2017

"""Initiates module."""
module = AnsibleModule(
argument_spec=dict(
dyn_customer_name=dict(required=False, default=None, type='str'),

This comment has been minimized.

@resmo

resmo Nov 25, 2017

Member

instead of the os env handling code in the class DynectAPI, I would just handle it here, e.g. for dyn_customer_name, the only thing in DynectAPI is to check for None.

- dyn_customer_name=dict(required=False, default=None, type='str'),
+ dyn_customer_name=dict(required=False, default=os.environ.get('DYN_CUSTOMER_NAME'), type='str'),

This comment has been minimized.

@oboukili

oboukili Nov 25, 2017

Contributor

Ok i'll get to it

@oboukili

This comment has been minimized.

Contributor

oboukili commented Nov 25, 2017

Hey @resmo ! Glad to have you onboard :), let me answer your questions :

I noticed you pass a list of records, but in the code you loop over the list to make the api call. So in my point of view, there is no much gain to pass a list and I would change it to only handle one record at a time

This is indeed a choice I made to reduce API calls not to trigger Dyn's API rate limiting:

  • As you noticed, for the authentication part first.
  • For the future, to be able to handle multiple record duplicates present in the parameters and/or upstream on the DNS zone. By duplicate records, I mean records that share the same fqdn for specific record types like TXT, but not the same value. Handling only a single record at a time would not get us the expected results (i.e. "which record among all these would be updated?").
    Therefore, I would like to keep things that way, but we could always add more optional input parameters ("node", "ttl", "rdata") to be able to answer this simpler need if the "records" parameter is not present, what do you think?

Dyn exposes endpoints to handle this particular update case which this PR does not handle yet.
I was working on a complete rework with a colleague of mine, but since this immediate need has vanished for our use case, I thought I would make a PR of the existing code.

You already separated out the authentication part, I assume when more dyn modules arrive, we should reuse it and move it to module_utils to share it across the dyn modules.

👍 , this was mainly to be able to import the module within a custom library path as long as the PR isn't merged, I couldn't seem to specify a "library_utils" path according to the docs! I'll get right to it.

Speaking of more dyn modules, what other modules are you working on currently?

None so far. Though I think I'll get to working on a traffic director module in the following weeks (probably january 2018) if you do not first ;)

Further, the path the module is currently located is not ideal. Please move it to to a subpath modules/net_tools/dynect (you have to create an emtpy init.py file).

Ok will do.

Thanks for your much appreciated feedback!

@resmo

This comment has been minimized.

Member

resmo commented Nov 26, 2017

For the future, to be able to handle multiple record duplicates present in the parameters and/or upstream on the DNS zone. By duplicate records, I mean records that share the same fqdn for specific record types like TXT, but not the same value. Handling only a single record at a time would not get us the expected results (i.e. "which record among all these would be updated?").
Therefore, I would like to keep things that way, but we could always add more optional input parameters ("node", "ttl", "rdata") to be able to answer this simpler need if the "records" parameter is not present, what do you think?

I would agree to have a record with multiple values per specific type. e.g. an A type record can have a list of values, e.g. IPs. But having a list of records does not help much to find out which one to update neither, does it?

Further the rate limiting is not addressed well, the loop in which you do a http request *for each records does not help for hitting the api rate limits. also see https://help.dyn.com/managed-dns-api-rate-limit/. The way I do is check if we hit the rate limit and act (sleep and retry) e.g. the way I did for vultr api https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/vultr.py#L124 . Further since ansible is not necessary a centralized executed program. Many systems or people can possibly run playbooks creating records. Next, the gain we have to "not reauth" for every record is small. Ansible will have "something" in the future to share sessions within a play or even the whole execution time. With your current implementation every task still has to authenticate anyway.

Also probably the most important part; The current implementation reinvents many parts what ansible already do in a higher level. That is why I am -1 with this implementation.

@oboukili

This comment has been minimized.

Contributor

oboukili commented Nov 27, 2017

I would agree to have a record with multiple values per specific type. e.g. an A type record can have a list of values, e.g. IPs. But having a list of records does not help much to find out which one to update neither, does it?

It does reduce the possibilities of erroneous updates if you provide the unchanged records within the input list (basically if you consider the zone as a whole). Futhermore it can reduce the number of API calls if you use Dyn's 'replace' endpoint (one call per record type, the replace logic is then handled server side), I probably get to work on that later on. I agree though that for the moment it's basically not used so I agree momentarily with your point :)

Further the rate limiting is not addressed well, the loop in which you do a http request *for each records does not help for hitting the api rate limits. also see https://help.dyn.com/managed-dns-api-rate-limit/. The way I do is check if we hit the rate limit and act (sleep and retry) e.g. the way I did for vultr api https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/vultr.py#L124 .

Ok i'll do that, thanks for the pointers.

@oboukili

This comment has been minimized.

Contributor

oboukili commented Nov 27, 2017

ready_for_review

@ansibot ansibot added the net_tools label Nov 27, 2017

@oboukili

This comment has been minimized.

Contributor

oboukili commented Dec 4, 2017

@resmo did you have the time to take a look at the changes?

@ansibot ansibot added the stale_ci label Dec 12, 2017

@oboukili oboukili force-pushed the oboukili:devel branch from 7b57096 to d4e3d2d Aug 8, 2018

@oboukili

This comment has been minimized.

Contributor

oboukili commented Aug 8, 2018

Seems to all work out, @resmo if you're still around let me know if you want me to rebase the commits before merging. Thanks.

@oboukili oboukili force-pushed the oboukili:devel branch from d4e3d2d to 591b3cb Oct 22, 2018

@oboukili

This comment has been minimized.

Contributor

oboukili commented Oct 22, 2018

I rebased to remove the stale_ci label.

@ansibot ansibot added the needs_ci label Oct 22, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 30, 2018

Closing and re-opening to trigger CI.

@mattclay mattclay closed this Oct 30, 2018

@mattclay mattclay reopened this Oct 30, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 30, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/net_tools/dynect/dyn_record.py:0:0: E307 version_added should be 2.8. Currently 2.7

click here for bot help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment