On 04/06/2016 01:42 PM, Petr Spacek wrote:
> Hello,
>
> attached patch set implements
> https://fedorahosted.org/bind-dyndb-ldap/ticket/160
> described in
> https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones
>
> It will be accompanied with upgrade code in FreeIPA which will automatically
> convert forward zones as necessary.
>

ACK.

I reviewed the patches on GitHub 
(https://github.com/pspacek/bind-dyndb-ldap/commits/empty_zones_unload_only)

I have these comments:

- commit 396 
(https://github.com/pspacek/bind-dyndb-ldap/commit/a1bb7c0f802107d1fc67de8a58d0f33095accd06)

  - it uses "forwarders" in wrong context. In many places and also in the 
commit message you should use "forward zone" instead of "forwarder". "forwarder 
is the server to which the queries are forwarded, not the zone.

  - It wold be nice to explicitly note in the commit message, that conflicting 
empty zones are unloaded regardless of the forward policy ("only" or "first").


- It would be good to document, that once you add forward zone, which causes 
removal of empty zone and then you remove the forward zone, the empty zone is 
not added back. The empty zone will appear again only after restart of BIND, 
when it loads all empty zones and only existing ones are removed. The situation 
is the same when you configure global forwarders and them remove them = all 
empty zones are unloaded and these are not added back until the restart of 
BIND. This is a limitation and IMHO the users should be informed about this 
behavior, as it may not be completely expected.

- Configuring global forwarders as forward first creates too many log messages 
(for each empty zone). This may not be a problem, but it is IMHO not necessary. 
Also when I configure global forwarders as forward "only" and then change it to 
forward "first", the log message is printed even though the empty zones are not 
there any more, which is a bit misleading.

- Warning log message about the configuration of global forward zone are logged 
every time on start-up regardless of the forward policy set in LDAP.



Other than that the changes look good to me. The functionality works as 
documented with the shortcomings mentioned above.


PS: There seems to be some kind of race condition when you configure forward 
zone and you have global forwarding turned off. In case you configure a forward 
zone with forward "only" and it conflicts with an empty zone, you can get 
SERVFAIL from the server for hostname from such zone. It is reproducible only 
if you are quick enough. The next query is forwarded correctly and the response 
from forwarder is returned.


Regards,
-- 
Tomas Hozza
Senior Software Engineer - EMEA ENG Developer Experience

PGP: 1D9F3C2D
UTC+1 (CET)
Red Hat Inc.                 http://cz.redhat.com

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to