[Freeipa-devel] [PATCH 0065] Use private ccache in ipa-server-install

[Petr Viktorin]

On 06/05/2013 10:47 AM, Tomas Babej wrote:
> On 06/05/2013 10:07 AM, Petr Viktorin wrote:
>> On 06/05/2013 09:20 AM, Tomas Babej wrote:
>>> On 06/04/2013 06:09 PM, Petr Viktorin wrote:
>>>> On 06/04/2013 01:29 PM, Tomas Babej wrote:
>>>>> On 06/03/2013 02:58 PM, Martin Kosek wrote:
>>>>>> On 06/03/2013 02:43 PM, Tomas Babej wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> this patch fixes the installation problems on master on F19 with
>>>>>>> krb5
>>>>>>> packages
>>>>>>>> = 1.11.2-6
>>>>>>> https://fedorahosted.org/freeipa/ticket/3666
>>>>>>>
>>>>>>> Tomas
>>>>>> 1) Leaving cache_desc open:
>>>>>>
>>>>>> +        (cache_desc, cache_path) = tempfile.mkstemp(prefix='krbcc')
>>>>>> +        os.environ['KRB5CCNAME'] = cache_path
>>>>>>
>>>>>> Why do we keep the descriptor open and close it at the and of the
>>>>>> installation?
>>>>>> Can we close it right after tempfile.mkstemp? I think we do it this
>>>>>> way in
>>>>>> other places in installation.
>>>>>>
>>>>>> 2) What about other installers where we handle Kerberos auth, like
>>>>>> ipa-{replica,dns,ca}-install?
>>>>>>
>>>>>> A common function, other shared means, of handling KRB5CCNAME may be
>>>>>> appropriate to avoid duplicating code too much.
>>>>>>
>>>>>> Martin
>>>>> I moved the code responsible to PrivateCCache class, both for
>>>>> readability and conciseness.
>>>>>
>>>>> Private ccache now used in replica,dns and ca the installers. I
>>>>> managed
>>>>> to reproduce the error only with
>>>>> dns-install though(fails on adding the service principal), but
>>>>> having a
>>>>> private ccache for the installer should not hurt.
>>>>>
>>>>> Ipa-adtrust-install requires the admin ticket, so there shouldn't
>>>>> be an
>>>>> issue.
>>>>>
>>>>> Tomas
>>>>
>>>> Shouldn't the context manager restore os.environ['KRB5CCNAME'] on exit?
>>>
>>> There's no need to, since the value of the environment variable is
>>> inherited only into child processes (pkispawn, etc.). Hence the
>>> KRB5CCNAME
>>> environment variable is not available outside the install script.
>>
>> Yes, but what if you call a child process after the context manager
>> exits?
>> I know that currently there is no code after the context manager
>> exits, but that's no reason to surprise people who will want to reuse
>> it later.
>>
>> Context managers are expected to clean up after themselves. If there's
>> no way to do this then you should at least document that this one is
>> special. But in this case it shouldn't be that hard to clean up.
>>
> Not at all, I actually had the code there at some point, but removed it.
>
> Updated patch attached.
>
> Tomas


Thanks. ACK, pushed to master, ipa-3-2.

master: 6f51f92138ff12eff732bf028751dcfa8ef9b442
ipa-3-2: 4ec1de1a65f1fabe7f5b26b4c4487deec5cea0cf

-- 
Petr³