[Freeipa-devel] [PATCH 0325] Add Domain Level feature

Tue May 26 12:00:24 UTC 2015

Dne 26.5.2015 v 13:54 Tomas Babej napsal(a):
>
>
> On 05/26/2015 01:51 PM, Tomas Babej wrote:
>>
>>
>> On 05/26/2015 12:39 PM, Tomas Babej wrote:
>>>
>>>
>>> On 05/26/2015 11:57 AM, Jan Cholasta wrote:
>>>> Dne 25.5.2015 v 17:15 Tomas Babej napsal(a):
>>>>>
>>>>>
>>>>> On 05/25/2015 12:42 PM, Tomas Babej wrote:
>>>>>>
>>>>>>
>>>>>> On 05/25/2015 07:30 AM, Jan Cholasta wrote:
>>>>>>> Dne 22.5.2015 v 12:36 Petr Vobornik napsal(a):
>>>>>>>> On 05/22/2015 07:08 AM, Jan Cholasta wrote:
>>>>>>>>> Dne 21.5.2015 v 18:18 Tomas Babej napsal(a):
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 05/19/2015 04:07 PM, Tomas Babej wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 05/19/2015 03:59 PM, Martin Kosek wrote:
>>>>>>>>>>>> On 05/19/2015 03:56 PM, Tomas Babej wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 05/19/2015 03:51 PM, Martin Kosek wrote:
>>>>>>>>>>>>>> On 05/19/2015 03:49 PM, Ludwig Krispenz wrote:
>>>>>>>>>>>>>>> On 05/19/2015 03:36 PM, Martin Kosek wrote:
>>>>>>>>>>>>>>>> On 05/19/2015 03:22 PM, Tomas Babej wrote:
>>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>>>> 3) Domain level is just a single integer and it should be
>>>>>>>>>>>>>>>>>> treated as such,
>>>>>>>>>>>>>>>>>> there's no need for an LDAPObject plugin and other
>>>>>>>>>>>>>>>>>> unnecessary
>>>>>>>>>>>>>>>>>> complexities.
>>>>>>>>>>>>>>>>>> The implemetation could be as simple as (from top of my
>>>>>>>>>>>>>>>>>> head,
>>>>>>>>>>>>>>>>>> untested):
>>>>>>>>>>>>>>>>> That's right, I also considered this approach, but as far
>>>>>>>>>>>>>>>>> as I
>>>>>>>>>>>>>>>>> know you do
>>>>>>>>>>>>>>>>> not
>>>>>>>>>>>>>>>>> get the permission handling for the global DomainLevel entry
>>>>>>>>>>>>>>>>> otherwise.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Ludwig, I changed the path for the global entry to
>>>>>>>>>>>>>>>>> cn=DomainLevel.
>>>>>>>>>>>>>>>> I know this particular DN was added to the design by Simo, but
>>>>>>>>>>>>>>>> why do we want
>>>>>>>>>>>>>>>> to use CamelCase with LDAP object?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Wouldn't "cn=Domain Level,cn=ipa,cn=etc,SUFFIX" be a better
>>>>>>>>>>>>>>>> place
>>>>>>>>>>>>>>>> for it? This
>>>>>>>>>>>>>>>> is the last time we can change it, so I am asking now.
>>>>>>>>>>>>>>>> Then, we
>>>>>>>>>>>>>>>> will be stuck
>>>>>>>>>>>>>>>> with this DN forever.
>>>>>>>>>>>>>>> I don't mind using ""cn=Domain Level" ,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> but where does the entry live, here you say
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> cn=Domain Level,cn=ipa,cn=etc,SUFFIX"
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> and in the design page it is:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> cn=DomainLevel,cn=etc,SUFFIX
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The current version of the topology plugin is looking for
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> cn=DomainLevel,cn=ipa,cn=etc,SUFFIX"
>>>>>>>>>>>>>>> but I want to change it to do a search on
>>>>>>>>>>>>>>> objectclass=ipaDomainLevelConfig
>>>>>>>>>>>>>> I see - we all need to unify the location apparently. I
>>>>>>>>>>>>>> updated the
>>>>>>>>>>>>>> design page
>>>>>>>>>>>>>> to use "cn=Domain Level,cn=ipa,cn=etc,SUFFIX". Tomas, please
>>>>>>>>>>>>>> send
>>>>>>>>>>>>>> the updated
>>>>>>>>>>>>>> patch set, it should be an extremely simple change :-)
>>>>>>>>>>>>> I prefer the ipa parent and the space in the name, so I'm glad we
>>>>>>>>>>>>> could agree
>>>>>>>>>>>>> on this without much bikeshedding.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Updated patch attaced.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Tomas
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> I still see
>>>>>>>>>>>>
>>>>>>>>>>>> +# Create default Domain Level entry if it does not exist
>>>>>>>>>>>> +dn: cn=DomainLevel,cn=ipa,cn=etc,$SUFFIX
>>>>>>>>>>>> +default: objectClass: top
>>>>>>>>>>>> +default: objectClass: nsContainer
>>>>>>>>>>>> +default: objectClass: ipaDomainLevelConfig
>>>>>>>>>>>> +default: ipaDomainLevel: 0
>>>>>>>>>>>>
>>>>>>>>>>>> ...
>>>>>>>>>>>
>>>>>>>>>>> Right, the space eluded me there, thanks for the catch.
>>>>>>>>>>>
>>>>>>>>>>> Tomas
>>>>>>>>>>
>>>>>>>>>> A new iteration of the patch, including the server-side checks
>>>>>>>>>> for the
>>>>>>>>>> installers.
>>>>>>>>>>
>>>>>>>>>> Tomas
>>>>>>>>>
>>>>>>>>> 1)
>>>>>>>>> https://www.redhat.com/archives/freeipa-devel/2015-May/msg00228.html
>>>>>>>>> - I still don't agree that the plugin should be based on LDAPObject.
>>>>>>>>
>>>>>>>> On the other hand, with LDAPObject base, Web UI for this feature is
>>>>>>>> much
>>>>>>>> more simpler because it can rely on existing conventions.
>>>>>>>
>>>>>>> Following this logic, should *everything* be based on LDAPObject,
>>>>>>> because it would satisfy the convetion? I don't think so. The convetion
>>>>>>> should not apply here, because domain level is conceptually *not* an
>>>>>>> object, it is a property. IMHO having a clean API should be preferred
>>>>>>> over implementation convenience.
>>>>>>>
>>>>>>
>>>>>> I do not have strong opinions over this. Attached version implements
>>>>>> a lightweight approach to the domainlevel related commands.
>>>>>>
>>>>>> Tomas
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Fixes a slight schema glitch.
>>>>>
>>>>
>>>> Thanks for the patch!
>>>>
>>>> 1)
>>>>
>>>> +            # Detect the current domain level
>>>> +            try:
>>>> +                current = remote_api.Command['domainlevel_show']['result']
>>>> +            except KeyError:
>>>> +                # If we're joining an older master, domainlevel_show is
>>>> not
>>>> +                # available
>>>> +                current = 0
>>>>
>>>> KeyError? That does not look right. remote_api differs from api only in
>>>> that it sets up ldap2 to connect to the remote server, but it uses local
>>>> plugins and everything, so domainlevel_show should always be available.
>>>>
>>>>
>>>> 2) Could you also set supported domain levels in
>>>> install/share/master-entry.ldif? I think it makes sense to have them
>>>> there right from the beginning of server install.
>>>>
>>>>
>>>> 3) I think you should use the per-plugin api object instead of
>>>> ipalib.api when constructing DNs (domainlevel_dn, container_masters).
>>>>
>>>>
>>>> 4) I would say the opposite of "domainlevel-set" should be
>>>> "domainlevel-get", not "domainlevel-show". IMO it's OK since property
>>>> commands are an uncharted territory and don't have to (maybe even
>>>> shouldn't) use the same convention as objects.
>>>>
>>>>
>>>> 5)
>>>>
>>>> +    'System: Read Domain Level': {
>>>> +        'ipapermlocation': DN('cn=masters,cn=ipa,cn=etc', api.env.basedn),
>>>> +        'ipapermtargetfilter': {'(objectclass=ipadomainlevelconfig)'},
>>>> +        'ipapermbindruletype': 'all',
>>>> +        'ipapermright': {'read', 'search', 'compare'},
>>>> +        'ipapermdefaultattr': {
>>>> +            'ipadomainlevel', 'objectclass',
>>>> +        },
>>>> +    },
>>>>
>>>> Shouldn't ipapermlocation say "cn=Domain Level,cn=ipa,cn=etc"?
>>>>
>>>
>>> Thanks for the review, I fixed all the issues raised.
>>>
>>> Tomas
>>>
>>
>> Added a small fix for replca install, to avoid duplicated creation of
>> the domainlevel entry.
>>
>> Tomas
>>
>
> Aand the correct patch.

Thanks, ACK.

Pushed to master: f3010498af2a4b98512d219b8e09101176c172fe

-- 
Jan Cholasta