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

[Jan Cholasta]

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"?

-- 
Jan Cholasta