On 01/13/2015 04:48 PM, Martin Babinsky wrote:
On 01/13/2015 09:46 AM, Jan Cholasta wrote:
Dne 13.1.2015 v 09:22 Martin Kosek napsal(a):
On 01/12/2015 05:45 PM, Martin Babinsky wrote:
related to ticket https://fedorahosted.org/freeipa/ticket/4808

Patch attached.

Martin^3

I think the --tgt-kinit-attempts approach is good one. Couple comments
I have
when reading the patch:

1) Function
+def get_host_tgt(options, keytab, host, realm, env):
should be made more general purpose and instead of whole "options", it
should
rather accept just "kinit_attemps". It will then enable future
generations to
reuse the function for something else. Just a generally good practice,
nothing
critical.

2) I think
+        if returncode == 0:
+            root_logger.info("Attempt %d succeeded." % n_attempts)
+            break

can be just DEBUG level. People do not need to know we will try
multiple attempts.

3) It may be even better to print
"Attempt %d/%d failed." instead of just number. But this is up to you.

4) I see several C-isms in the code, as a programming practice, let us
remove
them :-) In Python, the OK/notOK status is generally passed via
exceptions, not
return codes unless you really need them for anything meaningful.

So, you can omit "raiseonerr=False" and have the handling code in an
Except
clause. When max number of attempts is breached, you then just raise the
exception further (use bare "raise", to re-raise to keep the original
stack).

+1

Additionally:

5) I would prefer if the option was named --kinit-attempts instead of
--tgt-kinit-attempts (the "tgt" seems redundant).

6) Please do not use backslashes for line wrapping, unless it is
absolutely necessary. Instead, enclose the expression in parens for
implicit continuation:

+                           help=("number of attempts to obtain host TGT"
+                                 "if the first one fails (defaults to
%default)."))

7) Please follow PEP8 in new code:

ipa-client/ipa-install/ipa-client-install:151:80: E501 line too long (93
 > 79 characters)
ipa-client/ipa-install/ipa-client-install:1100:1: E302 expected 2 blank
lines, found 1
ipa-client/ipa-install/ipa-client-install:1107:29: E126 continuation
line over-indented for hanging indent
ipa-client/ipa-install/ipa-client-install:1107:41: E231 missing
whitespace after ','
ipa-client/ipa-install/ipa-client-install:1108:29: E128 continuation
line under-indented for visual indent
ipa-client/ipa-install/ipa-client-install:1116:51: E225 missing
whitespace around operator
ipa-client/ipa-install/ipa-client-install:2453:80: E501 line too long
(88 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2454:80: E501 line too long
(89 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2531:80: E501 line too long
(83 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2532:80: E501 line too long
(81 > 79 characters)

Honza

Thank you for your comments. Attaching the updated patch (I have sent
the message much earlier, but only to Jan because I messed up the reply
addresses in Thunderbird. Sorry for that).

Martin


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Attaching updated patch.

https://fedorahosted.org/freeipa/ticket/4808

Martin^3

From a6ade537190215accaaf1bc534d25383ca7cc787 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 9 Jan 2015 17:38:39 +0100
Subject: [PATCH] ipa-client-install: added new option '--kinit-attempts'.

The option specifies a total number of attempts to obtain TGT from KDC before giving up and aborting installation.

https://fedorahosted.org/freeipa/ticket/4808

---
 ipa-client/ipa-install/ipa-client-install | 57 +++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index dfe0e3b7597c2ea63c299969b3a9d76cf8ecc273..55cb7e8356a441d9032f883e98b6cad059b2f1bf 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -91,6 +91,13 @@ def parse_options():
 
         parser.values.ca_cert_file = value
 
+    def validate_kinit_attempts_option(option, opt, value, parser):
+        if value < 1 or value > sys.maxint:
+            raise OptionValueError(
+                "%s option has invalid value %d" % (opt, value))
+
+        parser.values.kinit_attempts = value
+
     parser = IPAOptionParser(version=version.VERSION)
 
     basic_group = OptionGroup(parser, "basic options")
@@ -144,6 +151,11 @@ def parse_options():
                       help="do not modify the nsswitch.conf and PAM configuration")
     basic_group.add_option("-f", "--force", dest="force", action="store_true",
                       default=False, help="force setting of LDAP/Kerberos conf")
+    basic_group.add_option('--kinit-attempts', dest='kinit_attempts',
+                           action='callback', type='int', default=5,
+                           callback=validate_kinit_attempts_option,
+                           help=("number of attempts to obtain host TGT"
+                                 " (defaults to %default)."))
     basic_group.add_option("-d", "--debug", dest="debug", action="store_true",
                       default=False, help="print debugging information")
     basic_group.add_option("-U", "--unattended", dest="unattended",
@@ -1089,6 +1101,31 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
 
     return 0
 
+
+def get_host_tgt(host, realm, keytab, kinit_attempts, env):
+    n_attempts = 0
+    max_attempts = kinit_attempts
+    root_logger.info("Attempting to get host TGT...")
+
+    while True:
+        n_attempts += 1
+        try:
+            run([
+                paths.KINIT, '-k', '-t', keytab,
+                'host/%s@%s' % (host, realm)
+                ], env=env)
+        except CalledProcessError:
+            root_logger.debug(
+                "Attempt %d/%d failed." % (n_attempts, max_attempts))
+            if n_attempts >= max_attempts:
+                raise
+            time.sleep(1)
+        else:
+            root_logger.debug(
+                "Attempt %d/%d succeeded." % (n_attempts, max_attempts))
+            return
+
+
 def configure_certmonger(fstore, subject_base, cli_realm, hostname, options,
                          ca_enabled):
     if not options.request_cert:
@@ -2421,17 +2458,13 @@ def install(options, env, fstore, statestore):
             elif options.keytab:
                 join_args.append("-f")
                 if os.path.exists(options.keytab):
-                    (stderr, stdout, returncode) = run(
-                        [paths.KINIT,'-k', '-t', options.keytab,
-                            'host/%s@%s' % (hostname, cli_realm)],
-                        env=env,
-                        raiseonerr=False)
-
-                    if returncode != 0:
+                    try:
+                        get_host_tgt(hostname, cli_realm, options.keytab,
+                                     options.kinit_attempts, env)
+                    except CalledProcessError:
                         print_port_conf_info()
                         root_logger.error("Kerberos authentication failed "
                                           "using keytab: %s", options.keytab)
-                        root_logger.info("%s", stdout)
                         return CLIENT_INSTALL_ERROR
                 else:
                     root_logger.error("Keytab file could not be found: %s"
@@ -2502,10 +2535,12 @@ def install(options, env, fstore, statestore):
             # Other KDCs might not have replicated the principal yet.
             # Once we have the TGT, it's usable on any server.
             env['KRB5CCNAME'] = os.environ['KRB5CCNAME'] = CCACHE_FILE
+
             try:
-                run([paths.KINIT, '-k', '-t', paths.KRB5_KEYTAB,
-                        'host/%s@%s' % (hostname, cli_realm)], env=env)
-            except CalledProcessError, e:
+                get_host_tgt(hostname, cli_realm, paths.KRB5_KEYTAB,
+                             options.kinit_attempts, env)
+            except CalledProcessError:
+                print_port_conf_info()
                 root_logger.error("Failed to obtain host TGT.")
                 # failure to get ticket makes it impossible to login and bind
                 # from sssd to LDAP, abort installation and rollback changes
-- 
2.1.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to