On (19/01/16 17:11), Pavel Reichl wrote:
On 01/19/2016 04:27 PM, Lukas Slebodnik wrote:
>On (19/01/16 12:51), Pavel Reichl wrote:
>
>//snip
>@see main comment in ldap_id_mapping.xml
>
>>From a832b47f9ce4f288831b9c0e214493bdac7a9b7a Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl <preichl(a)redhat.com>
>>Date: Fri, 27 Nov 2015 04:15:00 -0500
>>Subject: [PATCH 2/4] IDMAP: introduce secondary slices
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2188
>>---
>>src/config/SSSDConfig/__init__.py.in | 1 +
>>src/config/etc/sssd.api.d/sssd-ad.conf | 1 +
>>src/config/etc/sssd.api.d/sssd-ipa.conf | 1 +
>>src/config/etc/sssd.api.d/sssd-ldap.conf | 1 +
>>src/lib/idmap/sss_idmap.c | 470 +++++++++++++++++++++++++++----
>>src/lib/idmap/sss_idmap.exports | 5 +-
>>src/lib/idmap/sss_idmap.h | 59 ++++
>>src/lib/idmap/sss_idmap_private.h | 4 +
>>src/man/include/ldap_id_mapping.xml | 20 ++
>>src/providers/ad/ad_opts.c | 1 +
>>src/providers/ipa/ipa_opts.c | 1 +
>>src/providers/ldap/ldap_opts.c | 1 +
>>src/providers/ldap/sdap.h | 1 +
>>src/providers/ldap/sdap_idmap.c | 15 +-
>>src/tests/cmocka/test_sss_idmap.c | 86 +++++-
>>src/tests/sss_idmap-tests.c | 191 +++++++++++++
>>16 files changed, 795 insertions(+), 63 deletions(-)
>>
>>diff --git a/src/lib/idmap/sss_idmap.exports b/src/lib/idmap/sss_idmap.exports
>>index
52115636d5a6b936f18b4392e9d12adc26c85f53..ad1818e10506857b15927d95523b104fcf18f890 100644
>>--- a/src/lib/idmap/sss_idmap.exports
>>+++ b/src/lib/idmap/sss_idmap.exports
>>@@ -1,4 +1,4 @@
>>-SSS_IDMAP_0.4 {
>>+SSS_IDMAP_0.5 {
>>
>> # public functions
>> global:
>>@@ -8,13 +8,16 @@ SSS_IDMAP_0.4 {
>> sss_idmap_ctx_set_lower;
>> sss_idmap_ctx_set_upper;
>> sss_idmap_ctx_set_rangesize;
>>+ sss_idmap_ctx_set_extra_slice_init;
>> sss_idmap_ctx_get_autorid;
>> sss_idmap_ctx_get_lower;
>> sss_idmap_ctx_get_upper;
>> sss_idmap_ctx_get_rangesize;
>>+ sss_idmap_ctx_get_extra_slice_init;
>> sss_idmap_calculate_range;
>> sss_idmap_add_domain;
>> sss_idmap_add_domain_ex;
>>+ sss_idmap_add_auto_domain_ex;
>> sss_idmap_check_collision;
>> sss_idmap_check_collision_ex;
>> sss_idmap_sid_to_unix;
>The last change in this file was in sssd-1.12.0. There were many releases
>therefore we cannot touch version SSS_IDMAP_0.4. We can only extend it.
I'm afraid I don't understand and given the tight time schedule I would like to
ask you if you could be so kind and prepare the proper patch for this. It would be really
helpful.
I think it's clear from git log of two version files files what you should do.
BTW if there is a tie problem with schedule then we shoudl dorp this feature
rather then pushing something in the rush.
>
>@see git log for other files
>./src/sss_client/idmap/sss_nss_idmap.exports
>./src/providers/ipa/ipa_hbac.exports
>
>You also forgot to bump version-info in Makefile.am for this library.
>
>>diff --git a/src/lib/idmap/sss_idmap.h b/src/lib/idmap/sss_idmap.h
>>index
0797083293f7e010962828ddcd72709b290859b9..c19f6ad7bd4c4628d08bf5d90888d89bf1c79d5d 100644
>>--- a/src/lib/idmap/sss_idmap.h
>>+++ b/src/lib/idmap/sss_idmap.h
>>@@ -175,6 +175,17 @@ enum idmap_error_code
>>sss_idmap_ctx_set_rangesize(struct sss_idmap_ctx *ctx, id_t rangesize);
>>
>>/**
>>+ * @brief Set the number of secondary slices available for domain
>>+ *
>>+ * @param[in] ctx idmap context
>>+ * @param[in] extra_slice_init number of secondary slices to be generated
>>+ * at startup
>>+ */
>>+enum idmap_error_code
>>+sss_idmap_ctx_set_extra_slice_init(struct sss_idmap_ctx *ctx,
>>+ int extra_slice_init);
>>+
>>+/**
>> * @brief Check if autorid compatibility mode is set
>> *
>> * @param[in] ctx idmap context
>>@@ -211,6 +222,16 @@ enum idmap_error_code
>>sss_idmap_ctx_get_rangesize(struct sss_idmap_ctx *ctx, id_t *rangesize);
>>
>>/**
>>+ * @brief Get the maximal number of secondary slices available for domain
>>+ *
>>+ * @param[in] ctx idmap context
>>+ * @param[out] _extra_slice_init maximal number of secondary slices
>>+ */
>>+enum idmap_error_code
>>+sss_idmap_ctx_get_extra_slide_max(struct sss_idmap_ctx *ctx,
>>+ int *_extra_slide_max);
>>+
>>+/**
>> * @brief Calculate new range of available POSIX IDs
>> *
>> * @param[in] ctx Idmap context
>>@@ -291,6 +312,44 @@ enum idmap_error_code sss_idmap_add_domain_ex(struct
sss_idmap_ctx *ctx,
>> bool external_mapping);
>>
>>/**
>>+ * @brief Add a domain with the first mappable RID to the idmap context and
>>+ * generate automatically secondary slices
>>+ *
>>+ * @param[in] ctx Idmap context
>>+ * @param[in] domain_name Zero-terminated string with the domain name
>>+ * @param[in] domain_sid Zero-terminated string representation of the domain
>>+ * SID (S-1-15-.....)
>>+ * @param[in] range TBD Some information about the id ranges of this
>>+ * domain
>>+ * @param[in] range_id optional unique identifier of a range, it is needed
>>+ * to allow updates at runtime
>>+ * @param[in] rid The RID that should be mapped to the first ID of the
>>+ * given range.
>>+ * @param[in] external_mapping If set to true the ID will not be mapped
>>+ * algorithmically, but the *_to_unix and *_unix_to_*
>>+ * calls will return IDMAP_EXTERNAL to instruct the
>>+ * caller to check external sources. For a single
>>+ * domain all ranges must be of the same type. It is
>>+ * not possible to mix algorithmic and external
>>+ * mapping.
>>+ *
>>+ * @return
>>+ * - #IDMAP_OUT_OF_MEMORY: Insufficient memory to store the data in the idmap
>>+ * context
>>+ * - #IDMAP_SID_INVALID: Invalid SID provided
>>+ * - #IDMAP_NO_DOMAIN: No domain domain name given
>>+ * - #IDMAP_COLLISION: New domain collides with existing one
>>+ */
>>+enum idmap_error_code
>>+sss_idmap_add_auto_domain_ex(struct sss_idmap_ctx *ctx,
>>+ const char *domain_name,
>>+ const char *domain_sid,
>>+ struct sss_idmap_range *range,
>>+ const char *range_id,
>>+ uint32_t rid,
>>+ bool external_mapping);
>>+
>>+/**
>> * @brief Check if a new range would collide with any existing one
>> *
>> * @param[in] ctx Idmap context
>>diff --git a/src/lib/idmap/sss_idmap_private.h
b/src/lib/idmap/sss_idmap_private.h
>>index
1d3a36901781ae51ab79015d0b789559325c8de5..15300d11fc50a47c6d37149fdb79477069d931f4 100644
>>--- a/src/lib/idmap/sss_idmap_private.h
>>+++ b/src/lib/idmap/sss_idmap_private.h
>>@@ -29,6 +29,7 @@
>>#define SSS_IDMAP_DEFAULT_UPPER 2000200000
>>#define SSS_IDMAP_DEFAULT_RANGESIZE 200000
>>#define SSS_IDMAP_DEFAULT_AUTORID false
>>+#define SSS_IDMAP_DEFAULT_EXTRA_SLICE_INIT 10
>>
>>#define CHECK_IDMAP_CTX(ctx, ret) do { \
>> if (ctx == NULL || ctx->alloc_func == NULL || ctx->free_func == NULL) {
\
>>@@ -48,6 +49,9 @@ struct sss_idmap_opts {
>>
>> /* number of available UIDs (for single domain) */
>> id_t rangesize;
>>+
>>+ /* maximal number of secondary slices */
>>+ int extra_slice_init;
>>};
>>
>>struct sss_idmap_ctx {
>>diff --git a/src/man/include/ldap_id_mapping.xml
b/src/man/include/ldap_id_mapping.xml
>>index
17ef803289d14fa52b725c90062ee4ba0379acd0..301a7cc9ef62e85f5553d90c0323cc614ec6938c 100644
>>--- a/src/man/include/ldap_id_mapping.xml
>>+++ b/src/man/include/ldap_id_mapping.xml
>>@@ -243,6 +243,26 @@ ldap_schema = ad
>> </para>
>> </listitem>
>> </varlistentry>
>>+ <varlistentry>
>>+ <term>ldap_idmap_extra_slice_init
(integer)</term>
Previously used max was supposed to imply that there will be maximal such
number of slices, later init was used as the number could grove above this
number during run time....name fill change in next iteration, might to some
think ldap_idmap_cut_off_limit - as it will describe how many
'additional/secundary' slices will be tried during unix_to_sid() conversion.
Then probably less confusing name would be ldap_idmap_extra_slice_min.
But the most confusing was that design page was not up-to-date.
Would you be so kind and could you update it as well?
> ^^^^^
> Could you explain the suffix?
> I cannot see the string "init" anywhere
in
> man page.
>
>BTW the design document has the name ''ldap_idmap_extra_slice_max''
>>+ <listitem>
>>+ <para>
>>+ Number of secondary slices to use for ID mapping
>>+ that are generated at the startup.
>>+ </para>
>>+ <para>
>>+ Note: Additional secondary slices might be generated
>>+ when SID is being mapped to UNIX uid and RID part of
>>+ SID is out of range for secondary slices generated so
>>+ far. If value of ldap_idmap_extra_slice_init is equal
>>+ to 0 then no additional secondary slices are
>>+ generated.
>>+ </para>
>>+ <para>
>>+ Default: 10
>I'm not sure about the defualt value.
>
>Simo wrote few comments on IRC few days ago that this feature is not
>deterministic among more machines.
>e.g.
>there are few users
> jakub -- S-1-5-21-100-200-300-12809
> simo -- S-1-5-21-100-200-300-212809
> sumit -- S-1-5-21-300-200-100-12809
>
>Let say that S-1-5-21-100-200-300 will be mapped to the slice 10
>and S-1-5-21-300-200-100 to the slice 11
>
>machine A:
>* getent passwd jakub -> slice 10 is created
>* getent passwd simo -> slice 11 is created based on this feature
>
>machine B:
>* getent passwd jakub -> slice 10 is created
>* getent passwd sumit -> slice 11 is created
>
>
>And user 'simo' will have the same UID on 'machine A' as user
>'sumit' on 'machine B'. It can be a problem on nfs.
>
>
>So I'm not sure we should enable this feature by default.
>But on the other hand this feature might fix many users complications.
>
As I wrote in top of previous mail. We need to solve this think before pushing
patches.
LS
PS Please do not forget to include unit test for 3rd patch.
libsss_idmap is critical library and shoudl be properly unit tested.