389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 91 forks source link

tombstone dn format is not technically correct #2566

Open 389-ds-bot opened 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49507


Issue Description

Tombstone dn's are not technically correct:

nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a,uid=testuser,ou=People,dc=example,dc=com

However, the parent object is ou=People,dc=example,dc=com

As a result this means the rdn is actually:

C3
      ID: 14; RDN: "nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a,uid=testuser"; NRDN: "nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a,uid=testuser"
14
      ID: 14; RDN: "nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a,uid=testuser"; NRDN: "nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a,uid=testuser"
P14
      ID: 3; RDN: "ou=People"; NRDN: "ou=people"

This is not really compliant to ldap standards. We should investigate if it's possible to make this compliant as a multivalued rdn IE:

nsuniqueid=c04c0107-e4a611e7-b5dae1e9-cbd6b25a+uid=testuser,ou=People,dc=example,dc=com

This would required a detailed investigation to ensure that replication conflict management, and tombstone management are able to work with the existing format and corrected format.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-30 00:14:19

Metadata Update from @Firstyear:

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-06-14 14:39:10

I implemented a solution for this, triggered by the work on conflict resolution. A design doc is here:

http://www.port389.org/docs/389ds/design/use-correct-tombstone-rdns.html

A patch will follow

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-06-14 14:46:47

0001-Fix-for-tickest-49507-and-49615-use-correct-tombston.patch

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-06-18 21:06:07

Design doc looks great, nice job!

Patch review, only one very minor issues

entryrdn_rename_subtree() - indentation off around if/else entryrdn_rename_subtombstones() to TRACE logging calls, one has the old function name.

The rest looks good!

I'll let @tbordaz have a review, and he can ack if he agrees

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-06-21 12:27:13

updated the doc with a missing operation and a discussion of mixed version topologies

the patch needs to be extended

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2018-06-21 14:56:42

@elkris, the design doc and patch are looking good to me. Very minor comments: in moddn_remove_children_from_cache we remove child DNs from dncache. Why is it conditional to entryrdn_get_switch ?

in ldbm_delete Slapi_RDN *sdn is not used

in ldbm_entryrdn log msg with 'entryrdn_rename_subtree' in 'entryrdn_rename_subtombstones'

A question regarding DEL entry with only tombstone children. It moves the children tombstones under the deleted parent with the update of entryrdn. How was it done before ?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-06-21 15:31:48

@elkris, the design doc and patch are looking good to me. Very minor comments: in moddn_remove_children_from_cache we remove child DNs from dncache. Why is it conditional to entryrdn_get_switch ?

the code is taken from the code exising in ldbm_modrdn and put into a function, so it is a s before. I think it is to be seen with moddn_get_children where also the entryrdn switch triggers execution. In fact the tombstone children are only properly renamed if the entryrdn switch is on. And I think it is now always oo, but if you want to work tombstone children rename properly with teh switch to off I will have to invest time

in ldbm_delete Slapi_RDN *sdn is not used

yep

in ldbm_entryrdn log msg with 'entryrdn_rename_subtree' in 'entryrdn_rename_subtombstones'

good catch, will fix

A question regarding DEL entry with only tombstone children. It moves the children tombstones under the deleted parent with the update of entryrdn. How was it done before ?

It does not really move them, it updates the parent records with the proper new parent rdn. It was not done at all before and that is why we had the problems reported in ticket 49615

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2018-06-21 15:55:33

@elkris, please do not make it work with 'entryrdn switch: off', it is almost a deprecated mode. My remark was more about the DN cache, AFAIK DN cache should not be impacted with that switch on/off.

I missed that the culprit of the bug was missing update of parent records. So it explains why this code is new :)

So regarding that patch you have my ACK

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-08-27 10:10:33

The tickets 49615, 49616 and 49507 should be handled together, but they didn't get attention for quite some time. A problem might be that the format changes and we need to be carefule when to introduce it.

Please keep them open and I will look into them again.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2020-05-06 16:19:33

Metadata Update from @mreynolds389: