Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/24/2013 04:38 PM, Tomas Babej wrote: On 10/24/2013 01:29 PM, Petr Viktorin wrote: [...] Patch 106: In ADTrustBase, it looks like if test_install_adtrust or test_configure_dns_and_time fail, it doesn't make much sense to run the other tests. If that's the case they can go in an install() classmethod. Same with test_establish_trust* in the subclasses. I made them part of the install() classmethod. As for the test_estabilish_trust, I would have to still override that in each class that uses it, since all of them use it in a slightly different way. I'd prefer an establish_trust classmethod called from install(), but it's not really important in this case. Also, there's a typo in test_estabilish_trusts several times. -^ Typo fixed. Also attaching a patch that fixes the same type in the other parts of the codebase. ACK, ACK, pushed to master: df5f5c9fab5dd66c50bf202e1ebd19f558e3e0c6 ipa-3-3: 5dbd11722e365f50b7496b4ab2559122cd927d53 Thanks for your patience! -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/22/2013 02:24 PM, Tomas Babej wrote: On 10/22/2013 02:15 PM, Tomas Babej wrote: On 10/22/2013 12:27 PM, Tomas Babej wrote: On 10/22/2013 10:37 AM, Petr Viktorin wrote: Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? You could still set AD_env1 and MASTER_env1 to have the same domain. Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. This will, however, require filtering the domains depending on the fact whether they contain AD or not. If a testcase wants to access an AD domain, it will still need to loop over the list of domains to see which ones are of AD type. Any code that does not care what domain type it's dealing with, can easily access all the domains by chaining the respective iterables. We could have a wrapper in the Config class for that, along the lines of get_all_domains(). So what I see here is that we're trading one complexity over another. I think we can agree on your approach since it hides the complexity from the user, especially in the ipa-test-config, which I admit is getting rather ugly, as we need to introduce new option there and that causes splitting. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. With this check we should be fine. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. I'll send revised patchset soon. Updated patchset attached. Patch 105: Typo: 'Sets DNS ib given host for trust with the given AD ^^ Should be in, right? I'll fix it before pushing. Otherwise, these are good to go! Patch 106: In ADTrustBase, it looks like if test_install_adtrust or test_configure_dns_and_time fail, it doesn't make much sense to run the other tests. If that's the case they can go in an install() classmethod. Same with test_establish_trust* in the subclasses. Also, there's a typo in test_estabilish_trusts several times. -^ -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/24/2013 01:29 PM, Petr Viktorin wrote: On 10/22/2013 02:24 PM, Tomas Babej wrote: On 10/22/2013 02:15 PM, Tomas Babej wrote: On 10/22/2013 12:27 PM, Tomas Babej wrote: On 10/22/2013 10:37 AM, Petr Viktorin wrote: Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? You could still set AD_env1 and MASTER_env1 to have the same domain. Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. This will, however, require filtering the domains depending on the fact whether they contain AD or not. If a testcase wants to access an AD domain, it will still need to loop over the list of domains to see which ones are of AD type. Any code that does not care what domain type it's dealing with, can easily access all the domains by chaining the respective iterables. We could have a wrapper in the Config class for that, along the lines of get_all_domains(). So what I see here is that we're trading one complexity over another. I think we can agree on your approach since it hides the complexity from the user, especially in the ipa-test-config, which I admit is getting rather ugly, as we need to introduce new option there and that causes splitting. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. With this check we should be fine. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. I'll send revised patchset soon. Updated patchset attached. Patch 105: Typo: 'Sets DNS ib given host for trust with the given AD ^^ Should be in, right? I'll fix it before pushing. c Otherwise, these are good to go! Patch 106: In ADTrustBase, it looks like if test_install_adtrust or test_configure_dns_and_time fail, it doesn't make much sense to run the other tests. If that's the case they can go in an install() classmethod. Same with test_establish_trust* in the subclasses. I made them part of the install() classmethod. As for the test_estabilish_trust, I would have to still override that in each class that uses it, since all of them use it in a slightly different way. Also, there's a typo in test_estabilish_trusts several times. -^ Typo fixed. Also attaching a patch that fixes the same type in the other parts of the codebase. -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From e7f777b229534a65eb7f6bc39e77c2a5b5c51983 Mon Sep 17 00:00:00 2001 From: Tomas Babej tomasba...@gmail.com Date: Thu, 24 Oct 2013 14:24:33 +0200 Subject: [PATCH] trusts: Fix typo in error message for realm-domain mismatch --- ipalib/plugins/trust.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py index 0d651f8861446cf8d31eb1ea303237bcd2b73201..32a93834394273c9f896ff5fd17bfcc753fe7b8e 100644 --- a/ipalib/plugins/trust.py +++ b/ipalib/plugins/trust.py @@ -424,14 +424,14 @@ sides. ) # If domain name and realm does not match, IPA server is not be able -# to estabilish trust with Active Directory. +# to establish trust with Active Directory. realm_not_matching_domain = (api.env.domain.upper() !=
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/22/2013 10:37 AM, Petr Viktorin wrote: Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? You could still set AD_env1 and MASTER_env1 to have the same domain. Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. This will, however, require filtering the domains depending on the fact whether they contain AD or not. If a testcase wants to access an AD domain, it will still need to loop over the list of domains to see which ones are of AD type. Any code that does not care what domain type it's dealing with, can easily access all the domains by chaining the respective iterables. We could have a wrapper in the Config class for that, along the lines of get_all_domains(). So what I see here is that we're trading one complexity over another. I think we can agree on your approach since it hides the complexity from the user, especially in the ipa-test-config, which I admit is getting rather ugly, as we need to introduce new option there and that causes splitting. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. With this check we should be fine. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. I'll send revised patchset soon. -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/22/2013 12:27 PM, Tomas Babej wrote: On 10/22/2013 10:37 AM, Petr Viktorin wrote: Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? You could still set AD_env1 and MASTER_env1 to have the same domain. Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. This will, however, require filtering the domains depending on the fact whether they contain AD or not. If a testcase wants to access an AD domain, it will still need to loop over the list of domains to see which ones are of AD type. Any code that does not care what domain type it's dealing with, can easily access all the domains by chaining the respective iterables. We could have a wrapper in the Config class for that, along the lines of get_all_domains(). So what I see here is that we're trading one complexity over another. I think we can agree on your approach since it hides the complexity from the user, especially in the ipa-test-config, which I admit is getting rather ugly, as we need to introduce new option there and that causes splitting. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. With this check we should be fine. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. I'll send revised patchset soon. Updated patchset attached. -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From a23c379a865d5005539b7eb0ada76e36768b0f53 Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Mon, 14 Oct 2013 16:37:55 +0200 Subject: [PATCH 106/107] ipatests: Add AD integration test case Part of: https://fedorahosted.org/freeipa/ticket/3834 --- ipatests/test_integration/test_trust.py | 182 1 file changed, 182 insertions(+) create mode 100644 ipatests/test_integration/test_trust.py diff --git a/ipatests/test_integration/test_trust.py b/ipatests/test_integration/test_trust.py new file mode 100644 index ..343c9785b0bd74b891638443e9699268e1366254 --- /dev/null +++ b/ipatests/test_integration/test_trust.py @@ -0,0 +1,182 @@ +# Authors: +# Tomas Babej tba...@redhat.com +# +# Copyright (C) 2013 Red Hat +# see file 'COPYING' for use and warranty information +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. + +import re + +from ipatests.test_integration.base import IntegrationTest +from ipatests.test_integration import tasks +from ipatests.test_integration import util + + +class ADTrustBase(IntegrationTest): +Provides common checks for the AD trust integration testing. + +topology = 'line' +
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/22/2013 02:15 PM, Tomas Babej wrote: On 10/22/2013 12:27 PM, Tomas Babej wrote: On 10/22/2013 10:37 AM, Petr Viktorin wrote: Replying to one part only: On 10/21/2013 04:50 PM, Tomas Babej wrote: On 10/16/2013 03:44 PM, Petr Viktorin wrote: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. I would rather not join IPA and AD domains as they even cannot be in the same domain, as the service records would clash. So these will always be separate / sub / super domain relationship. You're right that they should never share the same domain. But you should never say never, especially in testing -- what if we'll want to, in the future, test that the records *do* clash, or that IPA refuses to install in an AD domain? You could still set AD_env1 and MASTER_env1 to have the same domain. Another problem is that they are now separate namespaces. In all code that deals with domains you have to deal separately with the list of AD domains and separately with IPA domains. This makes every piece of code that doesn't care much about what type of domain it's dealing with (configuration, listing, possible automation scripts for turning on the VMs, etc.) more complicated. Also, in this scheme, adding a new type of domain would be quite hard, especially after more code is written with this split in mind. Do keep the domain type, though. tl;dr I'd really prefer domain 1 (IPA); domain 2 (AD) rather than IPA domain 1; AD domain 1. This will, however, require filtering the domains depending on the fact whether they contain AD or not. If a testcase wants to access an AD domain, it will still need to loop over the list of domains to see which ones are of AD type. Any code that does not care what domain type it's dealing with, can easily access all the domains by chaining the respective iterables. We could have a wrapper in the Config class for that, along the lines of get_all_domains(). So what I see here is that we're trading one complexity over another. I think we can agree on your approach since it hides the complexity from the user, especially in the ipa-test-config, which I admit is getting rather ugly, as we need to introduce new option there and that causes splitting. If needed we can have a special check that would reject IPA masters in AD domains and vice versa, if that really turns out to be necessary. With this check we should be fine. As we already pass ad_domain flag to Domain.from_env, I did incorporate code that joins the machines to the domain depending on the their role. Is that a viable solution for you? Sorry. I think this design is less sustainable than having a shared namespace for the domains. I'll send revised patchset soon. Updated patchset attached. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Attaching the patch 100 I missed. -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From 6f2dc306542540f0db1c27f4efc301574bfdcf60 Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Wed, 4 Sep 2013 14:12:28 +0200 Subject: [PATCH 100/107] ipatests: Add Active Directory support to configuration Part of: https://fedorahosted.org/freeipa/ticket/3834 --- ipatests/man/ipa-test-config.1 | 20 +++- ipatests/test_integration/config.py | 30 -- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/ipatests/man/ipa-test-config.1 b/ipatests/man/ipa-test-config.1 index 4b998adb4f65265f4b0cfc49cac2979740562db5..a2fa96b57129b5adad3c15cfe7e97b8a6c9724d0 100644 --- a/ipatests/man/ipa-test-config.1 +++ b/ipatests/man/ipa-test-config.1 @@ -69,6 +69,11 @@ These are normally included for backwards compatibility. .SH ENVIRONMENT VARIABLES .TP +Domain configuration: +Domain is implicitly defined by _envX suffix of the environment variables, +if either AD_envX or MASTER_envX is defined. + +.TP Host configuration: .TP @@ -81,9 +86,14 @@ Host configuration: \fB$CLIENT\fR FQDNs of IPA clients (space-separated) .TP -\fB$MASTER_env2\fR, \fB$REPLICA_env2\fR, \fB$CLIENT_env2\fR, \fB$MASTER_env3\fR, ... +\fB$MASTER_env2\fR, \fB$REPLICA_env2\fR, \fB$CLIENT_env2\fR, \fB$MASTER_env3\fR, \fB$AD_env4\fR,... can be used for additional domains when needed .TP +\fB$AD_env1\fR, \fB$AD_env2\fR, \fB$AD_env3\fR, \fB$AD_env4\fR, ... +can be used to define Active Directory domains. Please note that these +domains are not separate from the IPA domains, so please
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 10/14/2013 05:47 PM, Tomas Babej wrote: Hi, Sending the updated version of the patchset. It includes many fixes, plus incorporations of the comments by the QE. Tests are now broken down into multiple test cases. This requires my patch 119. Tomas Patches 0100 0101: I still think it would be simpler if IPA and AD domains shared the numbering namespace (users would need to define $AD_env2; if they had $MASTER_env1 and $AD_env1 they would be in the same Domain). But if we use _env1 for both the AD and the IPA domain, they need to be separated in Domain.from_env. With patch 0101 both MASTER_env1 and AD_env1 will be in both domains[0] and ad_domains[0]. Also ipa-test-config should have an --ad-domain option to get info about the AD domain. Patch 0102: Looks good Patch 0103: Looks like run_repeatedly should always be called with an assert in front. We may want it to raise an exception directly if it times out, so forgetting the assert won't hide errors. Patch 0104: Looks good Patch 0105: In /ipatests/man/ipa-test-task.1: - Typo in - s/generatee/generate/ - Subcommand names have unescaped hyphens (e.g. configure\-dns-for-trust should be configure\-dns\-for\-trust) - The last subcommand should be sync-time Wrappers for the tasks need to be added to ipatests/ipa-test-task. Nitpicks: - In configure_dns_for_trust:is_subdomain, lockstep iteration over two lists at once is better done with zip() than `for i in range(len(...))`. - In estabilish_trust_with_ad, don't use mutable values for argument defaults; do `extra_args=()` and `+ list(extra_args)` - I can't say I'm a fan of configure_auth_to_local_rule, but I guess it's OK for tests. Patch 0106: Looks OK; unfortunately I don't have an AD machine to test functionality -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
Hi,
Sending the updated version of the patchset. It includes many fixes,
plus incorporations of the comments by the QE. Tests are now broken down
into multiple test cases.
This requires my patch 119.
Tomas
From 0a8645eaa1d8826a9544df288b12bde71a83dcb4 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Mon, 14 Oct 2013 16:37:55 +0200
Subject: [PATCH 106/106] ipatests: Add AD integration test case
---
ipatests/test_integration/test_trust.py | 182
1 file changed, 182 insertions(+)
create mode 100644 ipatests/test_integration/test_trust.py
diff --git a/ipatests/test_integration/test_trust.py b/ipatests/test_integration/test_trust.py
new file mode 100644
index ..202db2a2f6fb83a1e3cdf05b9c5c9c2617862736
--- /dev/null
+++ b/ipatests/test_integration/test_trust.py
@@ -0,0 +1,182 @@
+# Authors:
+# Tomas Babej tba...@redhat.com
+#
+# Copyright (C) 2013 Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+
+import re
+
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration import tasks
+from ipatests.test_integration import util
+
+
+class ADTrustBase(IntegrationTest):
+Provides common checks for the AD trust integration testing.
+
+topology = 'line'
+num_ad_domains = 1
+
+@classmethod
+def setup_class(cls):
+super(ADTrustBase, cls).setup_class()
+cls.ad = cls.ad_domains[0].ads[0]
+
+def test_install_adtrust(self):
+Test adtrust support installation
+
+tasks.install_adtrust(self.master)
+
+def test_check_sid_generation(self):
+Test SID generation
+
+command = ['ipa', 'user-show', 'admin', '--all', '--raw']
+
+# TODO: remove duplicate definition and import from common module
+_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
+
+assert util.run_repeatedly(self.master, command,
+ test=lambda x: re.search(stdout_re, x))
+
+def test_configure_dns_and_time(self):
+tasks.configure_dns_for_trust(self.master, self.ad)
+tasks.sync_time(self.master, self.ad)
+
+
+class TestBasicADTrust(ADTrustBase):
+Basic Integration test for Active Directory
+
+def test_estabilish_trust(self):
+Tests estabilishing trust with Active Directory
+
+tasks.estabilish_trust_with_ad(self.master, self.ad,
+extra_args=['--range-type', 'ipa-ad-trust'])
+
+def test_range_properties_in_nonposix_trust(self):
+Check the properties of the created range
+
+range_name = self.ad.domain.name.upper() + '_id_range'
+result = self.master.run_command(['ipa', 'idrange-show', range_name,
+ '--all', '--raw'])
+assert ipaRangeType: ipa-ad-trust in result.stdout_text
+assert ipaIDRangeSize: 20 in result.stdout_text
+
+def test_user_gid_uid_resolution_in_nonposix_trust(self):
+Check that user has SID-generated UID
+
+testuser = 'testuser@%s' % self.ad.domain.realm
+result = self.master.run_command(['getent', 'passwd', testuser])
+
+# This regex checks that Test User does not have UID 10042 nor belongs
+# to the group with GID 10047
+testuser_regex = ^testuser@%s:\*:(?!10042)(\d+):(?!10047)(\d+):\
+ Test User:/home/testuser:/bin/sh$\
+ % re.escape(self.ad.domain.name)
+
+assert re.search(testuser_regex, result.stdout_text)
+
+def test_remove_nonposix_trust(self):
+tasks.remove_trust_with_ad(self.master, self.ad)
+tasks.clear_sssd_cache(self.master)
+
+
+class TestPosixADTrust(ADTrustBase):
+Integration test for Active Directory with POSIX support
+
+def test_estabilish_trust_with_posix_attributes(self):
+# Not specifying the --range-type directly, it should be detected
+tasks.estabilish_trust_with_ad(self.master, self.ad)
+
+def test_range_properties_in_posix_trust(self):
+# Check the properties of the created range
+
+range_name =
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 09/17/2013 04:35 PM, Tomas Babej wrote: On 09/17/2013 10:43 AM, Petr Viktorin wrote: On 09/16/2013 03:45 PM, Tomas Babej wrote: Hi, this set of patches extends ipatests module to support integration testing with Active Directory, as well as provides an basic (working without artificial sleeps!) trust test case. Thanks, this is great news! I haven't testes the test yet. Here are my comments from reading them. Patch 100: Please document the new configuration options. There are two places: - ipa-test-config man page - http://www.freeipa.org/page/V3/Integration_testing#Test_configuration (or if you end up writing a separate design page for AD tests, link to it) I added the options to the the man page. Looks good. Patch 101: The `if role == 'ad':` block should rather be in the `Host.from_env` factory. Moved. Looks good. With my patches for #3890, this should use BaseHost.from_env. Patch 102: Thanks for cleaning that up! You could go one step further with cls.replicas = get_resources(domain.replicas, 'replicas', cls.num_replicas) and `return container[:num_needed]` there You're welcome, refactoring part expanded. Looks good. Patch 103: This patch should come before 101 which uses it. We can push this in the correct order if this is an issue, right? Patches are independent (meaning they do not touch the same source code, so they should apply cleanly). Sure. Ideally there would be a BaseHost with common functionality, and concrete Host and WinHost subclassing it. Yes, I agree, that's what we discussed. I'll be making changes here for #3890; please concentrate on other parts for now to avoid conflicts. I'll take Windows hosts into consideration. My patches are on the list. Conflicts should be minimal; withthem WinHost should end up empty. Raise instances rather than the exception classes: raise NotImplementedError() Fixed. Patch 104: [...] I'd prefer to keep the function in test_trust.py until we see there's a wider scope for it. And rename it to run_repeatedly or some other name that describes it better. I renamed the function. Thanks Why do you think there's not scope for it? I'd rather keep it in tasks, since it addresses a general use case of waiting in a loop until a command returns expected output. This can happen with any command that starts asynchronous actions and we want to make sure that the changes are propagated before continuing (such as DNS updates via our CLI). I wouldn't consider this being specific for AD trust testing. I'd like to keep tasks to high-level operations, the kind that ipa-test-task would do. (Putting wait_for_replication there was, in hindsight, a mistake.) We can add an util module later. It's easy to move things to a shared module when needed, but if we do it too early and often the module will be hard to maintain. Predicting the future in this respect hasn't worked very well for me :) Patch 105: Please add these tasks to ipa-test-task (and its man page) as well. The instructions in the docstring in configure_dns_for_trust should go to a test design document. I think just starting a section in Integration_testing would be fine if you mark it as not implemented yet (and link to the ticket). This still needs some work (unfortunately I haven't found a way to make it less tedious). Use parentheses instead of \ for line continuation (see PEP8). I don't really like wrapping arbitrary expressions in parentheses, particularly when assigning the result to a variable: result = (something or something_completely_else) To me, the following looks better: result = something or \ something completely else I checked with PEP8. I remember there was a section that backslash can be used in cases it produces nicer results, but it's gone now. Backslash is still recommended in some cases. Still (but no strong opinions here), I would not consider this a violation unless it happens between brackets (in which case it is redundant). Personal opinions aside, the PEP8 is pretty clear on this, sorry. It lists `with` and `assert` statements where you can't use parentheses around everything after the keyword, but that's not case here. Patch 106: The instructions in the TestADTrust docstring should go to a test design document. Please use the SID regex from a shared location. You'll need to assign it to a variable and make the Fuzzy from that, so that variable can be imported and used with the standard re module. This is something that I tried to address with the Fuzzy refactoring. This is a temporary solution, once we resolve that patchset, of course, the test will use the shared location. I added a comment there, I'd rather not create conflicts. OK, let's worry about this later. [...] test_user_gid_uid_resolution_in_nonposix_trust: - For a one-off regex, compile() is unnecessary: assert re.search(testuser_regex, result.stdout_text) - Whenever substituting a literal string into
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 09/16/2013 03:45 PM, Tomas Babej wrote: Hi, this set of patches extends ipatests module to support integration testing with Active Directory, as well as provides an basic (working without artificial sleeps!) trust test case. Thanks, this is great news! I haven't testes the test yet. Here are my comments from reading them. Patch 100: Please document the new configuration options. There are two places: - ipa-test-config man page - http://www.freeipa.org/page/V3/Integration_testing#Test_configuration (or if you end up writing a separate design page for AD tests, link to it) Patch 101: The `if role == 'ad':` block should rather be in the `Host.from_env` factory. Patch 102: Thanks for cleaning that up! You could go one step further with cls.replicas = get_resources(domain.replicas, 'replicas', cls.num_replicas) and `return container[:num_needed]` there Patch 103: This patch should come before 101 which uses it. Ideally there would be a BaseHost with common functionality, and concrete Host and WinHost subclassing it. I'll be making changes here for #3890; please concentrate on other parts for now to avoid conflicts. I'll take Windows hosts into consideration. Raise instances rather than the exception classes: raise NotImplementedError() Patch 104: Instead of stdout_re, allow the user to pass in a checking function. For example, `lambda result: re.search(sid_regex, result.stdout_text)` This makes wait_assert complete, you won't need to add other cases to it when you need to check stderr or evaluate some condition a regex is too weak for. Pass `raiseonerr` to run_command directly, not by setting it in kwargs. That way wait_assert will fail if it gets raiseonerr. Also, run_command's arguments except `command` are supposed to be keyword-only. (This is only not enforced because that's awkward to do in Python 2.) Don't accept or pass along *args. Use parentheses instead of \ for line continuation (see PEP8). I'd prefer to keep the function in test_trust.py until we see there's a wider scope for it. And rename it to run_repeatedly or some other name that describes it better. Patch 105: Please add these tasks to ipa-test-task (and its man page) as well. The instructions in the docstring in configure_dns_for_trust should go to a test design document. I think just starting a section in Integration_testing would be fine if you mark it as not implemented yet (and link to the ticket). Use parentheses instead of \ for line continuation (see PEP8). Patch 106: The instructions in the TestADTrust docstring should go to a test design document. Please use the SID regex from a shared location. You'll need to assign it to a variable and make the Fuzzy from that, so that variable can be imported and used with the standard re module. Avoid commented-out code in patches for review. No need to import fuzzy. test_user_gid_uid_resolution_in_nonposix_trust: - For a one-off regex, compile() is unnecessary: assert re.search(testuser_regex, result.stdout_text) - Whenever substituting a literal string into a regex, please use re.escape(). Use parentheses instead of \ for line continuation (see PEP8). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
On 09/17/2013 10:43 AM, Petr Viktorin wrote: On 09/16/2013 03:45 PM, Tomas Babej wrote: Hi, this set of patches extends ipatests module to support integration testing with Active Directory, as well as provides an basic (working without artificial sleeps!) trust test case. Thanks, this is great news! I haven't testes the test yet. Here are my comments from reading them. Patch 100: Please document the new configuration options. There are two places: - ipa-test-config man page - http://www.freeipa.org/page/V3/Integration_testing#Test_configuration (or if you end up writing a separate design page for AD tests, link to it) I added the options to the the man page. Patch 101: The `if role == 'ad':` block should rather be in the `Host.from_env` factory. Moved. Patch 102: Thanks for cleaning that up! You could go one step further with cls.replicas = get_resources(domain.replicas, 'replicas', cls.num_replicas) and `return container[:num_needed]` there You're welcome, refactoring part expanded. Patch 103: This patch should come before 101 which uses it. We can push this in the correct order if this is an issue, right? Patches are independent (meaning they do not touch the same source code, so they should apply cleanly). Ideally there would be a BaseHost with common functionality, and concrete Host and WinHost subclassing it. Yes, I agree, that's what we discussed. I'll be making changes here for #3890; please concentrate on other parts for now to avoid conflicts. I'll take Windows hosts into consideration. Raise instances rather than the exception classes: raise NotImplementedError() Fixed. Patch 104: Instead of stdout_re, allow the user to pass in a checking function. For example, `lambda result: re.search(sid_regex, result.stdout_text)` This makes wait_assert complete, you won't need to add other cases to it when you need to check stderr or evaluate some condition a regex is too weak for. +1, improved. Pass `raiseonerr` to run_command directly, not by setting it in kwargs. That way wait_assert will fail if it gets raiseonerr. Also, run_command's arguments except `command` are supposed to be keyword-only. (This is only not enforced because that's awkward to do in Python 2.) Don't accept or pass along *args. Agreed, fixed. Use parentheses instead of \ for line continuation (see PEP8). I'd prefer to keep the function in test_trust.py until we see there's a wider scope for it. And rename it to run_repeatedly or some other name that describes it better. I renamed the function. Why do you think there's not scope for it? I'd rather keep it in tasks, since it addresses a general use case of waiting in a loop until a command returns expected output. This can happen with any command that starts asynchronous actions and we want to make sure that the changes are propagated before continuing (such as DNS updates via our CLI). I wouldn't consider this being specific for AD trust testing. Patch 105: Please add these tasks to ipa-test-task (and its man page) as well. The instructions in the docstring in configure_dns_for_trust should go to a test design document. I think just starting a section in Integration_testing would be fine if you mark it as not implemented yet (and link to the ticket). Use parentheses instead of \ for line continuation (see PEP8). I don't really like wrapping arbitrary expressions in parentheses, particularly when assigning the result to a variable: result = (something or something_completely_else) To me, the following looks better: result = something or \ something completely else I checked with PEP8. I remember there was a section that backslash can be used in cases it produces nicer results, but it's gone now. Backslash is still recommended in some cases. Still (but no strong opinions here), I would not consider this a violation unless it happens between brackets (in which case it is redundant). Patch 106: The instructions in the TestADTrust docstring should go to a test design document. Please use the SID regex from a shared location. You'll need to assign it to a variable and make the Fuzzy from that, so that variable can be imported and used with the standard re module. This is something that I tried to address with the Fuzzy refactoring. This is a temporary solution, once we resolve that patchset, of course, the test will use the shared location. I added a comment there, I'd rather not create conflicts. Avoid commented-out code in patches for review. No need to import fuzzy. Fixed. test_user_gid_uid_resolution_in_nonposix_trust: - For a one-off regex, compile() is unnecessary: assert re.search(testuser_regex, result.stdout_text) - Whenever substituting a literal string into a regex, please use re.escape(). Fixed. Use parentheses instead of \ for line continuation (see PEP8). Design will follow soon. -- Tomas Babej Associate
[Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
Hi,
this set of patches extends ipatests module to support integration
testing with Active Directory,
as well as provides an basic (working without artificial sleeps!) trust
test case.
--
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
From 793dfd9dc33ef8387761e68058c9e8c9b07c3015 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Wed, 4 Sep 2013 14:12:28 +0200
Subject: [PATCH 100/106] ipatests: Add Active Directory support to
configuration
---
ipatests/test_integration/config.py | 21 +
1 file changed, 21 insertions(+)
diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index d43812c514bf1c9d23740e6f75cc3574235e86d3..ac0131e435d33b6730093cbc64a1b47910d3971e 100644
--- a/ipatests/test_integration/config.py
+++ b/ipatests/test_integration/config.py
@@ -1,5 +1,6 @@
# Authors:
# Petr Viktorin pvikt...@redhat.com
+# Tomas Babej tba...@redhat.com
#
# Copyright (C) 2013 Red Hat
# see file 'COPYING' for use and warranty information
@@ -50,11 +51,14 @@ class Config(object):
self.nis_domain = kwargs.get('nis_domain') or 'ipatest'
self.ntp_server = kwargs.get('ntp_server') or (
'%s.pool.ntp.org' % random.randint(0, 3))
+self.ad_admin_name = kwargs.get('ad_admin_name') or 'Administrator'
+self.ad_admin_password = kwargs.get('ad_admin_password') or 'Secret123'
if not self.root_password and not self.root_ssh_key_filename:
self.root_ssh_key_filename = '~/.ssh/id_rsa'
self.domains = []
+self.ad_domains = []
@classmethod
def from_env(cls, env):
@@ -76,6 +80,8 @@ class Config(object):
ADMINPW: Administrator password
ROOTDN: Directory Manager DN
ROOTDNPWD: Directory Manager password
+ADADMINID: Active Directory Administrator username
+ADADMINPW: Active Directory Administrator password
DNSFORWARD: DNS forwarder
NISDOMAIN
NTPSERVER
@@ -83,6 +89,7 @@ class Config(object):
MASTER_env1: FQDN of the master
REPLICA_env1: space-separated FQDNs of the replicas
CLIENT_env1: space-separated FQDNs of the clients
+AD_env1: space-separated FQDNs of the Active Directories
OTHER_env1: space-separated FQDNs of other hosts
(same for _env2, _env3, etc)
BEAKERREPLICA1_IP_env1: IP address of replica 1 in env 1
@@ -104,13 +111,23 @@ class Config(object):
dns_forwarder=env.get('DNSFORWARD'),
nis_domain=env.get('NISDOMAIN'),
ntp_server=env.get('NTPSERVER'),
+ ad_admin_name=env.get('ADADMINID'),
+ ad_admin_password=env.get('ADADMINPW'),
)
+# Either IPA master or AD can define a domain
+
domain_index = 1
while env.get('MASTER_env%s' % domain_index):
self.domains.append(Domain.from_env(env, self, domain_index))
domain_index += 1
+domain_index = 1
+while env.get('AD_env%s' % domain_index):
+self.ad_domains.append(Domain.from_env(env, self, domain_index,
+ ad_domain=True))
+domain_index += 1
+
return self
def to_env(self, simple=True):
@@ -133,6 +150,9 @@ class Config(object):
env['ROOTDN'] = str(self.dirman_dn)
env['ROOTDNPWD'] = self.dirman_password
+env['ADADMINID'] = self.ad_admin_name
+env['ADADMINPW'] = self.ad_admin_password
+
env['DNSFORWARD'] = self.dns_forwarder
env['NISDOMAIN'] = self.nis_domain
env['NTPSERVER'] = self.ntp_server
@@ -145,6 +165,7 @@ class Config(object):
for role, hosts in [('MASTER', domain.masters),
('REPLICA', domain.replicas),
('CLIENT', domain.clients),
+('AD', domain.ads),
('OTHER', domain.other_hosts)]:
hostnames = ' '.join(h.hostname for h in hosts)
env['%s%s' % (role, domain._env)] = hostnames
--
1.8.3.1
From b9dcf30106d5d4913357cf0e70c5d6c4b2e7ba50 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Wed, 4 Sep 2013 14:24:41 +0200
Subject: [PATCH 101/106] ipatests: Extend domain object with 'ad' role support
and WinHosts
---
ipatests/test_integration/config.py | 39 +++--
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index ac0131e435d33b6730093cbc64a1b47910d3971e..284b511ac93061b44ca612bed006338fb001833c 100644
--- a/ipatests/test_integration/config.py
+++ b/ipatests/test_integration/config.py
@@ -27,7 +27,7 @@ import random
from ipapython import ipautil
from ipapython.dn
