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

[Martin Basti]

On 26.07.2016 12:17, Lenka Doudova wrote:
>
>
> 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
>
>
Pushed to ipa-4-3: 40b1459ad0299e95331699be9684682fca02a570

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160726/a765e2f7/attachment.htm>