[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 15:31:51 UTC 2015
On 14/12/15 15:25, David Kupka wrote:
> On 14/12/15 14:52, David Kupka wrote:
>> 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.
>>
> As always, FreeIPA's code is really live before release. Rebased patches
> attached.
>
Updated patches attached.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0072.4-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/cec53541/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0058.15-dns-do-not-add-forward-zone-if-it-is-already-resolva.patch
Type: text/x-patch
Size: 12390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/cec53541/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0064.12-dns-Check-if-domain-already-exists.patch
Type: text/x-patch
Size: 19528 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/cec53541/attachment-0002.bin>
More information about the Freeipa-devel
mailing list