On Wed, Jul 17, 2013 at 08:23:57AM -0400, Simo Sorce wrote:
On Wed, 2013-07-17 at 14:18 +0200, Jakub Hrozek wrote:
> On Tue, Jul 16, 2013 at 03:01:31PM +0200, Lukas Slebodnik wrote:
> > ehlo,
> >
> > Group structure is described in ticket
https://fedorahosted.org/sssd/ticket/1846
> > Please ensure that sysdb cache is empty.
> >
> > call command "getent group cyclegroup1"
> >
> > sssd_be will call following function:
> > a) sdap_store_group_with_gid (name:cyclegroup1)
> > --added attributes [ ghost: user1, origMember: user1, ... ]
> > b) sdap_store_group_with_gid (name:cyclegroup2)
> > --added attributes [ ghost: user2, origMember: user2, ... ]
> > c) sdap_save_grpmem (name:cyclegroup1)
> > --added attributes [ member:cyclegroup2, ... ]
> > d) sdap_save_grpmem (name:cyclegroup2)
> > --added attributes [ member:cyclegroup1, ... ]
> >
> > Subtransaction "d)" will fail and log file contains:
> > [ldb] (0x4000): will add 2 ghost users to 1 parents
> > ... snip ...
> > [ldb] (0x4000): cancel ldb transaction (nesting: 2)
> > [sysdb_set_entry_attr] (0x0080): ldb_modify failed: [Attribute or value
exists]
> > [sysdb_set_entry_attr] (0x0400): Error: 17 (File exists)
> > [sysdb_store_group] (0x0400): Error: 17 (File exists)
> > [sdap_save_grpmem] (0x0040): Failed to save user cyclegroup2
> > [sdap_save_groups] (0x0040): Failed to store group 1 members.
> > [ldb] (0x4000): commit ldb transaction (nesting: 1)
> > [ldb] (0x4000): commit ldb transaction (nesting: 0)
> >
> > Attached patch should fix this bug.
> >
> > LS
>
> I personally don't have a problem with the patch. It *might* hide
> legitimate bugs but on the other hand the call was issued so that the
> attribute is added and if it was added already during processing of a
> cycle, then fine.
>
> But the memberof plugin is delicate code, so I would also like to get
> Simo's opinion. Simo, are you OK with this approach or would you prefer
> do detect the cycle instead?
Ideally we would detect, but using the permissive control is ok for now.
OK. Ack to Lukas's patch, then. Thanks for the second look!
> Please note that the function that Lukas added the permissive control to
> is called to distribute both ghost and memberuid attributes.
Yup and I appreciate the complexity around these functions, so go with
the current fix and maybe open a ticket to revisit the issue when there
is more time.
Sure:
https://fedorahosted.org/sssd/ticket/2021
we already have another memberof fix scheduled (#1654), so it makes
sense to do them at once to avoid re-learning the memberof plugin twice.