[Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
David Kupka
dkupka at redhat.com
Thu Sep 18 13:07:18 UTC 2014
On 09/17/2014 07:25 AM, David Kupka wrote:
> 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.
Removed.
>
>> 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.
Done.
>
>>
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0009-7-Detect-and-configure-all-usable-IP-addresses.patch
Type: text/x-patch
Size: 36481 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140918/a7c3736b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0009-7-ipa41-Detect-and-configure-all-usable-IP-addresses.patch
Type: text/x-patch
Size: 36465 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140918/a7c3736b/attachment-0001.bin>
More information about the Freeipa-devel
mailing list