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

[Lenka Doudova]

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.

Thanks,
Lenka