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

Thu Oct 11 10:27:09 UTC 2012

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 (), 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 = $('', {});
>>>>>
>>>>>I would rather see a <div /> as a container. Span is and should
>>>>>contain only inline elements.
>>>>Fixed.
>>>>
>>>>>2)
>>>>>var line_span = $('', {
>>>>> class: 'error-message-hinted',
>>>>> text: lines[i].substr(1)
>>>>> }).appendTo(container);
>>>>>
>>>>>Why do you use for hinted message and for other lines?
>>>>>Shouldn't we use for both?
>>>>Fixed to use 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?

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']


-- 
/ Alexander Bokovoy
-------------- next part --------------
>From 3f5999237b2585701a7e40ad196933989fa4bc6c Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Thu, 11 Oct 2012 13:23:28 +0300
Subject: [PATCH 7/7] Introduce multiline flag to PublicError

In order to control whether arguments should be converged into a single
string or left as they are, add PublicError.multiline boolean flag.

If PublicError.multiline is True, all arguments to an error instance would
be joined with '\n' to a single string to support multi-lined arguments.
---
 ipalib/errors.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ipalib/errors.py b/ipalib/errors.py
index 7f1113200b6b2cd80ea9156d347211e4d978b9be..13a054b33217af43b90ea0c3c7c999070466693d 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -248,6 +248,7 @@ class PublicError(StandardError):
     errno = 900
     rval = 1
     format = None
+    multiline = True
 
     def __init__(self, format=None, message=None, **kw):
         self.kw = kw
@@ -270,7 +271,10 @@ class PublicError(StandardError):
                     result=u'\n'.join(map(lambda line: unicode(line), value))
                     return result
                 return value
-            kwargs = dict(map(lambda (k,v): (k, convert_keyword(v)), kw.items()))
+            if self.multiline:
+                kwargs = dict(map(lambda (k,v): (k, convert_keyword(v)), kw.items()))
+            else:
+                kwargs = kw
             self.msg = self.format % kwargs
             if isinstance(self.format, basestring):
                 self.strerror = ugettext(self.format) % kwargs
@@ -736,6 +740,7 @@ class OverlapError(InvocationError):
 
     errno = 3006
     format = _('overlapping arguments and options: %(names)r')
+    multiline = False
 
 
 class RequirementError(InvocationError):
-- 
1.7.12

