[SSSD] Re: [PATCH] Unit tests for pam_sss using pam_wrapper (need help with CI..)
On (09/08/16 10:18), Jakub Hrozek wrote: >On Tue, Aug 09, 2016 at 08:04:38AM +0200, Lukas Slebodnik wrote: >> On (09/05/16 10:07), Jakub Hrozek wrote: >> >On Wed, May 04, 2016 at 11:36:57PM +0200, Lukas Slebodnik wrote: >> >> On (27/04/16 10:51), Jakub Hrozek wrote: >> >> >Hi, >> >> > >> >> >the attached patches implement unit tests for the pam_sss module using >> >> >pam_wrapper and libpamtest. In my testing, the coverage is around 75% >> >> >with mostly the parts that require running as root being untested. >> >> > >> >> >I worked on this patchset even though the features for 1.14 are in full >> >> >swing because there are several tickets that will require us to patch >> >> >pam_sss, so it's important to have the code that changes tested. In >> >> >addition, when we merge Dan's patches to use TLS with integration tests, >> >> >then we'll be able to also test authentication in integration tests >> >> >easily using libpamtest-python. >> >> > >> >> >However, our CI fails for me constantly: >> >> >http://sssd-ci.duckdns.org/logs/job/42/75/fedora_rawhide/ci.html >> >> >The strange thing is that running CI locally works fine and so does make >> >> >check. Can anyone help point me in the right direction as to what should >> >> >I check next? I suspect some of the environment variables might not be >> >> >set correctly, but I don't see why.. >> >> >> >> Are you sure it pass locally with valgrind? >> >> >> >> It failed because there are valgrind errors. >> >> I can see then on fedora 22 and fedora 23 >> >> http://sssd-ci.duckdns.org/logs-test/job/2/70/fedora22/ci-build-debug/src/tests/cwrap/pam_sss-tests.sh.1444.valgrind.log >> >> http://sssd-ci.duckdns.org/logs-test/job/2/70/fedora22/ci-build-debug/src/tests/cwrap/pam_sss-tests.sh.log >> >> >> >> I cannot see them on fedora 24 or fedora rawhide >> >> http://sssd-ci.duckdns.org/logs-test/job/2/70/fedora_rawhide/ci-build-debug/src/tests/cwrap/pam_sss-tests.sh.10504.valgrind.log >> >> but it fails as well >> >> http://sssd-ci.duckdns.org/logs-test/job/2/70/fedora_rawhide/ci-build-debug/src/tests/cwrap/pam_sss-tests.sh.log >> >> >> >> If the problem is with missing environment variables >> >> then you might log all environment variables in main function >> > >> >just a quick update: the issues in tests were resolved. I hit two bugs >> >in pam_wrapper: >> > >> > https://github.com/jhrozek/pam_wrapper/commit/ff7ec1c5ea7ed2360cbc59bd58f9caae7c9fa53d >> > >> > https://github.com/jhrozek/pam_wrapper/commit/c9b11ac8947bc0ff2ec3c4140b4d7413bfaadb37 >> >so once Andreas approves them, I will build a new pam_wrapper RPM for >> >Fedora and EPEL so that we can formally review the pam_sss test patches. >> Bump > >I only had time to rebase the patches: >https://github.com/jhrozek/sssd/tree/pwrap >But now the tests fail, so I did something wrong, somewhere. I don't >think I will have time to look into this until next week, sorry. http://sssd-ci.duckdns.org/logs-test/job/4/48/summary.html http://sssd-ci.duckdns.org/logs-test/job/4/49/summary.html All test passed only on rhel6 :-( Maybe because new test was skipped. cwrap tests were not executed on debian_testing due to following failures /var/lib/jenkins/workspace/ci-test/label/debian_testing/src/tests/cwrap/become_user-tests.sh: 3: .: Can't open /cwrap_test_setup.sh FAIL become_user-tests.sh (exit status: 2) @see http://sssd-ci.duckdns.org/logs-test/job/4/48/debian_testing/ci-build-debug/src/tests/cwrap/become_user-tests.sh.log http://sssd-ci.duckdns.org/logs-test/job/4/48/debian_testing/ci-build-debug/src/tests/cwrap/responder_common-tests.sh.log http://sssd-ci.duckdns.org/logs-test/job/4/48/debian_testing/ci-build-debug/src/tests/cwrap/server-tests.sh.log http://sssd-ci.duckdns.org/logs-test/job/4/48/debian_testing/ci-build-debug/src/tests/cwrap/usertools-tests.sh.log mock build failed on epel7 (maybe missing pam_sss-tests.sh in tarball) make[3]: Entering directory `/builddir/build/BUILD/sssd-1.14.2/src/tests/cwrap' make[4]: Entering directory `/builddir/build/BUILD/sssd-1.14.2/src/tests/cwrap' make[4]: *** No rule to make target `pam_sss-tests.sh', needed by `pam_sss-tests.sh.log'. Stop. make[4]: *** Waiting for unfinished jobs... The pam_sss-tests test failed on fedora24. There are valgrind errors. ==28387== Invalid read of size 1 ==28387==at 0x9CC2600: __GI_strchr (in /usr/lib64/libc-2.23.so) ==28387==by 0x402234: service_arg (test_wrapper_pam_sss.c:93) ==28387==by 0x402D73: test_pam_authenticate_root_ignore (test_wrapper_pam_sss.c:571) ==28387==by 0x565B542: ??? (in /usr/lib64/libcmocka.so.0.3.1) ==28387==by 0x565BC7A: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.3.1) ==28387==by 0x401AE0: main (test_wrapper_pam_sss.c:1343) ==28387== Address 0xc81b6b3 is 0 bytes after a block of size 227 alloc'd ==28387==at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==28387==by 0x97FF38D: talloc_named_const (in /usr/lib64/libtalloc.so.2.1.6) ==28387==by 0x4021A4: se
[SSSD] [sssd PR#22] LDAP: Return partial results from adminlimit exceeded (comment)
mzidek-rh commented on a pull request """ Makes sense, but I would prefer to add new "else if" branch with ADMINLIMIT_EXCEEDED specific debug message. It would also make sense to change the SDAP_SRCH_FLG_SIZELIMIT_SILENT into something more generic, like SDAP_SRCH_FLG_SILENT because we want to use it for both of these cases. """ See the full comment at https://github.com/SSSD/sssd/pull/22#issuecomment-246612968 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#22] LDAP: Return partial results from adminlimit exceeded (+Changes requested)
jhrozek's pull request #22: "LDAP: Return partial results from adminlimit exceeded" label *Changes requested* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/22 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#22] LDAP: Return partial results from adminlimit exceeded (comment)
lslebodn commented on a pull request """ On (13/09/16 01:33), mzidek-rh wrote: >Makes sense, but I would prefer to add new "else if" branch with >ADMINLIMIT_EXCEEDED specific debug message. > >It would also make sense to change the SDAP_SRCH_FLG_SIZELIMIT_SILENT >into something more generic, like SDAP_SRCH_FLG_SILENT because we want >to use it for both of these cases. > This change is not related to the bug. Feel free to send a separate patch The test passed for me. I saw some unrelated failures. Might be worth to check the reason. :: [ FAIL ] :: id lookup on next 1000 users increased sssd_nss memory usage by 10960 kB :: [ FAIL ] :: id lookup on next 1000 users increased sssd_nss memory usage by 9604 kB LS """ See the full comment at https://github.com/SSSD/sssd/pull/22#issuecomment-246616579 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#11] SECRETS: Don't remove a container when it has children (synchronize)
fidencio's pull request #11: "SECRETS: Don't remove a container when it has
children" was synchronize
See the full pull-request at https://github.com/SSSD/sssd/pull/11
... or pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/11/head:pr11
git checkout pr11
From 73c878007bb44cb3bd234367d50b6ea9fb8edf3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?=
Date: Tue, 30 Aug 2016 10:42:58 +0200
Subject: [PATCH 1/2] SECRETS: Search by the right type when checking
containers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We've been searching for the wrong type ("simple") in
local_db_check_containers(), which always gives us a NULL result.
Let's introduce the new LOCAL_CONTAINER_FILTER and do the search for the
right type ("container") from now on.
Resolves:
https://fedorahosted.org/sssd/ticket/3137
Signed-off-by: Fabiano Fidêncio
---
src/responder/secrets/local.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
index ac3049b..5b5745d 100644
--- a/src/responder/secrets/local.c
+++ b/src/responder/secrets/local.c
@@ -168,6 +168,7 @@ char *local_dn_to_path(TALLOC_CTX *mem_ctx,
}
#define LOCAL_SIMPLE_FILTER "(type=simple)"
+#define LOCAL_CONTAINER_FILTER "(type=container)"
int local_db_get_simple(TALLOC_CTX *mem_ctx,
struct local_context *lctx,
@@ -306,7 +307,7 @@ int local_db_check_containers(TALLOC_CTX *mem_ctx,
/* and check the parent container exists */
ret = ldb_search(lctx->ldb, mem_ctx, &res, dn, LDB_SCOPE_BASE,
- attrs, LOCAL_SIMPLE_FILTER);
+ attrs, LOCAL_CONTAINER_FILTER);
if (ret != LDB_SUCCESS) return ENOENT;
if (res->count != 1) return ENOENT;
talloc_free(res);
From 9ab6cc5eb3fd6b605f4324938f60cdf1ad0a3c7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?=
Date: Thu, 1 Sep 2016 12:04:30 +0200
Subject: [PATCH 2/2] SECRETS: Don't remove a container when it has children
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Let's return and log an error in case the container to be removed has
children.
The approach taken introduced at least one new search in every delete
operation. As far as I understand searching in the BASE scope is quite
cheap and that's the reason I decided to just do the search in the
ONELEVEL scope when the requested to be deleted dn is for sure a
container.
Resolves:
https://fedorahosted.org/sssd/ticket/3167
Signed-off-by: Fabiano Fidêncio
---
src/responder/secrets/local.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
index 5b5745d..b13e77f 100644
--- a/src/responder/secrets/local.c
+++ b/src/responder/secrets/local.c
@@ -372,14 +372,43 @@ int local_db_delete(TALLOC_CTX *mem_ctx,
struct local_context *lctx,
const char *req_path)
{
+TALLOC_CTX *tmp_ctx;
struct ldb_dn *dn;
+static const char *attrs[] = { NULL };
+struct ldb_result *res;
int ret;
+tmp_ctx = talloc_new(mem_ctx);
+if (!tmp_ctx) return ENOMEM;
+
ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn);
-if (ret != EOK) return ret;
+if (ret != EOK) goto done;
+
+ret = ldb_search(lctx->ldb, tmp_ctx, &res, dn, LDB_SCOPE_BASE,
+attrs, LOCAL_CONTAINER_FILTER);
+if (ret != EOK) goto done;
+
+if (res->count == 1) {
+ret = ldb_search(lctx->ldb, tmp_ctx, &res, dn, LDB_SCOPE_ONELEVEL,
+ attrs, NULL);
+if (ret != EOK) goto done;
+
+if (res->count > 0) {
+ret = EEXIST;
+DEBUG(SSSDBG_OP_FAILURE,
+ "Failed to remove '%s': Container is not empty\n",
+ ldb_dn_get_linearized(dn));
+
+goto done;
+}
+}
ret = ldb_delete(lctx->ldb, dn);
-return sysdb_error_to_errno(ret);
+ret = sysdb_error_to_errno(ret);
+
+done:
+talloc_free(tmp_ctx);
+return ret;
}
int local_db_create(TALLOC_CTX *mem_ctx,
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#22] LDAP: Return partial results from adminlimit exceeded (+Accepted)
jhrozek's pull request #22: "LDAP: Return partial results from adminlimit exceeded" label *Accepted* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/22 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#22] LDAP: Return partial results from adminlimit exceeded (-Changes requested)
jhrozek's pull request #22: "LDAP: Return partial results from adminlimit exceeded" label *Changes requested* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/22 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#20] sss_override fails to export (comment)
lslebodn commented on a pull request """ On (09/09/16 05:50), mzidek-rh wrote: >See the updated tests. It uses export and import and checks if override works >after import. > ACK for test; just waiting for CI LS """ See the full comment at https://github.com/SSSD/sssd/pull/20#issuecomment-246641793 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#20] sss_override fails to export (comment)
lslebodn commented on a pull request """ On (09/09/16 05:50), mzidek-rh wrote: >See the updated tests. It uses export and import and checks if override works >after import. > ACK http://sssd-ci.duckdns.org/logs/job/53/24/summary.html LS """ See the full comment at https://github.com/SSSD/sssd/pull/20#issuecomment-246656016 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#20] sss_override fails to export (comment)
lslebodn commented on a pull request """ On (09/09/16 05:50), mzidek-rh wrote: >See the updated tests. It uses export and import and checks if override works >after import. > master: * 1c72723cde8bea0d390b928c7cd29e48e7a7deab * 07e7683f5a86991feaa764e2055116554ada1b93 LS """ See the full comment at https://github.com/SSSD/sssd/pull/20#issuecomment-246657182 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#23] sss_groupshow does not work with mpg (comment)
lslebodn commented on a pull request """ On (12/09/16 10:58), mzidek-rh wrote: >We recently fixed this with normal groups. The MPG code path was still using >shortname in sysdb search operation. > >Upstream ticket: >https://fedorahosted.org/sssd/ticket/3184 ACK http://sssd-ci.duckdns.org/logs/job/53/25/summary.html LS """ See the full comment at https://github.com/SSSD/sssd/pull/23#issuecomment-246658318 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#23] sss_groupshow does not work with mpg (comment)
lslebodn commented on a pull request """ master: * bb14556c1df503314644fc424fbbf95759791db9 * 812bed08943df8bf3fd1ff9eabcaf5bedc635c92 """ See the full comment at https://github.com/SSSD/sssd/pull/23#issuecomment-246659465 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PROXY: Adding proxy_max_children option
On Mon, Sep 12, 2016 at 9:40 AM, Petr Cech wrote: > Bump. > > > -- > Petr^4 Čech > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org Patch looks good and all the requested changed were done. I haven't done any tests with the patch, but the changes themselves look good to me. Best Regards, -- Fabiano Fidêncio ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (comment)
lslebodn commented on a pull request """ On (07/09/16 10:13), Jakub Hrozek wrote: >On Tue, Sep 06, 2016 at 06:09:58AM -0700, Jakub Hrozek wrote: >> good idea > >ah, only when I started to implement this I realized it's already done :) > >See: > > https://github.com/SSSD/sssd/blob/master/src/providers/krb5/krb5_child.c#L1364 >in the current master, KRB5_CHILD_DEBUG() expands into sss_log() as well >unconditionally. > Does it mean that the latest version of patch is Accepted? Or what is a state of this patch? LS """ See the full comment at https://github.com/SSSD/sssd/pull/15#issuecomment-246661975 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (-Changes requested)
jhrozek's pull request #15: "Avoid returning System Error on clock skew" label *Changes requested* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/15 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (+Accepted)
jhrozek's pull request #15: "Avoid returning System Error on clock skew" label *Accepted* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/15 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (comment)
sumit-bose commented on a pull request """ I think it is still an ACK. """ See the full comment at https://github.com/SSSD/sssd/pull/15#issuecomment-246674999 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#23] sss_groupshow does not work with mpg (+Pushed)
mzidek-rh's pull request #23: "sss_groupshow does not work with mpg" label *Pushed* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/23 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#23] sss_groupshow does not work with mpg (closed)
mzidek-rh's pull request #23: "sss_groupshow does not work with mpg" was closed See the full pull-request at https://github.com/SSSD/sssd/pull/23 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/23/head:pr23 git checkout pr23 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (comment)
lslebodn commented on a pull request """ On (13/09/16 06:07), sumit-bose wrote: >I think it is still an ACK. > master: * d3348f49260998880bb7cd3b2fb72d562b1b7a64 LS """ See the full comment at https://github.com/SSSD/sssd/pull/15#issuecomment-246680797 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (+Pushed)
jhrozek's pull request #15: "Avoid returning System Error on clock skew" label *Pushed* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/15 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (-Accepted)
jhrozek's pull request #15: "Avoid returning System Error on clock skew" label *Accepted* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/15 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (closed)
jhrozek's pull request #15: "Avoid returning System Error on clock skew" was closed See the full pull-request at https://github.com/SSSD/sssd/pull/15 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/15/head:pr15 git checkout pr15 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache
On 09/12/2016 10:01 AM, Lukas Slebodnik wrote: On (11/09/16 23:49), Jakub Hrozek wrote: On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote: Let me explain why wrappers are not good idea in production. There was introduced new wrapper(#1991) for ldb_search SSS_LDB_SEARCH. It should guarantee that ENONET will be returned and not EOK + res->count == 0. I found just a single usage of this wrapper since introducing but many usage of ldb_search (I stopped counting after 10). And there will be the same problem with newly introduced wrappers. It's crystal clear that review does not help. Otherwise we would use SSS_LDB_SEARCH everywhere. That is a reason why I am fine with using wrappers just for a for development but not for productions. Or try to propose some automatic way how to simply log ldifs for *ALL* required ldb functions. IMHO, it would be the best to implement it in libldb itself. You have a point here (and I regret adding the ENOENT retval in general, but the difference is that ldb_search wrapper changes /functionality/, this just adds logging. So the only thing we would miss if we forget to use the wrapper is the extra debugging. And in that case we would have to build a new package and commit the messages to master, but that's no different from missing debug messages in general. Inconsistencies are bad. it does not matter wheter it's about ENOENT or logging. There's been quite a few cases where I wanted to see what exactly is being added for example with duplicate member: attributes or a member attribute that points nowhere or 'binary' attributes. These patches would solve it nicely. And for such few cases you can prepare custom build of sssd. I am fine with first 3 patches but rest of patches (replacement ldb_* with wrappers) have NACK or a) you can find other way how to consistently log messages from ldb_ functions BTW you will still be able to prepare test builds for debugging because wrappers will be part of upstream (but will be ununsed by default.) Hello Lukas, I will be glad if we push first 3 patches now. I am not happy we have no consensus for the other 4. We could discuss if there is other way how to consistently log messages from ldb_ functions on SSSD meeting. Regards -- Petr^4 Čech ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] SDAP: Fix settig paging attribute in sdap_get_generic_ext_send
On (09/09/16 13:09), Sumit Bose wrote: >On Fri, Sep 09, 2016 at 12:48:47PM +0200, Lukas Slebodnik wrote: >> On (31/08/16 07:45), Fabiano Fidêncio wrote: >> >On Tue, Aug 30, 2016 at 5:26 PM, Lukas Slebodnik >> >wrote: >> >> ehlo, >> >> >> >> We should set pagging flag in state and not in local >> >> variable which is not read anywhere in the function. >> >> >> >> Found by clang static analyzer. >> >> >> >> Do we need this patch also to stable branch? >> >> >> >> LS >> >> >> >> >> > >> >Acked-by: Fabiano Fidêncio >> >> Thank you for review. >> >> But I would like to know what other developers think >> about backporting this patch to stable branch. > >The old code is really wrong and the patch is quite simple and touches >only a single function, so I don't mind backporting it, so ACK. > >Nevertherless I think the chances are quite low that the old code might >cause issues. It looks like the deref/asq code paths already set the >flag. And according to https://fedorahosted.org/sssd/ticket/1202 paging >is disabled for base searches to not consume unneeded resources on the >server side, but I'm not aware of related bug reports although the old >code is around since some time. > master: * 6c335dee38da943796710b5e336472a10cf641f2 sssd-1-13: * d8cf127e988f19f66e00d40b127e488973376ef9 LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PROXY: Adding proxy_max_children option
On (13/09/16 14:11), Fabiano Fidêncio wrote: >On Mon, Sep 12, 2016 at 9:40 AM, Petr Cech wrote: >> Bump. >> >> >> -- >> Petr^4 Čech >> ___ >> sssd-devel mailing list >> sssd-devel@lists.fedorahosted.org >> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org > >Patch looks good and all the requested changed were done. >I haven't done any tests with the patch, but the changes themselves >look good to me. > master: * aef0171e0bdc9a683958d69c7ee984fb10cd5de7 http://sssd-ci.duckdns.org/logs/job/53/30/summary.html LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PROXY: Adding proxy_max_children option
On (13/09/16 16:24), Lukas Slebodnik wrote: >On (13/09/16 14:11), Fabiano Fidêncio wrote: >>On Mon, Sep 12, 2016 at 9:40 AM, Petr Cech wrote: >>> Bump. >>> >>> >>> -- >>> Petr^4 Čech >>> ___ >>> sssd-devel mailing list >>> sssd-devel@lists.fedorahosted.org >>> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org >> >>Patch looks good and all the requested changed were done. >>I haven't done any tests with the patch, but the changes themselves >>look good to me. >> >master: >* aef0171e0bdc9a683958d69c7ee984fb10cd5de7 > >http://sssd-ci.duckdns.org/logs/job/53/30/summary.html > Could you also prepare patch for 1.13 branch? LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] SECRETS: Search by the right type when checking containers
On Tue, Aug 30, 2016 at 11:08:48AM +0200, Fabiano Fidêncio wrote:
> We've been searching for the wrong type ("simple") in
> local_db_check_containers(), which always gives us a NULL result.
>
> Let's introduce the new LOCAL_CONTAINER_FILTER and do the search for the
> right type ("container") from now on.
ACK, verified using a new test:
https://github.com/jhrozek/sssd/commit/0c9c39d6523fbe6106d6654b851d323245ef69f5#diff-7ab0d6e768219c391f1461663675c9a1R130
CI is pending..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] SECRETS: Search by the right type when checking containers
On Tue, Sep 13, 2016 at 04:30:57PM +0200, Jakub Hrozek wrote:
> On Tue, Aug 30, 2016 at 11:08:48AM +0200, Fabiano Fidêncio wrote:
> > We've been searching for the wrong type ("simple") in
> > local_db_check_containers(), which always gives us a NULL result.
> >
> > Let's introduce the new LOCAL_CONTAINER_FILTER and do the search for the
> > right type ("container") from now on.
>
> ACK, verified using a new test:
>
> https://github.com/jhrozek/sssd/commit/0c9c39d6523fbe6106d6654b851d323245ef69f5#diff-7ab0d6e768219c391f1461663675c9a1R130
>
> CI is pending..
Actually here it is:
http://sssd-ci.duckdns.org/logs/job/53/31/summary.html
* master: a8361f37af31a8a9767056bd27c418c947293f56
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] ini_augment: Use full path when reporting pattern mismatch
On (01/09/16 17:35), Michal Židek wrote: >On 09/01/2016 05:26 PM, Dmitri Pal wrote: >> Hello, >> >> I do not like either of the versions of the patch. >> It is OK to use path_concat instead of snprintf. The whole point of not >> using it was to simplify the code and not have to check yet another >> error clause. But using path_concat is fine. >> The thing that I do not like is that in the current code it is done in >> both branches of the if clause. >> I think we should move the path_concat call above the >> >> |/* Match names */ match = ini_aug_match_name(entry->d_name, ra_regex);| >> >> Logic would be: >> - create the full path >> - check errors >> - call the function above >> - use full name in both clauses as needed >> >> That approach IMO would be cleaner. >> >> -- >> Thank you, >> Dmitri Pal >> >> Engineering Director, Identity Management and Platform Security >> Red Hat, Inc. >> > >Indeed. New patch is attached. > >Michal > >From 5079c97723e2865b41daa4bebe6b43e0e6685fe3 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Michal=20=C5=BDidek?= >Date: Thu, 1 Sep 2016 12:59:07 +0200 >Subject: [PATCH] ini_augment: Use full path when reporting pattern mismatch > >We used full path name when reporting access check >failures but only write filename when reporting >on pattern mismatch. This inconsistency does not >look good when the messages are used in sssctl >config-check. > >Resolves: >https://fedorahosted.org/sssd/ticket/3166 >--- > ini/ini.d/merge.validator | 16 > ini/ini_augment.c | 15 ++- > 2 files changed, 18 insertions(+), 13 deletions(-) > ACK LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] ini_augment: Use full path when reporting pattern mismatch
On 09/13/2016 04:53 PM, Lukas Slebodnik wrote: On (01/09/16 17:35), Michal Židek wrote: On 09/01/2016 05:26 PM, Dmitri Pal wrote: Hello, I do not like either of the versions of the patch. It is OK to use path_concat instead of snprintf. The whole point of not using it was to simplify the code and not have to check yet another error clause. But using path_concat is fine. The thing that I do not like is that in the current code it is done in both branches of the if clause. I think we should move the path_concat call above the |/* Match names */ match = ini_aug_match_name(entry->d_name, ra_regex);| Logic would be: - create the full path - check errors - call the function above - use full name in both clauses as needed That approach IMO would be cleaner. -- Thank you, Dmitri Pal Engineering Director, Identity Management and Platform Security Red Hat, Inc. Indeed. New patch is attached. Michal From 5079c97723e2865b41daa4bebe6b43e0e6685fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Thu, 1 Sep 2016 12:59:07 +0200 Subject: [PATCH] ini_augment: Use full path when reporting pattern mismatch We used full path name when reporting access check failures but only write filename when reporting on pattern mismatch. This inconsistency does not look good when the messages are used in sssctl config-check. Resolves: https://fedorahosted.org/sssd/ticket/3166 --- ini/ini.d/merge.validator | 16 ini/ini_augment.c | 15 ++- 2 files changed, 18 insertions(+), 13 deletions(-) ACK LS ding-libs master: commit 72ca1a3552fdc33b7fa7e832ecfc235dd685a7e7 Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PROXY: Adding proxy_max_children option
On 09/13/2016 04:27 PM, Lukas Slebodnik wrote:
On (13/09/16 16:24), Lukas Slebodnik wrote:
On (13/09/16 14:11), Fabiano Fidêncio wrote:
On Mon, Sep 12, 2016 at 9:40 AM, Petr Cech wrote:
Bump.
--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
Patch looks good and all the requested changed were done.
I haven't done any tests with the patch, but the changes themselves
look good to me.
master:
* aef0171e0bdc9a683958d69c7ee984fb10cd5de7
http://sssd-ci.duckdns.org/logs/job/53/30/summary.html
Could you also prepare patch for 1.13 branch?
Yes, see attachment, please.
Regards
--
Petr^4 Čech
>From 476ec80536205bb538c329252a6a162009210253 Mon Sep 17 00:00:00 2001
From: Petr Cech
Date: Wed, 24 Aug 2016 14:41:09 +0200
Subject: [PATCH] PROXY: Adding proxy_max_children option
The new option 'proxy_max_children' is applicable
in domain section. Default value is 10.
Resolves:
https://fedorahosted.org/sssd/ticket/3153
---
src/confdb/confdb.h | 1 +
src/config/SSSDConfig/__init__.py.in | 3 +++
src/config/etc/sssd.api.d/sssd-proxy.conf | 1 +
src/man/sssd.conf.5.xml | 17 +
src/providers/proxy/proxy_init.c | 21 +++--
5 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 6d8601b31cf4ce1a42f824a8400cef8c4ffadf9a..3161fc2181f9af641c3019adbdb67bcb417efdd8 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -215,6 +215,7 @@
#define CONFDB_PROXY_LIBNAME "proxy_lib_name"
#define CONFDB_PROXY_PAM_TARGET "proxy_pam_target"
#define CONFDB_PROXY_FAST_ALIAS "proxy_fast_alias"
+#define CONFDB_PROXY_MAX_CHILDREN "proxy_max_children"
struct confdb_ctx;
struct config_file_ctx;
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index a400c831eb0e44f562c010f2a3649def21913287..56ede34c4b4bf8002f0fe4ac8212ed8523726092 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -419,6 +419,9 @@ option_strings = {
'default_shell' : _('Default shell, /bin/bash'),
'base_directory' : _('Base for home directories'),
+# [provider/proxy]
+'proxy_max_children' : _('The number of preforked proxy children.'),
+
# [provider/proxy/id]
'proxy_lib_name' : _('The name of the NSS library to use'),
'proxy_fast_alias' : _('Whether to look up canonical group name from cache if possible'),
diff --git a/src/config/etc/sssd.api.d/sssd-proxy.conf b/src/config/etc/sssd.api.d/sssd-proxy.conf
index 89a6503f9b84b7eab5fb3b0dd591dea905b43adb..09bf82affcb4263de3abbb67d1d484f6b01a1824 100644
--- a/src/config/etc/sssd.api.d/sssd-proxy.conf
+++ b/src/config/etc/sssd.api.d/sssd-proxy.conf
@@ -1,4 +1,5 @@
[provider/proxy]
+proxy_max_children = int, None, false
[provider/proxy/id]
proxy_lib_name = str, None, true
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 4f138e21940b1f13d864dd7c461dd981093ed2db..a76b19f447d4e1a441b64f2a4b3b99941b8bf9cd 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -2325,6 +2325,23 @@ pam_account_locked_message = Account locked, please contact help desk.
+
+
+proxy_max_children (integer)
+
+
+This option specifies the number of pre-forked
+proxy children. It is useful for high-load SSSD
+environments where sssd may run out of available
+child slots, which would cause some issues due to
+the requests being queued.
+
+
+Default: 10
+
+
+
+
diff --git a/src/providers/proxy/proxy_init.c b/src/providers/proxy/proxy_init.c
index 0a6b11d4a3f102782322c913d280b26fe47aecab..275a92c47e1009f36b2db51323a7d06858e31692 100644
--- a/src/providers/proxy/proxy_init.c
+++ b/src/providers/proxy/proxy_init.c
@@ -27,6 +27,8 @@
#include "util/sss_format.h"
#include "providers/proxy/proxy.h"
+#define OPT_MAX_CHILDREN_DEFAULT 10
+
static int client_registration(struct sbus_request *dbus_req, void *data);
static struct data_provider_iface proxy_methods = {
@@ -478,6 +480,7 @@ int sssm_proxy_auth_init(struct be_ctx *bectx,
int ret;
int hret;
char *sbus_address;
+int max_children;
/* If we're already set up, just return that */
if(bectx->bet_info[BET_AUTH].mod_name &&
@@ -523,8 +526,22 @@ int sssm_proxy_auth_init(struct be_ctx *bectx,
}
/* Set up request hash table */
-/* FIXME: get max_children from confi
