[Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'

[Petr Vobornik]

On 25.8.2014 14:52, Martin Basti wrote:
> Patches attached.
>
> Ticket: https://fedorahosted.org/freeipa/ticket/4149
>
> There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the
> named service is stopped after deleting zone.
> Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138
>
>

Review of:
http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html

1. Please follow pep8 for the new code.
  # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501
Produces 25 erros.

Only E124 and E128 could be ignored if they are part of old code.

Patch 114:

2. Also should mention reverse zones:
-        # except for our own domain
+        # except for our own domain, and root zone

Patch 115: looks ok (except #1)

Patch 120:

3. This patch uses term 'deprecated' in a different meaning than a 
DeprecatedParam. It creates inconsistency -> future confusion. IMHO this 
usage is correct since the usual understanding of deprecation is that 
the param is still usable but user should be prepared that it will be 
removed in a future.  IMHO DeprecatedParam is badly designed but that's 
not an issue of this patch.

I think we can leave this as is and create a ticket to rename 
DeprecatedParam e.g. to RemovedParam. What do you think?

That said, I don't see a reason for keeping 'doc', 'label', 
_validate_ipaddr in 'ip_address' param definition since the param is not 
used anywhere, and is not present in CLI nor Web UI.

4. Convention for defining optional param is to use '?' at the end of 
name not: `required=False,` (idnssoamname)

5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add 
precallback still uses these params. What is the intended purpose?

Patch 121:

6. Could this be evaluated as 'None' string? 
idnssoamname=unicode(ns_hostname),

the method doesn't guarantee that 'hostname  is not None' and 
ipa_replica_prepare calls it without args: `add_zone(domain)`


7. This line is too long:
+        self.step("adding NS record to the zones", self.__add_self_ns) 
  # all zones must be created before this step

8. Forgotten debugging lines:
     print "no ldap2 connection"  #TODO remove

and maybe:
     print "entry", repr(entry)


9. Is the LDAP connection check in remove_server_ns_records required? 
Other RR removal methods are called before this one and they don't have 
it. IMO if it has to be done, it should be done by a caller.


Note: I have not tested replica installation and removal in this round 
of review process.


Patch 123:

10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't 
it be normalized to contain '.' at the end? Or is it handled by 
bind-dyndb-ldap?

11. You can remove `validate_zonemgr` import from dns.py	

Patch 124:

12. 'idnssoamname' field in dnszone details page should be hidden or 
marked as required since the LDAP attribute is a MUST

13. You can completely remove `IPA.dnszone_adder_dialog` class since it 
existed only to contain the logic for idnssamname and ip_address fields 
(depends on #5).

Then remove `$factory: IPA.dnszone_adder_dialog,` from the dialog spec.

Patch 125:
14. Got:

   File 
"/home/pvoborni/dev/freeipa/ipatests/test_xmlrpc/test_dns_plugin.py", 
line 332, in <module>
     class test_dns(Declarative):
   File 
"/home/pvoborni/dev/freeipa/ipatests/test_xmlrpc/test_dns_plugin.py", 
line 422, in test_dns
     'nsrecord': nameservers,
NameError: name 'nameservers' is not defined

Did not investigate much.. but this line is weird:
   `e = "No DNS servers found in LDAP"` shouldn't 'e' be 
'get_nameservers_error'?


Unrelated to this patch set:

a. One is able to run:
   # ipa dnszone-remove-permission $zone
multiple times and it always returns success

Is it intentional?

b. Web UI doesn't have means to call dnszone-mod with --force option

-- 
Petr Vobornik