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)

> 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.
> 
> Feel free to ask if something is unclear.
> 
> (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
> 
> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 6332e338ca59b9ff6342eda97a5e25ab68b69483 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Mon, 1 Feb 2016 10:49:33 +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 | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index dc2e5a12ea8b1acf28eb9f6bad6820f21536d9a5..02c84000eb3e7719811318ac44093b705ef85356 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -66,9 +66,12 @@ 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):
@@ -334,8 +337,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:
@@ -375,6 +377,14 @@ def install_client(master, client, extra_args=()):
     client.collect_log(paths.IPACLIENT_INSTALL_LOG)
 
     apply_common_fixes(client)
+    allow_sync_ptr(master)
+    # 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.
+    zone, error = prepare_reverse_zone(master, client.ip)
+    if not error:
+        master.run_command(["ipa", "dnszone-mod", zone,
+                            "--dynamic-update=TRUE"])
 
     client.run_command(['ipa-client-install', '-U',
                         '--domain', client.domain.name,
-- 
2.4.3

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