URL: https://github.com/SSSD/sssd/pull/511 Author: fidencio Title: #511: Do not shutdown KCM/Secrets responders when activities are happening ... Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/511/head:pr511 git checkout pr511
From 5eb00fd032cf6393db6ee75a11e81b9192cf1247 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 13:20:31 +0100 Subject: [PATCH 1/8] SECRETS: reset last_request_time on any activity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As all the activities are being handled by the secrets responder itself and not by responder's common code, we have to take care of re-setting the last_request_time by ourselves here. Without this patch, the responder would be shot down after reaching the idle_timeout with activities happening or not. Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/secrets/secsrv_cmd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/responder/secrets/secsrv_cmd.c b/src/responder/secrets/secsrv_cmd.c index fa5970504..1b405a23e 100644 --- a/src/responder/secrets/secsrv_cmd.c +++ b/src/responder/secrets/secsrv_cmd.c @@ -588,6 +588,9 @@ static void sec_fd_handler(struct tevent_context *ev, errno_t ret; struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); + /* Always reset the responder idle timer on any activity */ + cctx->rctx->last_request_time = time(NULL); + /* Always reset the idle timer on any activity */ ret = reset_client_idle_timer(cctx); if (ret != EOK) { From cd172dd8ba979b8fb029c487343d32a69f973afb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 13:24:05 +0100 Subject: [PATCH 2/8] KCM: reset last_request_time on any activity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As all the activities are being handled by the kcm responder itself and not by responder's common code, we have to take care of re-setting the last_request_time by ourselves here. Without this patch, the responder would be shot down after reaching the idle_timeout with activities happening or not. Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/kcm/kcmsrv_cmd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/responder/kcm/kcmsrv_cmd.c b/src/responder/kcm/kcmsrv_cmd.c index 0b933f0b4..903c89417 100644 --- a/src/responder/kcm/kcmsrv_cmd.c +++ b/src/responder/kcm/kcmsrv_cmd.c @@ -588,6 +588,9 @@ static void kcm_fd_handler(struct tevent_context *ev, errno_t ret; struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); + /* Always reset the responder idle timer on any activity */ + cctx->rctx->last_request_time = time(NULL); + /* Always reset the idle timer on any activity */ ret = reset_client_idle_timer(cctx); if (ret != EOK) { From 42d0f1ae4392f43d04740b3696e8b1d4d9429c03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 17:06:39 +0100 Subject: [PATCH 3/8] RESPONDER: Add sss_client_fd_handler() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we have 3 functions to handle client fds: - sec_fd_handler(): for secrets responder - kcm_fd_handler(): for kcm responder - client_fd_handler(): for all the others reponders As those functions only differ by the functions used to handle sending and receiving data to the fds, let's create a generic function that receives the specific send_fn() and recv_fn() functions. With this newly introduced function we'll be able to simply remove duplicated code from those 3 handlers and just call sss_client_fd_handler() from all of those. Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/common/responder.h | 5 +++++ src/responder/common/responder_common.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index 9400e4b60..987a5d17d 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -197,6 +197,11 @@ typedef int (*connection_setup_t)(struct cli_ctx *cctx); int sss_connection_setup(struct cli_ctx *cctx); +void sss_client_fd_handler(void *ptr, + void (*recv_fn) (struct cli_ctx *cctx), + void (*send_fn) (struct cli_ctx *cctx), + uint16_t flags); + int sss_process_init(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct confdb_ctx *cdb, diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index 6130c1201..e2ac34651 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -982,6 +982,37 @@ int activate_unix_sockets(struct resp_ctx *rctx, return ret; } +void sss_client_fd_handler(void *ptr, + void (*recv_fn) (struct cli_ctx *cctx), + void (*send_fn) (struct cli_ctx *cctx), + uint16_t flags) +{ + errno_t ret; + struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); + + /* Always reset the responder idle timer on any activity */ + cctx->rctx->last_request_time = time(NULL); + + /* Always reset the client idle timer on any activity */ + ret = reset_client_idle_timer(cctx); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Could not create idle timer for the client. " + "This connection may not auto-terminate.\n"); + /* Non-fatal, continue */ + } + + if (flags & TEVENT_FD_READ) { + recv_fn(cctx); + return; + } + + if (flags & TEVENT_FD_WRITE) { + send_fn(cctx); + return; + } +} + int sss_connection_setup(struct cli_ctx *cctx) { cctx->protocol_ctx = talloc_zero(cctx, struct cli_protocol); From e48f1a6fe1c2842f10897a10b31e39093ba70863 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 17:15:10 +0100 Subject: [PATCH 4/8] RESPONDER: Make use of sss_client_fd_handler() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make use of the sss_client_fd_handler() on client_fd_handler(). Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/common/responder_common.c | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index e2ac34651..1ea13207b 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -441,29 +441,7 @@ static void client_fd_handler(struct tevent_context *ev, struct tevent_fd *fde, uint16_t flags, void *ptr) { - errno_t ret; - struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); - - /* Always reset the idle timer on any activity */ - cctx->rctx->last_request_time = time(NULL); - - /* Always reset the idle timer on any activity */ - ret = reset_client_idle_timer(cctx); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Could not create idle timer for client. " - "This connection may not auto-terminate\n"); - /* Non-fatal, continue */ - } - - if (flags & TEVENT_FD_READ) { - client_recv(cctx); - return; - } - if (flags & TEVENT_FD_WRITE) { - client_send(cctx); - return; - } + sss_client_fd_handler(ptr, client_recv, client_send, flags); } static errno_t setup_client_idle_timer(struct cli_ctx *cctx); From 78a5588151d1594929f5910ca291b0f7c75a14a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 17:21:48 +0100 Subject: [PATCH 5/8] SECRETS: Make use of sss_client_fd_handler() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make use of the sss_client_fd_handler() on sec_fd_handler(). Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/secrets/secsrv_cmd.c | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/responder/secrets/secsrv_cmd.c b/src/responder/secrets/secsrv_cmd.c index 1b405a23e..9664d666d 100644 --- a/src/responder/secrets/secsrv_cmd.c +++ b/src/responder/secrets/secsrv_cmd.c @@ -585,29 +585,7 @@ static void sec_fd_handler(struct tevent_context *ev, struct tevent_fd *fde, uint16_t flags, void *ptr) { - errno_t ret; - struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); - - /* Always reset the responder idle timer on any activity */ - cctx->rctx->last_request_time = time(NULL); - - /* Always reset the idle timer on any activity */ - ret = reset_client_idle_timer(cctx); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Could not create idle timer for client. " - "This connection may not auto-terminate\n"); - /* Non-fatal, continue */ - } - - if (flags & TEVENT_FD_READ) { - sec_recv(cctx); - return; - } - if (flags & TEVENT_FD_WRITE) { - sec_send(cctx); - return; - } + sss_client_fd_handler(ptr, sec_recv, sec_send, flags); } static http_parser_settings sec_callbacks = { From 76df0e55c007ef7ee1ab292b9fe4bb43d8a26894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 17:27:35 +0100 Subject: [PATCH 6/8] KCM: Make use of sss_client_fd_handler() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make use of the sss_client_fd_handler() on kcm_fd_handler() Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/kcm/kcmsrv_cmd.c | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/responder/kcm/kcmsrv_cmd.c b/src/responder/kcm/kcmsrv_cmd.c index 903c89417..fb9f276ac 100644 --- a/src/responder/kcm/kcmsrv_cmd.c +++ b/src/responder/kcm/kcmsrv_cmd.c @@ -585,29 +585,7 @@ static void kcm_fd_handler(struct tevent_context *ev, struct tevent_fd *fde, uint16_t flags, void *ptr) { - errno_t ret; - struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx); - - /* Always reset the responder idle timer on any activity */ - cctx->rctx->last_request_time = time(NULL); - - /* Always reset the idle timer on any activity */ - ret = reset_client_idle_timer(cctx); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Could not create idle timer for client. " - "This connection may not auto-terminate\n"); - /* Non-fatal, continue */ - } - - if (flags & TEVENT_FD_READ) { - kcm_recv(cctx); - return; - } - if (flags & TEVENT_FD_WRITE) { - kcm_send(cctx); - return; - } + sss_client_fd_handler(ptr, kcm_recv, kcm_send, flags); } int kcm_connection_setup(struct cli_ctx *cctx) From b51e3f4b8c75296db1564a1d5f4f0996e6243e0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 13:26:46 +0100 Subject: [PATCH 7/8] TESTS: Rename test_idle_timeout() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As this test is related to the client_idle_timeout, let's rename it accordingly. Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/tests/intg/test_secrets.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py index a145045ee..96b6f6b4a 100644 --- a/src/tests/intg/test_secrets.py +++ b/src/tests/intg/test_secrets.py @@ -369,7 +369,7 @@ def get_fds(pid): def setup_for_cli_timeout_test(request): """ Same as the generic setup, except a short client_idle_timeout so that - the test_idle_timeout() test closes the fd towards the client. + the test_cli_idle_timeout() test closes the fd towards the client. """ conf = generate_sec_config() + \ unindent(""" @@ -380,7 +380,7 @@ def setup_for_cli_timeout_test(request): return create_sssd_secrets_fixture(request) -def test_idle_timeout(setup_for_cli_timeout_test): +def test_cli_idle_timeout(setup_for_cli_timeout_test): """ Test that idle file descriptors are reaped after the idle timeout passes From e0c525cc91477b3e7d137e07b804de6ea7c35b4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 7 Feb 2018 13:26:46 +0100 Subject: [PATCH 8/8] TESTS: Add test for responder_idle_timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new tests have been added in order to test the following scenarios of responder_idle_timeout: - responder is shutdown after n seconds; - responder has its shutdown delayed due to some activity and then is shutdown after n seconds; In order to have the tests added, a new dep has been introduced: python-psutil Keep in mind those newly added tests make our test suite to take a few minutes more to finish. Resolves: https://pagure.io/SSSD/sssd/issue/3633 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- contrib/ci/deps.sh | 2 + src/tests/intg/test_secrets.py | 84 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index 4467e117c..23379ac9d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -42,6 +42,7 @@ if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then openldap-servers pytest python-ldap + python-psutil pyldb rpm-build uid_wrapper @@ -118,6 +119,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap python-ldb python-requests + python-psutil ldap-utils slapd systemtap-sdt-dev diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py index 96b6f6b4a..3632351cc 100644 --- a/src/tests/intg/test_secrets.py +++ b/src/tests/intg/test_secrets.py @@ -26,6 +26,7 @@ import time import socket import pytest +import psutil from requests import HTTPError from util import unindent @@ -41,7 +42,7 @@ def create_conf_fixture(request, contents): request.addfinalizer(lambda: os.unlink(config.CONF_PATH)) -def create_sssd_secrets_fixture(request): +def create_sssd_secrets_fixture(request, teardown=True): if subprocess.call(['sssd', "--genconf"]) != 0: raise Exception("failed to regenerate confdb") @@ -72,13 +73,21 @@ def create_sssd_secrets_fixture(request): assert os.path.exists(sock_path) + def unlink_secdb(): + for secdb_file in os.listdir(config.SECDB_PATH): + os.unlink(config.SECDB_PATH + "/" + secdb_file) + def sec_teardown(): + if teardown is False: + unlink_secdb() + return + if secpid == 0: return os.kill(secpid, signal.SIGTERM) - for secdb_file in os.listdir(config.SECDB_PATH): - os.unlink(config.SECDB_PATH + "/" + secdb_file) + unlink_secdb() + request.addfinalizer(sec_teardown) return secpid @@ -602,3 +611,72 @@ def test_unlimited_quotas(setup_for_unlimited_quotas, secrets_cli): for i in range(DEFAULT_CONTAINERS_NEST_LEVEL): container += "%s/" % str(i) cli.create_container(container) + + +@pytest.fixture +def setup_for_resp_timeout_test(request): + """ + Same as the generic setup, except a short responder_idle_timeout + so that the test_responder_idle_timeout() test verifies that the + responder has been shot down. + """ + conf = generate_sec_config() + \ + unindent(""" + responder_idle_timeout = 60 + """).format() + + create_conf_fixture(request, conf) + return create_sssd_secrets_fixture(request, False) + + +def test_resp_idle_timeout_shutdown(setup_for_resp_timeout_test): + """ + Test that the responder is shutdown after the respoder_idle_timeout is + over + """ + secpid = setup_for_resp_timeout_test + p = psutil.Process(secpid) + + # With the responder_idle_timeout set to 60 seconds, we need to wait at + # least 90, because the internal timer ticks every timeout/2 seconds, so + # so it would tick at 30, 60 and 90 seconds and the responder_idle_timeout + # uses a greater-than comparison, so the 60-seconds tick wouldn't yet + # trigger the process' shutdown. + p.wait(timeout=90) + assert p.is_running() is False + + +def test_resp_idle_timeout_postpone_shutdown(setup_for_resp_timeout_test, + secrets_cli): + """ + Test that the responder's shutdown is postponed in case an activity + happens, but it's still shutdown after the responder_idle_timeout is + over + """ + cli = secrets_cli + + secpid = setup_for_resp_timeout_test + p = psutil.Process(secpid) + + # Wait for 65 seconds and then fire a request to the responder, so its + # last_request_time gets updated and the process doesn't get shutdown. + time.sleep(65) + cli.set_secret("foo", "bar") + try: + # Wait for the process to finish for more 25 seconds, which is the + # time it'd be shutdown in case the last_request_time is not updated. + p.wait(timeout=25) + except psutil.TimeoutExpired: + # In case the timeout expired, we're fine, it just means that the + # last_request_time has been updated properly. + pass + + # Assert that the process is still running after the 60s idle timeout has + # expired but some activity happened (thus,the last_request_time has been + # updated). + assert p.is_running() is True + + # Wait more 60s in order to be sure that the process actually is shutdown + # when it should be. + p.wait(timeout=60) + assert p.is_running() is False
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
