[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Jan Cholasta]

Dne 16.3.2015 v 13:30 Martin Babinsky napsal(a):
> On 03/16/2015 12:15 PM, Martin Kosek wrote:
>> On 03/13/2015 05:37 PM, Martin Babinsky wrote:
>>> Attaching the next iteration of patches.
>>>
>>> I have tried my best to reword the ipa-client-install man page bit
>>> about the
>>> new option. Any suggestions to further improve it are welcome.
>>>
>>> I have also slightly modified the 'kinit_keytab' function so that in
>>> Kerberos
>>> errors are reported for each attempt and the text of the last error
>>> is retained
>>> when finally raising exception.
>>
>> The approach looks very good. I think that my only concern with this
>> patch is
>> this part:
>>
>> +            ccache.init_creds_keytab(keytab=ktab, principal=princ)
>> ...
>> +        except krbV.Krb5Error as e:
>> +            last_exc = str(e)
>> +            root_logger.debug("Attempt %d/%d: failed: %s"
>> +                              % (attempt, attempts, last_exc))
>> +            time.sleep(1)
>> +
>> +    root_logger.debug("Maximum number of attempts (%d) reached"
>> +                      % attempts)
>> +    raise StandardError("Error initializing principal %s: %s"
>> +                        % (principal, last_exc))
>>
>> The problem here is that this function will raise the super-generic
>> StandardError instead of the proper with all the context and
>> information about
>> the error that the caller can then process.
>>
>> I think that
>>
>>      except krbV.Krb5Error as e:
>>          if attempt == max_attempts:
>>              log something
>>              raise
>>
>> would be better.
>>
>
> Yes that seems reasonable. I'm just thinking whether we should re-raise
> Krb5Error or raise ipalib.errors.KerberosError? the latter options makes
> more sense to me as we would not have to additionally import Krb5Error
> everywhere and it would also make the resulting errors more consistent.
>
> I am thinking about someting like this:
>
>      except krbV.Krb5Error as e:
>         if attempt == attempts:
>             # log that we have reaches maximum number of attempts
>             raise KerberosError(minor=str(e))
>
> What do you think?
>

NACK, don't use ipalib from ipapython in new code, we are trying to get 
rid of this circular dependency. Krb5Error is OK in this case.

-- 
Jan Cholasta