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
From 83d4a8586d58d84ff5988b738311fd48fa0da900 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 enables the host to make multiple attempts to obtain TGT from KDC
before giving up and aborting client installation.
https://fedorahosted.org/freeipa/ticket/4808
---
ipa-client/ipa-install/ipa-client-install | 49 ++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index dfe0e3b7597c2ea63c299969b3a9d76cf8ecc273..35e305617d18747875bd8abf4855ad7880f3f1ff 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -144,6 +144,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='store', type='int', default=4,
+ help=("number of attempts to obtain host TGT"
+ " if the first one fails"
+ " (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 +1094,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 + 1
+ 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 +2451,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 +2528,11 @@ 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:
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
