[Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
Martin Basti
mbasti at redhat.com
Tue Jul 19 11:31:12 UTC 2016
On 18.07.2016 18:07, Martin Babinsky wrote:
> On 07/18/2016 04:59 PM, Lenka Doudova wrote:
>>
>>
>> On 07/18/2016 04:55 PM, Martin Babinsky wrote:
>>> On 07/14/2016 11:42 AM, Lenka Doudova wrote:
>>>>
>>>>
>>>> On 07/13/2016 05:40 PM, Martin Babinsky wrote:
>>>>> On 07/01/2016 04:15 PM, Lenka Doudova wrote:
>>>>>>
>>>>>>
>>>>>> On 07/01/2016 02:38 PM, Martin Babinsky wrote:
>>>>>>> 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)
>>>>>>>
>>>>>>>
>>>>>> Thank you very much for the example, fixed patch attached.
>>>>>> Lenka
>>>>>
>>>>> Hi Lenka,
>>>>>
>>>>> 'TestExternalTrustWithSubdomain' and
>>>>> 'TestExternalTrustWithRootDomain'
>>>>> suites fail due to incorrectly used '--external' option in the
>>>>> trust-add command. This option is Boolean, not Flag so it should be
>>>>> '--external=True'.
>>>>>
>>>> Hi,
>>>> that must have changes since I sent the patch, it was fine before.
>>>> Anyway, fixed patch attached.
>>>>
>>>> Lenka
>>>
>>> Hi Lenka,
>>>
>>> one of the tests keep failing on non-posix user UID/GID resolution. Is
>>> it a bug of the test or incorrect setup of AD machines?
>>>
>>> https://paste.fedoraproject.org/392195/68853561/
>>>
>> Hi,
>>
>> this is due to incorrect setup of AD machines - some attributes are
>> missing for AD users. Same issue should arise in any trust test that
>> includes this method, i.e. also in the tests from patch 0026.
>>
>> Lenka
>
> Ok, ACK then.
>
New ticket created for tests https://fedorahosted.org/freeipa/ticket/6093
Pushed to master: f487233df002bf73dd48d5c87a146b90542bd034
More information about the Freeipa-devel
mailing list