diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index 70840b1da..8283afad6 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -211,7 +211,9 @@ Index enumeration is the only thing an `-@admin` rule breaks. It is reached by ` Other categories gate different operations. `FT.DROPINDEX` is tagged `@dangerous` and `@write` as well as `@search`, so a policy that subtracts `@dangerous` denies `index.delete()`, `rvl index delete`, and `rvl index destroy`. -Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO` and falls back to `ECHO`. A credential permitted to run neither currently fails when the connection is created, before any index operation. +Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO`. That command is tagged `@connection` and `@slow`, and belongs to neither `@read` nor `@write`, so a rule built up from those categories never grants it. A credential that cannot run it still connects: identification only populates the `lib-name` field that `CLIENT LIST` and `CLIENT INFO` display, so a refusal is ignored (and logged, if you have configured logging at debug level). Grant `+client|setinfo` if you want RedisVL to appear as the connecting library there — note that this labels the connection RedisVL opens, while redis-py labels the rest of the pool as plain `redis-py`. + +Cluster deployments need one more grant. `RedisCluster` discovers the topology with `CLUSTER SLOTS`, which is tagged `@slow` only, so a credential assembled from `+@read +@write` cannot open a clustered connection at all — redis-py reports this as `Redis Cluster cannot be connected`, with the underlying permission error chained beneath it. Grant `+cluster|slots` alongside the rules above. ### Redis Cloud and Redis Software diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 44247d1f6..20f22832b 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -212,6 +212,43 @@ def make_lib_name(*args) -> str: return f"redis-py({custom_libs})" +def _identify_client(client: SyncRedisClient, lib_name: str | None = None) -> None: + """Report RedisVL as the connecting library, tolerating a refusal. + + redis-py sends its own ``CLIENT SETINFO`` during the connection handshake, + so this call is here to overwrite that with the composed + ``redis-py(redisvl_v...;)`` name that adoption metrics read. Keep + it: without it the library and any wrapper above it go unattributed. + + A refusal is ignored, because ``CLIENT SETINFO`` only populates the + ``lib-name`` field that ``CLIENT LIST`` and ``CLIENT INFO`` display -- its + own documentation tells client libraries to ignore failures. Two credentials + hit this: one granting neither ``@connection`` nor the command itself, and + one on a server predating Redis 7.2, where the command does not exist. + + Only ``ResponseError`` is caught. In the connection-factory path this is the + first command issued on a freshly created connection, which makes it the + de-facto connectivity check, so swallowing ``ConnectionError`` would defer a + genuine failure to some later and more confusing command. Note that on a + cluster client redis-py routes the command to the default node only, so the + label reaches one node rather than the whole cluster. + """ + try: + client.client_setinfo("LIB-NAME", make_lib_name(lib_name)) + except ResponseError as e: + logger.debug(f"CLIENT SETINFO was not applied, continuing without it: {e}") + + +async def _aidentify_client( + client: AsyncRedisClient, lib_name: str | None = None +) -> None: + """Async version of :func:`_identify_client`.""" + try: + await client.client_setinfo("LIB-NAME", make_lib_name(lib_name)) + except ResponseError as e: + logger.debug(f"CLIENT SETINFO was not applied, continuing without it: {e}") + + def convert_index_info_to_schema(index_info: dict[str, Any]) -> dict[str, Any]: """Convert the output of FT.INFO into a schema-ready dictionary. @@ -540,15 +577,7 @@ def get_redis_connection( client = RedisCluster.from_url(url, **kwargs) else: client = Redis.from_url(url, **kwargs) - # Module validation removed - operations will fail naturally if modules are missing - # Set client library name only - _lib_name = make_lib_name(kwargs.get("lib_name")) - try: - client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(client, "echo"): - client.echo(_lib_name) + _identify_client(client, kwargs.get("lib_name")) return client @staticmethod @@ -599,15 +628,7 @@ async def _get_aredis_connection( ) client = AsyncRedis.from_url(cleaned_url, **cleaned_kwargs) - # Module validation removed - operations will fail naturally if modules are missing - # Set client library name only - _lib_name = make_lib_name(kwargs.get("lib_name")) - try: - await client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(client, "echo"): - await client.echo(_lib_name) + await _aidentify_client(client, kwargs.get("lib_name")) return client @staticmethod @@ -718,52 +739,50 @@ def validate_sync_redis( redis_client: SyncRedisClient, lib_name: str | None = None, ) -> None: - """Validates the sync Redis client. + """Check the client type and report the library name. - Note: Module validation has been removed. This method now only validates - the client type and sets the library name. + Identification is best effort: a server that refuses ``CLIENT SETINFO`` + is tolerated, so the only failure raised here is a wrong client type. + (Module validation was removed; a missing module now surfaces when an + operation needs it.) + + Args: + redis_client (SyncRedisClient): The client to check. + lib_name (Optional[str]): Name of a library wrapping RedisVL, to + report alongside it. Defaults to None. + + Raises: + TypeError: If the client is not a Redis or RedisCluster instance. """ if not issubclass(type(redis_client), (Redis, RedisCluster)): raise TypeError( "Invalid Redis client instance. Must be Redis or RedisCluster." ) - # Set client library name - _lib_name = make_lib_name(lib_name) - try: - redis_client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - # For RedisCluster, echo is not available - if hasattr(redis_client, "echo"): - redis_client.echo(_lib_name) - - # Module validation removed - operations will fail naturally if modules are missing + _identify_client(redis_client, lib_name) @staticmethod async def validate_async_redis( redis_client: AsyncRedisClient, lib_name: str | None = None, ) -> None: - """Validates the async Redis client. + """Async version of :meth:`validate_sync_redis`. - Note: Module validation has been removed. This method now only validates - the client type and sets the library name. + Args: + redis_client (AsyncRedisClient): The client to check. + lib_name (Optional[str]): Name of a library wrapping RedisVL, to + report alongside it. Defaults to None. + + Raises: + TypeError: If the client is not an async Redis or RedisCluster + instance. """ if not issubclass(type(redis_client), (AsyncRedis, AsyncRedisCluster)): raise TypeError( "Invalid async Redis client instance. Must be async Redis or async RedisCluster." ) - # Set client library name - _lib_name = make_lib_name(lib_name) - try: - await redis_client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(redis_client, "echo"): - await redis_client.echo(_lib_name) - # Module validation removed - operations will fail naturally if modules are missing + await _aidentify_client(redis_client, lib_name) @staticmethod @overload diff --git a/tests/conftest.py b/tests/conftest.py index cd09ef5d4..084d83447 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,14 +4,17 @@ import re import subprocess import sys +from contextlib import contextmanager, suppress from datetime import datetime, timezone import pytest +from redis.exceptions import ResponseError from testcontainers.compose import DockerCompose from redisvl.index.index import AsyncSearchIndex, SearchIndex from redisvl.redis.connection import RedisConnectionFactory, is_version_gte from redisvl.redis.utils import array_to_buffer +from redisvl.types import SyncRedisClient # Check if we're on Python 3.14+ where sentence-transformers may not work SKIP_HF = sys.version_info >= (3, 14) @@ -212,6 +215,83 @@ def client(redis_url): yield conn +class _AclUser: + """A temporary ACL user, plus the RedisVL connections opened as them. + + `username` and `password` are public so a test can hand them to anything + that takes connection kwargs, such as an extension constructor, instead of + going through `connect()`. + """ + + def __init__(self, username: str, password: str, redis_url: str): + self.username = username + self.password = password + self._redis_url = redis_url + self._clients: list[SyncRedisClient] = [] + + def connect(self, **kwargs): + """Open a RedisVL connection authenticated as this user.""" + conn = RedisConnectionFactory.get_redis_connection( + redis_url=self._redis_url, + username=self.username, + password=self.password, + **kwargs, + ) + self._clients.append(conn) + return conn + + def close_all(self) -> None: + for conn in self._clients: + with suppress(Exception): + conn.close() + + +@pytest.fixture +def acl_user(client, redis_url, redis_test_name): + """Create a temporary ACL user, yielding a handle that connects as them. + + The returned factory takes a required `name` and the ACL rules to apply, in + the order Redis applies them: rules are evaluated left to right, so + `+@all -@admin` and `-@admin +@all` do not mean the same thing. `on` and a + password are supplied here, so callers pass only key patterns, channel + patterns and command rules:: + + with acl_user("~*", "&*", "+@read", "+@write", name="read_write") as user: + restricted = user.connect() + + Rules are applied after `reset`, because `ACL SETUSER` is additive and + usernames are derived from the test's node id -- without it, a run whose + teardown was skipped by a crash would layer new rules onto a stale user. + + ACL users are server-global, so a leaked one outlives the test and its + worker; the user is dropped before the connections it authenticated. Redis + Software manages ACLs through its own control plane and does not support the + `>password` syntax used here, so tests built on this fixture only run + against Redis Open Source -- they skip elsewhere. + """ + + @contextmanager + def _make_acl_user(*rules: str, name: str): + username = redis_test_name(name) + password = "test-acl-password" + try: + client.execute_command( + "ACL", "SETUSER", username, "reset", "on", f">{password}", *rules + ) + except ResponseError as e: + pytest.skip(f"Deployment does not support ACL SETUSER: {e}") + user = _AclUser(username, password, redis_url) + try: + yield user + finally: + try: + client.execute_command("ACL", "DELUSER", username) + finally: + user.close_all() + + return _make_acl_user + + @pytest.fixture def cluster_client(redis_cluster_url): """ diff --git a/tests/integration/test_connection.py b/tests/integration/test_connection.py index f3a8ea727..b62d79bef 100644 --- a/tests/integration/test_connection.py +++ b/tests/integration/test_connection.py @@ -3,7 +3,7 @@ import pytest from redis import Redis from redis.asyncio import Redis as AsyncRedis -from redis.exceptions import ConnectionError +from redis.exceptions import ConnectionError, NoPermissionError from redisvl.redis.connection import ( RedisConnectionFactory, @@ -136,6 +136,31 @@ def test_unknown_redis(self): bad_client.ping() +def test_connection_opens_without_identification_permission(acl_user): + """A credential that cannot identify the client must still connect. + + RedisVL announces itself with CLIENT SETINFO when a connection is made, and + used to fall back to ECHO if that was refused. Both commands are + `@connection` and are in neither `@read` nor `@write`, so a role built up + from those categories never grants either -- and the unguarded fallback made + RedisVL fail while the connection was being created, before any index + operation could be attempted. + + Only a live server can show that this role really denies the command, which + is why this is an integration test. + """ + with acl_user("~*", "&*", "+@read", "+@write", name="acl_no_connection") as user: + restricted = user.connect() + + # Pin the premise. If a future Redis lets this role run CLIENT SETINFO, + # the test is no longer exercising the tolerance and should say so. + with pytest.raises(NoPermissionError): + restricted.client_setinfo("LIB-NAME", "probe") + + # And the connection is genuinely usable for what the role does permit. + assert restricted.exists("no-such-key") == 0 + + def test_validate_redis(client): skip_if_redis_version_below(client, "7.2.0") RedisConnectionFactory.validate_sync_redis(client) diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index bac05612e..2c3740040 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -313,9 +313,7 @@ def test_search_index_delete(index): assert index.name not in index.listall() -def test_exists_under_acl_without_admin_commands( - index, client, redis_url, redis_test_name -): +def test_exists_under_acl_without_admin_commands(index, client, acl_user): """exists() must work for a credential that grants search but drops @admin. Redis tags FT._LIST @admin as well as @search, so this ACL shape used to @@ -327,30 +325,18 @@ def test_exists_under_acl_without_admin_commands( skip_if_no_redis_search(client) index.create(overwrite=True, drop=True) - username = redis_test_name("acl_no_admin") - password = "test-acl-password" - client.execute_command( - "ACL", "SETUSER", username, "on", f">{password}", "~*", "&*", "+@all", "-@admin" - ) - restricted = None try: - restricted = RedisConnectionFactory.get_redis_connection( - redis_url=redis_url, username=username, password=password - ) - # Pin the premise so this test cannot quietly become vacuous: if a - # future Redis stops gating FT._LIST behind @admin, the reason for - # preferring FT.INFO is gone and we want to hear about it here. - with pytest.raises(NoPermissionError): - restricted.execute_command("FT._LIST") - - restricted_index = SearchIndex(schema=index.schema, redis_client=restricted) - assert restricted_index.exists() is True + with acl_user("~*", "&*", "+@all", "-@admin", name="acl_no_admin") as user: + restricted = user.connect() + # Pin the premise so this test cannot quietly become vacuous: if a + # future Redis stops gating FT._LIST behind @admin, the reason for + # preferring FT.INFO is gone and we want to hear about it here. + with pytest.raises(NoPermissionError): + restricted.execute_command("FT._LIST") + + restricted_index = SearchIndex(schema=index.schema, redis_client=restricted) + assert restricted_index.exists() is True finally: - # Drop the ACL user before anything else that could raise: users are - # server-global, so a leaked one outlives this test and its worker. - client.execute_command("ACL", "DELUSER", username) - if restricted is not None: - restricted.close() index.delete(drop=True) diff --git a/tests/unit/test_client_identification.py b/tests/unit/test_client_identification.py new file mode 100644 index 000000000..9c741bd1d --- /dev/null +++ b/tests/unit/test_client_identification.py @@ -0,0 +1,210 @@ +""" +Unit tests for client identification tolerance. + +RedisVL announces itself with ``CLIENT SETINFO LIB-NAME`` when a connection is +made, so that adoption metrics can attribute traffic to the library and to any +wrapper above it. The command is cosmetic -- it only populates the ``lib-name`` +field of ``CLIENT LIST`` and ``CLIENT INFO`` -- so a refusal must never stop a +connection from opening. Two refusals matter: a credential that grants neither +`@connection` nor the command itself, and a server predating Redis 7.2, where +the command does not exist. +""" + +import logging +from unittest.mock import AsyncMock, Mock, patch + +import pytest +from redis import Redis +from redis.asyncio import Redis as AsyncRedis +from redis.exceptions import ConnectionError, NoPermissionError, ResponseError + +from redisvl.redis import connection as connection_module +from redisvl.redis.connection import RedisConnectionFactory, make_lib_name + +PLAIN_URL = "redis://localhost:6379" +CLUSTER_URL = "redis://localhost:6379?cluster=true" +SENTINEL_URL = "redis+sentinel://localhost:26379/mymaster" + +# Both must be tolerated, and only the first is a permission problem. The second +# is the case that lets the ECHO fallback go: redis-py's own handshake already +# absorbs it, so RedisVL does not need a workaround for old servers. +REFUSALS = [ + ( + NoPermissionError, + "User acl_user has no permissions to run the 'client|setinfo' command", + ), + ( + ResponseError, + "ERR Unknown subcommand or wrong number of arguments for 'setinfo'", + ), +] +REFUSAL_IDS = ["denied-by-acl", "unsupported-by-server"] + +UNREACHABLE = "Error 111 connecting to localhost:6379. Connection refused." + + +def _sync_client(setinfo_error=None): + """A real Redis instance -- no socket is opened -- with stubbed commands. + + The type has to survive ``issubclass`` in ``validate_sync_redis``, so this + cannot be a bare ``MagicMock``. Returns the stubs so assertions do not have + to reach back through the client. + """ + client = Redis.from_url(PLAIN_URL) + setinfo, echo = Mock(side_effect=setinfo_error), Mock() + client.client_setinfo, client.echo = setinfo, echo + return client, setinfo, echo + + +def _async_client(setinfo_error=None): + client = AsyncRedis.from_url(PLAIN_URL) + setinfo, echo = AsyncMock(side_effect=setinfo_error), AsyncMock() + client.client_setinfo, client.echo = setinfo, echo + return client, setinfo, echo + + +class TestConnectionFactoryIdentification: + """get_redis_connection() must survive a refused identification.""" + + @pytest.mark.parametrize("exc_type,message", REFUSALS, ids=REFUSAL_IDS) + def test_refused_identification_still_returns_a_client(self, exc_type, message): + client, _, echo = _sync_client(setinfo_error=exc_type(message)) + with patch.object(connection_module.Redis, "from_url", return_value=client): + returned = RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + assert returned is client + # ECHO used to carry the library name when SETINFO was refused. It is + # denied by the same ACL rule and reaches nothing that reads lib-name, + # so the fallback is gone and must not come back. + echo.assert_not_called() + + def test_connection_failure_is_not_swallowed(self): + # SETINFO is the first command on a freshly created connection, which + # makes it the de-facto connectivity check. Widening the except clause + # would defer a real failure to some later, more confusing command. + client, _, _ = _sync_client(setinfo_error=ConnectionError(UNREACHABLE)) + with patch.object(connection_module.Redis, "from_url", return_value=client): + with pytest.raises(ConnectionError): + RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + + @pytest.mark.parametrize( + "url", + [PLAIN_URL, CLUSTER_URL, SENTINEL_URL], + ids=["standalone", "cluster", "sentinel"], + ) + def test_identification_reaches_every_url_shape(self, url): + # Identification sits after the sentinel/cluster/standalone fan-out. + # Only the standalone branch has live coverage elsewhere -- cluster + # integration tests need --run-cluster-tests and never run in CI -- so + # this is the only guard against the call sliding into one branch. + client, setinfo, _ = _sync_client() + if url == SENTINEL_URL: + target = patch.object( + RedisConnectionFactory, "_redis_sentinel_client", return_value=client + ) + elif url == CLUSTER_URL: + target = patch.object( + connection_module.RedisCluster, "from_url", return_value=client + ) + else: + target = patch.object( + connection_module.Redis, "from_url", return_value=client + ) + with target: + RedisConnectionFactory.get_redis_connection(redis_url=url) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + + def test_wrapper_lib_name_is_reported(self): + # The composed string is the reason the explicit call is kept at all: + # redis-py's handshake reports its own name, not this one. + client, setinfo, _ = _sync_client() + with patch.object(connection_module.Redis, "from_url", return_value=client): + RedisConnectionFactory.get_redis_connection( + redis_url=PLAIN_URL, lib_name="langchain-redis_v1.0.0" + ) + reported = setinfo.call_args.args[1] + assert "redisvl_v" in reported and "langchain-redis_v1.0.0" in reported + + def test_refusal_is_logged(self, caplog): + # A refusal is otherwise invisible to the caller, so the log line is the + # only affordance for "why is lib-name empty?". + client, _, _ = _sync_client(setinfo_error=NoPermissionError(REFUSALS[0][1])) + with caplog.at_level(logging.DEBUG, logger="redisvl.redis.connection"): + with patch.object(connection_module.Redis, "from_url", return_value=client): + RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + assert "CLIENT SETINFO" in caplog.text + + +class TestAsyncConnectionFactoryIdentification: + """The async twin is a hand-maintained copy, so each branch needs a witness.""" + + @pytest.mark.parametrize("exc_type,message", REFUSALS, ids=REFUSAL_IDS) + @pytest.mark.asyncio + async def test_refused_identification_still_returns_a_client( + self, exc_type, message + ): + client, _, echo = _async_client(setinfo_error=exc_type(message)) + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + returned = await RedisConnectionFactory._get_aredis_connection( + redis_url=PLAIN_URL + ) + assert returned is client + # assert_not_called, not assert_not_awaited: the latter passes for a + # reintroduced fallback that forgot its await, which is exactly the slip + # a hand-maintained twin invites. + echo.assert_not_called() + + @pytest.mark.asyncio + async def test_connection_failure_is_not_swallowed(self): + client, _, _ = _async_client(setinfo_error=ConnectionError(UNREACHABLE)) + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + with pytest.raises(ConnectionError): + await RedisConnectionFactory._get_aredis_connection(redis_url=PLAIN_URL) + + @pytest.mark.asyncio + async def test_wrapper_lib_name_is_reported(self): + client, setinfo, _ = _async_client() + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + await RedisConnectionFactory._get_aredis_connection( + redis_url=PLAIN_URL, lib_name="langchain-redis_v1.0.0" + ) + reported = setinfo.call_args.args[1] + assert "redisvl_v" in reported and "langchain-redis_v1.0.0" in reported + + +class TestValidateSyncRedis: + """The user-supplied-client path shares the same tolerance.""" + + def test_refused_identification_is_tolerated(self): + client, setinfo, echo = _sync_client( + setinfo_error=NoPermissionError(REFUSALS[0][1]) + ) + RedisConnectionFactory.validate_sync_redis(client) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + echo.assert_not_called() + + def test_client_type_is_still_validated(self): + with pytest.raises(TypeError): + RedisConnectionFactory.validate_sync_redis("not a client") + + +class TestValidateAsyncRedis: + @pytest.mark.asyncio + async def test_client_type_is_still_validated(self): + # A sync client is the mistake this guard exists to catch. + with pytest.raises(TypeError): + await RedisConnectionFactory.validate_async_redis(Redis.from_url(PLAIN_URL)) + + @pytest.mark.asyncio + async def test_refused_identification_is_tolerated(self): + client, setinfo, echo = _async_client( + setinfo_error=NoPermissionError(REFUSALS[0][1]) + ) + await RedisConnectionFactory.validate_async_redis(client) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + echo.assert_not_called() diff --git a/tests/unit/test_url_deprecation.py b/tests/unit/test_url_deprecation.py index b70385fca..d70ff4cb8 100644 --- a/tests/unit/test_url_deprecation.py +++ b/tests/unit/test_url_deprecation.py @@ -11,9 +11,6 @@ class DummyAsyncClient: async def client_setinfo(self, *args, **kwargs): return None - async def echo(self, *args, **kwargs): - return None - @pytest.mark.asyncio async def test__get_aredis_connection_deprecates_url_kwarg_only():