From 3cefb01d7110508258ba883c19189f7cd1333b6e Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Thu, 8 Oct 2026 18:28:58 +0200 Subject: [PATCH] fix(web-app-suitecrm): keep sessions in Redis so replicas share them With three swarm replicas the PHP session files on the shared NFS volume were read as empty under concurrent requests of one browser, so SuiteCRM answered 403 on /api/graphql and dropped the session. Sessions now live in Redis with session locking that waits like flock did; the phpredis version is pinned in services.yml with its own update source, and the engine lookup renders the save path. Refs #879 Co-Authored-By: Claude Opus 5.5 (1M context) --- plugins/lookup/engine.py | 35 +++++++++++++ roles/web-app-suitecrm/files/Dockerfile | 5 ++ roles/web-app-suitecrm/meta/services.yml | 24 +++++++-- roles/web-app-suitecrm/meta/variants.yml | 4 ++ .../web-app-suitecrm/templates/compose.yml.j2 | 1 + roles/web-app-suitecrm/templates/env.j2 | 2 + roles/web-app-suitecrm/templates/php.ini.j2 | 6 ++- roles/web-app-suitecrm/vars/main.yml | 1 + .../lookup/test_engine_redis_session_path.py | 49 +++++++++++++++++++ 9 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 tests/unit/python/plugins/lookup/test_engine_redis_session_path.py diff --git a/plugins/lookup/engine.py b/plugins/lookup/engine.py index 5c769cc538..c428c9acdf 100644 --- a/plugins/lookup/engine.py +++ b/plugins/lookup/engine.py @@ -2,6 +2,7 @@ {{ lookup('engine', 'redis', application_id) }} # full dict {{ lookup('engine', 'redis', application_id, 'url') }} # one field + {{ lookup('engine', 'redis', application_id, 'session_path') }} # phpredis session.save_path Mirrors the ``database`` lookup but for the non-RDBMS engines registered in :data:`utils.roles.applications.services.engines.ENGINES` (redis, memcached, @@ -16,6 +17,7 @@ See ``docs/architecture/central-engines.md``. from __future__ import annotations from typing import Any +from urllib.parse import quote from ansible.errors import AnsibleError from ansible.plugins.loader import lookup_loader @@ -44,9 +46,31 @@ _FIELDS = ( "prefix", "url", "container", + "session_path", ) +def redis_session_path( + host: str, port: int, prefix: str, username: str, password: str +) -> str: + """Return the phpredis ``session.save_path`` of one consumer. + + Args: + host: redis host the consumer reaches. + port: redis port. + prefix: consumer prefix; session keys become ``:session:``. + username: ACL user, sent only together with a password. + password: redis password; empty for an embedded sidecar. + """ + path = f"tcp://{host}:{port}?prefix={quote(prefix, safe='')}:session:" + if password: + path += ( + f"&auth[user]={quote(username, safe='')}" + f"&auth[pass]={quote(password, safe='')}" + ) + return path + + def qdrant_consumer_key(applications: dict[str, Any], svc_id: str) -> str: """Return the engine-wide key a data-owning qdrant consumer presents. @@ -197,6 +221,17 @@ class LookupModule(LookupBase): "prefix": prefix, "url": url, "container": container, + "session_path": ( + redis_session_path( + host, + port, + prefix, + username if shared else "", + password if shared else "", + ) + if engine == "redis" + else "" + ), } if want == "all": return [resolved] diff --git a/roles/web-app-suitecrm/files/Dockerfile b/roles/web-app-suitecrm/files/Dockerfile index 681672774b..08c8f95e71 100644 --- a/roles/web-app-suitecrm/files/Dockerfile +++ b/roles/web-app-suitecrm/files/Dockerfile @@ -5,6 +5,7 @@ FROM ${SUITECRM_BASE_IMAGE}:${SUITECRM_BASE_VERSION} AS base ARG SUITECRM_APP_DIR ARG SUITECRM_PHP_MEMORY_LIMIT +ARG SUITECRM_PHPREDIS_VERSION RUN rm -rf /var/lib/apt/lists/* \ && apt-get -o Acquire::Retries=3 update && apt-get -o Acquire::Retries=3 install -y \ @@ -32,6 +33,10 @@ RUN rm -rf /var/lib/apt/lists/* \ ldap \ && rm -rf /var/lib/apt/lists/* +RUN : "${SUITECRM_PHPREDIS_VERSION:?}" \ + && pecl install "redis-${SUITECRM_PHPREDIS_VERSION}" \ + && docker-php-ext-enable redis + RUN : "${SUITECRM_PHP_MEMORY_LIMIT:?}" && { \ echo 'file_uploads = On'; \ echo 'upload_max_filesize = 32M'; \ diff --git a/roles/web-app-suitecrm/meta/services.yml b/roles/web-app-suitecrm/meta/services.yml index 68c73b211d..cc7a861a50 100644 --- a/roles/web-app-suitecrm/meta/services.yml +++ b/roles/web-app-suitecrm/meta/services.yml @@ -29,6 +29,16 @@ mariadb: bond: 1 enabled: true # nocheck: dynamic-flag shared: "{{ 'svc-db-mariadb' in group_names }}" +redis: + bond: 1 + backup: + disabled: true + enabled: true # nocheck: dynamic-flag + shared: "{{ 'svc-db-redis' in group_names }}" + cpus: "0.25" + mem_reservation: 128m + mem_limit: 256m + pids_limit: 256 # nocheck: playwright-service-flag — self-provider entry; test_env_services_match.py:28-32 grants the forbidden-self-gate exemption suitecrm: healthcheck: @@ -49,12 +59,16 @@ suitecrm: version: 8.2-apache custom: true app_version: 8.9.1 + phpredis_version: 6.3.0 update: - key: app_version - type: git_tags - repository: https://github.com/salesagility/SuiteCRM-Core.git - match: "v" - strip: true + - key: app_version + type: git_tags + repository: https://github.com/salesagility/SuiteCRM-Core.git + match: "v" + strip: true + - key: phpredis_version + type: git_tags + repository: https://github.com/phpredis/phpredis.git name: suitecrm min_storage: 376MB cpus: 1.5 diff --git a/roles/web-app-suitecrm/meta/variants.yml b/roles/web-app-suitecrm/meta/variants.yml index 6353170e71..23ed53e8a9 100644 --- a/roles/web-app-suitecrm/meta/variants.yml +++ b/roles/web-app-suitecrm/meta/variants.yml @@ -24,6 +24,8 @@ shared: true mariadb: shared: true + redis: + shared: true matomo: enabled: true shared: true @@ -56,6 +58,8 @@ shared: false # nocheck: variants-static-override backup shared stays literally true mariadb: shared: false # nocheck: variants-static-override backup shared stays literally true + redis: + shared: false matomo: enabled: false shared: false # nocheck: variants-static-override backup shared stays literally true diff --git a/roles/web-app-suitecrm/templates/compose.yml.j2 b/roles/web-app-suitecrm/templates/compose.yml.j2 index 62057023e5..a4a9f433e8 100644 --- a/roles/web-app-suitecrm/templates/compose.yml.j2 +++ b/roles/web-app-suitecrm/templates/compose.yml.j2 @@ -6,6 +6,7 @@ SUITECRM_BASE_IMAGE: "{{ SUITECRM_BASE_IMAGE }}" SUITECRM_BASE_VERSION: "{{ SUITECRM_BASE_VERSION }}" SUITECRM_APP_VERSION: "{{ SUITECRM_APP_VERSION }}" + SUITECRM_PHPREDIS_VERSION: "{{ SUITECRM_PHPREDIS_VERSION }}" SUITECRM_ENTRYPOINT_SCRIPT_HOST_REL: "{{ SUITECRM_ENTRYPOINT_SCRIPT_HOST_REL }}" SUITECRM_ENTRYPOINT_SCRIPT_DOCKER: "{{ SUITECRM_ENTRYPOINT_SCRIPT_DOCKER }}" SUITECRM_LDAP_EXTENSION_DIR: "{{ SUITECRM_LDAP_EXTENSION_DIR }}" diff --git a/roles/web-app-suitecrm/templates/env.j2 b/roles/web-app-suitecrm/templates/env.j2 index 826cb31386..acc107ad15 100644 --- a/roles/web-app-suitecrm/templates/env.j2 +++ b/roles/web-app-suitecrm/templates/env.j2 @@ -21,6 +21,8 @@ SUITECRM_ADMIN_EMAIL={{ lookup('users', 'administrator').email }} SUITECRM_URL={{ SUITECRM_URL }} +SUITECRM_SESSION_SAVE_PATH={{ lookup('engine', 'redis', application_id, 'session_path') }} + {% if SUITECRM_SSO_ENABLED | bool %} AUTH_TYPE=saml diff --git a/roles/web-app-suitecrm/templates/php.ini.j2 b/roles/web-app-suitecrm/templates/php.ini.j2 index e818dbc83e..1ff9b031e4 100644 --- a/roles/web-app-suitecrm/templates/php.ini.j2 +++ b/roles/web-app-suitecrm/templates/php.ini.j2 @@ -3,5 +3,7 @@ display_startup_errors = Off log_errors = On error_reporting = E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED -session.save_handler = files -session.save_path = "{{ SUITECRM_APP_DIR }}/tmp" +session.save_handler = redis +session.save_path = ${SUITECRM_SESSION_SAVE_PATH} +redis.session.locking_enabled = 1 +redis.session.lock_retries = -1 diff --git a/roles/web-app-suitecrm/vars/main.yml b/roles/web-app-suitecrm/vars/main.yml index 2b1708dfdd..aaf9602ea6 100644 --- a/roles/web-app-suitecrm/vars/main.yml +++ b/roles/web-app-suitecrm/vars/main.yml @@ -11,6 +11,7 @@ SUITECRM_PHP_CONFIG_HOST: "{{ [ lookup('container', application_id, 'directories SUITECRM_BASE_IMAGE: "{{ lookup('config', application_id, 'services.' ~ entity_name ~ '.image') }}" SUITECRM_BASE_VERSION: "{{ lookup('config', application_id, 'services.' ~ entity_name ~ '.version') }}" SUITECRM_APP_VERSION: "{{ lookup('config', application_id, 'services.' ~ entity_name ~ '.app_version') }}" +SUITECRM_PHPREDIS_VERSION: "{{ lookup('config', application_id, 'services.' ~ entity_name ~ '.phpredis_version') }}" SUITECRM_CONTAINER: "{{ lookup('config', application_id, 'services.' ~ entity_name ~ '.name') }}" SUITECRM_CONTAINER_ADDRESS: "{{ lookup('container_address', application_id, entity_name) }}" diff --git a/tests/unit/python/plugins/lookup/test_engine_redis_session_path.py b/tests/unit/python/plugins/lookup/test_engine_redis_session_path.py new file mode 100644 index 0000000000..0b8dba098e --- /dev/null +++ b/tests/unit/python/plugins/lookup/test_engine_redis_session_path.py @@ -0,0 +1,49 @@ +"""phpredis reads ``auth[user]``/``auth[pass]`` and ``prefix`` from the query of +``session.save_path``; a password with a reserved character that is not +percent-encoded splits the query and the handler authenticates with a fragment. +""" + +from __future__ import annotations + +import importlib +import unittest +from urllib.parse import parse_qs, urlsplit + +plugin_module = importlib.import_module("plugins.lookup.engine") +redis_session_path = plugin_module.redis_session_path + + +class TestRedisSessionPath(unittest.TestCase): + def test_an_embedded_sidecar_gets_no_credentials(self) -> None: + self.assertEqual( + "tcp://redis:6379?prefix=suitecrm:session:", + redis_session_path("redis", 6379, "suitecrm", "", ""), + ) + + def test_a_shared_redis_gets_user_and_password(self) -> None: + query = parse_qs( + urlsplit( + redis_session_path("redis-central", 6379, "suitecrm", "default", "pw") + ).query + ) + self.assertEqual(["default"], query["auth[user]"]) + self.assertEqual(["pw"], query["auth[pass]"]) + + def test_a_password_with_reserved_characters_survives_the_query(self) -> None: + password = "a&b=c?d#e/f+g h" + query = parse_qs( + urlsplit( + redis_session_path("redis-central", 6379, "suitecrm", "default", password) + ).query + ) + self.assertEqual([password], query["auth[pass]"]) + self.assertEqual(["suitecrm:session:"], query["prefix"]) + + def test_a_user_without_a_password_is_not_sent(self) -> None: + self.assertNotIn( + "auth", redis_session_path("redis", 6379, "suitecrm", "default", "") + ) + + +if __name__ == "__main__": + unittest.main()