#50970 Work in Progress - EntryUUID and EntryUUID Syntax Plugins
Closed by spichugi. Opened by firstyear.
firstyear/389-ds-base 137-entryuuid  into  master

Download 50970.patch

This is a work in progress of entryuuid and entryuuid syntax support. Both of these plugins are going to be written in rust as a proof of concept, and to add the needed layers of support between a rust plugin and our C code base. This is not ready for review yet.

Please add a general comment that it aligns with slapi-plugin.h
Also for each of them something like

PreBind = 401,    /// SLAPI_PLUGIN_PRE_BIND_FN
PreUnbind = 402,   /// SLAPI_PLUGIN_PRE_UNBIND_FN

Metadata Update from @tbordaz:
- Pull-request tagged with: WIP

A general comment that it must align with SLAPI_LOG_FATAL,...

@tbordaz I did say it wasn't ready for review yet, but I appreciate your enthusiasm! I'll add these comments for you :)

rebased onto 795cd4261cb1e39538a54012d6156b388242fa35

Still a work in progress, but a lot of it is coming together now. Entryuuid schema was moved to it's own file to still allow non-rust builds, and we can now see the start of value iteration in the filterava check:

[30/Mar/2020:16:49:26.824742900 +1000] - DEBUG - entryuuid_syntax_plugin_syntax_filter_ava - => begin
[30/Mar/2020:16:49:26.826044600 +1000] - DEBUG - plugins/entryuuid_syntax/src/lib.rs:26 - valid uuid -> 973e1bbf-ba9c-45d4-b01b-ff7371fd9008
[30/Mar/2020:16:49:26.827390700 +1000] - DEBUG - plugins/entryuuid_syntax/src/lib.rs:26 - valid uuid -> 973e1bbf-ba9c-45d4-b01b-ff7371fd9008
[30/Mar/2020:16:49:26.828582700 +1000] - DEBUG - plugins/entryuuid_syntax/src/lib.rs:64 - filter_ava debug -> ValueRef { raw_slapi_val: 0x7fffac016070 }
[30/Mar/2020:16:49:26.830039000 +1000] - DEBUG - plugins/entryuuid_syntax/src/lib.rs:68 - filter_ava result -> false
[30/Mar/2020:16:49:26.831529700 +1000] - DEBUG - entryuuid_syntax_plugin_syntax_filter_ava - <= success

Changes were also made to better handle memory ownership between reference pointers and truly owned memory.

A good example of how this ends up is here: https://pagure.io/389-ds-base/pull-request/50970#_21__41

The plugin for entryuuid is starting to take a bit more shape, and it looks very clean, and all is safe rust.

rebased onto 7be6e91719c6b699d0ec77ec64dc3d1f291cf2b5

2 new commits added

  • fmt
  • Add task handler

Still not quite done - just need to add the core of the task to the plugin. But this has a working entryuuid generator, with tests, a syntax plugin, and a foundation of the plugin system in rust, including all the nitty gritty bits of memory ownership that we have to care about.

rebased onto aa803561cbf05cc0e4ceef9d756c9188db257801

Metadata Update from @firstyear:
- Pull-request untagged with: WIP
- Pull-request tagged with: RFE

me late again.

Looking at the proposed change i have a split mind.
On one side the implementation looks good, the use of rust is promising (although I did not thorouhly look at it)
On the other hand I am not sure we need this and should go this way. We have a uuid for each entry, the nsuniqeid. And when you say it is a replication thing this is not true, the nsuniqueid exists for standalone instances and I think it was there before multimaster replication.
Its two problems are izts attr name and the string formatting. Otherwise I think in uuid.c we generate uuids along the line of the rust uuid package. In ds we generate a _v3 uuid whereas you choose a _v4.
So we are adding a second unique attribute to an entry, it looks like an unecessary duplication to me. In sync_repl, eg. where the rfc asks for a entryUUID to be returned we us teh nsuniqueid and just reformat it to match the entryUUID grouping.

me late again.

Never too late, and your input is always valuable :)

Looking at the proposed change i have a split mind.
On one side the implementation looks good, the use of rust is promising (although I did not thorouhly look at it)
On the other hand I am not sure we need this and should go this way. We have a uuid for each entry, the nsuniqeid. And when you say it is a replication thing this is not true, the nsuniqueid exists for standalone instances and I think it was there before multimaster replication.

Yes, but it is used as part of replication now for identifying entries, so it would be unwise for an administrator to change the nsuniqueid of an entry that already exists ...

Its two problems are izts attr name and the string formatting. Otherwise I think in uuid.c we generate uuids along the line of the rust uuid package. In ds we generate a _v3 uuid whereas you choose a _v4.
So we are adding a second unique attribute to an entry, it looks like an unecessary duplication to me. In sync_repl, eg. where the rfc asks for a entryUUID to be returned we us teh nsuniqueid and just reformat it to match the entryUUID grouping.

I didn't know about this piece of code, so it's good to have a look at. It looks like sync_nsuniqueid2uuid just is taking the string to a byte array, rather than the uuid grouping, so you'd need to go sync_nsunique2uuid -> byte array -> from bytes ( https://docs.rs/uuid/0.8.1/uuid/struct.Uuid.html#method.from_bytes , assuming endianness). Still possible though.

A major reason for entryuuid to be separate was that EntryUUID may also be imported from another application. For example, openldap may have created it's own entryUUID's that we need to import and apply, followed by generation of new ones for entries that are created subsequently. By importing these directly, it avoids admins needing to manipulate the uuids into the nsuniqueid format.

People may also export/import ldifs from one topology to another without replication metadata, but want the EntryUUID to remain the same (IE from production to a staging instance you want to test). You want the EntryUUID to remain stable across this boundary, but the replication meta isn't needed to bring across to the other topology. It would also be possible for admins to change the EntryUUID of an entry (but I think that currently we can't as schema defines it as dir-use only).

I think that nsuniqueid is internally a uuid for 389-ds to use for various things, but that for external facing applications it's better to detach this to a separate attribute.

@mreynolds you may be interested in this.

While I agree with Ludwigs comments, it would be nice to handle existing uuids from different ldap vendors. I feel it's worth the "duplication" if it makes 389 more robust.

I can also envisage a scenario where 389 and openldap or other have been run in parallel and there is a need to sync the entryUUID later, which means it would be disconnected from the nsUniqueId field too. So I'm biased that having it seperate gives us more flexibility in import and migration scenarioes.

I see no reason to hold this up, ack

rebased onto b405a9091383b45cd80b3e97da64a7e0bdc59fa1

Pull-Request has been merged by firstyear

This introduced compiler warnings, can you please fix it @firstyear ?

../389-ds-base/ldap/servers/slapd/entry.c: In function ‘slapi_entry_attr_get_valuearray’:
../389-ds-base/ldap/servers/slapd/entry.c:2894:34: warning: returning ‘struct slapi_value **’ from a function with incompatible return type ‘const struct slapi_value **’ [-Wincompatible-pointer-types]
 2894 |     return attr->a_present_values.va;
      |            ~~~~~~~~~~~~~~~~~~~~~~^~~

Yep, I'll do it today.

  CC       ldap/servers/slapd/libslapd_la-dse.lo
  CC       ldap/servers/slapd/libslapd_la-dynalib.lo
  CC       ldap/servers/slapd/libslapd_la-entry.lo
  CC       ldap/servers/slapd/libslapd_la-entrywsi.lo
  CC       ldap/servers/slapd/libslapd_la-errormap.lo

No errors, PR: https://pagure.io/389-ds-base/pull-request/51138

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/4023

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.

Pull-Request has been closed by spichugi

Metadata