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

[Tomas Babej]

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
>> [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
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0065-4-Use-private-ccache-in-ipa-server-install.patch
Type: text/x-patch
Size: 6604 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130605/52049990/attachment.bin>