[Freeipa-devel] [PATCH 0025][Tests] RFE: External trust

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky