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

[Lenka Doudova]

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