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

[Martin Babinsky]

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/

-- 
Martin^3 Babinsky