fixes 50236

@mreynolds Is there a lib389 test case for this?

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

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.

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

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...

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.

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)

Changes made please review...

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.

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

The rest looks good to me

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

Fixed

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', ), (MOD_ADD,'memberof',))
The second memberof_add_memberof_attr does ((MOD_ADD, 'memberof', ))

To handle the case where the member is starting with
'memberof: '
'memberof: '
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 did not exist) and LDAP_TYPE_OR_VALUE_EXISTS (The MOD_ADD fails because the already exist).

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

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.

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!

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

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.

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

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

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.

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.

I agree that if both MODs are invalid (for example initial state is only 'memberof: ' ) 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

I agree that if both MODs are invalid (for example initial state is only 'memberof: ' ) 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

@mreynolds , 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-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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3296

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.