Hi, I started working on https://fedorahosted.org/sssd/ticket/1784 and I'd like to get some comments so I know I'm heading the right direction.
*Current state:* There is one tevent request per a nesting level. The request continues with many "step" and other-named functions. The path is different for deref and noderef processing, but the two branches intersect many times. It uses a lot of recursion. It often translates return codes.
All of this make it hard to understand the process, hard to maintain and especially it is hard to step out of the process at chosen point.
*New approach:* There will be several tevent request per nesting level: main-req (from sdap_nested_group_send) | nesting-level-req / \ noderef-req deref-req | possible-noderef-req
I believe this will give us clean tevent solution. It is closed API with the following interface: - sdap_nested_group_send() - sdap_nested_group_recv()
The code is far from complete, I have finished only the main interface so far. Otherwise it is just a skeleton. Especially sdap_nested_group_(deref|single)* functions are just a generated code so you don't have to bother reading them.
On Wed, Feb 13, 2013 at 01:22:40PM +0100, Pavel Březina wrote:
Hi, I started working on https://fedorahosted.org/sssd/ticket/1784 and I'd like to get some comments so I know I'm heading the right direction.
*Current state:* There is one tevent request per a nesting level. The request continues with many "step" and other-named functions. The path is different for deref and noderef processing, but the two branches intersect many times. It uses a lot of recursion. It often translates return codes.
All of this make it hard to understand the process, hard to maintain and especially it is hard to step out of the process at chosen point.
*New approach:* There will be several tevent request per nesting level: main-req (from sdap_nested_group_send) | nesting-level-req / \ noderef-req deref-req | possible-noderef-req
^^^^^ Shouldn't this request get back to the nesting-level-req ? Keep in mind that on one level you can have several groups, some of them might have enough missing members to be dereferenced some of them might not.
What is the nesting-level-req going to do? Check if the entry is already in the hash (or the sysdb as we can search both users and groups by originalDN in one go) and count how many members are missing?
The noderef branch is going to search bythe DN for users first and then groups, right?
It is closed API with the following interface:
- sdap_nested_group_send()
- sdap_nested_group_recv()
When is the groups being saved? In sdap_nested_group_done() or in the consumer of sdap_nested_group_recv() ?
The code is far from complete, I have finished only the main interface so far. Otherwise it is just a skeleton. Especially sdap_nested_group_(deref|single)* functions are just a generated code so you don't have to bother reading them.
Looks good, but please don't name the functions that save the group into hash with _save_ that makes it sound like they are saving into the sysdb. Also the attr variable could have been named name for better readability.
But I think the concept is sound.
I'm looking forward to see the complete patch along with unit tests :-)
On 02/13/2013 02:24 PM, Jakub Hrozek wrote:
On Wed, Feb 13, 2013 at 01:22:40PM +0100, Pavel Březina wrote:
Hi, I started working on https://fedorahosted.org/sssd/ticket/1784 and I'd like to get some comments so I know I'm heading the right direction.
*Current state:* There is one tevent request per a nesting level. The request continues with many "step" and other-named functions. The path is different for deref and noderef processing, but the two branches intersect many times. It uses a lot of recursion. It often translates return codes.
All of this make it hard to understand the process, hard to maintain and especially it is hard to step out of the process at chosen point.
*New approach:* There will be several tevent request per nesting level: main-req (from sdap_nested_group_send) | nesting-level-req / \ noderef-req deref-req | possible-noderef-req
^^^^^Shouldn't this request get back to the nesting-level-req ? Keep in mind that on one level you can have several groups, some of them might have enough missing members to be dereferenced some of them might not.
Yes. I think it will not be needed.
What is the nesting-level-req going to do? Check if the entry is already in the hash (or the sysdb as we can search both users and groups by originalDN in one go) and count how many members are missing?
The idea is to do here as much as possible, to keep the look ups simple. So yes, this is definitely good description.
In more general way: everything that is present in both deref and noderef and doesn't need some special knowledge.
The noderef branch is going to search bythe DN for users first and then groups, right?
If we don't know what object belongs to dn, we search users first and then groups. Yes.
It is closed API with the following interface:
- sdap_nested_group_send()
- sdap_nested_group_recv()
When is the groups being saved? In sdap_nested_group_done() or in the consumer of sdap_nested_group_recv()?
Groups are saved to sysdb at once in the consumer of _recv() (current behaviour). I'm not against moving it to _done().
The code is far from complete, I have finished only the main interface so far. Otherwise it is just a skeleton. Especially sdap_nested_group_(deref|single)* functions are just a generated code so you don't have to bother reading them.
Looks good, but please don't name the functions that save the group into hash with _save_ that makes it sound like they are saving into the sysdb. Also the attr variable could have been named name for better readability.
But I think the concept is sound.
I'm looking forward to see the complete patch along with unit tests :-)
Yeah, I need to check out your responder unit test.
On Wed, Feb 13, 2013 at 03:53:10PM +0100, Pavel Březina wrote:
When is the groups being saved? In sdap_nested_group_done() or in the consumer of sdap_nested_group_recv()?
Groups are saved to sysdb at once in the consumer of _recv() (current behaviour). I'm not against moving it to _done().
No, the consumer should do it, you're right. This new request should only download the data.
Happy weekend everyone, if you are bored you can look at this code.
I believe noderef path is fully completed. I haven't tried it so far, it will not even compile - hopefully only because there are no function prototypes. Those task are next on my schedule.
There is one logical change that may need explaining. In the current code, every group from member attribute is always pulled from LDAP. But it is not further processed nor stored in hash table (and therefore in sysdb) if we have reached the maximum nesting level.
Now, if we know ahead that the dn is a group and we have reached the maximum nesting level, we skip the group without contacting LDAP. That brings a little change in condition level > max_level to level >= max_level, because we do the check before increasing the nesting level.
Otherwise everything should be well explained from the code or comments. If not, than I have apparently failed the job.
On 02/15/2013 07:42 PM, Pavel Březina wrote:
Happy weekend everyone, if you are bored you can look at this code.
I believe noderef path is fully completed. I haven't tried it so far, it will not even compile - hopefully only because there are no function prototypes. Those task are next on my schedule.
There is one logical change that may need explaining. In the current code, every group from member attribute is always pulled from LDAP. But it is not further processed nor stored in hash table (and therefore in sysdb) if we have reached the maximum nesting level.
Now, if we know ahead that the dn is a group and we have reached the maximum nesting level, we skip the group without contacting LDAP. That brings a little change in condition level > max_level to level >= max_level, because we do the check before increasing the nesting level.
Otherwise everything should be well explained from the code or comments. If not, than I have apparently failed the job.
Hi, I tried it, fixed it and it works... at least the scenarios I have tried. I'm attaching also a second patch, that replaces the old code with the new one. I did not remove the old code yet though, it is more convenient for me to write deref version when the old code is present within the same project.
My testing discovered one more problem in ghost users. The reason why we decided to do this refactoring is that we couldn't cope with a member that is outside the search bases - we waited 60 seconds for timeout.
Currently we have fixed 1.9 so that it does not hang, however it breaks nesting. For example when we have:
dn: cn=test-group-1,ou=Groups,dc=example,dc=com objectClass: groupOfNames objectClass: top objectClass: posixGroup cn: test-group-1 gidNumber: 20001 member: cn=user-1,ou=People,dc=example,dc=com member: cn=user-2,ou=People,dc=example,dc=com member: cn=test-group-2,ou=Groups,dc=example,dc=com member: cn=user-2-1,ou=People2,dc=example,dc=com
user-2-1 is outside configured search bases.
test-group-2 is correctly downloaded, processed and stored, but getent doesn't show members of test-group-2.
getent group test-group-1
test-group-1:*:20001:user-1,user-2
test-group-1 contains only user-1 and user-2 ghost users.
On 02/18/2013 02:40 PM, Pavel Březina wrote:
On 02/15/2013 07:42 PM, Pavel Březina wrote:
Happy weekend everyone, if you are bored you can look at this code.
I believe noderef path is fully completed. I haven't tried it so far, it will not even compile - hopefully only because there are no function prototypes. Those task are next on my schedule.
There is one logical change that may need explaining. In the current code, every group from member attribute is always pulled from LDAP. But it is not further processed nor stored in hash table (and therefore in sysdb) if we have reached the maximum nesting level.
Now, if we know ahead that the dn is a group and we have reached the maximum nesting level, we skip the group without contacting LDAP. That brings a little change in condition level > max_level to level >= max_level, because we do the check before increasing the nesting level.
Otherwise everything should be well explained from the code or comments. If not, than I have apparently failed the job.
Hi, I tried it, fixed it and it works... at least the scenarios I have tried. I'm attaching also a second patch, that replaces the old code with the new one. I did not remove the old code yet though, it is more convenient for me to write deref version when the old code is present within the same project.
My testing discovered one more problem in ghost users. The reason why we decided to do this refactoring is that we couldn't cope with a member that is outside the search bases - we waited 60 seconds for timeout.
Currently we have fixed 1.9 so that it does not hang, however it breaks nesting. For example when we have:
dn: cn=test-group-1,ou=Groups,dc=example,dc=com objectClass: groupOfNames objectClass: top objectClass: posixGroup cn: test-group-1 gidNumber: 20001 member: cn=user-1,ou=People,dc=example,dc=com member: cn=user-2,ou=People,dc=example,dc=com member: cn=test-group-2,ou=Groups,dc=example,dc=com member: cn=user-2-1,ou=People2,dc=example,dc=com
user-2-1 is outside configured search bases.
test-group-2 is correctly downloaded, processed and stored, but getent doesn't show members of test-group-2.
getent group test-group-1
test-group-1:*:20001:user-1,user-2
test-group-1 contains only user-1 and user-2 ghost users.
This turned out to be the same issue that was already reported: https://fedorahosted.org/sssd/ticket/1194
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina