#49197 RFE: memberof should not support circular groups by default
Closed: wontfix 6 years ago Opened 7 years ago by tbordaz.

Issue Description

This is possible to define circular groups. For example, G0 is member of G1 and G1 is member of G0.

memberof supports circular groups (updates correctly memberof attribute) but disable ancestors cache. The update is then very slow.

The issue is that circular groups reveal something broken in the environment itself.
memberof should fail when it detects circular groups (by default) unless a "new" flag says circular groups are supported

Package Version and Platform

Any versions

Steps to reproduce

  1. Enable memberof plugin
    2 Create circular group and update one of them
    2.
    3.

Actual results

memberof correctly update memberof attribute

Expected results

By default it should fail as soon as it detects a circular group


Implementation should be easy but it exists a dependency on fixing a complex behavior.

Detecting circular group, memberof will fail, so the txn will be aborted.
A problem is that memberof may detect circular group quite late. Meaning after having fixup many members. Those fixup were successful updating entry cache.
Currently there is a pb invalidating updated entries (in the entry cache) when the txn that updated them is aborted.

Metadata Update from @tbordaz:
- Custom field reviewstatus adjusted to new
- Custom field type adjusted to defect

7 years ago

I'm against this change. I think it's easy to resolve circular groups, we have a poc algorithm to do this efficiently on complex scenarios (IE arbitrary length graphs).

This is avoiding the problem, rather than resolving it.

Metadata Update from @mreynolds:
- Custom field reviewstatus reset (from new)
- Issue set to the milestone: 1.3.7 backlog

6 years ago

Given we mentioned this in the triage:

Simply, if you can detect a circular group you have already expended all of the effort to resolve it. Because once you are at that point, you may as well just finish writing the membership information.

So first, there is my memberOf algo python example, which handles this efficiently, and correctly. Second, if we detect this we can already just finish the work to fix it.

So I think that this "feature" is an anti feature. Because if we have this scenario:

A -> B -> C

Now, I join C to A, which link is "missing"? A's member's wont be a member of C? Or B to A? Or C to B? We now add a condition by which an admin has to add groups in certain orders to make expressions work. Plus, they may actually want a circular group - they are not a mistake. In huge domains sometimes they happen by accident (and this "feature" would mask that), in some places they want it.

As a result, I am against this ticket, and do not believe that we should implement it.

We have discussed this further and come to the same conclusion. @tbordaz agrees we have already done the work and that we may as well perform the circular update if we have gotten to this point.

@lkrispen made an excellent point that this may break replication also. If we have master A and B, and we add some groups 1 and 2 on each, then on A we add 1 -> 2, and B 2 -> 1, on each they are happy, but on replication we would block the replication because they would respectively create a loop. Thus, A would only see 1->2 and B only 2->1.

As a result, I think that we should close this issue, and replace it with a new one: make circular group handling more effecient. As discussed, I have a POC algorithm that is capable of this, and it is highly efficient to operate.

Closing based on IRC discussions.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2256

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata