[Freeipa-devel] [TEST][Patch 0021] Fixed recent replica installation issues in the lab
Petr Spacek
pspacek at redhat.com
Thu Feb 4 14:43:51 UTC 2016
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 <ofayans at 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 <ofayans at 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
More information about the Freeipa-devel
mailing list