[Freeipa-devel] [PATCH] 195-199 New DNS features

[Martin Kosek]

On Wed, 2012-01-25 at 16:45 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > This set of patches implements support and API for features introduced
> > in a new bind-dyndb-ldap (bind-dyndb-ldap-1.1.0-0.6.a1):
> >    - global bind-dyndb-ldap settings in LDAP (cn=dns,$SUFFIX)
> >    - conditional per-zone forwarding
> >    - per-zone configuration of automatic PTR updates
> >    - zone transfer
> >    - AllowQuery and AllowTransfer ACIs
> >    - new bind-dyndb-ldap now also skips invalid records in a zone instead
> > of refusing to load an entire zone
> >
> > More detailed description and examples are in these separate patches. In
> > order to test it, a new bind-dyndb-ldap version is needed. It is not in
> > updates-testing repo yet as it waits for a new release of bind which
> > should occur in few next days. But it can be downloaded from koji:
> >
> > F15: http://koji.fedoraproject.org/koji/buildinfo?buildID=294138
> > F16: http://koji.fedoraproject.org/koji/buildinfo?buildID=294137
> >
> > Have fun!
> > Martin
> 
> 

Rob, thanks for review, I know that all this may really time-consuming.
I updated all tickets where the testing may be tricky and provided a
howto for testing.

I also did some more tests of bind-dyndb-ldap and found few bugs:

Dynamic update for zone without idnsAllowSyncPTR does not work:
https://bugzilla.redhat.com/show_bug.cgi?id=784815

Conditional forwarding does not work for sub-zones:
https://bugzilla.redhat.com/show_bug.cgi?id=784820

> In patch 195 there is a white-space fix to the idnsRecord. Was this 
> intentional? 

Yes.

> Also a typo in the commit message, AllowTransger.

Fixed. You have very keen eyes :-)

> 
> In patch 197 there is this suspicious code in _normalize_ipnetmask
> 
> +        ipnetmask = ipnetmask

Fixed.

> 
> The comment and copyright date in dns.py::update_dnszone_acls() needs to 
> be updated

Fixed. I explained why we need to set idnsAllowTransfer in all
configured zones in the fixed comment.

> 
> Patch 98 I think you want to drop the worth "with" in this?
> 
> + Forward all request for sub-zone of example.com to another nameserver with
> + using a "first" policy (it will send the queries to the selected forwarder
> yes,
> 

Yup. Fixed.

> And now for some things I saw when testing.
> 
> I upgraded an existing instance installed with DNS.
> 
> ipa dnsconfig-show returned nothing. I disabled persistent search then 
> set it to '' and now I always see
> 
> Zone refresh interval: 0
> 
> Not sure if I should have seen that initially or not.

I used to automatically disable zone refresh when persistent search was
enabled, but I see it may confuse users. I fixed this part to rather
check the current configuration in LDAP, compare it to passed values and
raise ValidationError only when an invalid combination could happen.

> 
> I tried testing the query policy but was unable to get it working:
> 
> # ipa dnszone-mod example.com --allow-query="\!10.0.0.1,any"
> # service named restart
> 
> 'dig -t soa example.com' always worked.
> 
> My test hosts are behind a NAT but I tried both the real and the NAT IP 
> address and in both cases it worked.
> 
> So I set up transfer rules instead and this time was very picky about 
> what IP address to accept and used on the NAT address. Using that it 
> worked as expected.
> 
> So I went back and worked on query again. It seems like the ! addresses 
> aren't working as expected, that or it is an ordering problem perhaps 
> (e.g. I wonder if I'm seeing the problem in your comment #16 in ticket 
> 1211).

I see you hit the same error as I hit yesterday. The main problem here
is that idnsAllowQuery and idnsAllowTransfer attribute holding the query
and transfer ACLs are multi-valued attributes. However, the evaluation
is very order-sensitive which does not play well with LDAP nature.

I discussed this with Adam Tkac and the plan is to change these ACL
attributes to single-valued and divide the ACLs with semicolon (the same
approach is already used for idnsupdatepolicy) to ensure the order.

This is a relevant upstream ticket:
https://fedorahosted.org/bind-dyndb-ldap/ticket/50

> 
> I wonder if the summary should reflect that named needs to be restarted.
> 
> rob

Hmm, interesting idea. I will have to discuss the situations and changes
of configuration in LDAP when named needs to be reloaded/restarted. A
notice in the Summary may help prevent confusion.

I will keep fixed patches in my local tree until the bind-dyndb-ldap
with fixed ACLs is released as it will require some more changes in this
patch set.

Martin