[Freeipa-devel] [PATCH 0058, 0064] dns: do not add (forward)zone if it is already resolvable.
David Kupka
dkupka at redhat.com
Mon Dec 14 13:52:51 UTC 2015
On 11/12/15 15:00, Petr Spacek wrote:
> On 11.12.2015 12:35, David Kupka wrote:
>> On 10/12/15 18:10, Petr Spacek wrote:
>>> On 10.12.2015 17:31, David Kupka wrote:
>>>> On 09/12/15 18:55, Petr Spacek wrote:
>>>>> On 9.12.2015 13:37, David Kupka wrote:
>>>>>> On 08/12/15 15:24, Petr Spacek wrote:
>>>>>>> On 8.12.2015 12:19, David Kupka wrote:
>>>>>>>> On 08/12/15 08:56, Petr Spacek wrote:
>>>>>>>>> On 7.12.2015 14:41, David Kupka wrote:
>>>>>>>>>> +def is_host_resolvable(fqdn):
>>>>>>>>>> + if not isinstance(fqdn, DNSName):
>>>>>>>>>> + fqdn = DNSName(fqdn)
>>>>>>>>>> + for rdtype in (rdatatype.A, rdatatype.AAAA):
>>>>>>>>>> + try:
>>>>>>>>>> + resolver.query(fqdn.make_absolute(), rdtype)
>>>>>>>>>> + except DNSException:
>>>>>>>>>> + continue
>>>>>>>>>> + else:
>>>>>>>>>> + return True
>>>>>>>>>> +
>>>>>>>>>> + return False
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> NACK, you are re-introducing duplicate function which was removed in
>>>>>>>>> 498471e4aed1367b72cd74d15811d0584a6ee268.
>>>>>>>>>
>>>>>>>>> Please amend the patch ASAP to use new verify_host_resolvable() function
>>>>>>>>> so I
>>>>>>>>> can test it and get it into 4.3.
>>>>>>>>>
>>>>>>>> Updated patches attached.
>>>>>>>
>>>>>>> Hmm, my review script screams:
>>>>>>>
>>>>>>> Detected base branch: origin/master
>>>>>>> Checks will be made against following base commit: 848912a add missing
>>>>>>> /ipaplatform/constants.py to .gitignore
>>>>>>> Writing API to API.txt
>>>>>>> ./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
>>>>>>> indentation
>>>>>>> ./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
>>>>>>> visual indent
>>>>>>> ./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
>>>>>>> indentation
>>>>>>> ./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
>>>>>>> visual indent
>>>>>>> ./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
>>>>>>> ./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
>>>>>>> visual indent
>>>>>>> ./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
>>>>>>> ./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
>>>>>>> ./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
>>>>>>> ./ipaserver/install/bindinstance.py:49:9: E128 continuation line
>>>>>>> under-indented for visual indent
>>>>>>> ./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
>>>>>>> characters)
>>>>>>> ./ipaserver/install/bindinstance.py:438:19: E128 continuation line
>>>>>>> under-indented for visual indent
>>>>>>> ./ipaserver/install/server/common.py:271:63: E261 at least two spaces
>>>>>>> before
>>>>>>> inline comment
>>>>>>> ./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
>>>>>>> characters)
>>>>>>> ERROR: PEP8 --diff failed, continuing ...
>>>>>>> ERROR: API.txt was changed without a change in VERSION, continuing ...
>>>>>>> Summary of detected errors:
>>>>>>> ERROR: PEP8 --diff failed
>>>>>>> ERROR: API.txt was changed without a change in VERSION
>>>>>>>
>>>>>>> Please fix it :-)
>>>>>>>
>>>>>>> Make make this more pleasant for you, you can use (and review) my attached
>>>>>>> patch. It adds 'review' target to Makefile, it will do the same checks as
>>>>>>> I do.
>>>>>>>
>>>>>>
>>>>>> Unfortunately your tool does not work for me (output w/ -o xtrace attached).
>>>>>> Anyway I have incremented API version and fixed most of the pep8 errors
>>>>>> except
>>>>>> for E124 twice in ipalib/plugins/dns.py as it seems to be convention in the
>>>>>> file and E501 twice in ipapython/ipautil.py because IMO breaking string
>>>>>> constant into multiple lines does not help readability.
>>>>>>
>>>>>> Updated patches also attached.
>>>>>
>>>>> We are almost there, but NACK for now.
>>>>>
>>>>> 1) Following attempt to install IPA explodes. Please note that
>>>>> dom-245.idm.lab.eng.brq.redhat.com DNS domain is delegated to the IPA server
>>>>> before installation is started, so it gives 'timeout' or possibly SERVFAIL.
>>>>>
>>>>> # ipa-server-install --ds-password=root4lab --admin-password=root4lab
>>>>> --setup-dns --forwarder=10.38.5.26 --no-reverse --allow-zone-overlap
>>>>> --domain=dom-245.idm.lab.eng.brq.redhat.com
>>>>> --realm=DOM-245.IDM.LAB.ENG.BRQ.REDHAT.COM
>>>>> [...]
>>>>>
>>>>> Configuring DNS (named)
>>>>> [1/11]: generating rndc key file
>>>>> [2/11]: adding DNS container
>>>>> [3/11]: setting up our zone
>>>>> [error] InvocationError: DNS check for domain
>>>>> dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
>>>>> 30.000453949 seconds. Please make sure that the domain is properly delegated
>>>>> to this IPA server.
>>>>> ipa.ipapython.install.cli.install_tool(Server): ERROR DNS check for domain
>>>>> dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
>>>>> 30.000453949 seconds. Please make sure that the domain is properly delegated
>>>>> to this IPA server.
>>>>> ipa.ipapython.install.cli.install_tool(Server): ERROR The
>>>>> ipa-server-install command failed. See /var/log/ipaserver-install.log for
>>>>> more
>>>>> information
>>>>>
>>>>> 2015-12-09T17:15:37Z DEBUG File
>>>>> "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in
>>>>> execute
>>>>> return_value = self.run()
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/cli.py", line
>>>>> 318,
>>>>> in run
>>>>> cfgr.run()
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 310,
>>>>> in run
>>>>> self.execute()
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 332,
>>>>> in execute
>>>>> for nothing in self._executor():
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 372,
>>>>> in __runner
>>>>> self._handle_exception(exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 394,
>>>>> in _handle_exception
>>>>> six.reraise(*exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 362,
>>>>> in __runner
>>>>> step()
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 359,
>>>>> in <lambda>
>>>>> step = lambda: next(self.__gen)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/util.py",
>>>>> line 81,
>>>>> in run_generator_with_yield_from
>>>>> six.reraise(*exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/util.py",
>>>>> line 59,
>>>>> in run_generator_with_yield_from
>>>>> value = gen.send(prev_value)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 571,
>>>>> in _configure
>>>>> next(executor)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 372,
>>>>> in __runner
>>>>> self._handle_exception(exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 449,
>>>>> in _handle_exception
>>>>> self.__parent._handle_exception(exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 394,
>>>>> in _handle_exception
>>>>> six.reraise(*exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 446,
>>>>> in _handle_exception
>>>>> super(ComponentBase, self)._handle_exception(exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 394,
>>>>> in _handle_exception
>>>>> six.reraise(*exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 362,
>>>>> in __runner
>>>>> step()
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/core.py",
>>>>> line 359,
>>>>> in <lambda>
>>>>> step = lambda: next(self.__gen)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/util.py",
>>>>> line 81,
>>>>> in run_generator_with_yield_from
>>>>> six.reraise(*exc_info)
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/util.py",
>>>>> line 59,
>>>>> in run_generator_with_yield_from
>>>>> value = gen.send(prev_value)
>>>>>
>>>>> File "/usr/lib/python2.7/site-packages/ipapython/install/common.py", line
>>>>> 63, in _install
>>>>> for nothing in self._installer(self.parent):
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
>>>>> line 1471, in main
>>>>> install(self)
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
>>>>> line 265, in decorated
>>>>> func(installer)
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
>>>>> line 958, in install
>>>>> dns.install(False, False, options)
>>>>> File "/usr/lib/python2.7/site-packages/ipaserver/install/dns.py", line
>>>>> 322,
>>>>> in install
>>>>> bind.create_instance()
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
>>>>> line 680, in create_instance
>>>>> self.start_creation()
>>>>> File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py",
>>>>> line
>>>>> 447, in start_creation
>>>>> run_step(full_msg, method)
>>>>> File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py",
>>>>> line
>>>>> 437, in run_step
>>>>> method()
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
>>>>> line 805, in __setup_zone
>>>>> ns_hostname=self.api.env.host, force=True, api=self.api)
>>>>> File
>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
>>>>> line 331, in add_zone
>>>>> force=force)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 447, in
>>>>> __call__
>>>>> ret = self.run(*args, **options)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 764,
>>>>> in run
>>>>> return self.execute(*args, **options)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line
>>>>> 2781, in
>>>>> execute
>>>>> result = super(dnszone_add, self).execute(*keys, **options)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py", line
>>>>> 1233, in execute
>>>>> *keys, **options)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line
>>>>> 2747, in
>>>>> pre_callback
>>>>> ldap, dn, entry_attrs, attrs_list, *keys, **options)
>>>>> File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line
>>>>> 2153, in
>>>>> pre_callback
>>>>> raise errors.InvocationError(e.message)
>>>>>
>>>>>
>>>>> 2) Please print 'Checking DNS domain <xyz>, please wait ...' when validating
>>>>> domain parameter in installer. The timeout is 30 seconds and users may be
>>>>> nervous when the installed blocks for 30 seconds without a visible reason.
>>>>>
>>>>> Precedent for this is in check_forwarders() in
>>>>> ipaserver/install/bindinstance.py . Encapsulating check_zone_overlap() in
>>>>> auxiliary method may be an option.
>>>>>
>>>>>
>>>>> 3) Timeout is a fatal error instead of warning. We have been discussing this
>>>>> and it should really be just a warning.
>>>>>
>>>>>
>>>>> 4) Nitpicks are attached in .patch file.
>>>>>
>>>>>
>>>>> 5) ipa dnszone-add checks work as expected, good job!
>>>>>
>>>>>
>>>>> Thank you for patience!
>>>>>
>>>> Updated patches attached. Added patch introducing --auto-reverse option that
>>>> should generate needed reverse zones automatically.
>>>>
>>> NACK:
>>>
>>> $ ipa-server-install --setup-dns
>>>
>>> Do you want to configure the reverse zone? [yes]:
>>> ipa.ipapython.install.cli.install_tool(Server): ERROR 'CheckedIPAddress'
>>> object has no attribute 'split'
>>>
>> Updated patches attached.
>>
> NACK
>
> 1) It breaks replica installation if replica is second+ DNS server.
>
> # ipa-replica-install --setup-dns
> Password for admin at ABC.IDM.LAB.ENG.BRQ.REDHAT.COM:
> Checking DNS domain abc.idm.lab.eng.brq.redhat.com., please wait ...
> Your system may be partly configured.
> Run /usr/sbin/ipa-server-install --uninstall to clean up.
>
> ipa.ipapython.install.cli.install_tool(Replica): ERROR DNS zone
> abc.idm.lab.eng.brq.redhat.com. already exists in DNS and is handled by
> server(s): vm-058-155.abc.idm.lab.eng.brq.redhat.com.
>
> Maybe the validator should have condition like
> "if not replica or not dns_is_configured()" or so ...
>
>
> 2) Check for automatic empty zones does not work:
> # ipa dnsforwardzone-add 10.in-addr.arpa. --forwarder=10.34.78.1
> Server will check DNS forwarder(s).
> This may take some time, please wait ...
> ipa: ERROR: DNS zone 10.in-addr.arpa. already exists in DNS and is handled by
> server(s): 10.IN-ADDR.ARPA.
>
> Here you have to compare names as actual DNS names and not as strings.
>
>
> Have a nice weekend!
>
Updated patches attached.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0072.2-dns-Add-auto-reverse-option.patch
Type: text/x-patch
Size: 6702 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/5948e7c9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0058.13-dns-do-not-add-forward-zone-if-it-is-already-resolva.patch
Type: text/x-patch
Size: 12507 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/5948e7c9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0064.10-dns-Check-if-domain-already-exists.patch
Type: text/x-patch
Size: 19635 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/5948e7c9/attachment-0002.bin>
More information about the Freeipa-devel
mailing list