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

Tue Jul 26 10:17:11 UTC 2016

On 06/30/2016 01:14 PM, Martin Basti wrote:
>
>
> 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
>
Hi,

I just realized the same problem occurs in 4.3 branch, but the original 
patch was pushed to master only, hence I ask for second review.
The originally pushed patch attached, should not need any modifications 
for ipa-4-3 branch.

Ticket: https://fedorahosted.org/freeipa/ticket/6133

Thanks,
Lenka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0022.3-Tests-Remove-DNS-configuration-from-trust-tests.patch
Type: text/x-patch
Size: 2753 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160726/70242643/attachment.bin>