[Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.

[Martin Basti]

<snip />

Be careful, reviewed on friday! :-)

1)
whitespace error + pep8 error
patch:76: trailing whitespace.
     # there is reverse zone for every ip address
warning: 1 line adds whitespace errors.

./ipaserver/install/bindinstance.py:640:9: E265 block comment should 
start with '# '


2) (server install)
+    for ip in ip_addresses:
+        for rev_zone in reverse_zones:
+            if bindinstance.verify_reverse_zone(rev_zone, str(ip)):
+                break
+            else:
+                sys.exit(1)

Please add there error message instead 1 to exit function

3) (server install)
Code above, bindinstance.verify_reverse_zone(rev_zone, str(ip)):
IMHO this will cause errors (not tested) try:
rev-zones: [10.10.10.in-addr.arpa., 16.172.in-addr.arpa.]
ip_addreses: [10.10.10.1, 172.16.0.1]

it should be any() of zone can handle ip

4) (replica-install)
I'm not sure about behavior of combination ip addresses, and reverse zones,
In original code, if user specify reverse zone, the ip address must match.

In patched version, you just add new zones for ip-addresses which 
doesn't math the user zones.

IMO we should check if all addresses belongs to user specified zones, 
otherwise raise an error.
But I'm open to suggestions.

5)
Code for ipa-replica-install, and ipa-server-install, differs in parts 
where we expecting same behavior
   - ip addresses and reverse zones

6)
<https://engineering.redhat.com/irc-services.txt>+    # there is at 
least one ip address in every zone
+    if options.reverse_zones:
+        for reverse_zone in options.reverse_zones:
+            for ip in config.ips: 
<---------------------------------------------------------------------------------- 
A
+                if bindinstance.verify_reverse_zone(reverse_zone, ip):
+ reverse_zones.append(bindinstance.normalize_zone(reverse_zone))
+                    break
+            else:
+                sys.exit(1) 
<------------------------------------------------------------------------------------------------*
+    # there is reverse zone for every ip address
+    for ip in config.ips:
+        for reverse_zone in options.reverse_zones: 
<------------------------------------------------------- B
+            if bindinstance.verify_reverse_zone(reverse_zone, ip):
+                if reverse_zone not in reverse_zones:
+ reverse_zones.append(bindinstance.normalize_zone(reverse_zone))
+                break
+        else: 
<------------------------------------------------------------------------------------------------------------ 
C
+            reverse_zone = bindinstance.find_reverse_zone(ip)
+            if reverse_zone is None and not options.no_reverse:
+                reverse_zone = util.get_reverse_zone_default(ip) 
<----------------------------------------- D1
+                if not options.unattended and 
bindinstance.create_reverse(): <------------------------- D
+                    reverse_zone = 
bindinstance.read_reverse_zone(reverse_zone, ip)
+ reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) 
<------------- D2

You added all possible zones in step A,  B step is not needed.
If user doesn't specify reverse_zones you can just jump to C step
*add error message
C: elif not options.no_reverse
D: you asked user if wants to create zone, but you don't care about 
answer, and in step D2 append zone from D1
note: --no-reverse and --reverse-zones cant be used together, you can 
use it in new code, test it before cycle

7)
          # Always use force=True as named is not set up yet
          add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup,
-                ns_hostname=api.env.host, 
ns_ip_address=nameserver_ip_address,
-                force=True)
+                 ns_hostname=api.env.host, ns_ip_address=None, force=True)
+        #for ns_ip_address in nameserver_ip_address:
+        #    add_zone(self.domain, self.zonemgr, 
dns_backup=self.dns_backup,
+        #            ns_hostname=api.env.host, ns_ip_address=ns_ip_address,
+        #            force=True)

Please remove commented section

You can remove 'ns_ip_address=None' from function

8)
+            if set(options.ip_addresses) <= set(ips):
+                ips = options.ip_addresses
+            else:
+                print >>sys.stderr, "Error: the hostname resolves to an 
IP address that is different"
+                print >>sys.stderr, "from the one provided on the 
command line.  Please fix your DNS"
+                print >>sys.stderr, "or /etc/hosts file and restart the 
installation."
+                sys.exit(1)

Could you write those extra addresses to output? We need to improve 
usability of our error messages


-- 
Martin Basti

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140912/8d8887eb/attachment.htm>