[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust
Alexander Bokovoy
abokovoy at redhat.com
Fri Oct 12 09:20:09 UTC 2012
On Thu, 11 Oct 2012, Petr Viktorin wrote:
>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.
Updated the patch with new test case and fixes. Also split out trust.py
changes to a separate patch.
Both attached.
--
/ Alexander Bokovoy
-------------- next part --------------
>From 317f59aab403f17ef280832ba8202d63250f5c18 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Fri, 12 Oct 2012 12:13:59 +0300
Subject: [PATCH 7/8] 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 | 21 ++++++++++++---------
tests/test_ipalib/test_errors.py | 17 +++++++++++++++++
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 7f1113200b6b2cd80ea9156d347211e4d978b9be..a6c8e76836c0b4c9809f891a423abaf51d057108 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -265,17 +265,20 @@ class PublicError(StandardError):
)
self.format = format
self.forwarded = False
- def convert_keyword(value):
- if isinstance(value, list):
- 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:
+ def convert_instructions(value):
+ if isinstance(value, list):
+ result=u'\n'.join(map(lambda line: unicode(line), value))
+ return result
+ return value
+ instructions = u'\n'.join((unicode(_('Additional instructions:')),
+ convert_instructions(kw['instructions'])))
+ self.strerror = u'\n'.join((self.strerror, instructions))
else:
if isinstance(message, (Gettext, NGettext)):
message = unicode(message)
diff --git a/tests/test_ipalib/test_errors.py b/tests/test_ipalib/test_errors.py
index 2fcdc56c804891b89870adc8fb7eeaed9a686803..1421e7844a7068605568bccf78715669dfe0d8ec 100644
--- a/tests/test_ipalib/test_errors.py
+++ b/tests/test_ipalib/test_errors.py
@@ -318,6 +318,23 @@ class test_PublicError(PublicExceptionTester):
assert inst.text is kw['text']
assert inst.number is kw['number']
+ # Test with instructions:
+ # first build up "instructions", then get error and search for
+ # lines of instructions appended to the end of the strerror
+ # despite the parameter 'instructions' not existing in the format
+ instructions = u"The quick brown fox jumps over the lazy dog".split()
+ # this expression checks if each word of instructions
+ # exists in a string as a separate line, with right order
+ regexp = re.compile('(?ims).*' +
+ ''.join(map(lambda x: '(%s).*' % (x),
+ instructions)) +
+ '$')
+ inst = subclass(instructions=instructions, **kw)
+ assert inst.format is subclass.format
+ assert_equal(inst.instructions, instructions)
+ inst_match = regexp.match(inst.strerror).groups()
+ assert_equal(list(inst_match),list(instructions))
+
def test_public_errors():
"""
--
1.7.12
-------------- next part --------------
>From d386180e7d60cef609e5c9d85b769de6425a4614 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Fri, 12 Oct 2012 12:14:24 +0300
Subject: [PATCH 8/8] Use PublicError instructions support for trust-add case
when domain is not found
https://fedorahosted.org/freeipa/ticket/3167
---
ipalib/plugins/trust.py | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
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