On 4.2.2016 15:38, Oleg Fayans wrote: > Hi, > > On 02/04/2016 02:07 PM, Martin Basti wrote: >> >> >> On 04.02.2016 13:49, Oleg Fayans wrote: >>> Hi Petr, >>> >>> An updated patch is attached. Please see my comment inline. >>> >>> On 02/01/2016 12:47 PM, Petr Spacek wrote: >>>> On 1.2.2016 11:52, Oleg Fayans wrote: >>>>> Hi Petr, >>>>> >>>>> Please find the new version of the patch attached. Comments are inline >>>>> >>>>> On 01/29/2016 11:58 AM, Petr Spacek wrote: >>>>>> On 27.1.2016 11:16, Oleg Fayans wrote: >>>>>>> Sorry, trailing whitespace detected. This version passes lint >>>>>>> >>>>>>> On 01/27/2016 09:23 AM, Oleg Fayans wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 01/21/2016 04:41 PM, Petr Spacek wrote: >>>>>>>>>>> Hello, >>>>>>>>>>> >>>>>>>>>>> On 21.1.2016 13:42, Oleg Fayans wrote: >>>>>>>>>>>>> freeipa-ofayans-0021-Removed-ip-address-option-from-replica-installation.patch >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> From d7ab06a4dcddb919fda351b983d478f1b6968578 Mon Sep 17 00:00:00 >>>>>>>>>>>>> 2001 >>>>>>>>>>>>> From: Oleg Fayans <ofay...@redhat.com> >>>>>>>>>>>>> Date: Thu, 21 Jan 2016 13:30:02 +0100 >>>>>>>>>>>>> Subject: [PATCH] Removed --ip-address option from replica >>>>>>>>>>>>> installation >>>>>>>>>>>>> >>>>>>>>>>>>> Explicitly specifying ip-address of the replica messes up with >>>>>>>>>>>>> the current >>>>>>>>>>>>> bind-dyndb-ldap logic, causing reverse zone not to be created. >>>>>>>>>>>>> >>>>>>>>>>>>> Enabled reverse-zone creation for the clients residing in >>>>>>>>>>>>> different subnet from >>>>>>>>>>>>> master >>>>>>>>>>>>> --- >>>>>>>>>>>>> ipatests/test_integration/tasks.py | 19 ++++++++++++------- >>>>>>>>>>>>> 1 file changed, 12 insertions(+), 7 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/ipatests/test_integration/tasks.py >>>>>>>>>>>>> b/ipatests/test_integration/tasks.py >>>>>>>>>>>>> index >>>>>>>>>>>>> 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..43ef78b0c55deed24a0444f0ac6c38ddb2517481 >>>>>>>>>>>>> 100644 >>>>>>>>>>>>> --- a/ipatests/test_integration/tasks.py >>>>>>>>>>>>> +++ b/ipatests/test_integration/tasks.py >>>>>>>>>>>>> @@ -69,6 +69,8 @@ def prepare_reverse_zone(host, ip): >>>>>>>>>>>>> host.run_command(["ipa", >>>>>>>>>>>>> "dnszone-add", >>>>>>>>>>>>> zone], raiseonerr=False) >>>>>>>>>>>>> + return zone >>>>>>>>>>>>> + >>>>>>>>>>>>> >>>>>>>>>>>>> def prepare_host(host): >>>>>>>>>>>>> if isinstance(host, Host): >>>>>>>>>>>>> @@ -319,11 +321,8 @@ def domainlevel(host): >>>>>>>>>>>>> def replica_prepare(master, replica): >>>>>>>>>>>>> apply_common_fixes(replica) >>>>>>>>>>>>> fix_apache_semaphores(replica) >>>>>>>>>>>>> - prepare_reverse_zone(master, replica.ip) >>>>>>>>>>>>> - master.run_command(['ipa-replica-prepare', >>>>>>>>>>>>> - '-p', replica.config.dirman_password, >>>>>>>>>>>>> - '--ip-address', replica.ip, >>>>>>>>>>>>> - replica.hostname]) >>>>>>>>>>>>> + master.run_command(['ipa-replica-prepare', '-p', >>>>>>>>>>>>> replica.config.dirman_password, >>>>>>>>>>>>> + '--auto-reverse', replica.hostname]) >>>>>>>>>>> I guess that you will need --ip-address option in cases where >>>>>>>>>>> master's reverse >>>>>>>>>>> record does not exist (yet). >>>>>>>>> And yo were right. Fixed >>>>>>>>> >>>>>>>>>>> I would recommend you to test this in libvirt or somewhere without >>>>>>>>>>> revere >>>>>>>>>>> records, I suspect that it might blow up. >>>>>>>>>>> >>>>>>>>>>>>> replica_bundle = master.get_file_contents( >>>>>>>>>>>>> paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname) >>>>>>>>>>>>> replica_filename = get_replica_filename(replica) >>>>>>>>>>>>> @@ -339,8 +338,7 @@ def install_replica(master, replica, >>>>>>>>>>>>> setup_ca=True, setup_dns=False, >>>>>>>>>>>>> # and replica installation would fail >>>>>>>>>>>>> args = ['ipa-replica-install', '-U', >>>>>>>>>>>>> '-p', replica.config.dirman_password, >>>>>>>>>>>>> - '-w', replica.config.admin_password, >>>>>>>>>>>>> - '--ip-address', replica.ip] >>>>>>>>>>>>> + '-w', replica.config.admin_password] >>>>>>>>>>>>> if setup_ca: >>>>>>>>>>>>> args.append('--setup-ca') >>>>>>>>>>>>> if setup_dns: >>>>>>>>>>>>> @@ -380,6 +378,13 @@ def install_client(master, client, >>>>>>>>>>>>> extra_args=()): >>>>>>>>>>>>> client.collect_log(paths.IPACLIENT_INSTALL_LOG) >>>>>>>>>>>>> >>>>>>>>>>>>> apply_common_fixes(client) >>>>>>>>>>>>> + # Now, for the situations where a client resides in a >>>>>>>>>>>>> different subnet from >>>>>>>>>>>>> + # master, we need to explicitly tell master to create a >>>>>>>>>>>>> reverse zone for >>>>>>>>>>>>> + # the client and enable dynamic updates for this zone. >>>>>>>>>>>>> + allow_sync_ptr(master) >>>>>>>>>>>>> + zone = prepare_reverse_zone(master, client.ip) >>>>>>>>>>>>> + master.run_command(["ipa", "dnszone-mod", zone, >>>>>>>>>>>>> + "--dynamic-update=TRUE"], >>>>>>>>>>>>> raiseonerr=False) >>>>>>>>>>> I'm not a big fan of ignoring exceptions here, it might be better to >>>>>>>>>>> encapsulate the first command with try: except: and run the >>>>>>>>>>> zone-mod only if >>>>>>>>>>> the add worked as expected. >>>>>>>>>>> >>>>>>>>>>> Also, logging an message that reverse zone was not added might be a >>>>>>>>>>> good idea. >>>>>>>>> Agreed. Done. >>>>>>>>> >>>>>>>>>>> HTH >>>>>>>>>>> >>>>>>>>>>> Petr^2 Spacek >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> client.run_command(['ipa-client-install', '-U', >>>>>>>>>>>>> '--domain', client.domain.name, >>>>>>>>> >>>>>>>>> >>>>>>> -- Oleg Fayans Quality Engineer FreeIPA team RedHat. >>>>>>> >>>>>>> >>>>>>> freeipa-ofayans-0021.2-Removed-ip-address-option-from-replica-installation.patch >>>>>>> >>>>>>> >>>>>>> From e70daf4ed9dfbac7e8ea75cb8e9ab0f2af12ad48 Mon Sep 17 00:00:00 2001 >>>>>>> From: Oleg Fayans <ofay...@redhat.com> >>>>>>> Date: Wed, 27 Jan 2016 11:09:03 +0100 >>>>>>> Subject: [PATCH] Removed --ip-address option from replica installation >>>>>>> >>>>>>> Explicitly specifying ip-address of the replica messes up with the >>>>>>> current >>>>>>> bind-dyndb-ldap logic, causing reverse zone not to be created. >>>>>> NACK >>>>>> >>>>>> The commit message and logic is incorrect. In fact it has nothing to do >>>>>> with >>>>>> bind-dyndb-ldap. Either: >>>>>> a] The zone already exists somewhere so you should not create it in IPA, >>>>>> and >>>>>> this --ip-address option is invalid. >>>>>> b] The zone does not exist yet and then --ip-address option is required. >>>>>> >>>>>> I do not see any logic around >>>>>>> '--ip-address', replica.ip, >>>>> The explicit ip-address passing was removed (that's the main point of >>>>> this patch) >>>> Sure, but it cannot be done unconditionally (without additional logic). >>> Fair point. Implemented a check whether ipa master is authoritative for >>> the client's domain. >>> >>>>>> thus the NACK. >>>>>> >>>>>> >>>>>> In other words, network setup is job for a provisioning system. IPA >>>>>> needs to >>>>>> respect whatever the provisioning system did or did not do. >>>> Let me rephrase it: >>>> If IPA DNS manages the zone with replica FQDN or associated reverse zone, >>>> we >>>> need the --ip-address option. >>>> >>>> If IPA does not manage any of these, we do not need --ip-address option. >>>> >>>>>> Feel free to ask if something is unclear. >>>> Petr^2 Spacek >>>> >>>>>> (other comments inline) >>>>>> >>>>>> >>>>>>> Enabled reverse-zone creation for the clients residing in different >>>>>>> subnet from >>>>>>> master >>>>>>> --- >>>>>>> ipatests/test_integration/tasks.py | 20 +++++++++++++++----- >>>>>>> 1 file changed, 15 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/ipatests/test_integration/tasks.py >>>>>>> b/ipatests/test_integration/tasks.py >>>>>>> index >>>>>>> 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..b8b7fcfcbbb1dc672349119bbbb4cdbbd68c54ec >>>>>>> 100644 >>>>>>> --- a/ipatests/test_integration/tasks.py >>>>>>> +++ b/ipatests/test_integration/tasks.py >>>>>>> @@ -66,9 +66,13 @@ def check_arguments_are(slice, instanceof): >>>>>>> >>>>>>> def prepare_reverse_zone(host, ip): >>>>>>> zone = get_reverse_zone_default(ip) >>>>>>> - host.run_command(["ipa", >>>>>>> + result = host.run_command(["ipa", >>>>>>> "dnszone-add", >>>>>>> zone], raiseonerr=False) >>>>>>> + if result.returncode > 0: >>>>>>> + log.warn(result.stderr_text) >>>>>>> + return zone, result.returncode >>>>>>> + >>>>>>> >>>>>>> def prepare_host(host): >>>>>>> if isinstance(host, Host): >>>>>>> @@ -319,11 +323,10 @@ def domainlevel(host): >>>>>>> def replica_prepare(master, replica): >>>>>>> apply_common_fixes(replica) >>>>>>> fix_apache_semaphores(replica) >>>>>>> - prepare_reverse_zone(master, replica.ip) >>>>>>> master.run_command(['ipa-replica-prepare', >>>>>>> '-p', replica.config.dirman_password, >>>>>>> '--ip-address', replica.ip, >>>>>>> - replica.hostname]) >>>>>>> + '--auto-reverse', replica.hostname]) >>>>>>> replica_bundle = master.get_file_contents( >>>>>>> paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname) >>>>>>> replica_filename = get_replica_filename(replica) >>>>>>> @@ -339,8 +342,7 @@ def install_replica(master, replica, setup_ca=True, >>>>>>> setup_dns=False, >>>>>>> # and replica installation would fail >>>>>>> args = ['ipa-replica-install', '-U', >>>>>>> '-p', replica.config.dirman_password, >>>>>>> - '-w', replica.config.admin_password, >>>>>>> - '--ip-address', replica.ip] >>>>>>> + '-w', replica.config.admin_password] >>>>>>> if setup_ca: >>>>>>> args.append('--setup-ca') >>>>>>> if setup_dns: >>>>>>> @@ -380,6 +382,14 @@ def install_client(master, client, extra_args=()): >>>>>>> client.collect_log(paths.IPACLIENT_INSTALL_LOG) >>>>>>> >>>>>>> apply_common_fixes(client) >>>>>>> + # Now, for the situations where a client resides in a different >>>>>>> subnet from >>>>>>> + # master, we need to explicitly tell master to create a reverse >>>>>>> zone for >>>>>>> + # the client and enable dynamic updates for this zone. >>>>>>> + allow_sync_ptr(master) >>>>>>> + zone, error = prepare_reverse_zone(master, client.ip) >>>>>>> + if not error: >>>>>>> + master.run_command(["ipa", "dnszone-mod", zone, >>>>>>> + "--dynamic-update=TRUE"], raiseonerr=False) >>>>>> I believe that this call to dnszone-mod should use raiseonerr=True so >>>>>> you know >>>>>> that environment was not configured to accept dynamic updates. >>>>> Agreed. Fixed. >>>>> >>>>>>> >>>>>>> client.run_command(['ipa-client-install', '-U', >>>>>>> '--domain', client.domain.name, >>>>>>> -- 2.4.3 >>>>>> >>>> >>> >>> >> >>> + log.warn(result.stderr_text) >> >> warn is deprecated, please use log.warning instead > > Fixed
I'm still not big fan of this, but never mind. ACK -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code