On 6.5.2016 16:41, Tomas Hozza wrote:
> 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").

I've fixed this before push.

> - 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.
Good idea! This will be documented in follow-up patch.


> - 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.
Okay, I can improve this a little bit so zones which were once unloaded will
not be reported again. This is a matter for a follow-up patch.

On the other hand, I'm going to print message for every zone because this is
was BIND 9.11 is doing.


> - 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.
This is caused by forwarding policy configured in named.conf. I will think
about possible fix but it is not that easy. For now I will leave it as it is.
Maybe rebase to BIND 9.11 will fix it for us because they print warnings, too.

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

Thanks, pushed to master:
3232aa4f35850c5164e7ec0b9cc523e3cf7bdb5d Unload automatic empty zones only if
conflicting forward zone has policy 'only'.
4174e96496aad3dc8b7040b552aa589157d9ede6 Add ability to log warnings.
5c3312cee1c080c5df45741da26ed801a9262d74 Unload automatic empty zones which
are super/sub/equal domain as forward zone.
998b3fbd055dd455c0c0f3ebf7a68efe12e5ce52 Do not touch global forwarders from
named.conf if there is no config in LDAP.
ffcfb9563ae97cdc345d5611ccc168101fdffeeb Add missing includes to util.h.
168ed7bb2145f1c515e0f6166e24797630949df2 Move zone_isempty() and
delete_bind_zone() to zone_register.c.


Thank you very much for your time!


> 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.

I think that this is there since forever and I have no idea why it happens and
what we can do to fix it :-(

-- 
Petr^2 Spacek

-- 
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