[Freeipa-devel] [PATCH 0017] Improve error message in ipa-replica-manage

Thu Oct 25 10:40:13 UTC 2012

On 10/24/2012 04:40 AM, Rob Crittenden wrote:
> Tomas Babej wrote:
>> On 10/19/2012 09:55 AM, Petr Viktorin wrote:
>>> On 10/18/2012 08:01 PM, Rob Crittenden wrote:
>>>> Tomas Babej wrote:
>>>>> On 10/02/2012 03:55 PM, Rob Crittenden wrote:
>>>>>> Tomas Babej wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> When executing ipa-replica-manage connect to an unknown or 
>>>>>>> irrelevant
>>>>>>> master, we now print a sensible error message informing the user
>>>>>>> about this possiblity as well.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/3105
>>>>>>>
>>>>>>> Tomas
>>>>>>
>>>>>> I put a whole bunch of code into a try/except and this may be 
>>>>>> catching
>>>>>> errors in unexpected ways.
>>>>>>
>>>>>> I'm not entirely sure right now what we should do, but looking at 
>>>>>> the
>>>>>> code in the try:
>>>>>>
>>>>>> repl1.conn.getEntry(master1_dn, ldap.SCOPE_BASE)
>>>>>> repl1.conn.getEntry(master2_dn, ldap.SCOPE_BASE)
>>>>>>
>>>>>> We take in replica1 and replica1 as arguments (the default for
>>>>>> replica1 is the current host).
>>>>>>
>>>>>> If either of these raise a NotFound it means there there is no 
>>>>>> master
>>>>>> of that name. Does that mean that the master was deleted? Well,
>>>>>> clearly not.
>>>>>>
>>>>>> A lot has changed since I did this, I may have been relying on a
>>>>>> side-effect, or just hadn't tested well-enough.
>>>>>>
>>>>>> I wonder if we need that message at all. Is "foo" is not an IPA 
>>>>>> server
>>>>>> good enough? It still might be confusing if someone didn't know that
>>>>>> "foo" was deleted and it was still running. We could probably verify
>>>>>> that it is at least an IPA server by doing similar checking in the
>>>>>> client, it all depends on how far we want to take it.
>>>>>>
>>>>>> rob
>>>>>
>>>>> I modified the patch. Now if the NotFound error is encountered, we 
>>>>> try
>>>>> to investigate whether we're trying to connect to an IPA server at 
>>>>> all.
>>>>> Please see if you have any suggestions.
>>>>>
>>>>> Tomas
>>>>
>>>> Getting there, the errors are a lot better.
>>>>
>>>> Can you modify the 'Connection unsuccessful' message to include the
>>>> server we failed to connect to?
>>>>
>>>> The logger isn't so nice either, I think I'd prefer it if we could
>>>> exclude the severity:
>>>>
>>>> ipa: ERROR: LDAP Error: Connect error: TLS error -8172:Peer's
>>>> certificate issuer has been marked as not trusted by the user.
>>>> Connection unsuccessful.
>>>>
>>>> So drop the 'ipa: ERROR: ' part for consistency. TI don't believe we
>>>> configure the root logger at all in this tool so it is using the
>>>> defaults.
>>>
>>> It's not very easy to find the right call to configure the logger to
>>> drop the "ipa: ERROR:" part:
>>> standard_logging_setup(console_format='%(message)s')
>>> The function is in ipapython.ipa_log_manager. Hopefully that helps.
>>>
>> Thanks!
>>>> I'd also replace the test for -4 with the constant
>>>> ipadiscovery.NOT_IPA_SERVER
>>>>
>>>> rob
>>>>
>>>
>> I implemented your suggestions. Please have a look at the new patch
>> version.
>>
>> Tomas
>
> Getting a traceback:
>
> # ipa-replica-manage connect otherinstall.example.com
> LDAP Error: Connect error: TLS error -8054:You are attempting to 
> import a cert with the same issuer/serial as an existing cert, but 
> that is not the same cert.
> Traceback (most recent call last):
>   File "/usr/sbin/ipa-replica-manage", line 862, in <module>
>     main()
>   File "/usr/sbin/ipa-replica-manage", line 845, in main
>     add_link(realm, replica1, replica2, dirman_passwd, options)
>   File "/usr/sbin/ipa-replica-manage", line 732, in add_link
>     sys.exit("Connection to %s unsuccessful.", replica2)
> TypeError: exit expected at most 1 arguments, got 2
>
> I was just going to fix this and push and discovered another thing to 
> improve.
>
> Trying to connect to a host not in DNS doesn't return a very nice 
> message. It says that the CA wasn't found, which is true in an offbeat 
> sort of way I guess.
>
> Here is a quick mock-up to see if a host resolves. Note that we can't 
> use ipapython.ipautil.is_host_resolvable() because that only uses DNS.
>
> I whipped this up in 2 seconds just to see how it'd work. The name 
> stinks, it should probably go into ipapython/ipautil and the way I'm 
> checking in add_link would lead to a lot of duplication if tried in 
> the other commands. Probably best to have try to centralize the 
> message and sys.exit().
>
> --- ipa-replica-manage  2012-10-23 22:08:36.426435673 -0400
> +++ /usr/sbin/ipa-replica-manage        2012-10-23 22:34:36.946371449 
> -0400
> @@ -21,6 +21,7 @@
>  import os
>
>  import ldap, re, krbV
> +import socket
>  import traceback
>  from urllib2 import urlparse
>
> @@ -54,6 +55,19 @@
>      "list-clean-ruv":(0, 0, "", ""),
>  }
>
> +def host_exists(host):
> +    """
> +    Resolve the host to see if it exists.
> +
> +    Returns True/False
> +    """
> +    try:
> +        socket.getaddrinfo(host, 80)
> +    except socket.gaierror:
> +        return False
> +    else:
> +        return True
> +
>  def convert_error(exc):
>      """
>      LDAP exceptions are a dictionary, make them prettier.
> @@ -652,6 +666,11 @@
>
>  def add_link(realm, replica1, replica2, dirman_passwd, options):
>
> +    if not host_exists(replica1):
> +        sys.exit("unknown host %s" % replica1)
> +    if not host_exists(replica2):
> +        sys.exit("unknown host %s" % replica2)
> +
>      if options.winsync:
>          if not options.binddn or not options.bindpw or not 
> options.cacert or not options.passsync:
>              root_logger.error("The arguments --binddn, --bindpw, 
> --passsync and --cacert are required to create a winsync agreement")
> @@ -729,7 +748,7 @@
>                      "but it might be unknown, foreign or previously 
> deleted "
>                      "one." % replica2)
>              else:
> -                sys.exit("Connection to %s unsuccessful.", replica2)
> +                sys.exit("Connection to %s unsuccessful." % replica2)
>
>          repl1.setup_gssapi_replication(replica2, DN(('cn', 'Directory 
> Manager')), dirman_passwd)
>      print "Connected '%s' to '%s'" % (replica1, replica2)
>
I added the host_exists function to ipautil.py and added check for
hosts in each relevant function in ipa-replica-manage. I retested
the patch, seems to work as expected.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0017-4-IPA-Server-check-in-ipa-replica-manage.patch
Type: text/x-patch
Size: 6058 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121025/5e106603/attachment.bin>