[Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

On 06/29/2016 07:49 PM, Petr Spacek wrote:
> On 29.6.2016 18:52, Lenka Doudova wrote:
>>
>> On 06/29/2016 06:51 PM, Petr Spacek wrote:
>>> On 29.6.2016 18:48, Lenka Doudova wrote:
>>>> On 06/27/2016 11:05 AM, Lenka Doudova wrote:
>>>>> On 06/27/2016 10:33 AM, Martin Babinsky wrote:
>>>>>> On 06/27/2016 10:28 AM, Petr Spacek wrote:
>>>>>>> On 27.6.2016 10:26, Petr Spacek wrote:
>>>>>>>> On 27.6.2016 10:18, Martin Babinsky wrote:
>>>>>>>>> On 06/27/2016 10:04 AM, Petr Vobornik wrote:
>>>>>>>>>> On 06/27/2016 09:42 AM, Lenka Doudova wrote:
>>>>>>>>>>> Hi!
>>>>>>>>>>>
>>>>>>>>>>> With newly created AD machines in Brno lab, existing trust tests
>>>>>>>>>>> fail on
>>>>>>>>>>> 'ipa dnsforwardzone-add' command claiming the zone is already present,
>>>>>>>>>>> as new AD domain is dom-221.idm.lab.eng.brq.redhat.com.
>>>>>>>>>>>
>>>>>>>>>>> To prevent these failures I prepared attached patch, that will still
>>>>>>>>>>> attempt to add the forward zone, but in case of non-zero return code
>>>>>>>>>>> will check the message if it says that the forward zone is already
>>>>>>>>>>> configured, and lets the tests continue, if it is so.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Lenka
>>>>>>>>>>>
>>>>>>>>>> Current approach expects that every error of ipa dnsforward-add here
>>>>>>>>>> will mean that the zone exists. So it might hide other issues - not very
>>>>>>>>>> good.
>>>>>>>>>>
>>>>>>>>>> On the other hand it is not very robust to parse error message.
>>>>>>>>>>
>>>>>>>>>> Question for general audience: What do you think if IPA client's exit
>>>>>>>>>> status would be the IPA error code instead of "1" for every error. E.g.
>>>>>>>>>> in DuplicateEntry case it's 4002.
>>>>>>>>>>
>>>>>>>>>> Btw, this is not a NACK.
>>>>>>>>>>
>>>>>>>>> Well AFAIK the exit status on POSIX systems is encoded into a single
>>>>>>>>> byte so
>>>>>>>>> you cannot have the return value greater that 255. We would have to
>>>>>>>>> devise
>>>>>>>>> some mapping between our XMLRPC status codes and subprocess return codes.
>>>>>>>>>
>>>>>>>>> Some of our exceptions have defined return values outside plain '1', e.g.
>>>>>>>>> NotFound has return value of 2. It would be possible to extend this
>>>>>>>>> concept on
>>>>>>>>> other common errors.
>>>>>>>> Even more importantly, the forward zone is completely unnecessary because
>>>>>>>> DNS
>>>>>>>> when DNS is set up properly. I would simply remove the dnsforwardzone-add.
>>>>>>>>
>>>>>>> Grr, I meant this:
>>>>>>> Even more importantly, the forward zone is completely unnecessary when
>>>>>>> DNS is
>>>>>>> set up properly. I would simply remove the dnsforwardzone-add.
>>>>>>>
>>>>>> +1, our tests should not fiddle with the provisioned environment as much as
>>>>>> they sometimes do.
>>>>>>
>>>>> Well, I have nothing against removing it completely, but left it there just
>>>>> because with previous AD machines with "wild" domains it was necessary.
>>>>> Looking at the code, your suggestion would probably mean to entirely remove
>>>>> method configure_dns_for_trust from ipatests/test_integration/tasks.py,
>>>>> right? I'll have to verify this won't break anything else.
>>>>>
>>>>> Lenka
>>>>>
>>>> Hi,
>>>>
>>>> to get back to this issue: do we really want to have the DNS configuration
>>>> method removed? I mean, we no longer need it for our CI tests, with new AD VMs
>>>> it works without it, but should somebody else with different setup run these
>>>> tests, they could experience failures because their AD domain would not be
>>>> configured in DNS by default and the test would no longer provide that
>>>> configuration. It doesn't feel right to delete something we needed before but
>>>> don't need anymore, in case somebody else is depending on the same
>>>> configuration. But of course, I'll abide by your counsel.
>>>> In case the call on DNS configuration method really is removed, should I
>>>> remove even it's definition? It's not used anywhere else, so it would be quite
>>>> logical.
>>> Feel free to keep empty method around as a "hook" for other people. The
>>> important thing is that it should do nothing by default.
>>>
>> So leave the method call, but erase method contents and let it just pass?
> Fine with me. (List re-added.)
>
Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the 
original DNS configuring function is now empty, I modified the comment 
to explain significance of this strange thing. I also changed patch 
title to better reflect proposed changes.

Lenka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0022.2-Tests-Remove-DNS-configuration-from-trust-tests.patch
Type: text/x-patch
Size: 2754 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160630/298f3d6c/attachment.bin>