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).

>> 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
>>
>>
> 


-- 
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

Reply via email to