PR - Ticket 50236 - memberOf should be more robust

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50237

• Created at 2019-02-21 23:01:51 by mreynolds (@mreynolds389)
• Merged at 2019-02-26 15:20:46

---

Bug Description:

When doing a modrdn, or any memberOf update, if the entry already has the memberOf attribute with the same value the operation is incorrectly rejected.

Fix Description:

If we get an error 20 (type or value e3xists) return success.

Also fixed a coding mistake that causes the wrong error code to be returned

Resolves: #3295

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-21 23:02:02

fixes 50236

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-02-22 01:08:57

@mreynolds389 Is there a lib389 test case for this?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-02-22 01:09:36

This looks correct to me, would be good to have some comments around the situations in which the conditions are taken.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-22 01:34:25

This looks correct to me, would be good to have some comments around the situations in which the conditions are taken.

It happens under a very complex IPA deployment - where there are many nested groups, and cross references. I was only able to reproduce with an IPA install, and since the fix approach is really universal I just sent it out.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-02-22 02:01:21

:( that's annoying. OKay in that case ack from me provided commenting about the situation and fix is added before the if statement.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-02-22 02:10:37

I've run a couple of tests we have for memberOf. Fedora 29

suites/memberof_plugin/regression_test.py::test_entrycache_on_modrdn_failure
suites/memberof_plugin/regression_test.py::test_silent_memberof_failure

These are failing on the modrdn operation. OBJECT_CLASS_VIOLATION...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-22 04:00:30

I've run a couple of tests we have for memberOf. Fedora 29
suites/memberof_plugin/regression_test.py::test_entrycache_on_modrdn_failure
suites/memberof_plugin/regression_test.py::test_silent_memberof_failure

These are failing on the modrdn operation. OBJECT_CLASS_VIOLATION...

Is this after applying my fix? Because the one thing I did was fix the error test condition. Previous there was a coding error and we were not testing for the error correctly.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-02-22 13:58:08

Is this after applying my fix? Because the one thing I did was fix the error test condition. Previous there was a coding error and we were not testing for the error correctly.

Yeah, LDAP_TYPE_OR_VALUE_EXISTS part is correct and it doesn't fail.

And another part (testing for the error correctly) fails when I build only with this fix alone.

The error test output is filled with dragons and warnings produced by @Firstyear's PR and the log is unreadable now, so I can't properly paste it here... But it is 100% reproducible as far as I can see, you can just run the tests I mentioned.

Maybe we should fix the new OBJECT_CLASS_VIOLATION issue in another PR - in case your fix has just revealed the real issue (and it is not a new regression)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-22 19:29:33

rebased onto 8d91fe7bb314356784af8ad49308faad92f6f62e

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-22 19:30:42

Changes made please review...

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-02-25 01:29:13

Is this after applying my fix? Because the one thing I did was fix the error test condition. Previous there was a coding error and we were not testing for the error correctly.

Yeah, LDAP_TYPE_OR_VALUE_EXISTS part is correct and it doesn't fail.
And another part (testing for the error correctly) fails when I build only with this fix alone.
The error test output is filled with dragons and warnings produced by @Firstyear's PR and the log is unreadable now, so I can't properly paste it here... But it is 100% reproducible as far as I can see, you can just run the tests I mentioned.
Maybe we should fix the new OBJECT_CLASS_VIOLATION issue in another PR - in case your fix has just revealed the real issue (and it is not a new regression)

You can disable them when you have DEBUGGING=True set.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-02-25 11:12:41

Probably, should be for num in sample(list(range(1000)), users_num): because list accepts only one argument.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-02-25 11:12:55

The rest looks good to me

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 13:53:12

Probably, should be for num in sample(list(range(1000)), users_num): because list accepts only one argument.

Fixed

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 13:54:15

rebased onto 655771bc719dba319898d429472a88ff4a6de94a

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-25 16:13:35

The fix looks good but I wonder if the problem can also happen in the previous memberof_add_memberof_attr (few lines above)

The first memberof_add_memberof_attr does ((MOD_DEL, 'memberof', <old_dn>), (MOD_ADD,'memberof',<new_dn>))
The second memberof_add_memberof_attr does ((MOD_ADD, 'memberof', <new_dn>))

To handle the case where the member is starting with
'memberof: <new_dn>'
'memberof: <old_dn>'
The first memberof_add_memberof_attr will successfully apply the MOD_DEL but will also return TYPE_OR_VALUE on the MOD_ADD.

It looks good to have the MOD_DEL+MOD_ADD in a single update but we should catch both error conditions LDAP_NO_SUCH_ATTRIBUTE (the MOD_DEL fails because the <old_dn> did not exist) and LDAP_TYPE_OR_VALUE_EXISTS (The MOD_ADD fails because the <new_dn> already exist).

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 16:23:05

rebased onto 6097abd5516679d34b184d50dc816f9df4a6faa8

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 16:23:45

@tbordaz, changes made and rebased. Please review...

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-25 17:29:53

Just realize that my previous update was lost :(

The first update is MOD_DEL(old_dn), MOD_ADD(new_dn). Catching the failure, the next update should be if TYPE_OR_VALUE then MOD_DEL(old_dn) else if NO_SUCH MOD_ADD(new_dn). But again this next update can fail, for example if the target entry is already in good condition, upon failure of the next update it should return Success.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 18:18:09

Just realize that my previous update was lost :(
The first update is MOD_DEL(old_dn), MOD_ADD(new_dn). Catching the failure, the next update should be if TYPE_OR_VALUE then MOD_DEL(old_dn) else if NO_SUCH MOD_ADD(new_dn). But again this next update can fail, for example if the target entry is already in good condition, upon failure of the next update it should return Success.
I'm not fully following your concern.

On the first pass there are two mods: (DEL old, ADD new). Only the DEL mod will return NO_SUCH_ATTRIBUTE, and only the ADD can return TYPE_OR_VALUE_EXISTS (but this should not be possible unless the environment is already broken). Anyway we can still correctly check for both.

If we got one of those errors then on the second pass the only option left it just to try and add the new memberOf val again. If its already there (TYPE_OR_VALUE_EXISTS) then we return success.

This is what the current/rebased already patch does. So I'm not seeing what condition I could still be missing. Please clarify. Thanks!

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-25 18:24:08

Just realize that my previous update was lost :(
The first update is MOD_DEL(old_dn), MOD_ADD(new_dn). Catching the failure, the next update should be if TYPE_OR_VALUE then MOD_DEL(old_dn) else if NO_SUCH MOD_ADD(new_dn). But again this next update can fail, for example if the target entry is already in good condition, upon failure of the next update it should return Success.
I'm not fully following your concern.

On the first pass there are two mods: (DEL old, ADD new). Only the DEL mod will return NO_SUCH_ATTRIBUTE, and only the ADD can return TYPE_OR_VALUE_EXISTS (but this should not be possible unless the environment is already broken). Anyway we can still correctly check for both.
If we got one of those errors then on the second pass the only option left it just to try and add the new memberOf val again. If its already there (TYPE_OR_VALUE_EXISTS) then we return success.
This is what the current/rebased already patch does. So I'm not seeing what condition I could still be missing. Please clarify. Thanks!

Okay I think I see your concern. The first mod group, the delete works but the add fails (again that should never happen, but it could if things are very broken). So on the retry we should only retry the delete (since the add is not needed). I'll rework it

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-25 18:29:49

Assume the initial state of the entry is
memberof: new_value

if first pass returns NO_SUCH_ATTRIBUTE (because it tests DEL old first) the second pass should try ADD 'new'. But the second phase will also fail (TYPE_OR_VALUE_EXISTS) but it is not a problem and returning SUCCESS is good

If the first pass returns TYPE_OR_VALUE_EXISTS, the second pass should try DEL 'old'. But the second pass will also fail (NO_SUCH_ATTRIBUTE) but it is not a problem and returning SUCCESS is good.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 13:53:55

@tbordaz I did address your concerns yesterday, can you please review again?

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-26 14:07:37

I think if rc is LDAP_NO_SUCH_ATTRIBUTE (MOD_DEL old_dn), it should ignore the error and set rc=LDAP_SUCCESS.
Except that the patch looks good

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 14:19:47

I think if rc is LDAP_NO_SUCH_ATTRIBUTE (MOD_DEL old_dn), it should ignore the error and set rc=LDAP_SUCCESS.

We still need to try to add the new DN/member though, we can not blindly set success. I really don't see what I am missing with the current patch.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 14:27:18

We do not need to check LDAP_NO_SUCH_ATTRIBUTE twice in this code block. Only the DEL triggers no such attribute, and that would be the first error caught. So in this code block for TYPE_OR_VALUE_EXISTS it is impossible for NO_SUCH_ATTRIBUTE to happen.

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-26 14:42:01

I agree that if both MODs are invalid (for example initial state is only 'memberof: <new_dn>' ) the error should be LDAP_NO_SUCH_ATTRIBUTE. Just wanted to be paranoid not guessing which MOD is evaluated first.

The fix is good to me. ACK

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 14:54:08

I agree that if both MODs are invalid (for example initial state is only 'memberof: <new_dn>' ) the error should be LDAP_NO_SUCH_ATTRIBUTE. Just wanted to be paranoid not guessing which MOD is evaluated first.

The first mod is "always" the DEL, which means the first error that can occur is the no such attribute. After that we change the mod list to single mod based on the initial error we get from the first call to memberof_add_memberof_attr().

I'm not trying to pressure you into an ack, I want to make sure you are okay with the patch.

The fix is good to me. ACK

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 15:20:21

rebased onto 47c4259

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-02-26 15:20:46

Pull-Request has been merged by mreynolds389

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-02-26 15:22:22

@mreynolds389 , no pressure here. The fix looks really good to me, especially error handling.

I was persnickety to capture any returned code in any branch. The MODs are evaluated in order and clearly if MOD_DEL and MOD_ADD are supposed to fail, the order of evaluation make sure that we will get LDAP_NO_SUCH_ATTRIBUTE.

[389-ds-bot]

Patch
50237.patch