Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-03 Thread Rob Crittenden
Martin Babinsky wrote:
 On 03/02/2015 04:28 PM, Rob Crittenden wrote:
 Petr Vobornik wrote:
 On 01/12/2015 05:45 PM, Martin Babinsky wrote:
 related to ticket https://fedorahosted.org/freeipa/ticket/4808

 this patch seems to be a bit forgotten.

 It works, looks fine.

 One minor issue: trailing whitespaces in the man page.

 I also wonder if it shouldn't be used in other tools which call kinit
 with keytab:
 * ipa-client-automount:434
 * ipa-client-install:2591 (this usage should be fine since it's used for
 server installation)
 * dcerpc.py:545
 * rpcserver.py: 971, 981 (armor for web ui forms base auth)

 Most importantly the ipa-client-automount because it's called from
 ipa-client-install (if location is specified) and therefore it might
 fail during client installation.

 Or also, kinit call with admin creadentials worked for the user but I
 wonder if it was just a coincidence and may break under slightly
 different but similar conditions.

 I think that's a fine idea. In fact there is already a function that
 could be extended, kinit_hostprincipal().

 rob

 
 So in principle we could add multiple TGT retries to
 kinit_hostprincipal() and then plug this function to all the places
 Petr mentioned in order to provide this functionality each time TGT is
 requested using keytab.
 
 Do I understand it correctly?
 

Honestly I think I'd only do the retries on client installation.  I
don't know that the other uses would really benefit or need this.

But this is an opportunity to consolidate some code, so I guess the
approach I'd take is to add an option to kinit_hostprincipal of
retries=0 so that only a single kinit is done. The client installers
would pass in some value.

This change is quite a bit more invasive but it's also early in the
release cycle so the risk will be spread out.

rob

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-03 Thread Martin Babinsky

On 03/03/2015 02:32 PM, Rob Crittenden wrote:

Martin Babinsky wrote:

On 03/02/2015 04:28 PM, Rob Crittenden wrote:

Petr Vobornik wrote:

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

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


this patch seems to be a bit forgotten.

It works, looks fine.

One minor issue: trailing whitespaces in the man page.

I also wonder if it shouldn't be used in other tools which call kinit
with keytab:
* ipa-client-automount:434
* ipa-client-install:2591 (this usage should be fine since it's used for
server installation)
* dcerpc.py:545
* rpcserver.py: 971, 981 (armor for web ui forms base auth)

Most importantly the ipa-client-automount because it's called from
ipa-client-install (if location is specified) and therefore it might
fail during client installation.

Or also, kinit call with admin creadentials worked for the user but I
wonder if it was just a coincidence and may break under slightly
different but similar conditions.


I think that's a fine idea. In fact there is already a function that
could be extended, kinit_hostprincipal().

rob



So in principle we could add multiple TGT retries to
kinit_hostprincipal() and then plug this function to all the places
Petr mentioned in order to provide this functionality each time TGT is
requested using keytab.

Do I understand it correctly?



Honestly I think I'd only do the retries on client installation.  I
don't know that the other uses would really benefit or need this.

But this is an opportunity to consolidate some code, so I guess the
approach I'd take is to add an option to kinit_hostprincipal of
retries=0 so that only a single kinit is done. The client installers
would pass in some value.

This change is quite a bit more invasive but it's also early in the
release cycle so the risk will be spread out.

rob

Ok I will try to implement these changes and submit them as separate 
patchset.


--
Martin^3 Babinsky

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-02 Thread Martin Babinsky

On 03/02/2015 04:28 PM, Rob Crittenden wrote:

Petr Vobornik wrote:

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

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


this patch seems to be a bit forgotten.

It works, looks fine.

One minor issue: trailing whitespaces in the man page.

I also wonder if it shouldn't be used in other tools which call kinit
with keytab:
* ipa-client-automount:434
* ipa-client-install:2591 (this usage should be fine since it's used for
server installation)
* dcerpc.py:545
* rpcserver.py: 971, 981 (armor for web ui forms base auth)

Most importantly the ipa-client-automount because it's called from
ipa-client-install (if location is specified) and therefore it might
fail during client installation.

Or also, kinit call with admin creadentials worked for the user but I
wonder if it was just a coincidence and may break under slightly
different but similar conditions.


I think that's a fine idea. In fact there is already a function that
could be extended, kinit_hostprincipal().

rob



So in principle we could add multiple TGT retries to 
kinit_hostprincipal() and then plug this function to all the places 
Petr mentioned in order to provide this functionality each time TGT is 
requested using keytab.


Do I understand it correctly?

--
Martin^3 Babinsky

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-02 Thread Petr Vobornik

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

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


this patch seems to be a bit forgotten.

It works, looks fine.

One minor issue: trailing whitespaces in the man page.

I also wonder if it shouldn't be used in other tools which call kinit 
with keytab:

* ipa-client-automount:434
* ipa-client-install:2591 (this usage should be fine since it's used for 
server installation)

* dcerpc.py:545
* rpcserver.py: 971, 981 (armor for web ui forms base auth)

Most importantly the ipa-client-automount because it's called from 
ipa-client-install (if location is specified) and therefore it might 
fail during client installation.


Or also, kinit call with admin creadentials worked for the user but I 
wonder if it was just a coincidence and may break under slightly 
different but similar conditions.

--
Petr Vobornik

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-02 Thread Rob Crittenden
Petr Vobornik wrote:
 On 01/12/2015 05:45 PM, Martin Babinsky wrote:
 related to ticket https://fedorahosted.org/freeipa/ticket/4808
 
 this patch seems to be a bit forgotten.
 
 It works, looks fine.
 
 One minor issue: trailing whitespaces in the man page.
 
 I also wonder if it shouldn't be used in other tools which call kinit
 with keytab:
 * ipa-client-automount:434
 * ipa-client-install:2591 (this usage should be fine since it's used for
 server installation)
 * dcerpc.py:545
 * rpcserver.py: 971, 981 (armor for web ui forms base auth)
 
 Most importantly the ipa-client-automount because it's called from
 ipa-client-install (if location is specified) and therefore it might
 fail during client installation.
 
 Or also, kinit call with admin creadentials worked for the user but I
 wonder if it was just a coincidence and may break under slightly
 different but similar conditions.

I think that's a fine idea. In fact there is already a function that
could be extended, kinit_hostprincipal().

rob

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-03-02 Thread Alexander Bokovoy

On Mon, 02 Mar 2015, Petr Vobornik wrote:

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

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


this patch seems to be a bit forgotten.

It works, looks fine.

One minor issue: trailing whitespaces in the man page.

I also wonder if it shouldn't be used in other tools which call kinit 
with keytab:

* ipa-client-automount:434
* ipa-client-install:2591 (this usage should be fine since it's used 
for server installation)

* dcerpc.py:545
* rpcserver.py: 971, 981 (armor for web ui forms base auth)

Most importantly the ipa-client-automount because it's called from 
ipa-client-install (if location is specified) and therefore it might 
fail during client installation.


Or also, kinit call with admin creadentials worked for the user but I 
wonder if it was just a coincidence and may break under slightly 
different but similar conditions.

dcerpc.py use is special, don't change anything there please.
--
/ Alexander Bokovoy

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-02-13 Thread Martin Babinsky

On 01/14/2015 05:52 PM, Martin Babinsky wrote:

On 01/14/2015 04:53 PM, Martin Babinsky wrote:

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



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



Forgot to update 'ipa-client-install' man page. The updated patch fixes
this. Thanks to Rob for pointing it out.

Martin^3


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



Anyone to review this patch?

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

--
Martin^3 Babinsky
From e7fc3c31840d5b14c29a027b418704a024d5db1d 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.

---
 ipa-client/ipa-install/ipa-client-install | 57 +--
 ipa-client/man/ipa-client-install.1   |  5 +++
 2 files changed, 51 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 

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-14 Thread Martin Babinsky

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', 

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-14 Thread Martin Babinsky

On 01/14/2015 04:53 PM, Martin Babinsky wrote:

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



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



Forgot to update 'ipa-client-install' man page. The updated patch fixes 
this. Thanks to Rob for pointing it out.


Martin^3
From e7fc3c31840d5b14c29a027b418704a024d5db1d 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.

---
 ipa-client/ipa-install/ipa-client-install | 57 +--
 ipa-client/man/ipa-client-install.1   |  5 +++
 2 files changed, 51 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():
   

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-13 Thread Martin Kosek
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).

Martin

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-13 Thread Jan Cholasta

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

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-13 Thread Martin Babinsky

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 += 

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-13 Thread Rob Crittenden
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
 

I'd recommend a min/max validator too. Min 1 because 0 makes no sense,
and max, I dunno, MAXINT if nothing else. Otherwise guaranteed to get
kicked back by QE at some point.

rob

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


Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-13 Thread Martin Babinsky

On 01/13/2015 04:03 PM, Rob Crittenden wrote:

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



I'd recommend a min/max validator too. Min 1 because 0 makes no sense,
and max, I dunno, MAXINT if nothing else. Otherwise guaranteed to get
kicked back by QE at some point.

rob

Yes I was also thinking about that. The code kinda works even with 
negative values (makes at least one pass of TGT kinit), but I guess 
that's really not how it should behave. I will update the patch accordingly.


There is also a question of semantics: should this option mean Perform 
N additional attempts to get TGT if the first one fails (this is the 
current implementation and N=0 then means no additional attempts except 
the first one), or should the meaning be Perform N TGT attempts in 
total (then N=0 would mean do not do any TGT kinit calls and thus 
makes no sense)?


I hope I made the distinction clear enough.

Martin^3

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


[Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

2015-01-12 Thread Martin Babinsky

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

Patch attached.

Martin^3
From 5988842868303d6a9feffeb2ec5ce873b42876e0 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 '--tgt-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 | 41 +++
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index dfe0e3b7597c2ea63c299969b3a9d76cf8ecc273..b0070883edaa7c94fb31265bcce90a8a851d226f 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -144,6 +144,10 @@ 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('--tgt-kinit-attempts', dest='tgt_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 +1093,27 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
 
 return 0
 
+def get_host_tgt(options, keytab, host, realm, env):
+n_attempts = 0
+root_logger.info(Attempting to get host TGT...)
+
+while n_attempts = options.tgt_kinit_attempts:
+n_attempts += 1
+(stderr, stdout, returncode) = run(
+[paths.KINIT,'-k', '-t', keytab,
+'host/%s@%s' % (host, realm)],
+env=env,
+raiseonerr=False)
+
+if returncode == 0:
+root_logger.info(Attempt %d succeeded. % n_attempts)
+break
+
+root_logger.warning(Attempt %d failed. %n_attempts)
+time.sleep(1)
+
+return (stderr, stdout, returncode)
+
 def configure_certmonger(fstore, subject_base, cli_realm, hostname, options,
  ca_enabled):
 if not options.request_cert:
@@ -2421,12 +2446,8 @@ 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)
-
+(stderr, stdout, returncode) = get_host_tgt(options, options.keytab,
+hostname, cli_realm, env)
 if returncode != 0:
 print_port_conf_info()
 root_logger.error(Kerberos authentication failed 
@@ -2502,10 +2523,10 @@ 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:
+
+(stderr, stdout, returncode) = get_host_tgt(options, paths.KRB5_KEYTAB,
+hostname, cli_realm, env)
+if returncode != 0:
 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