<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 04.02.2016 13:49, Oleg Fayans wrote:<br>
</div>
<blockquote cite="mid:56B348E0.70301@redhat.com" type="cite">
<pre wrap="">Hi Petr,
An updated patch is attached. Please see my comment inline.
On 02/01/2016 12:47 PM, Petr Spacek wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 1.2.2016 11:52, Oleg Fayans wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Petr,
Please find the new version of the patch attached. Comments are inline
On 01/29/2016 11:58 AM, Petr Spacek wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 27.1.2016 11:16, Oleg Fayans wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Sorry, trailing whitespace detected. This version passes lint
On 01/27/2016 09:23 AM, Oleg Fayans wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hi,
On 01/21/2016 04:41 PM, Petr Spacek wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hello,
On 21.1.2016 13:42, Oleg Fayans wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">freeipa-ofayans-0021-Removed-ip-address-option-from-replica-installation.patch
>From d7ab06a4dcddb919fda351b983d478f1b6968578 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <a class="moz-txt-link-rfc2396E" href="mailto:ofayans@redhat.com"><ofayans@redhat.com></a>
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])
</pre>
</blockquote>
</blockquote>
<pre wrap="">
I guess that you will need --ip-address option in cases where master's reverse
record does not exist (yet).
</pre>
</blockquote>
</blockquote>
<pre wrap="">And yo were right. Fixed
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">
I would recommend you to test this in libvirt or somewhere without revere
records, I suspect that it might blow up.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> 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)
</pre>
</blockquote>
</blockquote>
<pre wrap="">
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.
</pre>
</blockquote>
</blockquote>
<pre wrap="">Agreed. Done.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">
HTH
Petr^2 Spacek
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">
client.run_command(['ipa-client-install', '-U',
'--domain', client.domain.name,
</pre>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
</blockquote>
<pre wrap="">-- 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 <a class="moz-txt-link-rfc2396E" href="mailto:ofayans@redhat.com"><ofayans@redhat.com></a>
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.
</pre>
</blockquote>
<pre wrap="">
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
</pre>
<blockquote type="cite">
<pre wrap=""> '--ip-address', replica.ip,
</pre>
</blockquote>
</blockquote>
<pre wrap="">
The explicit ip-address passing was removed (that's the main point of
this patch)
</pre>
</blockquote>
<pre wrap="">
Sure, but it cannot be done unconditionally (without additional logic).
</pre>
</blockquote>
<pre wrap="">
Fair point. Implemented a check whether ipa master is authoritative for
the client's domain.
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Feel free to ask if something is unclear.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
Petr^2 Spacek
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">(other comments inline)
</pre>
<blockquote type="cite">
<pre wrap="">
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)
</pre>
</blockquote>
<pre wrap="">
I believe that this call to dnszone-mod should use raiseonerr=True so you know
that environment was not configured to accept dynamic updates.
</pre>
</blockquote>
<pre wrap="">
Agreed. Fixed.
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">
client.run_command(['ipa-client-install', '-U',
'--domain', client.domain.name,
-- 2.4.3
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
<blockquote cite="mid:56B348E0.70301@redhat.com" type="cite">
<pre wrap="">+ log.warn(result.stderr_text)</pre>
</blockquote>
<br>
warn is deprecated, please use log.warning instead<br>
<br>
Martin^2<br>
</body>
</html>