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

[Petr Spacek]

On 30.6.2016 12:32, Lenka Doudova wrote:
> 
> 
> 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.

ACK if it passes your tests.

-- 
Petr^2 Spacek