[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust

[Martin Kosek]

On 10/08/2012 02:22 PM, Alexander Bokovoy wrote:
> On Mon, 08 Oct 2012, Petr Vobornik wrote:
>> On 10/05/2012 08:14 PM, Alexander Bokovoy wrote:
>>> On Fri, 05 Oct 2012, Petr Vobornik wrote:
>>>> On 10/05/2012 03:24 PM, Alexander Bokovoy wrote:
>>>>> On Fri, 05 Oct 2012, Petr Vobornik wrote:
>>>>>> On 10/04/2012 05:06 PM, Alexander Bokovoy wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> two attached patches attempt to solve
>>>>>>> https://fedorahosted.org/freeipa/ticket/3103
>>>>>>>
>>>>>>> We cannot make educated guess where trusted domain's DNS server is
>>>>>>> located as we ended up with NotFound exception precisely because we
>>>>>>> were
>>>>>>> unable to discover trusted domain's domain controller location.
>>>>>>>
>>>>>>> Thus, all we can do is to suggest ways to fix the issue. Since
>>>>>>> suggestion becomes relatively long (multi-line, at least), it creates
>>>>>>> few issues for current exception error message handling:
>>>>>>> - root_logger does not use textui to format output
>>>>>>> - textui is only printing to standard output
>>>>>>> - multi-line error messages thus become non-wrapped
>>>>>>> - localizing such messages would become a harder context-wise.
>>>>>>>
>>>>>>> Web UI is showing error message as a single paragraph (<p/>), all
>>>>>>> multi-line markup would be lost.
>>>>>>>
>>>>>>> In the end, I decided to support list-based multi-line error
>>>>>>> messages in
>>>>>>> PublicError class. Its constructor will join all list-based arguments
>>>>>>> into single string per argument (first patch).
>>>>>>>
>>>>>>> In Web UI I've added special text processing of error messages that
>>>>>>> splits message into multiple lines and wraps those which start with a
>>>>>>> TAB symbol into 'error-message-hinted' span to visually separate it
>>>>>>> from
>>>>>>> the rest of error message. Trust code uses this to display
>>>>>>> suggested CLI
>>>>>>> command to set up DNS forwarder (second patch).
>>>>>>>
>>>>>>> In the end, CLI shows such error message fine (note tabulated CLI
>>>>>>> command):
>>>>>>> -----------------------------------------------------------------------
>>>>>>>
>>>>>>> # ipa trust-add --type=ad --admin Administrator at ad.local1 --password
>>>>>>> ad.local1
>>>>>>> Active directory domain administrator's password: ipa: ERROR:
>>>>>>> Unable to
>>>>>>> resolve domain controller for 'ad.local1' domain. IPA manages DNS,
>>>>>>> please configure forwarder to 'ad.local1' domain by using following
>>>>>>> CLI
>>>>>>> command. Please replace DNS_SERVER and IP_ADDRESS by name and
>>>>>>> address of
>>>>>>> the domain's name server:
>>>>>>>   ipa dnszone-add ad.local1 --name-server=DNS_SERVER
>>>>>>> --admin-email='hostmaster at ad.local1' --force --forwarder=IP_ADDRESS
>>>>>>> --forward-policy=only
>>>>>>> When using Web UI, please create DNS zone for domain 'ad.local1' first
>>>>>>> and then set forwarder and forward policy
>>>>>>> -----------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>> Web UI looks like this: http://abbra.fedorapeople.org/.paste/ui.png
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> You have undeclared identifier: lines.
>>>>>>
>>>>>> I recommend to run `jsl -conf jsl.conf` command in install/ui folder
>>>>>> to prevent these issues.
>>>>> Fixed.
>>>>>
>>>>>
>>>>>> I'm not convinced that "beautify" call should be in command object. I
>>>>>> would rather see it in error_dialog.
>>>>> I moved everything to error_dialog and used $() selectors instead of
>>>>> directly playing with text.
>>>>
>>>> 1)
>>>> +        var error_message = $('<span />', {});
>>>>
>>>> I would rather see a <div /> as a container. Span is and should
>>>> contain only inline elements.
>>> Fixed.
>>>
>>>> 2)
>>>> var line_span = $('<span />', {
>>>>                   class: 'error-message-hinted',
>>>>                   text: lines[i].substr(1)
>>>>               }).appendTo(container);
>>>>
>>>> Why do you use <span> for hinted message and <p> for other lines?
>>>> Shouldn't we use <p> for both?
>>> Fixed to use <p> everywhere.
>>>
>>>
>>>> 3) var line_span is declared twice. JS use function scope, not block
>>>> scope.
>>> Fixed.
>>>
>>> Thanks for the review. New patch 0082 attached.
>>>
>>>
>> ACK on the UI part with a little change (updated patch attached):
>>    class: 'error-message-hinted',
>> needs to be changed to
>>    'class': 'error-message-hinted',
>> because class is a keyword and should not be used this way. Sorry that I
>> missed it before.
> Thanks!
> 
> 
>> The patch works as advertised. I would give ACK to the python part of 82 and
>> patch 83 as well but probably someone more skilled in python and ipa
>> internals should do it.
>>
>> Why do you have to concatenate the value in PublicErrors' __init__ method?
>> Shouldn't it be a responsibility of error source (in this case 'execute_ad'
>> method)? - just a question, not an issue
> This is a good question and gives me some space to explain why certain
> decisions are made.
> 
> The whole idea of the patch is to introduce a way to provide multi-line
> error messages as a feature of error handling in the FreeIPA framework.
> 
> Suppose we had to join multiple lines always before creating an error
> object. This means we would have hundreds of '\n'.join() calls across
> the code. Besides maintenance burden, it would also make computational
> burden later if we would add proper content formating (which we don't do
> now for errors) -- since we would need to split strings later to
> reformat them.
> 
> Python is flexible enough to discover parameter types which
> allows to design APIs that easer to use. "Easier to use" means least
> surprise for the caller rather than callee. So, if you pass multiple
> lines (list) of error message, each array element gets processed
> separately before displaying.
> So, in the end there is only one place where this processing happens,
> and it encapsulates all the knowledge required to accept multi-lines
> messages, regardless how they come, since it is applied uniformly to every
> argument of a constructor of a PublicError-derived class.
> 

Pushed both patches to master, ipa-3-0.

Martin