From 1f108e219124c21219815895a591e1c4982d9cfb Mon Sep 17 00:00:00 2001 From: Codex Date: Sun, 1 Nov 2026 18:41:47 +0100 Subject: [PATCH] fix(redis): persist tenant ACLs across Redis-only restarts --- OPERATOR.md | 28 ++- openserverless/redis.py | 77 +++++++- openserverless/templates/redis-conf.yaml | 1 + openserverless/templates/redis-set.yaml | 18 ++ openserverless/util.py | 2 + tests/test_redis_persistence.py | 223 +++++++++++++++++++++++ 6 files changed, 343 insertions(+), 6 deletions(-) create mode 100644 tests/test_redis_persistence.py diff --git a/OPERATOR.md b/OPERATOR.md index 912a1bb..ff5275d 100644 --- a/OPERATOR.md +++ b/OPERATOR.md @@ -242,4 +242,30 @@ spec: ... ``` -In lean mode it is recommended to set at least `configs.controller.javaOpts=-Xmx2048M` \ No newline at end of file +In lean mode it is recommended to set at least `configs.controller.javaOpts=-Xmx2048M` + +## Redis ACL persistence + +With `redis.persistence-enabled: true`, Redis stores its ACLs in +`/bitnami/redis/data/users.acl` on the existing Redis data PVC. Account creation, +reconciliation, quota permission changes and successful revocation save the +current authorized state. An RDB data snapshot alone does not contain ACL users. + +On a fresh installation an init container creates the initial default-user ACL +with the configured administrator password hash. It never overwrites an existing +ACL file and refuses an empty existing file. The system and tenant accounts are +then provisioned and saved through the normal operator lifecycle. + +On operator resume, an existing persistent Redis first saves its live ACL state, +then updates the managed ConfigMap to load that file on the next Redis restart. +Because `aclfile` is immutable at runtime, account changes made before that +first restart also update the file atomically through protected stdin. This +preparation does not restart Redis or replay old ACL exports. Credentials, +key/channel grants, read-only state and successful revocations are preserved. +A failed save stops configuration migration rather than enabling a missing file. +Existing explicitly configured ACL files retain their configured storage path. + +When data persistence is disabled, this change does not introduce an ACL volume +or enable file persistence; ephemeral storage retains its existing semantics. +This fix concerns identity persistence, not the independent RDB snapshot schedule +or recovery of data that was never saved. diff --git a/openserverless/redis.py b/openserverless/redis.py index 86799b8..c6efa79 100644 --- a/openserverless/redis.py +++ b/openserverless/redis.py @@ -25,6 +25,8 @@ import urllib.parse import hashlib import contextlib +import subprocess +import shlex import redis as redislib from openserverless.redis_acl import tenant_rules from openserverless.datastore_connection import service_endpoint @@ -36,6 +38,41 @@ from openserverless.user_config import UserConfig from openserverless.user_metadata import UserMetadata +ACL_FILE = '/bitnami/redis/data/users.acl' + + +def _prepare_existing_acl_storage(data): + """Persist live identities before an existing Redis can load the new config.""" + if not data['persistence']: + return + existing = kube.kubectl('get', 'statefulset', 'redis', '--ignore-not-found', '-o', 'name') + if not existing.strip(): + return + wait_for_redis_ready() + with admin_client() as client, client.lock('__ops_acl_lock__', timeout=120, blocking_timeout=15): + path = _save_acl_if_configured(client) + # Explicitly configured ACL files retain their existing configuration. + if path != ACL_FILE: + return + cm = kube.get('configmap/redis-cm') + if not cm or 'redis-conf' not in cm.get('data', {}): + raise RuntimeError('Redis configuration is unavailable') + text = cm['data']['redis-conf'] + directives = [] + for line in text.splitlines(): + fields = line.strip().split(None, 1) + if fields and fields[0].lower() == 'aclfile': + directives.append(shlex.split(line)[1:]) + directive = 'aclfile ' + ACL_FILE + if directives: + if any(fields != [ACL_FILE] for fields in directives): + raise RuntimeError('Redis ACL file configuration does not match the running server') + return + patch = {'metadata': {'resourceVersion': cm['metadata']['resourceVersion']}, + 'data': {'redis-conf': text.rstrip() + '\n' + directive + '\n'}} + kube.kubectl('patch', 'configmap', 'redis-cm', '--type=merge', '--patch-file=/dev/stdin', + input=json.dumps(patch), timeout=30) + def _add_redis_user_metadata(ucfg: UserConfig, user_metadata:UserMetadata): """ @@ -67,6 +104,7 @@ def _add_redis_user_metadata(ucfg: UserConfig, user_metadata:UserMetadata): def create(owner=None): logging.info("create redis") data = util.get_redis_config_data() + _prepare_existing_acl_storage(data) tplp = ["security-set-attach.yaml","redis-set.yaml"] @@ -97,6 +135,7 @@ def wait_for_redis_ready(): def restore_openserverless_db_user(): data = util.get_redis_config_data() + _prepare_existing_acl_storage(data) create_openserverless_db_user(data) def create_openserverless_db_user(data): @@ -108,7 +147,10 @@ def create_openserverless_db_user(data): pod_name = util.get_pod_name("{.items[?(@.metadata.labels.name == 'redis')].metadata.name}") if(pod_name): - res = exec_redis_command(pod_name,path_to_script) + with admin_client() as client, client.lock('__ops_acl_lock__', timeout=120, blocking_timeout=15): + res = exec_redis_command(pod_name,path_to_script) + if res: + _save_acl_if_configured(client) if(res): redis_service = util.get_service("{.items[?(@.spec.selector.name == 'redis')]}") @@ -218,11 +260,36 @@ def _tenant_identity(namespace, prefix=None): return prefix +def _write_acl_snapshot(client): + """Bootstrap an immutable aclfile setting without logging credential hashes.""" + pod = util.get_pod_name("{.items[?(@.metadata.labels.name == 'redis')].metadata.name}") + if not pod: + raise RuntimeError('Redis pod is unavailable for ACL persistence') + script = ('umask 077; tmp=$(mktemp /bitnami/redis/data/.users.acl.XXXXXX); ' + 'trap \'rm -f "$tmp"\' EXIT; cat > "$tmp"; test -s "$tmp"; ' + 'mv "$tmp" /bitnami/redis/data/users.acl') + # kube.kubectl logs stdin on errors; ACL hashes must not enter that path. + try: + result = subprocess.run(['kubectl', '-n', 'openserverless', 'exec', '-i', pod, + '--', '/bin/sh', '-ec', script], + input=('\n'.join(client.acl_list()) + '\n').encode(), + capture_output=True, timeout=30) + except (subprocess.SubprocessError, OSError): + raise RuntimeError('Could not persist Redis ACL state') from None + if result.returncode: + raise RuntimeError('Could not persist Redis ACL state') + + def _save_acl_if_configured(client): - # Preserve the 0.9.0 storage contract: persist when an ACL file is configured. - # With no ACL file, startup reconciliation recreates identities as before. - if client.config_get('aclfile').get('aclfile'): + path = client.config_get('aclfile').get('aclfile') + if not path and util.get_redis_config_data().get('persistence'): + # aclfile is immutable. Until the next Redis restart, save an atomic + # snapshot of the live authorized state instead of CONFIG SET. + _write_acl_snapshot(client) + return ACL_FILE + if path: client.acl_save() + return path def provision_user(namespace, password, prefix, readonly=False): @@ -239,7 +306,7 @@ def provision_user(namespace, password, prefix, readonly=False): *['#' + value for value in hashes], *tenant_rules(prefix, readonly)) try: _save_acl_if_configured(client) - except redislib.RedisError: + except (redislib.RedisError, RuntimeError): client.acl_deluser(namespace) raise RuntimeError('Could not persist Redis tenant ACL') from None return True diff --git a/openserverless/templates/redis-conf.yaml b/openserverless/templates/redis-conf.yaml index 84e12ec..5effb20 100644 --- a/openserverless/templates/redis-conf.yaml +++ b/openserverless/templates/redis-conf.yaml @@ -49,6 +49,7 @@ data: # the directory where redis will store data. dir /bitnami/redis/data + aclfile /bitnami/redis/data/users.acl {% endif %} diff --git a/openserverless/templates/redis-set.yaml b/openserverless/templates/redis-set.yaml index 8bee1d8..63f3a88 100644 --- a/openserverless/templates/redis-set.yaml +++ b/openserverless/templates/redis-set.yaml @@ -40,6 +40,24 @@ spec: fsGroup: 1001 runAsUser: 1001 #default BITNAMI user {% endif %} + {% if persistence %} + initContainers: + - name: redis-acl-init + image: docker.io/bitnamilegacy/redis:8.2.1 + command: + - /bin/sh + - -ec + - | + umask 077 + if [ ! -e /bitnami/redis/data/users.acl ]; then + set -C + printf '%s\n' 'user default on #{{redis_password_sha256}} ~* &* +@all' > /bitnami/redis/data/users.acl + fi + test -s /bitnami/redis/data/users.acl + volumeMounts: + - name: redis-pvc + mountPath: /bitnami/redis/data + {% endif %} containers: - name: redis image: docker.io/bitnamilegacy/redis:8.2.1 diff --git a/openserverless/util.py b/openserverless/util.py index ebfdd70..017b3e2 100644 --- a/openserverless/util.py +++ b/openserverless/util.py @@ -17,6 +17,7 @@ # # this module wraps utilities functions import logging +import hashlib import math import random import time @@ -495,6 +496,7 @@ def get_redis_config_data(): "size": cfg.get("redis.volume-size", "REDIS_VOLUME_SIZE", 10), "storageClass": cfg.get("openserverless.storageclass"), "redis_password":cfg.get("redis.default.password") or "s0meP@ass3", + "redis_password_sha256": hashlib.sha256((cfg.get("redis.default.password") or "s0meP@ass3").encode()).hexdigest(), "namespace":"openserverless", "password":cfg.get("redis.openserverless.password") or "s0meP@ass3", "prefix": prefix, diff --git a/tests/test_redis_persistence.py b/tests/test_redis_persistence.py new file mode 100644 index 0000000..dfc1f30 --- /dev/null +++ b/tests/test_redis_persistence.py @@ -0,0 +1,223 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +import hashlib +import json +from pathlib import Path +import subprocess +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +import yaml +from openserverless import redis, util +from openserverless.template import expand_template + + +class RedisAclPersistenceTests(unittest.TestCase): + def setUp(self): + self.data = {'persistence': True, 'redis_password': 'synthetic + > password', + 'redis_password_sha256': hashlib.sha256(b'synthetic + > password').hexdigest()} + self.config = patch.object(redis.util, 'get_redis_config_data', return_value=self.data) + self.config.start() + self.addCleanup(self.config.stop) + self.client = MagicMock() + self.client.__enter__.return_value = self.client + self.client.config_get.return_value = {'aclfile': redis.ACL_FILE} + + def test_persistent_installation_bootstraps_current_acl_state(self): + self.client.config_get.return_value = {'aclfile': ''} + with patch.object(redis, '_write_acl_snapshot') as snapshot: + self.assertEqual(redis._save_acl_if_configured(self.client), redis.ACL_FILE) + snapshot.assert_called_once_with(self.client) + self.client.config_set.assert_not_called() + self.client.acl_save.assert_not_called() + self.client.acl_setuser.assert_not_called() + self.client.acl_deluser.assert_not_called() + + def test_ephemeral_installation_does_not_enable_acl_storage(self): + self.data['persistence'] = False + self.client.config_get.return_value = {'aclfile': ''} + self.assertFalse(redis._save_acl_if_configured(self.client)) + self.client.config_set.assert_not_called() + self.client.acl_save.assert_not_called() + + def test_explicit_acl_file_is_preserved(self): + self.client.config_get.return_value = {'aclfile': '/custom/users.acl'} + self.assertEqual(redis._save_acl_if_configured(self.client), '/custom/users.acl') + self.client.config_set.assert_not_called() + self.client.acl_save.assert_called_once() + + def test_revocation_is_saved_before_reporting_success(self): + self.client.config_get.return_value = {'aclfile': redis.ACL_FILE} + with patch.object(redis, 'admin_client', return_value=self.client): + self.assertTrue(redis.delete_db_user('synthetic')) + calls = self.client.method_calls + self.assertLess(next(i for i,c in enumerate(calls) if c[0]=='acl_deluser'), + next(i for i,c in enumerate(calls) if c[0]=='acl_save')) + + def test_failed_revocation_save_does_not_report_success(self): + self.client.acl_save.side_effect = redis.redislib.RedisError('synthetic failure') + with patch.object(redis, 'admin_client', return_value=self.client): + self.assertIsNone(redis.delete_db_user('synthetic')) + + def test_bootstrap_write_failure_closes_new_grant(self): + self.client.config_get.return_value = {'aclfile': ''} + self.client.acl_getuser.return_value = None + with patch.object(redis, 'admin_client', return_value=self.client), patch.object(redis, '_write_acl_snapshot', side_effect=RuntimeError('Could not persist Redis ACL state')): + with self.assertRaisesRegex(RuntimeError, 'Could not persist'): + redis.provision_user('synthetic', 'synthetic-password', 'synthetic:') + self.client.acl_deluser.assert_called_once_with('synthetic') + self.client.config_set.assert_not_called() + + def test_quota_modes_save_without_rotating_credentials(self): + self.client.acl_getuser.return_value = {'passwords': ['original']} + with patch.object(redis, 'admin_client', return_value=self.client): + for readonly in (True, False): + self.assertTrue(redis.set_user_readonly('synthetic', 'synthetic:', readonly)) + self.assertEqual(self.client.acl_save.call_count, 2) + for call in self.client.execute_command.call_args_list: + self.assertFalse(any(str(arg).startswith('#') for arg in call.args)) + + def existing(self, cm=None): + self.client.config_get.return_value = {'aclfile': redis.ACL_FILE} + if cm is None: + cm = {'metadata': {'resourceVersion': '42'}, 'data': {'redis-conf': 'requirepass synthetic\ndir /bitnami/redis/data\n'}} + return (patch.object(redis.kube, 'kubectl', return_value='statefulset.apps/redis'), + patch.object(redis.kube, 'get', return_value=cm), + patch.object(redis, 'wait_for_redis_ready'), + patch.object(redis, 'admin_client', return_value=self.client)) + + def test_existing_acl_is_saved_before_configuration_changes(self): + first, second, third, fourth = self.existing() + events = [] + self.client.acl_save.side_effect = lambda: events.append('save') + with first as kubectl, second, third, fourth: + kubectl.side_effect = lambda *a, **kw: events.append('patch' if a[0]=='patch' else 'get') or 'statefulset.apps/redis' + redis._prepare_existing_acl_storage(self.data) + self.assertLess(events.index('save'), events.index('patch')) + args = kubectl.call_args + payload = json.loads(args.kwargs['input']) + self.assertEqual(payload['metadata']['resourceVersion'], '42') + self.assertIn('aclfile ' + redis.ACL_FILE, payload['data']['redis-conf']) + self.assertNotIn('spec', payload) + + def test_failed_snapshot_does_not_change_config_or_restart_redis(self): + first, second, third, fourth = self.existing() + self.client.acl_save.side_effect = redis.redislib.RedisError('synthetic failure') + with first as kubectl, second, third, fourth: + with self.assertRaises(redis.redislib.RedisError): + redis._prepare_existing_acl_storage(self.data) + self.assertEqual(kubectl.call_count, 1) + + def test_existing_configuration_is_idempotent(self): + cm = {'metadata': {'resourceVersion': '42'}, 'data': {'redis-conf': 'aclfile ' + redis.ACL_FILE + '\n'}} + first, second, third, fourth = self.existing(cm) + with first as kubectl, second, third, fourth: + redis._prepare_existing_acl_storage(self.data) + self.assertEqual(kubectl.call_count, 1) + self.client.acl_save.assert_called_once() + + def test_equivalent_quoted_acl_directives_are_preserved(self): + for directive in ['aclfile "' + redis.ACL_FILE + '"', 'ACLFILE\t' + redis.ACL_FILE]: + with self.subTest(directive=directive): + cm = {'metadata': {'resourceVersion': '42'}, 'data': {'redis-conf': directive + '\n'}} + first, second, third, fourth = self.existing(cm) + with first as kubectl, second, third, fourth: + redis._prepare_existing_acl_storage(self.data) + self.assertEqual(kubectl.call_count, 1) + + def test_conflicting_file_configuration_is_not_overwritten(self): + cm = {'metadata': {'resourceVersion': '42'}, 'data': {'redis-conf': 'aclfile /another/users.acl\n'}} + first, second, third, fourth = self.existing(cm) + with first as kubectl, second, third, fourth: + with self.assertRaisesRegex(RuntimeError, 'does not match'): + redis._prepare_existing_acl_storage(self.data) + self.assertEqual(kubectl.call_count, 1) + + def test_fresh_installation_does_not_connect_to_missing_redis(self): + with patch.object(redis.kube, 'kubectl', return_value=''), patch.object(redis, 'admin_client') as admin: + redis._prepare_existing_acl_storage(self.data) + admin.assert_not_called() + + def test_unconfigured_acl_export_is_atomic_and_keeps_hashes_on_stdin(self): + self.client.acl_list.return_value = ['user synthetic on #privatehash ~synthetic:* -@all +get'] + with patch.object(redis.util, 'get_pod_name', return_value='redis-0'), patch.object(redis.subprocess, 'run') as run: + run.return_value.returncode = 0 + redis._write_acl_snapshot(self.client) + self.assertIn(b'#privatehash', run.call_args.kwargs['input']) + self.assertNotIn('privatehash', str(run.call_args.args)) + self.assertIn('mktemp', run.call_args.args[0][-1]) + self.assertIn('mv ', run.call_args.args[0][-1]) + + def test_acl_export_failure_does_not_expose_hashes(self): + self.client.acl_list.return_value = ['user synthetic on #privatehash ~synthetic:* -@all +get'] + with patch.object(redis.util, 'get_pod_name', return_value='redis-0'), patch.object(redis.subprocess, 'run') as run: + run.return_value.returncode = 1 + run.return_value.stderr = b'privatehash' + with self.assertRaisesRegex(RuntimeError, '^Could not persist Redis ACL state$'): + redis._write_acl_snapshot(self.client) + + def test_operator_resume_prepares_storage_before_system_account(self): + with patch.object(redis, '_prepare_existing_acl_storage') as prepare, patch.object(redis, 'create_openserverless_db_user') as system: + order = MagicMock(); order.attach_mock(prepare, 'prepare'); order.attach_mock(system, 'system') + redis.restore_openserverless_db_user() + self.assertEqual([c[0] for c in order.mock_calls], ['prepare', 'system']) + + def manifest(self, persistent): + return yaml.safe_load(expand_template('redis-set.yaml', dict(self.data, persistence=persistent))) + + def test_fresh_init_is_only_used_with_a_persistent_volume(self): + self.assertNotIn('initContainers', self.manifest(False)['spec']['template']['spec']) + spec = self.manifest(True)['spec']['template']['spec'] + init = spec['initContainers'][0] + self.assertEqual(init['image'], spec['containers'][0]['image']) + self.assertEqual(init['volumeMounts'][0]['name'], 'redis-pvc') + self.assertNotIn(self.data['redis_password'], init['command'][2]) + + def test_fresh_init_keeps_existing_acl_file_and_rejects_empty_file(self): + command = self.manifest(True)['spec']['template']['spec']['initContainers'][0]['command'][2] + with tempfile.TemporaryDirectory() as tmp: + target = Path(tmp) / 'users.acl' + command = command.replace('/bitnami/redis/data/users.acl', str(target)) + subprocess.run(['/bin/sh', '-ec', command], check=True) + self.assertEqual(target.read_text(), 'user default on #' + self.data['redis_password_sha256'] + ' ~* &* +@all\n') + self.assertEqual(target.stat().st_mode & 0o777, 0o600) + original = 'user synthetic on #original ~synthetic:* -@all +get\n' + target.write_text(original) + subprocess.run(['/bin/sh', '-ec', command], check=True) + self.assertEqual(target.read_text(), original) + target.write_text('') + self.assertNotEqual(subprocess.run(['/bin/sh', '-ec', command]).returncode, 0) + self.assertEqual(target.read_text(), '') + + def test_redis_configuration_loads_acl_file_only_with_persistence(self): + for persistent in (True, False): + text = yaml.safe_load(expand_template('redis-conf.yaml', dict(self.data, persistence=persistent)))['data']['redis-conf'] + self.assertEqual('aclfile ' + redis.ACL_FILE in text, persistent) + + def test_system_account_mutation_is_saved(self): + self.client.config_get.return_value = {'aclfile': redis.ACL_FILE} + data = dict(self.data, namespace='openserverless', password='synthetic-system', prefix='openserverless:') + with patch.object(redis, 'render_redis_script', return_value='/synthetic'), patch.object(redis.util, 'get_pod_name', return_value='redis-0'), patch.object(redis, 'exec_redis_command', return_value=True), patch.object(redis, 'admin_client', return_value=self.client), patch.object(redis.util, 'get_service', return_value=None): + self.assertTrue(redis.create_openserverless_db_user(data)) + self.client.acl_save.assert_called_once() + + +if __name__ == '__main__': + unittest.main()