FreeIPA: Add SRV and MX record types to ipa_dnsrecord #42482

synical · 2018-07-08T21:12:23Z

SUMMARY

Found myself having to template shell scripts to add SRV records, figured would try to add to the ipa_dnsrecord module to make first class. Seems it was pretty straight forward, tested it out and worked as expected. Unclear if there are any other changes required that I am missing?

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

ipa_dnsrecord

ANSIBLE VERSION

ansible 2.6.0

ADDITIONAL INFORMATION

ansibot · 2018-07-08T21:20:34Z

cc @Akasurde @Nosmoht @fxfitz
click here for bot help

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py

Akasurde · 2018-07-09T03:38:42Z

@synical Thanks for the contribution. I will test and let you know the review.

Akasurde · 2018-07-09T08:07:45Z

@synical I tested code and it works fine. Could you please add example of SRV record in EXAMPLE section as SRV requires 'Priority Weight Port Target' as a value record_value?

Akasurde

see in-line comments

synical · 2018-07-09T12:04:37Z

@Akasurde thanks, I've pushed up a commit with the requested changes

Akasurde · 2018-07-09T12:17:27Z

@synical Thanks. MX record is also easy I guess.

synical · 2018-07-09T13:08:21Z

@Akasurde yeah I would imagine so. Want that on this PR and a rename or in a separate PR?

Akasurde · 2018-07-09T13:11:28Z

@synical I would prefer in this same PR and renaming title.

synical · 2018-07-09T18:16:11Z

@Akasurde added the change for MX records. Same idea as before.

I did notice that if I use the develop version of this file with these changes the record_ttl is mandatory (IPA complains it's not an int if missing) and that "state: present" results in an error if the record already exists. I don't think I got those on the 2.6 version of code with these changes.

Would you be able to test?

fxfitz

I just tested this patch too; SRV records seem to work as expected.

However, I'm getting an error with your MX example but I haven't dug into exactly why yet:

fatal: [vagrant-cms-freeipa-01]: FAILED! => {"changed": false, "msg": "response dnsrecord_add: no modifications to be performed"}

Additionally, I'm seeing your TTL issue as well. We added that in recently: #41768

fxfitz · 2018-07-09T18:51:47Z

(note: the MX error occurs when the record already exists, so that leads me to believe something is off with the diff)

synical · 2018-07-09T18:54:10Z

@fxfitz yeah I hit this too. Was wondering how "state: present" works as I figured that option should handle this? Unless you hit this issue the first time you create the record?

synical · 2018-07-19T02:48:34Z

@fxfitz @Akasurde apologies for the delay. Managed to get this working with the changes in the latest commit. As per the links @jamescassell shared, looks like when searching for the @ record dnsrecord_find won't be able to find the record.

I added a method for dnsrecord_show and call it only when the record name is equal to "@" to handle this case.

This might not be the best approach, but interested to see if this works for others when testing. I managed to add a new MX record, modify it and delete it using this code.

jamescassell

Looks good. Thanks for adding this feature!

jamescassell · 2018-07-19T02:56:17Z

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py

@@ -161,6 +161,9 @@ def __init__(self, module, host, port, protocol):
    def dnsrecord_find(self, zone_name, record_name):
        return self._post_json(method='dnsrecord_find', name=zone_name, item={'idnsname': record_name, 'all': True})


could you alternatively use a ternary operation to change the method to dnsrecord_find within the local dnsrecord_find() function only if record_name=="@"? Presumably this would be wanted for any attempt to access the '@' record... But I'll defer to those with more experience in this project and python.

Yeah was considering renaming the method to something like get_dns_record() and adding the if in there like you mention, however, unsure on conventions or style in this repo in general. @Akasurde @fxfitz probably better positioned to advise on that.

jamescassell · 2019-03-07T01:22:57Z

Any chance of getting movement on this one?

synical · 2019-03-07T02:27:26Z

@jamescassell I'm happy to make any changes required RE my last comment, not sure who is best to make the decision there?

jamescassell · 2019-03-07T02:40:01Z

Can you add it to the core meeting agenda and show up to discuss it? ansible/community#450

I'd love to see this merged in time for 2.8; freeze is a few weeks away.

Akasurde · 2019-03-07T12:10:17Z

@synical Could you please update PR as per @jamescassell's suggestion and let me know? Thanks.

synical · 2019-03-07T16:51:28Z

Thanks @Akasurde

@jamescassell I pushed a commit. Is this what you were thinking of?

ansibot · 2019-03-07T17:09:01Z

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

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:0: syntax-error invalid syntax (&lt;unknown&gt;, line 164)

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
PYTHONPATH=../../lib ../bin/plugin_formatter.py -t rst --template-dir=../templates --module-dir=../../lib/ansible/modules -o rst/modules/ 
Evaluating module files...
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 779, in <module>
    main()
  File "../bin/plugin_formatter.py", line 734, in main
    plugin_info, categories = get_plugin_info(options.module_dir, limit_to=options.limit_to, verbose=(options.verbosity > 0))
  File "../bin/plugin_formatter.py", line 294, in get_plugin_info
    doc, examples, returndocs, metadata = plugin_docs.get_docstring(module_path, fragment_loader, verbose=verbose)
  File "/root/ansible/lib/ansible/utils/plugin_docs.py", line 105, in get_docstring
    data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
  File "/root/ansible/lib/ansible/parsing/plugin_docs.py", line 40, in read_docstring
    M = ast.parse(b_module_data.read())
  File "/usr/lib/python3.6/ast.py", line 35, in parse
    return compile(source, filename, mode, PyCF_ONLY_AST)
  File "<unknown>", line 164
    else
       ^
SyntaxError: invalid syntax
make: *** [modules] Error 1

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test compile --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: else

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: SyntaxError: invalid syntax

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

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:165:13: E113 unexpected indentation

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

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:0:0: E401 Python SyntaxError while parsing module

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

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py:164:13: python-syntax-error invalid syntax (<unknown>, line 164)

click here for bot help

jamescassell · 2019-03-07T17:49:19Z

Looks great! That way the workaround is inherited for all three uses of the function.

* Add dnsrecord_show method for supporting base domain record * Replace dnsrecord_show with conditional in dnsrecord_find * Added changelog entry for MX and SRV Signed-off-by: synical <sjohnsondot@gmail.com>

Akasurde · 2019-03-08T04:19:03Z

@synical Thanks for the contribution and patients. @fxfitz @jamescassell @achinthagunasekara Thanks for the reviews.

achinthagunasekara approved these changes Jul 9, 2018

View reviewed changes

Akasurde reviewed Jul 9, 2018

View reviewed changes

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py Show resolved Hide resolved

Akasurde reviewed Jul 9, 2018

View reviewed changes

lib/ansible/modules/identity/ipa/ipa_dnsrecord.py Outdated Show resolved Hide resolved

Akasurde removed the needs_triage Needs a first human triage before being processed. label Jul 9, 2018

Akasurde requested changes Jul 9, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 9, 2018

fxfitz mentioned this pull request Jul 9, 2018

ipa_dnsrecord: add MX record #41842

Closed

synical changed the title ~~Add SRV record type to ipa_dnsrecord~~ Add SRV and MX record types to ipa_dnsrecord Jul 9, 2018

This comment has been minimized.

Sign in to view

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 9, 2018

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 9, 2018

fxfitz suggested changes Jul 9, 2018

View reviewed changes

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 18, 2018

jamescassell approved these changes Jul 19, 2018

View reviewed changes

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 28, 2018

ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Sep 22, 2018

ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 8, 2018

dagwieers added the ipa IPA community label Feb 8, 2019

ansibot added the identity Identity category label Feb 16, 2019

ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Mar 7, 2019

ipa_dnsrecord: Add SRV and MX record type

38bf018

* Add dnsrecord_show method for supporting base domain record * Replace dnsrecord_show with conditional in dnsrecord_find * Added changelog entry for MX and SRV Signed-off-by: synical <sjohnsondot@gmail.com>

Akasurde force-pushed the ipa/srv-record-support branch from 10ea47d to 38bf018 Compare March 8, 2019 04:02

Akasurde changed the title ~~Add SRV and MX record types to ipa_dnsrecord~~ FreeIPA: Add SRV and MX record types to ipa_dnsrecord Mar 8, 2019

Akasurde self-assigned this Mar 8, 2019

Akasurde added affects_2.8 This issue/PR affects Ansible v2.8 and removed affects_2.7 This issue/PR affects Ansible v2.7 labels Mar 8, 2019

Akasurde merged commit f470736 into ansible:devel Mar 8, 2019

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeIPA: Add SRV and MX record types to ipa_dnsrecord #42482

FreeIPA: Add SRV and MX record types to ipa_dnsrecord #42482

synical commented Jul 8, 2018

ansibot commented Jul 8, 2018

Akasurde commented Jul 9, 2018

Akasurde commented Jul 9, 2018

Akasurde left a comment

synical commented Jul 9, 2018

Akasurde commented Jul 9, 2018 •

edited

synical commented Jul 9, 2018

Akasurde commented Jul 9, 2018

This comment has been minimized.

synical commented Jul 9, 2018

fxfitz left a comment

fxfitz commented Jul 9, 2018

synical commented Jul 9, 2018 •

edited

synical commented Jul 19, 2018

jamescassell left a comment

jamescassell Jul 19, 2018

synical Jul 20, 2018

jamescassell commented Mar 7, 2019

synical commented Mar 7, 2019

jamescassell commented Mar 7, 2019

Akasurde commented Mar 7, 2019 •

edited

synical commented Mar 7, 2019

ansibot commented Mar 7, 2019

jamescassell commented Mar 7, 2019

Akasurde commented Mar 8, 2019

		@@ -161,6 +161,9 @@ def __init__(self, module, host, port, protocol):
		def dnsrecord_find(self, zone_name, record_name):
		return self._post_json(method='dnsrecord_find', name=zone_name, item={'idnsname': record_name, 'all': True})

FreeIPA: Add SRV and MX record types to ipa_dnsrecord #42482

FreeIPA: Add SRV and MX record types to ipa_dnsrecord #42482

Conversation

synical commented Jul 8, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jul 8, 2018

Akasurde commented Jul 9, 2018

Akasurde commented Jul 9, 2018

Akasurde left a comment

Choose a reason for hiding this comment

synical commented Jul 9, 2018

Akasurde commented Jul 9, 2018 • edited

synical commented Jul 9, 2018

Akasurde commented Jul 9, 2018

This comment has been minimized.

synical commented Jul 9, 2018

fxfitz left a comment

Choose a reason for hiding this comment

fxfitz commented Jul 9, 2018

synical commented Jul 9, 2018 • edited

synical commented Jul 19, 2018

jamescassell left a comment

Choose a reason for hiding this comment

jamescassell Jul 19, 2018

Choose a reason for hiding this comment

synical Jul 20, 2018

Choose a reason for hiding this comment

jamescassell commented Mar 7, 2019

synical commented Mar 7, 2019

jamescassell commented Mar 7, 2019

Akasurde commented Mar 7, 2019 • edited

synical commented Mar 7, 2019

ansibot commented Mar 7, 2019

jamescassell commented Mar 7, 2019

Akasurde commented Mar 8, 2019

Akasurde commented Jul 9, 2018 •

edited

synical commented Jul 9, 2018 •

edited

Akasurde commented Mar 7, 2019 •

edited