On 07/01/2016 06:36 AM, Lenka Doudova wrote:


On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,

attaching patch with some basic coverage for external trust feature. Bit
more detailed info in commit message.

Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable
change.


Lenka




Hi Lenka,

I have a few comments:

1.)

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False):

This modification seems extremely kludgy to me.

I would rather change the function signature to:

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):

and pass the domain name itself to the function instead of doing magic
inside. The same goes with `remove trust_with_ad`. You may want to fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.

2.)

+def sync_time_hostname(host, srv_hostname):
+    """
+    Syncs time with the remote server given by its hostname.
+    Please note that this function leaves ntpd stopped.
+    """
+    host.run_command(['systemctl', 'stop', 'ntpd'])
+    host.run_command(['ntpdate', srv_hostname])
+
+

this looks like a duplicate of the function above it, why is it even
needed?

3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+    """
+    Test establishing external trust with subdomain
+    """
+
+    @classmethod
+    def install(cls, mh):
+        super(ADTrustBase, cls).install(mh)
+        cls.ad = cls.ad_domains[0].ads[0]
+        cls.install_adtrust()
+        cls.check_sid_generation()
+
+        # Determine whether the subdomain AD is available
+        try:
+            child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+            cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+            cls.ad_subdomain_hostname = child_ad.hostname
+        except LookupError:
+            cls.ad_subdomain = None
+
+        cls.configure_dns_and_time()

Please use proper OOP practices when overriding the behavior of the
base test class.

For starters you could rewrite the install method like this:

+    @classmethod
+    def install(cls, mh):
+        # Determine whether the subdomain AD is available
+        try:
+            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+            cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+            cls.ad_subdomain_hostname = child_ad.hostname
+        except LookupError:
+            raise nose.SkipTest("AFAIK this will skip the whole test
class")
+        super(ADTrustBase, cls).install(mh)

with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+    def configure_dns_and_time(cls):
+        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
     # no need to re-implement the function just to get to the child
AD DC hostname now
+        tasks.sync_time(cls.master, cls.child_ad.hostname)

You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.

Hi,
thanks for review, fixed patch attached.

Lenka

Hi Lenka,

I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation.

I am attaching a quick untested patch illustrating how the implementation should look like.

Also you have some leftovers from previous revision, please fix them:

Pylint is running, please wait ...
************* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
sending incremental file list

(this is before I started meddling with it)


--
Martin^3 Babinsky
From f9849e9667e595d67ffd631f312106ae4278c135 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 1 Jul 2016 14:22:55 +0200
Subject: [PATCH] desired test class hierarchy

---
 ipatests/test_integration/test_trust.py | 104 +++++++++-----------------------
 1 file changed, 30 insertions(+), 74 deletions(-)

diff --git a/ipatests/test_integration/test_trust.py b/ipatests/test_integration/test_trust.py
index d662e80727b6eab3df93166d35ddbaea6a0f6f7a..337c39e3fd3458f6f535e7af500fa85b6d0ff3d4 100644
--- a/ipatests/test_integration/test_trust.py
+++ b/ipatests/test_integration/test_trust.py
@@ -225,21 +225,33 @@ class TestInvalidRangeTypes(ADTrustBase):
             assert result.returncode == 1
 
 
-class TestExternalTrustWithSubdomain(ADTrustBase):
+class ADTrustSubdomainBase(ADTrustBase):
+    """
+    Base class for tests involving subdomains of trusted forests
+    """
+    @classmethod
+    def configure_dns_and_time(cls):
+        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
+        tasks.sync_time_hostname(cls.master, cls.child_ad)
+
+    @classmethod
+    def install(cls, mh):
+        # Determine whether the subdomain AD is available
+        # if not, skip the whole suite
+        try:
+            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+            cls.ad_subdomain = '.'.join(cls.child_ad.hostname.split('.')[1:])
+        except LookupError:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+
+class TestExternalTrustWithSubdomain(ADTrustSubdomainBase):
     """
     Test establishing external trust with subdomain
     """
 
-    @classmethod
-    def configure_dns_and_time(cls):
-        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
-        tasks.sync_time_hostname(cls.master, cls.child_ad)
-
     def test_establish_trust(self):
         """ Tests establishing external trust with Active Directory """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
-
         tasks.establish_trust_with_ad(
             self.master, self.ad_subdomain,
             extra_args=['--range-type', 'ipa-ad-trust', '--external'],
@@ -247,9 +259,6 @@ class TestExternalTrustWithSubdomain(ADTrustBase):
 
     def test_all_trustdomains_found(self):
         """ Test that only one trustdomain is found """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
-
         result = self.master.run_command(['ipa', 'trustdomain-find',
                                           self.ad_subdomain])
 
@@ -258,17 +267,14 @@ class TestExternalTrustWithSubdomain(ADTrustBase):
 
     def test_user_gid_uid_resolution_in_nonposix_trust(self):
         """ Check that user has SID-generated UID """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
-
         testuser = 'subdomaintestuser@{0}'.format(self.ad_subdomain)
         result = self.master.run_command(['getent', 'passwd', testuser])
 
-        testuser_regex = "^subdomaintestuser@{0}:\*:(?!10042)(\d+):"\
-                         "(?!)10047(\d+):Subdomain TestUser:"\
-                         "/home/{1}/subdomaintestuser:/bin/sh$".format(
-                             re.escape(self.ad_subdomain),
-                             re.escape(self.ad_subdomain))
+        testuser_regex = ("^subdomaintestuser@{0}:\*:(?!10042)(\d+):"
+                          "(?!)10047(\d+):Subdomain TestUser:"
+                          "/home/{1}/subdomaintestuser:/bin/sh$".format(
+                              re.escape(self.ad_subdomain),
+                              re.escape(self.ad_subdomain)))
 
         assert re.search(testuser_regex, result.stdout_text)
 
@@ -281,7 +287,7 @@ class TestExternalTrustWithSubdomain(ADTrustBase):
         tasks.clear_sssd_cache(self.master)
 
 
-class TestNonexternalTrustWithSubdomain(IntegrationTest):
+class TestNonexternalTrustWithSubdomain(ADTrustSubdomainBase):
     """
     Tests that a non-external trust to a subdomain cannot be established
     """
@@ -289,50 +295,8 @@ class TestNonexternalTrustWithSubdomain(IntegrationTest):
     num_ad_domains = 1
     optional_extra_roles = ['ad_subdomain']
 
-    @classmethod
-    def install(cls, mh):
-        super(TestNonexternalTrustWithSubdomain, cls).install(mh)
-        cls.ad = cls.ad_domains[0].ads[0]
-        cls.ad_domain = cls.ad.domain.name
-        cls.install_adtrust()
-        cls.check_sid_generation()
-
-        # Determine whether the subdomain AD is available
-        try:
-            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
-            cls.ad_subdomain = '.'.join(cls.child_ad.hostname.split('.')[1:])
-        except LookupError:
-            cls.ad_subdomain = None
-
-        cls.configure_dns_and_time()
-
-    @classmethod
-    def install_adtrust(cls):
-        tasks.install_adtrust(cls.master)
-
-    @classmethod
-    def check_sid_generation(cls):
-        """Test SID generation"""
-
-        command = ['ipa', 'user-show', 'admin', '--all', '--raw']
-
-        _sid_identifier_authority = '(0x[0-9a-f]{1,12}|[0-9]{1,10})'
-        sid_regex = 'S-1-5-21-%(idauth)s-%(idauth)s-%(idauth)s'\
-                    % dict(idauth=_sid_identifier_authority)
-        stdout_re = re.escape('  ipaNTSecurityIdentifier: ') + sid_regex
-
-        util.run_repeatedly(cls.master, command,
-                            test=lambda x: re.search(stdout_re, x))
-
-    @classmethod
-    def configure_dns_and_time(cls):
-        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
-        tasks.sync_time_hostname(cls.master, cls.child_ad)
-
     def test_establish_trust(self):
         """ Tests establishing non-external trust with Active Directory """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
 
         self.master.run_command(['kinit', '-kt', paths.IPA_KEYTAB,
                                  'HTTP/%s' % self.master.hostname])
@@ -350,11 +314,11 @@ class TestNonexternalTrustWithSubdomain(IntegrationTest):
             raiseonerr=False)
 
         assert result != 0
-        assert "Domain '{0}' is not a root domain".format(self.ad_subdomain) \
-            in result.stderr_text
+        assert ("Domain '{0}' is not a root domain".format(self.ad_subdomain)
+                in result.stderr_text)
 
 
-class TestExternalTrustWithRootDomain(ADTrustBase):
+class TestExternalTrustWithRootDomain(ADTrustSubdomainBase):
     """
     Test establishing external trust with root domain
     Main purpose of this test is to verify that subdomains are not
@@ -364,17 +328,12 @@ class TestExternalTrustWithRootDomain(ADTrustBase):
 
     def test_establish_trust(self):
         """ Tests establishing external trust with Active Directory """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
-
         tasks.establish_trust_with_ad(
             self.master, self.ad_domain,
             extra_args=['--range-type', 'ipa-ad-trust', '--external'])
 
     def test_all_trustdomains_found(self):
         """ Test that only one trustdomain is found """
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
 
         result = self.master.run_command(['ipa', 'trustdomain-find',
                                           self.ad_domain])
@@ -383,8 +342,5 @@ class TestExternalTrustWithRootDomain(ADTrustBase):
         assert "Number of entries returned 1" in result.stdout_text
 
     def test_remove_nonposix_trust(self):
-        if self.ad_subdomain is None:
-            raise nose.SkipTest('AD subdomain is not available.')
-
         tasks.remove_trust_with_ad(self.master, self.ad_domain)
         tasks.clear_sssd_cache(self.master)
-- 
2.5.5

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