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

[Oleg Fayans]

On 06/30/2016 12:41 PM, Lenka Doudova wrote:
> 
> 
> On 06/30/2016 12:32 PM, 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.
>>
>> Lenka
>>
>>
> NACK the previous one, forgot PEP8. New patch attached.
> 
> Lenka

Functional ACK

> 
> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.