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

Tue May 26 10:39:02 UTC 2015

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0325-10-Add-Domain-Level-feature.patch
Type: text/x-patch
Size: 25183 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150526/1ffe941f/attachment.bin>