[Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
David Kupka
dkupka at redhat.com
Tue Sep 16 13:59:54 UTC 2014
On 09/12/2014 07:24 PM, Martin Basti wrote:
> <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 '# '
>
Stupid mistake, sorry.
>
> 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
You are right, it's better to say what's wrong.
>
> 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
I indented the else branch more that I wanted.
>
> 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.
The behavior now basically is:
Check if IP address matches any specified zone
a. if yes we are good
b. else search if there is existing zone that can be used
ba. if yes we are good
bb. else ask user for zone or create default if --unattended
>
> 5)
> Code for ipa-replica-install, and ipa-server-install, differs in parts
> where we expecting same behavior
> - ip addresses and reverse zones
The behavoir now should be almost the same. The only difference is that
when installing replica we need to search for existing reverse zones as
they could be added during on another server.
>
> 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
Rewritten.
>
> 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
I forget to clean the trash, thanks.
>
> 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
>
>
UX is the king :)
> --
> Martin Basti
>
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0009-6-Detect-and-configure-all-usable-IP-addresses.patch
Type: text/x-patch
Size: 37732 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/34ac5bfb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0009-6-ipa41-Detect-and-configure-all-usable-IP-addresses.patch
Type: text/x-patch
Size: 37869 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/34ac5bfb/attachment-0001.bin>
More information about the Freeipa-devel
mailing list