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

[Petr Viktorin]

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.

> [root at vm-002 labtool]# ipa-server-install
> [snip]
> Be sure to back up the CA certificate stored in /root/cacert.p12
> This file is required to create replicas. The password for this
> file is the Directory Manager password
>
> [root at vm-002 labtool]# echo $KRB5CCNAME
>
> [root at vm-002 labtool]#
>>
>>
>> Two nitpicks:
>>
>> ipaserver/install/installutils.py: new blank line at EOF
>>
>> For most context managers I prefer @contextlib.contextmanager rather
>> than writing out the class, it makes them shorter and easier to read
>
> Addressed in the updated patch.
>
> Tomas


-- 
Petr³