[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust
Petr Viktorin
pviktori at redhat.com
Thu Oct 11 13:22:29 UTC 2012
On 10/11/2012 02:44 PM, Alexander Bokovoy wrote:
> On Thu, 11 Oct 2012, Petr Viktorin wrote:
>> On 10/11/2012 12:27 PM, Alexander Bokovoy wrote:
>>> On Thu, 11 Oct 2012, Petr Viktorin wrote:
>>>> 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.
>>>>>
>>>>
>>>> Some of our errors already accept lists. From our doctest suite:
>>>>
>>>> File "/home/pviktori/freeipa/ipalib/errors.py", line 731, in
>>>> ipalib.errors.OverlapError
>>>> Failed example:
>>>> raise OverlapError(names=['givenname', 'login'])
>>>> Expected:
>>>> Traceback (most recent call last):
>>>> ...
>>>> OverlapError: overlapping arguments and options: ['givenname',
>>>> 'login']
>>>> Got:
>>>> Traceback (most recent call last):
>>>> File "/usr/lib64/python2.7/doctest.py", line 1289, in __run
>>>> compileflags, 1) in test.globs
>>>> File "<doctest ipalib.errors.OverlapError[0]>", line 1, in <module>
>>>> raise OverlapError(names=['givenname', 'login'])
>>>> OverlapError: overlapping arguments and options: u'givenname\nlogin'
>>>>
>>>> In this case, ['givenname', 'login'] is much better than
>>>> u'givenname\nlogin' (or givenname<newline>login, if we switched the
>>>> message from %r to %s).
>>>>
>>>>
>>>> I don't think what's best for trust-add's NotFound is the best thing
>>>> to do everywhere.
>>> Sure. Could you make a ticket so that I can investigate it more and find
>>> appropriate way to handle this? Perhaps an error class could define how
>>> it wants to treat its arguments?
>>
>> Sure. Please share findings of your investigation, then.
>>
>> https://fedorahosted.org/freeipa/ticket/3167
>>
>> Basically, I think you generalized too much. The result is much more
>> complex (=less understandable, maintainable, changeable) than a
>> '\n'.join() when creating the error.
>>
>>>
>>> With the following patch I have this behavior:
>>>>>> from ipalib import errors
>>>>>> raise errors.PublicError(lines=['foo','bar'], format="You've got a
>>>>>> message: %(lines)s")
>>> Traceback (most recent call last):
>>> File "<console>", line 1, in <module>
>>> PublicError: You've got a message: foo
>>> bar
>>>>>> raise errors.OverlapError(names=['foo','bar'])
>>> Traceback (most recent call last):
>>> File "<console>", line 1, in <module>
>>> OverlapError: overlapping arguments and options: ['foo', 'bar']
>>
>> The new patch adds even more complexity. Adding the conversions only
>> to error classes that need them (if any), as opposed to PublicError
>> with an option to turn them off, would be much more straightforward.
> After discussing more with Petr on irc, I came up with the following
> patch: allow `instructions' additional parameter to PublicError() to
> add instructions to an error message.
>
> Trust's error message is converted to show its use.
>
The rationale:
The need for multi-line error messages was prompted by requests that IPA
should either fix/prevent errors itself, or print out more detailed
instructions for the user.
Automatic joining of lists is problematic, because it repurposes a
standard datatype for the needs of a fairly specific use case.
Converting individual arguments that need it is better than a blanket
approach.
An extra argument for instructions allows us to separate the error
message and instructions in the future, if we need to e.g. add more
complex formatting than the \t.
Patch comments:
This needs some tests to verify it's right and prevent regressions.
Please rename the convert_keyword function to e.g. format_instructions.
It's not necessary to add the instructions to self.msg, that one is not
user-friendly.
Please wrap the long line in errors.py.
--
Petr³
More information about the Freeipa-devel
mailing list