On 06/30/2016 12:32 PM, Lenka Doudova wrote:


On 06/29/2016 07:49 PM, Petr Spacek wrote:
On 29.6.2016 18:52, Lenka Doudova wrote:

On 06/29/2016 06:51 PM, Petr Spacek wrote:
On 29.6.2016 18:48, Lenka Doudova wrote:
On 06/27/2016 11:05 AM, Lenka Doudova wrote:
On 06/27/2016 10:33 AM, Martin Babinsky wrote:
On 06/27/2016 10:28 AM, Petr Spacek wrote:
On 27.6.2016 10:26, Petr Spacek wrote:
On 27.6.2016 10:18, Martin Babinsky wrote:
On 06/27/2016 10:04 AM, Petr Vobornik wrote:
On 06/27/2016 09:42 AM, Lenka Doudova wrote:
Hi!

With newly created AD machines in Brno lab, existing trust tests
fail on
'ipa dnsforwardzone-add' command claiming the zone is already present,
as new AD domain is dom-221.idm.lab.eng.brq.redhat.com.

To prevent these failures I prepared attached patch, that will still attempt to add the forward zone, but in case of non-zero return code will check the message if it says that the forward zone is already
configured, and lets the tests continue, if it is so.


Lenka

Current approach expects that every error of ipa dnsforward-add here will mean that the zone exists. So it might hide other issues - not very
good.

On the other hand it is not very robust to parse error message.

Question for general audience: What do you think if IPA client's exit status would be the IPA error code instead of "1" for every error. E.g.
in DuplicateEntry case it's 4002.

Btw, this is not a NACK.

Well AFAIK the exit status on POSIX systems is encoded into a single
byte so
you cannot have the return value greater that 255. We would have to
devise
some mapping between our XMLRPC status codes and subprocess return codes.

Some of our exceptions have defined return values outside plain '1', e.g. NotFound has return value of 2. It would be possible to extend this
concept on
other common errors.
Even more importantly, the forward zone is completely unnecessary because
DNS
when DNS is set up properly. I would simply remove the dnsforwardzone-add.

Grr, I meant this:
Even more importantly, the forward zone is completely unnecessary when
DNS is
set up properly. I would simply remove the dnsforwardzone-add.

+1, our tests should not fiddle with the provisioned environment as much as
they sometimes do.

Well, I have nothing against removing it completely, but left it there just because with previous AD machines with "wild" domains it was necessary. Looking at the code, your suggestion would probably mean to entirely remove method configure_dns_for_trust from ipatests/test_integration/tasks.py,
right? I'll have to verify this won't break anything else.

Lenka

Hi,

to get back to this issue: do we really want to have the DNS configuration method removed? I mean, we no longer need it for our CI tests, with new AD VMs it works without it, but should somebody else with different setup run these tests, they could experience failures because their AD domain would not be configured in DNS by default and the test would no longer provide that configuration. It doesn't feel right to delete something we needed before but
don't need anymore, in case somebody else is depending on the same
configuration. But of course, I'll abide by your counsel.
In case the call on DNS configuration method really is removed, should I remove even it's definition? It's not used anywhere else, so it would be quite
logical.
Feel free to keep empty method around as a "hook" for other people. The
important thing is that it should do nothing by default.

So leave the method call, but erase method contents and let it just pass?
Fine with me. (List re-added.)

Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the original DNS configuring function is now empty, I modified the comment to explain significance of this strange thing. I also changed patch title to better reflect proposed changes.

Lenka


NACK the previous one, forgot PEP8. New patch attached.

Lenka
From a4772a1b8cc31f0020c86acabbeb944c6d5269b7 Mon Sep 17 00:00:00 2001
From: Lenka Doudova <ldoud...@redhat.com>
Date: Thu, 30 Jun 2016 12:26:28 +0200
Subject: [PATCH] Tests: Remove DNS configuration from trust tests

Since DNS configuration is no longer needed for running trust tests, this method's contents are removed. Method is left empty as reference for others, should they have issues with DNS configuration.
---
 ipatests/test_integration/tasks.py | 44 ++++----------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 38218fa709c2c220d5fea98a092b55e995d48d77..5be7cdae3ac777bbf0fc52e6c511969e9fabcd72 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -436,47 +436,11 @@ def install_adtrust(host):
 
 def configure_dns_for_trust(master, ad):
     """
-    This configures DNS on IPA master according to the relationship of the
-    IPA's and AD's domains.
+    This method is intentionally left empty. Originally it served for DNS
+    configuration on IPA master according to the relationship of the IPA's
+    and AD's domains.
     """
-
-    def is_subdomain(subdomain, domain):
-        subdomain_unpacked = subdomain.split('.')
-        domain_unpacked = domain.split('.')
-
-        subdomain_unpacked.reverse()
-        domain_unpacked.reverse()
-
-        subdomain = False
-
-        if len(subdomain_unpacked) > len(domain_unpacked):
-            subdomain = True
-
-            for subdomain_segment, domain_segment in zip(subdomain_unpacked,
-                                                         domain_unpacked):
-                subdomain = subdomain and subdomain_segment == domain_segment
-
-        return subdomain
-
-    kinit_admin(master)
-
-    if is_subdomain(ad.domain.name, master.domain.name):
-        master.run_command(['ipa', 'dnsrecord-add', master.domain.name,
-                            '%s.%s' % (ad.shortname, ad.netbios),
-                            '--a-ip-address', ad.ip])
-
-        master.run_command(['ipa', 'dnsrecord-add', master.domain.name,
-                            ad.netbios,
-                            '--ns-hostname',
-                            '%s.%s' % (ad.shortname, ad.netbios)])
-
-        master.run_command(['ipa', 'dnszone-mod', master.domain.name,
-                            '--allow-transfer', ad.ip])
-    else:
-        master.run_command(['ipa', 'dnsforwardzone-add', ad.domain.name,
-                            '--forwarder', ad.ip,
-                            '--forward-policy', 'only',
-                            ])
+    pass
 
 
 def establish_trust_with_ad(master, ad, extra_args=()):
-- 
2.7.4

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