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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From f60d191fb2ce3d77b2859842c0b230df09a2508c 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 | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 42d2e10..ac7f463 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):
@@ -310,15 +313,26 @@ def domainlevel(host):
         level = int(domlevel_re.findall(result.stdout_text)[0])
     return level
 
+def master_authoritative_for_client_domain(master, client):
+    zone = ".".join(client.hostname.split('.')[1:])
+    result = master.run_command(["ipa", "dnszone-show", zone],
+                                raiseonerr=False)
+    if result.returncode == 0:
+        return True
+    else:
+        return False
+
 
 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])
+    args = ['ipa-replica-prepare',
+            '-p', replica.config.dirman_password,
+            replica.hostname]
+    if master_authoritative_for_client_domain(master, replica):
+        args.extend(['--ip-address', replica.ip])
+    master.run_command(args)
     replica_bundle = master.get_file_contents(
         paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname)
     replica_filename = get_replica_filename(replica)
@@ -334,8 +348,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:
@@ -343,6 +356,8 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
             '--setup-dns',
             '--forwarder', replica.config.dns_forwarder
         ])
+    if master_authoritative_for_client_domain(master, replica):
+        args.extend(['--ip-address', replica.ip])
     if domainlevel(master) == DOMAIN_LEVEL_0:
         # prepare the replica file on master and put it to replica, AKA "old way"
         replica_prepare(master, replica)
@@ -375,6 +390,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