[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust
Alexander Bokovoy
abokovoy at redhat.com
Thu Oct 11 12:44:04 UTC 2012
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.
--
/ Alexander Bokovoy
-------------- next part --------------
>From 6c7113197cbcc9a495147f4ce240d7dffc4a6540 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] Add instructions support to PublicError
When long additional text should follow the error message, one can
supply instructions parameter to a class derived from PublicError.
This will cause following text added to the error message:
Additional instructions:
<additional text>
`instructions' optional parameter could be a list or anything that coerces
into unicode(). List entries will be joined with '\n'.
https://fedorahosted.org/freeipa/ticket/3167
---
ipalib/errors.py | 11 +++++++----
ipalib/plugins/trust.py | 15 ++++++++-------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 7f1113200b6b2cd80ea9156d347211e4d978b9be..99950011744d929bc372f6a3d5dea4f614fcaf76 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -270,12 +270,15 @@ 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()))
- self.msg = self.format % kwargs
+ self.msg = self.format % kw
if isinstance(self.format, basestring):
- self.strerror = ugettext(self.format) % kwargs
+ self.strerror = ugettext(self.format) % kw
else:
- self.strerror = self.format % kwargs
+ self.strerror = self.format % kw
+ if 'instructions' in kw:
+ instructions = u'\n'.join((unicode(_('Additional instructions:')), convert_keyword(kw['instructions'])))
+ self.strerror = u'\n'.join((self.strerror, instructions))
+ self.msg = u'\n'.join((self.msg, instructions))
else:
if isinstance(message, (Gettext, NGettext)):
message = unicode(message)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 8632d42df578d81b6fdbcd9e5be8979994699206..9fc6c5ad4638fa237632876207cc85c64ade6503 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -346,32 +346,33 @@ class trust_add(LDAPCreate):
try:
result = trustinstance.join_ad_full_credentials(keys[-1], realm_server, realm_admin, realm_passwd)
except errors.NotFound, e:
- error_message=[_("Unable to resolve domain controller for '%s' domain. ") % (keys[-1])]
+ error_message=_("Unable to resolve domain controller for '%s' domain. ") % (keys[-1])
+ instructions=[]
if dns_container_exists(self.obj.backend):
try:
dns_zone = api.Command.dnszone_show(keys[-1])['result']
if ('idnsforwardpolicy' in dns_zone) and dns_zone['idnsforwardpolicy'][0] == u'only':
- error_message.append(_("Forward policy is defined for it in IPA DNS, "
+ instructions.append(_("Forward policy is defined for it in IPA DNS, "
"perhaps forwarder points to incorrect host?"))
except (errors.NotFound, KeyError) as e:
- error_message.append(_("IPA manages DNS, please configure forwarder to "
+ instructions.append(_("IPA manages DNS, please configure forwarder to "
"'%(domain)s' domain using following CLI command. "
"Make sure to replace DNS_SERVER and IP_ADDRESS by "
"actual values corresponding to the trusted domain's "
"DNS server:") % dict(domain=keys[-1]))
# tab character at the beginning of a multiline error message will be replaced
# in the web UI by a colorful hint. Does not affect CLI.
- error_message.append(_("\tipa dnszone-add %(domain)s --name-server=[DNS_SERVER] "
+ instructions.append(_("\tipa dnszone-add %(domain)s --name-server=[DNS_SERVER] "
"--admin-email='hostmaster@%(domain)s' "
"--force --forwarder=[IP_ADDRESS] "
"--forward-policy=only") % dict(domain=keys[-1]))
- error_message.append(_("When using Web UI, please create DNS zone for domain '%(domain)s' "
+ instructions.append(_("When using Web UI, please create DNS zone for domain '%(domain)s' "
"first and then set forwarder and forward policy.") % dict(domain=keys[-1]))
else:
- error_message.append(_("Since IPA does not manage DNS records, ensure DNS "
+ instructions.append(_("Since IPA does not manage DNS records, ensure DNS "
"is configured to resolve '%(domain)s' domain from "
"IPA hosts and back.") % dict(domain=keys[-1]))
- raise errors.NotFound(reason=error_message)
+ raise errors.NotFound(reason=error_message, instructions=instructions)
if result is None:
raise errors.ValidationError(name=_('AD Trust setup'),
--
1.7.12
More information about the Freeipa-devel
mailing list