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

Tue May 26 11:54:17 UTC 2015

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.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0325-12-Add-Domain-Level-feature.patch
Type: text/x-patch
Size: 25229 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150526/6bcc775c/attachment.bin>