On 09/16/2014 06:09 PM, Martin Basti wrote:
On 16/09/14 15:59, David Kupka wrote:
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
Thank you for patch.
1)
+ if not options.no_reverse:
+ # check that there is reverse zone for every IP
+ if options.unattended:
+ for ip in config.ip_addresses:
+ if bindinstance.find_reverse_zone(str(ip)):
+ # reverse zone is already in LDAP
+ continue
+ for rz in reverse_zones:
+ if bindinstance.verify_reverse_zone(rz, ip):
+ # reverse zone is entered by user
+ break
+ else:
+ rz = util.get_reverse_zone_default(str(ip))
+ reverse_zones.append(rz)
+ elif options.reverse_zones or (not(options.no_reverse) and
bindinstance.create_reverse()):
-----------------------------------------------------^^^^^
There is double check of "options.no_reverse", you are in block inside
if not options.no_reverse, so not(options.no_reverse) is always True
Good point, will remove the redundancy.
2)
+ # check that there is IP address in every reverse zone
+ if options.reverse_zones:
+ for rz in options.reverse_zones:
+ for ip in config.ip_addresses:
+ if bindinstance.verify_reverse_zone(rz, ip):
+ reverse_zones.append(bindinstance.normalize_zone(rz))
+ break
+ else:
+ sys.exit("There is no IP address matching reverze_zone
%s." % rz)
+ if not options.no_reverse:
+ # check that there is reverse zone for every IP
+ if options.unattended:
+ for ip in config.ip_addresses:
+ if bindinstance.find_reverse_zone(str(ip)):
+ # reverse zone is already in LDAP
+ continue
+ for rz in reverse_zones:
+ if bindinstance.verify_reverse_zone(rz, ip):
+ # reverse zone is entered by user
+ break
+ else:
+ rz = util.get_reverse_zone_default(str(ip))
+ reverse_zones.append(rz)
+ elif options.reverse_zones or (not(options.no_reverse) and
bindinstance.create_reverse()):
+ for ip in config.ip_addresses:
+ if bindinstance.find_reverse_zone(str(ip)):
+ # reverse zone is already in LDAP
+ continue
+ for rz in reverse_zones:
+ if bindinstance.verify_reverse_zone(rz, ip):
+ # reverse zone is entered by user
+ break
+ else:
+ rz = util.get_reverse_zone_default(str(ip))
+ rz = bindinstance.read_reverse_zone(rz, str(ip))
+ reverse_zones.append(rz)
+ else:
+ options.no_reverse = True
+ reverse_zones = []
Code above is duplicated in replica-install and server-install, with
small difference, could you put it inside function, for example into
bindinstance module? Also there are duplicated parts inside in if and
elif code block, could you add it to one function as well?
Example:
def check_ip_reversezones(...., unattended=False, try_to_find_zone =
False):
reverse_zones = []
if options['reverse_zones']....#check reverse zones
if not unattended and not bindinstance.create_reverse():
# user doesn't want to create additional reverse zones
return reverse_zones
for ip in config.ip_addresses:
if try_to_find_zone and bindinstance.find_reverse_zone(str(ip)):
# reverse zone is already in LDAP
continue
for rz in reverse_zones:
if bindinstance.verify_reverse_zone(rz, ip):
# reverse zone is entered by user
break
else:
rz = util.get_reverse_zone_default(str(ip))
if not unattended:
rz = bindinstance.read_reverse_zone(rz, str(ip))
reverse_zones.append(rz)
This makes sense. There is one more difference in the code but it is
reasonably solvable.
3)
+ elif options.reverse_zones or (not(options.no_reverse) and
bindinstance.create_reverse()):
OR operator, this will create additional zones (non-specified by user)
even if user write NO
When user specifies some reverse zone (using --reverse-zone) we can
assume that he wants to configure reverse zones (otherwise his is kind
of indecisive, at least). So there is no reason to ask him again.
The question is asked only when the user didn't provided any reverse
zone nor specified --no-reverse.
4)
IF user specify zone 10.in-addr.arpa, and ip addresses 10.0.0.1,
192.168.1.1, and answer to not create additional zones, how is this case
handled?
The question is not asked. See the answer to 3).
--
David Kupka
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
