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

[Martin Basti]

On 30.06.2016 12:58, Lenka Doudova wrote:
>
>
> On 06/30/2016 12:51 PM, Petr Spacek wrote:
>> 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.
>>
> Yes, I've had no problems running the tests since I started to use this.
> Thanks.
>
Pushed to master: 1d9e1521c59a5b43c2322892ce5cbe8cceff2790