[Freeipa-devel] [PATCH 0393-0398] Unload automatic empty zones only if conflicting forward zone has policy 'only'Add ability to log warningsUnload automatic empty zones which are super/sub/equal domain as forward zoneDo not touch global forwarders from named.conf if there is no config in LDAPAdd missing includes to util.hMove zone_isempty() and delete_bind_zone() to zone_register.c

[Tomas Hozza]

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