sssd-devel October 2016

sssd-devel@lists.fedorahosted.org

21 participants
68 discussions

[sssd PR#42][opened] MAN: Typo in id mapping explanation
by fidencio 07 Oct '16

07 Oct '16

URL: https://github.com/SSSD/sssd/pull/42 Author: mzidek-rh Title: #42: MAN: Typo in id mapping explanation Action: opened PR body: """ It is probably result of modifying the code and not updating the man page properly. Resolves: https://fedorahosted.org/sssd/ticket/3205 """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/42/head:pr42 git checkout pr42

1 11

[sssd PR#40][opened] TESTS: Remove a leftover debug message
by fidencio 07 Oct '16

07 Oct '16

URL: https://github.com/SSSD/sssd/pull/40 Author: fidencio Title: #40: TESTS: Remove a leftover debug message Action: opened PR body: """ The debug message was introduced when I was testing 65a38b8c9, but ended up not removed before submitting the patch. Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com> """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/40/head:pr40 git checkout pr40

1 7

[PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
by Philip Prindeville 05 Oct '16

05 Oct '16

From: Philip Prindeville <philipp(a)fedoraproject.org> Add c_str as a "const char *" to the hash_key_t. Redux per comments from Michal Zidek. --- dhash/dhash.c | 29 +++++++++++++++++++++-------- dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..98439e8 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; - unsigned char *k; + const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ - for (h = 0, k = (unsigned char *) key->str; *k; k++) + for (h = 0, k = (const unsigned char *) key->str; *k; k++) + h = h * PRIME_1 ^ (*k - ' '); + break; + case HASH_KEY_CONST_STRING: + /* Convert string to integer */ + for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: + case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); + case HASH_KEY_CONST_STRING: + return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table) while (p != NULL) { q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, &p->entry); - if (p->entry.key.type == HASH_KEY_STRING) { - hfree(table, (char *)p->entry.key.str); + if (p->entry.key.type == HASH_KEY_STRING + || p->entry.key.type == HASH_KEY_CONST_STRING) { + /* Internally we do not use constant memory for keys + * in hash table elements. */ + hfree(table, p->entry.key.str); } hfree(table, (char *)p); p = q; @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) element->entry.key.ul = key->ul; break; case HASH_KEY_STRING: - len = strlen(key->str)+1; + case HASH_KEY_CONST_STRING: + len = strlen(key->c_str) + 1; element->entry.key.str = halloc(table, len); if (element->entry.key.str == NULL) { hfree(table, element); return HASH_ERROR_NO_MEMORY; } - memcpy((void *)element->entry.key.str, key->str, len); + memcpy(element->entry.key.str, key->str, len); break; } @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) return error; } } - if (element->entry.key.type == HASH_KEY_STRING) { - hfree(table, (char *)element->entry.key.str); + if (element->entry.key.type == HASH_KEY_STRING + || element->entry.key.type == HASH_KEY_CONST_STRING) { + hfree(table, element->entry.key.str); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t; typedef enum { HASH_KEY_STRING, - HASH_KEY_ULONG + HASH_KEY_ULONG, + HASH_KEY_CONST_STRING } hash_key_enum; typedef enum @@ -137,6 +138,7 @@ typedef struct hash_key_t { hash_key_enum type; union { char *str; + const char *c_str; unsigned long ul; }; } hash_key_t; diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c index 303e9f8..7613e23 100644 --- a/dhash/examples/dhash_test.c +++ b/dhash/examples/dhash_test.c @@ -64,6 +64,9 @@ static char *key_string(hash_key_t *key) case HASH_KEY_STRING: snprintf(buf, sizeof(buf), "key string = \"%s\"", key->str); break; + case HASH_KEY_CONST_STRING: + snprintf(buf, sizeof(buf), "key string = \"%s\"", key->c_str); + break; default: snprintf(buf, sizeof(buf), "unknown key type = %d", key->type); break; -- 2.7.4

5 12

[sssd PR#36][opened] Partial fix for #3169
by fidencio 05 Oct '16

05 Oct '16

URL: https://github.com/SSSD/sssd/pull/36 Author: fidencio Title: #36: Partial fix for #3169 Action: opened PR body: """ Ticket #3169 is about having a quota for sssd secrets. This quota depends basically on the number of stored secrets and the size of (the payload of the) each stored secret and this patch implements the former for now. This patch is rebased on top of the series that fixes #3168. Relevant Patches: 1e4e914 (Fabiano Fidêncio, 17 minutes ago) SECRETS: Add a configurable limit of secrets that can be stored Related: https://fedorahosted.org/sssd/ticket/3169 """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/36/head:pr36 git checkout pr36

1 14

[sssd PR#38][opened] IPA: Initialize has_changes boolean control value
by fidencio 04 Oct '16

04 Oct '16

URL: https://github.com/SSSD/sssd/pull/38 Author: jhrozek Title: #38: IPA: Initialize has_changes boolean control value Action: opened PR body: """ without this patch, valgrind was reporting: ==30955== Conditional jump or move depends on uninitialised value(s) ==30955== at 0xDBBACC3: ipa_subdomains_slave_search_done (ipa_subdomains.c:1111) ==30955== by 0xE73B34D: sdap_search_bases_ex_done (sdap_ops.c:222) ==30955== by 0xE6FFA98: sdap_get_generic_done (sdap_async.c:1872) ==30955== by 0xE6FF4E2: generic_ext_search_handler (sdap_async.c:1689) ==30955== by 0xE6FF840: sdap_get_and_parse_generic_done (sdap_async.c:1797) ==30955== by 0xE6FEFB5: sdap_get_generic_op_finished (sdap_async.c:1579) ==30955== by 0xE6FB1D2: sdap_process_message (sdap_async.c:353) ==30955== by 0xE6FAD51: sdap_process_result (sdap_async.c:197) ==30955== by 0xE6FAA14: sdap_ldap_next_result (sdap_async.c:145) ==30955== by 0x8E157FF: tevent_common_loop_timer_delay (tevent_timed.c:341) ==30955== by 0x8E16809: epoll_event_loop_once (tevent_epoll.c:911) ==30955== by 0x8E14F09: std_event_loop_once (tevent_standard.c:114) ==30955== """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/38/head:pr38 git checkout pr38

1 18

[sssd PR#37][opened] Add DEBUG messages to the sssd-secrets responder
by fidencio 04 Oct '16

04 Oct '16

URL: https://github.com/SSSD/sssd/pull/37 Author: jhrozek Title: #37: Add DEBUG messages to the sssd-secrets responder Action: opened PR body: """ This patchset decorates sssd-secrets with DEBUG messages so that errors are easier to discover and by default (up to debug_level=3) the usual operations don't emit any errors. Also adds two trivial fixes that fix minor code-style issues. """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/37/head:pr37 git checkout pr37

1 12

Refactoring work for the next couple of weeks
by Jakub Hrozek 04 Oct '16

04 Oct '16

Hi, we might be able to do some refactoring in the next couple of weeks prior to working on the next release. I wrote up a proposal here: https://fedorahosted.org/sssd/wiki/DesignDocs/OneFifteenCodeRefactoring for your convenience, I copied the contents inline as well. But the page is really meant to kick off the discussion, so feel free to add suggestions or push back against those that are included. The main points to keep in mind are that the work should be doable in a couple of weeks along with our regular maintenance work and should benefit SSSD, not just 'this code is ugly, let's change it' kind of work. I'm looking forward to the discussion, especially I would like to hear Michal's or Lukas' opinion about the memory cache changes.. The text of the page follows: = Code refactoring for the 1.15 release = Related ticket(s): * please see inline == Problem statement == SSSD is being very actively developed, adding several major features in each release. We need to make sure the code stays maintainable and adding new features in the upcoming release won't increase the cost of maintaining SSSD long-term. Since SSSD releases are primarily being driven by Fedora and RHEL releases, the Red Hat employed developers have a fixed amount of time for code refactoring. Of course, community members and developers are free to submit their patches on their schedule -- although discussion on the list would be needed prior to merging any refactoring to not disrupt SSSD release quality for everyone. == Use cases == A typical use-case would be: "A feature X depends on module Y that either is missing some functionality that is missing or a module that has outlived its initial design. Changing Y in that module would allow us to implement X more easily or with less maintenance effort in the future". The goal is to prepare the code for upcoming features without regressing, so testing after the refactoring is done is mandatory. We should consider also doing an upstream (pre)release to make it easier to test the changes. == Proposed items to be refactored == This section lists the proposed tickets along with justifications, scope and test impact. Given the fixed amount of time, each refactoring has a scope, expressed in just three high-level buckets - large (a couple of weeks, might take most of the time of the refactoring "sprint"), medium (a week to two weeks) or small (a couple of days, up to a week). Each item also lists the affected modules or functionality, so that we know where we need to improve tests. === Use the shared `cache_req` module for responder look-ups === Currently each responder (except the !InfoPipe responder and several parts of the NSS responder) copy the logic that checks the cache and contacts the Data Provider if needed. In 1.15, we should add the missing functionality into the cache_req module and convert the existing responders (especially those that look up users and groups, not necessarily other objects like autofs maps or hosts..) to cache_req. ==== Benefit to SSSD ==== In 1.15, we should look at allowing lookups from trusted domain with a shortname. But we need to take performance into account and avoid cycling over all domains including their LDAP server. Then we could switch to checking the caches of all domains first before checking each domain's cache and then its server. This goal is tracked by https://fedorahosted.org/sssd/ticket/843 (Login time increases strongly if more than one domain is configured) and ultimately by https://fedorahosted.org/sssd/ticket/3001 ([RFE] Short name input format with SSSD for users from all domains when domain autodiscovery is used). ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/3151 - cache_req: complete the needs of NSS responders * https://fedorahosted.org/sssd/ticket/1126 - Reuse cache_req() in responder code ==== Testing ==== We already have NSS and PAM responder tests. We need to extend them further to make sure all codepaths we change are tested. ==== Scope ==== Large === Refactor group lookups for better performance === The `sdap_async_groups.c` module grew organically over time. At the moment, the module is quite hard to read and repeats some potentially expensive operations (like looping over all attributes or all members) several times. In order to improve performance, we should refactor this module and test it extensively. ==== Benefit to SSSD ==== The `sdap_async_groups.c` module would be better maintainable and we would remove some performance bottlenecks from the code. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/3211 - Refactor the sdap_async_groups.c module ==== Testing ==== LDAP group lookups can be tested using integration tests, "just" all cases we change must have corresponding test cases. ==== Scope ==== Medium === Refactor the sdap_id_ops.c module === The `sdap_id_ops.c` module was written in time where SSSD only supported a single domain. One of the things that are repeatedly biting us is that the module can set the fail over status of the whole domain to offline. Moreover, the module has no tests and is not easy to read. At this time, it's not clear whether the refactoring would just result in documenting and testing the module or if it would be worth for example making the module return error codes for connection errors and let the caller handle the errors. Alternatively, we might decide to do even more work and let the fail over code work per-domain, not per-back end, which probably wouldn't be doable in the given scope. More research is needed. ==== Benefit to SSSD ==== The module would be better maintainable (currently there are some codepaths where we even don't know why they were added anymore..), have tests and we would work towards removing issues with trusted domains setting SSSD to the offline mode. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/1507 - Investigate terminating connections in sdap_ops.c and comment the code some more Other related tickets include: * https://fedorahosted.org/sssd/ticket/2767 - The sdap_op code always ends request with EAGAIN * https://fedorahosted.org/sssd/ticket/2976 - sdap code can mark the whole sssd_be offline ==== Testing ==== Currently upstream has only basic tests with the integration tests. Downstream has tests for fail over as well. ==== Scope ==== Medium to large, depending on what changes we decide to do. === Provide a way to pass intermediate data between requests === As long as a request is confined within a single domain, we can pass around `sysdb_attrs` or a similar data structure between different requests and avoid a costly cache writes. However, when a request must include processing in two different domain types, for example an IPA domain that includes overrides, the only way to pass intermediate data is to call a sysdb transaction and save the data to cache so that another request can read them. ==== Benefit to SSSD ==== Performance benefit in case SSSD must call identity lookup requests from different domains. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/2943 - Split out the requests for IPA users and groups that include overrides into reusable requests ==== Testing ==== Unfortunately, there are no upstream tests for requests that include overrides. Testing would be provided by downstream tests. ==== Scope ==== Medium to large === Upstream the PoC tests that utilize Samba AD for AD provider testing === At the moment, we don't have any upstream tests for the AD provider and we rely on downstream and manual testing completely. Nikolai Kondrashov wrote a proof-of-concept patches that provisions an AD DC server provided by the Samba project using the cwrap wrapper libraries. The scope of this effort would be to upstream this work. ==== Benefit to SSSD ==== SSSD integration tests would allow us to write tests for the AD provider. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/2818 - Investigate if Samba4 in Fedora can be used for SSSD CI ==== Testing ==== Some basic tests like looking up a user or a group would be part of this effort. ==== Scope ==== Medium === Decrease the functionality of the monitor process === SSSD is gradually moving to socket-activated services and in general more self-contained services rather than implementing a service manager in the monitor process. The scope of this work would be to further decrease the dependence of services on the monitor process, such as moving the register functionality to the services themselves. Ultimately, the monitor process would perform one-time tasks such as converting the configuration from INI to confdb and exit. Other work might include a fallback configuration or starting the services and domains even without having them explicitly enumerated in the services section. ==== Benefit to SSSD ==== Socket-activatable services would be better manageable by SSSD. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/2231 - RFE: Drop the monitor process ==== Testing ==== There are no upstream test for this functionality at the moment. Some service restart tests exist in downstream, though. ==== Scope ==== Medium to large, but hopefully this task could be split into several smaller tasks. === Memory cache changes === There are several improvements to the memory cache that we have been discussing lately, including a memory cache for by-SID lookups or having the memory cache respect case insensitive domains. The goal of this task would be to investigate what needs to be changed in the memory cache in order to implement these improvements. ==== Benefit to SSSD ==== Better performance through leveraging memory cache for SID lookups and lookups in case insensitive domains. ==== Tracking tickets ==== * https://fedorahosted.org/sssd/ticket/3193 - [RFE] Support aliases in the memory cache * https://fedorahosted.org/sssd/ticket/2727 - Add a memcache for SID-by-id lookups ==== Testing ==== We already have tests for memory cache which could be extended. Tests for by-SID lookups would probably require us to add the Samba-based tests first. ==== Scope ==== Probably large, but more investigation is needed. == How To Test == Run all available upstream and downstream tests, if possible, extend the upstream tests.

4 6

RFC: github PR workflow
by Jakub Hrozek 03 Oct '16

03 Oct '16

Hi, I documented workflow that we could use for submitting PRs: https://fedorahosted.org/sssd/wiki/GithubWorkflow It's quite similar to what the FreeIPA team uses (although I don't think they publicly document it yet). Comments or edits welcome. If there are none, I'll link the page from the Contribute page later.

3 7

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

sssd-devel October 2016