[Freeipa-devel] [PATCH] 377 Using LDAPI to setup CA and KRA agents.

[Jan Cholasta]

On 4.9.2015 16:53, Petr Vobornik wrote:
> On 09/04/2015 04:03 PM, Endi Sukma Dewata wrote:
>> On 9/4/2015 6:35 AM, Martin Basti wrote:
>>>
>>>
>>> On 09/02/2015 06:42 AM, Endi Sukma Dewata wrote:
>>>> On 9/1/2015 1:52 AM, Martin Basti wrote:
>>>>>>>>> The CA and KRA installation code has been modified to use LDAPI
>>>>>>>>> to create the CA and KRA agents directly in the CA and KRA
>>>>>>>>> database. This way it's no longer necessary to use the Directory
>>>>>>>>> Manager password or CA and KRA admin certificate.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5257
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thank you.
>>>>>>>>
>>>>>>>> 1) Can you use following code instead of direct call of
>>>>>>>> ldap2.ldap2()?
>>>>>>>>
>>>>>>>> if not api.Backend.ldap2.is_connected():
>>>>>>>>      api.Backend.ldap2.connect(autobind=True)
>>>>>>>>
>>>>>>>> conn = api.Backend.ldap2
>>>>>>
>>>>>> Why would you want to do that? The original code is fine, except the
>>>>>> connection check is not necessary (it is a new instance of ldap2, so
>>>>>> .isconnected() will always return False).
>>>>>>
>>>>>>>
>>>>>>> It's actually isconnected() instead of is_connected(), but even so,
>>>>>>> the
>>>>>>> proposed code doesn't work:
>>>>>>>
>>>>>>> ipa.ipapython.install.cli.install_tool(Server): DEBUG    The
>>>>>>> ipa-server-install command failed, exception: TypeError: 'ldap2'
>>>>>>> object
>>>>>>> is not callable
>>>>>>> ipa.ipapython.install.cli.install_tool(Server): ERROR 'ldap2' object
>>>>>>> is not callable
>>>>>>>
>>>>>>>> 2) Patch needs rebase to master branch.
>>>>>>>
>>>>>>> The original patch does apply cleanly to master. Did you see a
>>>>>>> conflict?
>>>>> Sorry my bad.
>>>>>
>>>>> Martin^2
>>>>>>>
>>>>>>>> 3)
>>>>>>>> +        user_dn = DN(('uid', "ipara"), ('ou', 'People'),
>>>>>>>> self.basedn)
>>>>>>>> +        conn.create(
>>>>>>>> +            dn=user_dn,
>>>>>>>>
>>>>>>>> can you use add entry() instead of create()? We don't use native
>>>>>>>> python-ldap, but rather ipaldap methods
>>>>>>>
>>>>>>> It's actually calling the ldap2.create() defined in
>>>>>>> ipaserver/plugins/ldap2.py, which calls add_entry().
>>>>>>
>>>>>> NACK. We don't use ldap2.create(). Use add_entry().
>>>>>>
>>>>>>>
>>>>>>> So my original patch still stands.
>>>>
>>>> New patch attached.
>>>>
>>> ACK, but IMO that comments is not necessary and I would like to push the
>>> patch without it.
>>>
>>> Martin^2
>>
>> It is necessary if we don't want people to use it. Otherwise someone
>> could make the same mistake. Or better yet, just remove the method.
>>
>
> +
> +        NOTE: Do not use this method.
>
> I agree that the comment should not be in this patch - it is not
> relevant to vaults.
>
> The comment or a removal of the method(if it is really useless) should
> be in a different patch. If comment is the way than please also add why
> it should not be used.

The method was intended to be used with frontend objects, but they never 
happened in IPA, so it was left unused (instead we have no clean 
interface between frontend and backend and call backend-specific methods 
ad-hoc, what a great design /sarcasm). I personally would like to revive 
the concept, so I would not remove the methods. I don't think a comment 
is necessary either, because up until now, nobody tried to use the method.

-- 
Jan Cholasta