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 1196a2dd39edeb432a62cf82a0d43e1f8eb3e0be 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 eeefb4889403f338d44716eceb2617319ce55e13 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 728979da9..9061ff186 100644 --- a/src/responder/kcm/kcmsrv_cmd.c +++ b/src/responder/kcm/kcmsrv_cmd.c @@ -615,6 +615,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 d899224d7e43a246b94ff930ac7da175d6a3bd42 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 6241d2f3b850305188c7d601e3fce0df6d79b7de 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 c2e5a1ef7e6743da147c74e70f26ffe0b35371dd 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 03288a3d60c733509245ba3fdbf7ac3d9a0ca657 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 9061ff186..421bf4bc5 100644 --- a/src/responder/kcm/kcmsrv_cmd.c +++ b/src/responder/kcm/kcmsrv_cmd.c @@ -612,29 +612,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 cc7a9a9c285da7546390c377802faa9085744ab4 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 f36f8e8db2a24aad24bf70c5883151ef48ef37de 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. As it may be an inconvenience for some developers, the tests have been explicitly marked as slow (both by the pytest markdown and by having _slow in their names) and can be skipped by doing: `make intgcheck-run make intgcheck-run \ INTGCHECK_PYTEST_ARGS="-k test_secrets.py -m 'not slow'"` 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 | 85 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index 8287918be..d8b1414c8 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 @@ -120,6 +121,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..6faeae888 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,73 @@ 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) + +@pytest.mark.slow +def test_resp_idle_timeout_shutdown_slow(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 + + +@pytest.mark.slow +def test_resp_idle_timeout_postpone_shutdown_slow(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
