fix: let a connection open when CLIENT SETINFO is denied by vishal-bala · Pull Request #685 · redis/redis-vl-python

vishal-bala · 2026-08-12T14:49:57Z

RedisVL announces itself on connect with CLIENT SETINFO LIB-NAME and, if that is refused, sends the same string through ECHO. The ECHO call was unguarded, so NoPermissionError escaped while the connection was being created, before any index operation could be attempted:

NoPermissionError: User <name> has no permissions to run the 'echo' command

An application role assembled from @read/@write hits this. CLIENT SETINFO is tagged @connection and @slow, ECHO is @connection and @fast, and neither is in @read or @write — so a rule built up from those categories never grants either.

Subtracting @dangerous is not what denies them. Measured on Redis 8.4.5, +@all -@dangerous permits both, so the shape that fails is a hand-written +@read +@write rule rather than a broad rule with exclusions — which also means Redis Cloud's predefined Read-Write shape is unaffected, while a @read-shaped Read-Only rule would be affected. Note too that +@read +@write +@slow permits CLIENT SETINFO while still denying ECHO, so the fallback was never a reliable second chance.

Why the fallback is deleted rather than guarded

It was added in 934d269 (#155) as a telemetry breadcrumb for servers older than Redis 7.2, where CLIENT SETINFO does not exist, so the library name would at least appear in MONITOR or the slowlog. It earns nothing today:

It fires only when CLIENT SETINFO errors, and the ACL rule that denies one denies the other.
Its argument reaches nothing that reads lib-name — CLIENT LIST and CLIENT INFO take that field from SETINFO alone, and an O(1) ECHO only enters the slowlog with slowlog-log-slower-than near zero.
redis-py already covers the old-server case one layer down, sending its own CLIENT SETINFO during the connection handshake under try/except ResponseError: pass.

The hasattr(client, "echo") guard goes with it, along with the comment claiming RedisCluster has no echo; both redis.cluster.RedisCluster and redis.asyncio.cluster.RedisCluster expose it.

What is kept, deliberately

The four duplicated blocks become one sync and one async helper. The explicit client_setinfo call stays: redis-py's handshake reports its own name, and this overwrites it with the composed redis-py(redisvl_v…;<wrapper>) string that adoption metrics read. The helper docstring says so, so it does not read as redundant with the handshake.

except ResponseError also stays narrow. In the connection-factory path this is the first command on a freshly created connection and therefore the de-facto connectivity check, so broadening to RedisError would swallow ConnectionError and defer a real failure to some later command. AuthenticationError subclasses ConnectionError, not ResponseError, so WRONGPASS and NOAUTH keep propagating either way.

Tests

tests/unit/test_client_identification.py — 16 cases over both twins. Mutation-checked: restoring the ECHO fallback fails six unit cases plus the integration test, and broadening the except to Exception fails two.

Both refusals are covered, since only one is a permission problem: a plain ResponseError stands for a pre-7.2 server, which is the case that lets the fallback go. Identification is asserted on all three URL shapes — sentinel, cluster, standalone — because it sits after that fan-out and moving it into one branch otherwise goes unnoticed; live cluster tests need --run-cluster-tests and never run in CI.

One integration test opens a connection under +@read +@write, with the premise pinned: CLIENT SETINFO must raise NoPermissionError for that user, so the test cannot go vacuous if Redis ever grants it to that role.

ACL user setup moves into an acl_user fixture, reused by the existing -@admin test. Rules are applied after reset because ACL SETUSER is additive and usernames are derived from the test's node id; the user is dropped before the connections it authenticated; and the fixture skips on deployments that reject ACL SETUSER.

Docs

docs/user_guide/installation.md said a credential permitted to run neither command fails at connection time. That is no longer true. The replacement names +client|setinfo for anyone who wants RedisVL attributed in CLIENT LIST, and is explicit that this labels only the connection RedisVL opens — redis-py labels the rest of the pool as plain redis-py.

It also gains a cluster caveat found while verifying this fix: CLUSTER SLOTS is tagged @slow only, so RedisCluster.from_url cannot discover the topology under a +@read +@write rule and the connection fails before identification is attempted, reported as the misleading Redis Cluster cannot be connected. Such a deployment needs +cluster|slots as well.

Not in scope

The rest of the ACL documentation pass. Four statements in installation.md are stale for a different reason — they predate a create_index=False opt-out for the extension constructors — and are corrected alongside it in a follow-up PR.

Identification still reaches only one connection: the explicit call labels whichever pooled connection it borrows, not the rest of the pool or any reconnect, and on a cluster redis-py routes it to the default node alone. Passing the composed name in as redis-py's lib_name/driver_info would fix that, but redis>=5.0,<8.0 straddles the deprecation of lib_name in favour of driver_info and needs version-conditional handling.

Note

Low Risk
Behavior change is limited to connect-time client labeling; real connectivity and auth errors still propagate, and the main effect is fixing false failures on restrictive ACLs.

Overview
Fixes connection failures for ACL roles built from +@read / +@write that cannot run CLIENT SETINFO. Identification is centralized in _identify_client / _aidentify_client: the library still attempts CLIENT SETINFO LIB-NAME for adoption metrics, but a ResponseError (ACL denial or pre-7.2 server) is logged at debug and ignored. The old ECHO fallback is removed because it was unguarded and failed with the same permission shape.

installation.md now states that denied identification does not block connect, documents optional +client|setinfo, and notes that cluster clients need +cluster|slots when topology discovery is not covered by read/write categories.

Tests add an acl_user fixture, integration coverage for restricted credentials, and unit tests that refused SETINFO still returns a client, ECHO is not called, and ConnectionError is not swallowed.

^{Reviewed by Cursor Bugbot for commit edadd6c. Bugbot is set up for automated code reviews on this repo. Configure here.}

nkanu17

lgtm!

@READ

RedisVL announces itself on connect with `CLIENT SETINFO LIB-NAME` and, if that is refused, sends the same string through `ECHO`. The `ECHO` call was unguarded, so `NoPermissionError` escaped while the connection was being created, before any index operation could be attempted: NoPermissionError: User <name> has no permissions to run the 'echo' command An application role assembled from `@read`/`@write` hits this. `CLIENT SETINFO` is tagged `@connection` and `@slow`, `ECHO` is `@connection` and `@fast`, and neither is in `@read` or `@write` -- so a rule built up from those categories never grants either. Subtracting `@dangerous` is not what denies them: measured on Redis 8.4.5, `+@ALL -@dangerous` permits both, which is why the shape that fails is a hand-written `+@READ +@write` rule rather than a broad rule with exclusions. Note also that `+@READ +@Write +@slow` permits `CLIENT SETINFO` while still denying `ECHO`, so the fallback was never a reliable second chance. The `ECHO` fallback is deleted rather than guarded. It was added in 934d269 (#155) as a telemetry breadcrumb for servers older than Redis 7.2, where `CLIENT SETINFO` does not exist, so the library name would at least appear in `MONITOR` or the slowlog. It earns nothing today: it fires only when `CLIENT SETINFO` errors, its argument reaches nothing that reads `lib-name` (`CLIENT LIST` and `CLIENT INFO` take that field from `SETINFO` alone, and it would only enter the slowlog with `slowlog-log-slower-than` near zero), and redis-py already covers the old-server case one layer down, sending its own `CLIENT SETINFO` during the connection handshake under `try/except ResponseError: pass`. The `hasattr(client, "echo")` guard goes with it, along with the comment claiming `RedisCluster` has no `echo`; both `redis.cluster.RedisCluster` and `redis.asyncio.cluster.RedisCluster` expose it. The four duplicated blocks are now one sync and one async helper. The explicit `client_setinfo` call is kept deliberately -- redis-py's handshake reports its own name, and this overwrites it with the composed `redis-py(redisvl_v...;<wrapper>)` string that adoption metrics read -- and the helper docstring says so, so it does not read as redundant with the handshake. `except ResponseError` is also deliberate. In the connection-factory path this is the first command on a freshly created connection and therefore the de-facto connectivity check, so broadening to `RedisError` would swallow `ConnectionError` and defer a real failure to some later command. `AuthenticationError` subclasses `ConnectionError`, not `ResponseError`, so `WRONGPASS` and `NOAUTH` keep propagating either way. ## Tests - `tests/unit/test_client_identification.py` -- 16 cases over both twins. Mutation-checked: restoring the `ECHO` fallback fails six unit cases plus the integration test, and broadening the `except` to `Exception` fails two. Both refusals are covered, since only one is a permission problem: a plain `ResponseError` stands for a pre-7.2 server, which is the case that lets the fallback go. Identification is asserted on all three URL shapes -- sentinel, cluster, standalone -- because it sits after that fan-out and moving it into one branch otherwise goes unnoticed; live cluster tests need `--run-cluster-tests` and never run in CI. - An integration test opening a connection under `+@READ +@write`, with the premise pinned: `CLIENT SETINFO` must raise `NoPermissionError` for that user, so the test cannot go vacuous if Redis ever grants it to that role. It lives in `test_connection.py` beside the other identification tests. - ACL user setup moves into an `acl_user` fixture, reused by the existing `-@admin` test in `test_search_index.py`. Rules are applied after `reset` because `ACL SETUSER` is additive and usernames are derived from the test's node id; the user is dropped before the connections it authenticated; and the fixture skips on deployments that reject `ACL SETUSER`. ## Docs `docs/user_guide/installation.md` said a credential permitted to run neither command fails at connection time. That is no longer true. The replacement names `+client|setinfo` for anyone who wants RedisVL attributed in `CLIENT LIST`, and is explicit that this labels only the connection RedisVL opens -- redis-py labels the rest of the pool as plain `redis-py`. It also gains a cluster caveat found while verifying this fix: `CLUSTER SLOTS` is tagged `@slow` only, so `RedisCluster.from_url` cannot discover the topology under a `+@READ +@write` rule and the connection fails before identification is even attempted, reported as `Redis Cluster cannot be connected`. Such a deployment needs `+cluster|slots` as well. ## Not in scope The rest of the ACL documentation pass. `installation.md:189`, `:208`, `:210` and `:223` are stale for a different reason -- they predate the `create_index=False` opt-out -- and are corrected alongside it, not here. Identification still reaches only one connection. The explicit call labels whichever pooled connection it borrows, not the rest of the pool or any reconnect, and on a cluster redis-py routes it to the default node alone. Passing the composed name in as redis-py's `lib_name`/`driver_info` would fix that, but `redis>=5.0,<8.0` straddles the deprecation of `lib_name` in favour of `driver_info` and needs version-conditional handling.

@READ

…687) Stacked on #685, which fixes the connection-time failure this PR's credential also hits. Review that one first. Every extension checks whether its index exists while being constructed, and that check is `FT.INFO`. A credential assembled from `+@READ +@write` is denied `FT.INFO` and `FT.CREATE` together — neither command is in either category, identically on Redis 8.0.6 through 8.8.1 — so such a role cannot construct `SemanticCache`, `MessageHistory`, `SemanticMessageHistory` or `SemanticRouter` at all, even against an index it can query perfectly well: ``` RedisSearchError: Error while fetching llmcache index info: User <name> has no permissions to run the 'FT.INFO' command ``` **There was no way to ask for less.** `overwrite=False` is the *reason* the check runs — `create()` calls `exists()` first and consults `overwrite` only afterwards — `drop` is not a constructor parameter at all, and `overwrite=True` is strictly worse, since it proceeds to `FT.DROPINDEX`. `SearchIndex` exposes no lifecycle seam either, and each constructor calls `create()` inline with no hook to subclass around. `create_index=False` lets the caller state what RedisVL cannot ask: the index exists. It skips the existence check, the schema comparison against the live index, and creation, so the constructor issues no index command at all. It is rejected together with `overwrite=True`, which asks for the opposite. ## Why not a runtime probe `FT.SEARCH` is permitted where `FT.INFO` is not, so probing with it looks attractive. Its reply is identical for an index and for an alias pointing at one, though, so a probe would reinstate the `create(overwrite=True, drop=True)` → `FT.DROPINDEX <alias> DD` data-loss path that #672 closed. And a credential that cannot ask whether the index exists cannot create one either, so there is nothing to work out at runtime — `exists()` keeps failing loudly instead. ## The invariant this had to fix first Constructor-time `create()` was the de-facto eager connect. `SearchIndex.client` returns the raw client and is `None` until the lazy `_redis_client` property runs, so skipping `create()` left ten `self._index.client` sites in `redisvl/extensions/` dereferencing `None` — starting with the router's `route_config` write, which runs immediately after index setup. All ten now use `_redis_client`. Two of the `# type: ignore` comments they carried turned out to cover a real `scan_by_pattern` signature mismatch rather than the Optional, and are kept with explicit codes. ## Router semantics `create_index=False` means the index exists, is already seeded, and is not ours to rewrite, so the router also skips writing route references and the stored `route_config`. Rewriting that blob from an unverified local route list would truncate a shared router's routes, and `JSON.SET` is `@write`, so a restricted credential can do it. `_update_router_state()` stays armed — `add_route()` and `remove_route()` are the caller acting deliberately — but that consequence is now stated in `add_route()`'s own docstring as well as the guide, since the docstring is the reference for anyone who never opens the guide. `SemanticRouter.from_existing()` threads the flag through. It reads the stored config with `JSON.GET` and reaches `FT.INFO` only via the constructor, so with `create_index=False` it issues no index command and becomes the way to attach to a router under a restricted credential. The flag is popped before `_split_from_existing_kwargs`, which retains only `SearchIndex` init kwargs and would otherwise pass it to the Redis client constructor — where `SearchIndex.__init__` discards unknown kwargs silently, making the mistake invisible. Separately, `routes=[]` now raises a useful error when matching instead of `max() arg is an empty sequence`. That is unconditional: emptiness is legal on either path, and a flag about index ownership should not decide it. ## Tests `tests/unit/test_extension_create_index_flag.py` — 17 cases. The contract is "no index command at all", so they assert on the client: a `MagicMock` records every call, and `ft()` is the gate every `FT.*` command passes through. All four constructors reach zero recorded calls. Two cases pin the lazy-connect invariant above by driving `drop(id=...)` and `get_route_references()` as the first operation — reverting any of the ten conversions otherwise breaks no test. Others pin that the flag survives as instance state, that it never reaches the router's stored config, and that `from_existing()` still verifies by default. The default construction path is not re-tested here; the existing 189 integration tests already fail if `create()` is dropped. One integration test round-trips `store()`/`check()` through a cache built with `create_index=False` under a real `+@READ +@Write -@dangerous` ACL user — the customer's rule, so the destructive commands it denies stay denied — with the premise pinned (`FT.INFO` must raise `NoPermissionError` for that user) and the negative alongside it: without the flag the same credential raises `RedisSearchError` naming `ft.info`, with `NoPermissionError` chained. ## Docs The ACL section of `docs/user_guide/installation.md` is restructured. Four statements were falsified by this change or were already wrong: that a credential needs `@search` at all, that all four extensions always call `create()`, that enumeration is the only thing an `-@admin` rule breaks, and the advice to grant `FT.CREATE`. The operation table gains a `+@READ +@write` column, the command-to-category mapping is labelled as measured rather than documented, and the wrapped error text appears verbatim under its own heading — an H3, so it has an anchor to link to. The pre-existing key-permission material became its own section rather than being dropped, and was corrected while there: partial key-pattern overlap is denied exactly like no overlap, not filtered down to the readable subset, and `FT.CREATE` is not key-checked at all, so a credential can create an index it cannot query. A new subsection covers what the flag gives up. An absent index fails loudly, and a vector dimension mismatch does too — but only once the index holds a document, which a freshly provisioned index will not. A wrong prefix, an `ON JSON` index written as hashes, and a differing datatype or distance metric are silent. The tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes`, not `hash_indexing_failures`, which stays `0` because those keys were never indexing candidates. Router provisioning gets its own subsection, since preparing one for this mode needs embedded reference vectors rather than a hand-written `FT.CREATE`. Two corrections worth calling out for reviewers who know this area: `clear()` is not uniform — only `SemanticCache.clear()` avoids `FT.INFO`, while the other three delegate to `SearchIndex.clear()`, which calls `info()` first — and Redis Cloud's predefined Read-Write rule reads as `@read`/`@write`-shaped from its published description, so it is a candidate for this problem rather than immune to it. ## Not in scope `create_index=False` restrains construction only; `delete()` and `clear()` stay armed. Coupling ownership to the flag is the coherent next step, and the flag is stored as instance state so it can be added without another parameter. `adk_redis` needs a matching field on `RedisVLCacheProviderConfig` before this reaches callers who construct through that provider. Until then the interim for the escalation remains the `exists()` monkeypatch already shared in the thread.  --- > [!NOTE] > **Medium Risk** > Behavior changes constructor and lifecycle semantics for four public extension APIs under restricted ACLs; mistakes with externally managed indexes can fail silently on schema mismatch, though destructive index operations are now guarded. > > **Overview** > Adds **`create_index=False`** to **`SemanticCache`**, **`MessageHistory`**, **`SemanticMessageHistory`**, and **`SemanticRouter`** so constructors skip **`FT.INFO`**, schema checks, and **`FT.CREATE`**—the path needed when a **`+@READ +@write`** role cannot run index-metadata commands. **`overwrite=True`** with this flag is rejected via shared **`CREATE_INDEX_OVERWRITE_CONFLICT`** messaging. > > **Index-wide `delete()` / `clear()`** (plus async cache **`adelete` / `aclear`**) now raise **`EXTERNAL_INDEX_LIFECYCLE_CONFLICT`** when the extension does not own the index. Per-entry **`drop`** remains allowed. > > **`SemanticRouter`** with the flag does not seed reference vectors or write **`route_config`**; **`from_existing(..., create_index=False)`** threads the flag and avoids **`FT.*`** on attach. Matching with **`routes=[]`** fails with an explicit error instead of an empty **`max()`**. > > Fixes lazy-connect breakage from skipping constructor **`create()`** by using **`_index._redis_client`** instead of **`_index.client`** across extensions. > > **Docs** expand Redis ACL guidance (operation vs **`+@READ +@write`** table, **`create_index=False`** usage, router provisioning, silent schema mismatches) and point **`FT.INFO`** permission errors to the new flag in **`exceptions.rst`**. **Tests** add unit coverage for “no index commands at construction” and an integration ACL round-trip. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 106f8b0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>  --------- Co-authored-by: Nitin Kanukolanu <nitinkanukolanu@gmail.com>

applied-ai-release-bot · 2026-08-19T15:53:18Z

🚀 PR was released in v0.26.0 🚀

vishal-bala mentioned this pull request Aug 13, 2026

feat: let callers opt out of index creation with create_index=False #687

Merged

vishal-bala linked an issue Aug 13, 2026 that may be closed by this pull request

Connection fails outright when ACL denies both CLIENT SETINFO and ECHO #673

Closed

vishal-bala marked this pull request as ready for review August 13, 2026 08:33

vishal-bala added the auto:patch Increment the patch version when merged label Aug 13, 2026

vishal-bala mentioned this pull request Aug 13, 2026

fix: do not fail connection setup when ACL denies both CLIENT SETINFO and ECHO #679

Closed

nkanu17 force-pushed the fix/acl-drop-echo-identification-fallback branch from 6d91657 to 93e9fb3 Compare August 14, 2026 14:53

nkanu17 approved these changes Aug 14, 2026

View reviewed changes

vishal-bala force-pushed the fix/acl-drop-echo-identification-fallback branch 3 times, most recently from ce519f7 to 1a70547 Compare August 17, 2026 09:59

vishal-bala force-pushed the fix/acl-drop-echo-identification-fallback branch from 1a70547 to edadd6c Compare August 19, 2026 15:13

vishal-bala merged commit 7e6ef8c into main Aug 19, 2026
42 checks passed

vishal-bala deleted the fix/acl-drop-echo-identification-fallback branch August 19, 2026 15:30

applied-ai-release-bot Bot added the released This issue/pull request has been released. label Aug 19, 2026

Navigation Menu

Uh oh!

fix: let a connection open when CLIENT SETINFO is denied - #685

fix: let a connection open when CLIENT SETINFO is denied#685
vishal-bala merged 1 commit into
mainfrom
fix/acl-drop-echo-identification-fallback

vishal-bala commented Aug 12, 2026 •

edited by cursor Bot

Loading

Uh oh!

nkanu17 left a comment

Uh oh!

Uh oh!

applied-ai-release-bot Bot commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vishal-bala commented Aug 12, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the fallback is deleted rather than guarded

What is kept, deliberately

Tests

Docs

Not in scope

Uh oh!

nkanu17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

applied-ai-release-bot Bot commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vishal-bala commented Aug 12, 2026 •

edited by cursor Bot

Loading