From ba7966cfffc6d32c5ab309ee8790eed76cf97837 Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Wed, 16 Sep 2026 10:00:46 -0700 Subject: [PATCH 1/6] feat(php): add READ_FROM_AZ_AFFINITY_ALL_NODES read strategy Signed-off-by: Prateek Kumar --- CHANGELOG.md | 1 + common.h | 3 ++- examples/basic/configuration.php | 5 ++-- tests/ConnectionRequestTest.php | 41 ++++++++++++++++++++++++++++++-- valkey_glide.c | 29 ++++++++++++++++++++++ valkey_glide.stub.php | 8 +++++++ valkey_glide_core_commands.c | 2 ++ 7 files changed, 84 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 252866ba9..eb8fe9e86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Changes +* Add `READ_FROM_AZ_AFFINITY_ALL_NODES` read strategy for standalone and cluster clients ([#316](https://github.com/valkey-io/valkey-glide-php/issues/316)) * Add mutual TLS (mTLS) support for standalone and cluster clients via `advanced_config['tls_config']` — byte-based (`client_cert`/`client_key`) or path-based with automatic certificate reload (`client_cert_path`/`client_key_path` + optional `cert_reload_interval_seconds`) ([#321](https://github.com/valkey-io/valkey-glide-php/pull/321)) * Add `NodeDiscoveryMode` configuration (`STANDARD`, `STATIC`, `DISCOVER_ALL`) for standalone client ([#302](https://github.com/valkey-io/valkey-glide-php/issues/302)) * Add `CONFIG` command for cluster client ([#284](https://github.com/valkey-io/valkey-glide-php/issues/284)) diff --git a/common.h b/common.h index 30b266b76..c45324144 100644 --- a/common.h +++ b/common.h @@ -130,7 +130,8 @@ typedef enum { VALKEY_GLIDE_READ_FROM_PRIMARY = 0, VALKEY_GLIDE_READ_FROM_PREFER_REPLICA = 1, VALKEY_GLIDE_READ_FROM_AZ_AFFINITY = 2, - VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY = 3 + VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY = 3, + VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_ALL_NODES = 4 } valkey_glide_read_from_t; typedef enum { diff --git a/examples/basic/configuration.php b/examples/basic/configuration.php index 1e5214376..75a185491 100644 --- a/examples/basic/configuration.php +++ b/examples/basic/configuration.php @@ -56,7 +56,8 @@ // All configuration options for standalone client $use_tls = false; $credentials = null; // ['username' => 'user', 'password' => 'pass'] -$read_from = 0; // 0=PRIMARY, 1=PREFER_REPLICA, 2=AZ_AFFINITY +$read_from = 0; // 0=PRIMARY, 1=PREFER_REPLICA, 2=AZ_AFFINITY, + // 3=AZ_AFFINITY_REPLICAS_AND_PRIMARY, 4=AZ_AFFINITY_ALL_NODES $request_timeout = 2000; // 2 seconds in milliseconds $reconnect_strategy = [ 'num_of_retries' => 3, @@ -66,7 +67,7 @@ $database_id = 0; // Database number (0 or higher for standalone) $client_name = 'valkey-glide-example'; $inflight_requests_limit = 250; -$client_az = null; // Availability zone for AZ_AFFINITY reads +$client_az = null; // Availability zone; required for AZ_AFFINITY, AZ_AFFINITY_REPLICAS_AND_PRIMARY, and AZ_AFFINITY_ALL_NODES reads $advanced_config = [ 'connection_timeout' => 5000, // Connection timeout in milliseconds 'socket_timeout' => 3000 // Socket timeout in milliseconds diff --git a/tests/ConnectionRequestTest.php b/tests/ConnectionRequestTest.php index 9b36d7421..654715cb5 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -204,16 +204,53 @@ public function testClusterCredentials() public function testStandaloneReadFrom() { - $request = ClientConstructorMock::simulate_standalone_constructor(read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY); + $request = ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY, + client_az: 'us-east-1a' + ); $this->assertEquals(\Connection_request\ReadFrom::AZAffinity, $request->getReadFrom()); } public function testClusterReadFrom() { - $request = ClientConstructorMock::simulate_cluster_constructor(read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY); + $request = ClientConstructorMock::simulate_cluster_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY, + client_az: 'us-east-1a' + ); $this->assertEquals(\Connection_request\ReadFrom::AZAffinityReplicasAndPrimary, $request->getReadFrom()); } + public function testStandaloneReadFromAzAffinityAllNodes() + { + $request = ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES, + client_az: 'us-east-1a' + ); + $this->assertEquals(\Connection_request\ReadFrom::AZAffinityAllNodes, $request->getReadFrom()); + } + + public function testClusterReadFromAzAffinityAllNodes() + { + $request = ClientConstructorMock::simulate_cluster_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES, + client_az: 'us-east-1a' + ); + $this->assertEquals(\Connection_request\ReadFrom::AZAffinityAllNodes, $request->getReadFrom()); + } + + public function testStandaloneAzAffinityAllNodesRequiresClientAz() + { + $this->assertThrowsMatch( + null, + function () { + ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES + ); + }, + '/client_az must be set when read_from is set to AZ_AFFINITY_ALL_NODES/' + ); + } + public function testStandaloneNodeDiscoveryModeDefault() { $request = ClientConstructorMock::simulate_standalone_constructor(); diff --git a/valkey_glide.c b/valkey_glide.c index 6fd5175cb..d3e6f9e53 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -241,6 +241,9 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa case 3: /* AZ_AFFINITY_REPLICAS_AND_PRIMARY */ config->read_from = VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY; break; + case 4: /* AZ_AFFINITY_ALL_NODES */ + config->read_from = VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_ALL_NODES; + break; default: { const char* error_message = "Invalid read_from value."; VALKEY_LOG_ERROR("valkey_glide_build_client_config_base", error_message); @@ -249,6 +252,32 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa } } + /* AZ affinity read strategies require a client availability zone to be set. */ + if (!config->client_az) { + const char* az_error_message = NULL; + switch (config->read_from) { + case VALKEY_GLIDE_READ_FROM_AZ_AFFINITY: + az_error_message = "client_az must be set when read_from is set to AZ_AFFINITY"; + break; + case VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY: + az_error_message = + "client_az must be set when read_from is set to AZ_AFFINITY_REPLICAS_AND_PRIMARY"; + break; + case VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_ALL_NODES: + az_error_message = + "client_az must be set when read_from is set to AZ_AFFINITY_ALL_NODES"; + break; + default: + break; + } + + if (az_error_message) { + VALKEY_LOG_ERROR("valkey_glide_build_client_config_base", az_error_message); + zend_throw_exception(get_valkey_glide_exception_ce(), az_error_message, 0); + return FAILURE; + } + } + /* Map node_discovery_mode enum value to client's NodeDiscoveryMode enum */ switch (params->node_discovery_mode) { case 0: /* STANDARD */ diff --git a/valkey_glide.stub.php b/valkey_glide.stub.php index 12b5bffb0..fa0abd5e2 100644 --- a/valkey_glide.stub.php +++ b/valkey_glide.stub.php @@ -195,6 +195,14 @@ class ValkeyGlide */ public const READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY = 3; + /** + * @var int + * Spread the read requests equally among all nodes (primary and replicas) within the + * client's Availability Zone (AZ) in a round robin manner, falling back to a round robin + * across all nodes if no node in the client's AZ is available. + */ + public const READ_FROM_AZ_AFFINITY_ALL_NODES = 4; + /** * @var int * Default node discovery mode. Verifies node roles via INFO REPLICATION and uses diff --git a/valkey_glide_core_commands.c b/valkey_glide_core_commands.c index 12a26367c..f7cd71f2b 100644 --- a/valkey_glide_core_commands.c +++ b/valkey_glide_core_commands.c @@ -203,6 +203,8 @@ uint8_t* create_connection_request(size_t* len conn_req.read_from = CONNECTION_REQUEST__READ_FROM__AZAffinity; } else if (config->read_from == VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY) { conn_req.read_from = CONNECTION_REQUEST__READ_FROM__AZAffinityReplicasAndPrimary; + } else if (config->read_from == VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_ALL_NODES) { + conn_req.read_from = CONNECTION_REQUEST__READ_FROM__AZAffinityAllNodes; } else { const char* error_message = "Invalid read_from value."; VALKEY_LOG_ERROR("create_connection_request", error_message); From be414b1cb273f9b679673873534c329d3e2848f7 Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Wed, 16 Sep 2026 10:12:21 -0700 Subject: [PATCH 2/6] Update tests Signed-off-by: Prateek Kumar --- tests/ConnectionRequestTest.php | 86 +++++++++++++++++++++++++++++++++ valkey_glide.c | 21 +++++++- 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/tests/ConnectionRequestTest.php b/tests/ConnectionRequestTest.php index 654715cb5..2e23618ed 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -251,6 +251,92 @@ function () { ); } + public function testClusterAzAffinityAllNodesRequiresClientAz() + { + $this->assertThrowsMatch( + null, + function () { + ClientConstructorMock::simulate_cluster_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES + ); + }, + '/client_az must be set when read_from is set to AZ_AFFINITY_ALL_NODES/' + ); + } + + /** + * Every AZ-affinity strategy must require a client AZ, and each must report its own name in the + * error. Mirrors the reference clients, which validate all three strategies rather than only the + * newly added one, so a regression in any of them is caught here. + */ + public function testAzAffinityStrategiesRequireClientAz() + { + $strategies = [ + ValkeyGlide::READ_FROM_AZ_AFFINITY => 'AZ_AFFINITY', + ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY => 'AZ_AFFINITY_REPLICAS_AND_PRIMARY', + ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES => 'AZ_AFFINITY_ALL_NODES', + ]; + + foreach ($strategies as $read_from => $name) { + $pattern = '/client_az must be set when read_from is set to ' . preg_quote($name, '/') . '/'; + + $this->assertThrowsMatch( + null, + function () use ($read_from) { + ClientConstructorMock::simulate_standalone_constructor(read_from: $read_from); + }, + $pattern + ); + + $this->assertThrowsMatch( + null, + function () use ($read_from) { + ClientConstructorMock::simulate_cluster_constructor(read_from: $read_from); + }, + $pattern + ); + } + } + + /** + * Non-AZ strategies never require a client AZ. Confirms the validation is scoped to exactly the + * AZ-affinity strategies and does not leak into the default/replica strategies. + */ + public function testNonAzStrategiesDoNotRequireClientAz() + { + $strategies = [ + ValkeyGlide::READ_FROM_PRIMARY => \Connection_request\ReadFrom::Primary, + ValkeyGlide::READ_FROM_PREFER_REPLICA => \Connection_request\ReadFrom::PreferReplica, + ]; + + foreach ($strategies as $read_from => $expected) { + $request = ClientConstructorMock::simulate_standalone_constructor(read_from: $read_from); + $this->assertEquals($expected, $request->getReadFrom()); + } + } + + /** + * A whitespace-only client_az is treated as absent. The core compares AZs with exact equality + * and never trims, so a blank value would otherwise engage the strategy, match no node, and + * silently fall back to routing across all nodes. Rejecting it surfaces the misconfiguration at + * client creation instead. + */ + public function testAzAffinityAllNodesRejectsBlankClientAz() + { + foreach (['', ' ', ' ', "\t", "\n", " \t\n "] as $blank) { + $this->assertThrowsMatch( + null, + function () use ($blank) { + ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES, + client_az: $blank + ); + }, + '/client_az must be set when read_from is set to AZ_AFFINITY_ALL_NODES/' + ); + } + } + public function testStandaloneNodeDiscoveryModeDefault() { $request = ClientConstructorMock::simulate_standalone_constructor(); diff --git a/valkey_glide.c b/valkey_glide.c index d3e6f9e53..6991eaa32 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -217,8 +217,25 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa since it is effectively one-request-at-a-time. */ config->inflight_requests_limit = -1; - /* Set client availability zone */ - config->client_az = (params->client_az && params->client_az_len > 0) ? params->client_az : NULL; + /* Set client availability zone. + * + * A value that is empty or only whitespace is treated as absent (NULL). The core compares + * availability zones with exact equality and never trims, so a blank value such as " " would + * otherwise satisfy the AZ-affinity requirement below, engage the strategy, match no node, and + * silently spread reads across all nodes. Rejecting it here surfaces the misconfiguration at + * client creation instead. Only the borrowed PHP pointer is forwarded, never a copy, so client + * config ownership is unchanged. */ + config->client_az = NULL; + if (params->client_az && params->client_az_len > 0) { + for (size_t az_i = 0; az_i < params->client_az_len; az_i++) { + char az_ch = params->client_az[az_i]; + if (az_ch != ' ' && az_ch != '\t' && az_ch != '\n' && az_ch != '\r' && + az_ch != '\f' && az_ch != '\v') { + config->client_az = params->client_az; + break; + } + } + } /* Set lazy connect option */ config->lazy_connect = params->lazy_connect_is_null ? false : params->lazy_connect; From 2952d98590a3bd8c7a7ef12530bd19ac72af6ac2 Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Wed, 16 Sep 2026 10:17:15 -0700 Subject: [PATCH 3/6] fix(php): format C code and set client_az in AZ-affinity tests - Apply clang-format-18 to valkey_glide.c (lint job). - Add required client_az to pre-existing AZ-affinity constructor tests in ValkeyGlideFeaturesTest and ValkeyGlideClusterFeaturesTest, which now throw under the new client-AZ validation. Signed-off-by: Prateek Kumar --- tests/ValkeyGlideClusterFeaturesTest.php | 6 ++++-- tests/ValkeyGlideFeaturesTest.php | 8 ++++---- valkey_glide.c | 7 ++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/ValkeyGlideClusterFeaturesTest.php b/tests/ValkeyGlideClusterFeaturesTest.php index c1aedf485..d31ccd0ec 100644 --- a/tests/ValkeyGlideClusterFeaturesTest.php +++ b/tests/ValkeyGlideClusterFeaturesTest.php @@ -307,7 +307,8 @@ public function testConstructorWithReadFromAzAffinity() addresses: [['host' => '127.0.0.1', 'port' => 7001]], use_tls: false, credentials: $this->getAuth(), - read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY, + client_az: 'us-east-1a' ); $this->assertTrue($valkey_glide->ping(['type' => 'primarySlotKey', 'key' => 'test'])); @@ -321,7 +322,8 @@ public function testConstructorWithReadFromAzAffinityReplicasAndPrimary() addresses: [['host' => '127.0.0.1', 'port' => 7001]], use_tls: false, credentials: $this->getAuth(), - read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY, + client_az: 'us-east-1a' ); $this->assertTrue($valkey_glide->ping(['type' => 'primarySlotKey', 'key' => 'test'])); diff --git a/tests/ValkeyGlideFeaturesTest.php b/tests/ValkeyGlideFeaturesTest.php index 8220c0b21..c16304ddc 100644 --- a/tests/ValkeyGlideFeaturesTest.php +++ b/tests/ValkeyGlideFeaturesTest.php @@ -597,13 +597,13 @@ public function testConstructorWithAzAffinityReadStrategy() // Test READ_FROM_AZ_AFFINITY if (!$this->getTLS()) { $valkey_glide = new ValkeyGlide(); - $valkey_glide->connect(addresses: $addresses, use_tls: $this->getTLS(), read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY); + $valkey_glide->connect(addresses: $addresses, use_tls: $this->getTLS(), read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY, client_az: 'us-east-1a'); } else { $advancedConfig = [ 'tls_config' => ['use_insecure_tls' => true] ]; $valkey_glide = new ValkeyGlide(); - $valkey_glide->connect(addresses: $addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY, advanced_config: $advancedConfig); + $valkey_glide->connect(addresses: $addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY, client_az: 'us-east-1a', advanced_config: $advancedConfig); } $this->assertTrue($valkey_glide->ping()); $valkey_glide->close(); @@ -611,13 +611,13 @@ public function testConstructorWithAzAffinityReadStrategy() // Test READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY if (!$this->getTLS()) { $valkey_glide = new ValkeyGlide(); - $valkey_glide->connect(addresses: $addresses, use_tls: $this->getTLS(), read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY); + $valkey_glide->connect(addresses: $addresses, use_tls: $this->getTLS(), read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY, client_az: 'us-east-1a'); } else { $advancedConfig = [ 'tls_config' => ['use_insecure_tls' => true] ]; $valkey_glide = new ValkeyGlide(); - $valkey_glide->connect(addresses: $addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY, advanced_config: $advancedConfig); + $valkey_glide->connect(addresses: $addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY, client_az: 'us-east-1a', advanced_config: $advancedConfig); } $this->assertTrue($valkey_glide->ping()); $valkey_glide->close(); diff --git a/valkey_glide.c b/valkey_glide.c index 6991eaa32..fac16ab0a 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -229,8 +229,8 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa if (params->client_az && params->client_az_len > 0) { for (size_t az_i = 0; az_i < params->client_az_len; az_i++) { char az_ch = params->client_az[az_i]; - if (az_ch != ' ' && az_ch != '\t' && az_ch != '\n' && az_ch != '\r' && - az_ch != '\f' && az_ch != '\v') { + if (az_ch != ' ' && az_ch != '\t' && az_ch != '\n' && az_ch != '\r' && az_ch != '\f' && + az_ch != '\v') { config->client_az = params->client_az; break; } @@ -278,7 +278,8 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa break; case VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY: az_error_message = - "client_az must be set when read_from is set to AZ_AFFINITY_REPLICAS_AND_PRIMARY"; + "client_az must be set when read_from is set to " + "AZ_AFFINITY_REPLICAS_AND_PRIMARY"; break; case VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_ALL_NODES: az_error_message = From 2e42f8cbeac78fccef32728dd4fb5f0012df27ba Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Wed, 16 Sep 2026 14:00:31 -0700 Subject: [PATCH 4/6] fix(php): reject empty-as-C-string client_az for AZ-affinity strategies Signed-off-by: Prateek Kumar --- tests/ConnectionRequestTest.php | 17 ++++++++++++----- valkey_glide.c | 8 +++++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/ConnectionRequestTest.php b/tests/ConnectionRequestTest.php index 2e23618ed..5e6cd26aa 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -316,14 +316,21 @@ public function testNonAzStrategiesDoNotRequireClientAz() } /** - * A whitespace-only client_az is treated as absent. The core compares AZs with exact equality - * and never trims, so a blank value would otherwise engage the strategy, match no node, and - * silently fall back to routing across all nodes. Rejecting it surfaces the misconfiguration at - * client creation instead. + * A client_az that is empty, whitespace-only, or empty as a C string (leading NUL) is treated + * as absent. The core compares AZs with exact equality and never trims, and the value reaches + * the core as a NUL-terminated C string, so any of these would otherwise engage the strategy, + * match no node, and silently fall back to routing across all nodes. Rejecting them surfaces + * the misconfiguration at client creation instead. */ public function testAzAffinityAllNodesRejectsBlankClientAz() { - foreach (['', ' ', ' ', "\t", "\n", " \t\n "] as $blank) { + $blanks = ['', ' ', ' ', "\t", "\n", " \t\n "]; + // Empty or whitespace-prefixed as a C string: everything from the first NUL is dropped. + $blanks[] = "\0"; + $blanks[] = "\0us-east-1a"; + $blanks[] = " \0us-east-1a"; + + foreach ($blanks as $blank) { $this->assertThrowsMatch( null, function () use ($blank) { diff --git a/valkey_glide.c b/valkey_glide.c index fac16ab0a..5447eef67 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -219,7 +219,9 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa /* Set client availability zone. * - * A value that is empty or only whitespace is treated as absent (NULL). The core compares + * A value that is empty, only whitespace, or empty as a C string is treated as absent (NULL). + * The value is forwarded to the core as a NUL-terminated C string, so scanning stops at the + * first NUL byte: a value like "\0..." is effectively empty on the wire. The core compares * availability zones with exact equality and never trims, so a blank value such as " " would * otherwise satisfy the AZ-affinity requirement below, engage the strategy, match no node, and * silently spread reads across all nodes. Rejecting it here surfaces the misconfiguration at @@ -229,6 +231,10 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa if (params->client_az && params->client_az_len > 0) { for (size_t az_i = 0; az_i < params->client_az_len; az_i++) { char az_ch = params->client_az[az_i]; + /* Stop at the first NUL: bytes past it never reach the core. */ + if (az_ch == '\0') { + break; + } if (az_ch != ' ' && az_ch != '\t' && az_ch != '\n' && az_ch != '\r' && az_ch != '\f' && az_ch != '\v') { config->client_az = params->client_az; From 4a084d0bda8dcfad095f3466ab7112fe0610248d Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Thu, 17 Sep 2026 10:22:27 -0700 Subject: [PATCH 5/6] Fix review comments Signed-off-by: Prateek Kumar --- tests/ConnectionRequestTest.php | 41 +++++++++++++++++++++++---------- valkey_glide.c | 22 ++++++++++++------ valkey_glide.stub.php | 3 +++ 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/tests/ConnectionRequestTest.php b/tests/ConnectionRequestTest.php index 5e6cd26aa..4e5c6e2eb 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -316,21 +316,14 @@ public function testNonAzStrategiesDoNotRequireClientAz() } /** - * A client_az that is empty, whitespace-only, or empty as a C string (leading NUL) is treated - * as absent. The core compares AZs with exact equality and never trims, and the value reaches - * the core as a NUL-terminated C string, so any of these would otherwise engage the strategy, - * match no node, and silently fall back to routing across all nodes. Rejecting them surfaces - * the misconfiguration at client creation instead. + * A client_az that is empty or whitespace-only is treated as absent. The core compares AZs with + * exact equality and never trims, so a blank value would otherwise engage the strategy, match no + * node, and silently fall back to routing across all nodes. Rejecting it surfaces the + * misconfiguration at client creation instead. */ public function testAzAffinityAllNodesRejectsBlankClientAz() { - $blanks = ['', ' ', ' ', "\t", "\n", " \t\n "]; - // Empty or whitespace-prefixed as a C string: everything from the first NUL is dropped. - $blanks[] = "\0"; - $blanks[] = "\0us-east-1a"; - $blanks[] = " \0us-east-1a"; - - foreach ($blanks as $blank) { + foreach (['', ' ', ' ', "\t", "\n", " \t\n "] as $blank) { $this->assertThrowsMatch( null, function () use ($blank) { @@ -344,6 +337,30 @@ function () use ($blank) { } } + /** + * A client_az containing a NUL byte is rejected outright. The value reaches the core as a + * NUL-terminated C string, so an embedded NUL would be silently truncated on the wire (e.g. + * "us-east-1a\0x" would arrive as "us-east-1a"), changing AZ routing without the caller's + * knowledge. A NUL is never part of a legitimate availability-zone name. + */ + public function testClientAzRejectsNulBytes() + { + $nul_values = ["\0", "\0us-east-1a", " \0us-east-1a", "us-east-1a\0", "us-east-1a\0x"]; + + foreach ($nul_values as $value) { + $this->assertThrowsMatch( + null, + function () use ($value) { + ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES, + client_az: $value + ); + }, + '/client_az must not contain NUL bytes/' + ); + } + } + public function testStandaloneNodeDiscoveryModeDefault() { $request = ClientConstructorMock::simulate_standalone_constructor(); diff --git a/valkey_glide.c b/valkey_glide.c index 5447eef67..dee5e6dcc 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -219,9 +219,12 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa /* Set client availability zone. * - * A value that is empty, only whitespace, or empty as a C string is treated as absent (NULL). - * The value is forwarded to the core as a NUL-terminated C string, so scanning stops at the - * first NUL byte: a value like "\0..." is effectively empty on the wire. The core compares + * The value is forwarded to the core as a NUL-terminated C string, so an embedded NUL would be + * silently truncated on the wire (e.g. "us-east-1a\0x" would reach the core as "us-east-1a"), + * changing AZ-based routing without the caller's knowledge. A NUL is never part of a legitimate + * availability-zone name, so reject any value containing one. + * + * A value that is empty or only whitespace is treated as absent (NULL). The core compares * availability zones with exact equality and never trims, so a blank value such as " " would * otherwise satisfy the AZ-affinity requirement below, engage the strategy, match no node, and * silently spread reads across all nodes. Rejecting it here surfaces the misconfiguration at @@ -229,18 +232,23 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa * config ownership is unchanged. */ config->client_az = NULL; if (params->client_az && params->client_az_len > 0) { + bool az_has_content = false; for (size_t az_i = 0; az_i < params->client_az_len; az_i++) { char az_ch = params->client_az[az_i]; - /* Stop at the first NUL: bytes past it never reach the core. */ if (az_ch == '\0') { - break; + const char* az_error_message = "client_az must not contain NUL bytes."; + VALKEY_LOG_ERROR("valkey_glide_build_client_config_base", az_error_message); + zend_throw_exception(get_valkey_glide_exception_ce(), az_error_message, 0); + return FAILURE; } if (az_ch != ' ' && az_ch != '\t' && az_ch != '\n' && az_ch != '\r' && az_ch != '\f' && az_ch != '\v') { - config->client_az = params->client_az; - break; + az_has_content = true; } } + if (az_has_content) { + config->client_az = params->client_az; + } } /* Set lazy connect option */ diff --git a/valkey_glide.stub.php b/valkey_glide.stub.php index fa0abd5e2..8c48cd1b6 100644 --- a/valkey_glide.stub.php +++ b/valkey_glide.stub.php @@ -200,6 +200,9 @@ class ValkeyGlide * Spread the read requests equally among all nodes (primary and replicas) within the * client's Availability Zone (AZ) in a round robin manner, falling back to a round robin * across all nodes if no node in the client's AZ is available. + * + * Requires `client_az` to be set on the client configuration. Connecting with this + * strategy and a missing or whitespace-only `client_az` throws a ValkeyGlideException. */ public const READ_FROM_AZ_AFFINITY_ALL_NODES = 4; From 4b3ebf9eb734e896b281a36a311b04404f12291f Mon Sep 17 00:00:00 2001 From: Prateek Kumar Date: Fri, 18 Sep 2026 09:52:18 -0700 Subject: [PATCH 6/6] Fix review comments Signed-off-by: Prateek Kumar --- CHANGELOG.md | 4 ++++ tests/ConnectionRequestTest.php | 24 ++++++++++++++++++++++++ valkey_glide.c | 28 ++++++++++++++++++++++------ 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb8fe9e86..535c80520 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Breaking Changes + +* An AZ-affinity read strategy (`READ_FROM_AZ_AFFINITY`, `READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY`, or `READ_FROM_AZ_AFFINITY_ALL_NODES`) configured without a valid `client_az` now throws at client creation. Previously the core logged a warning and downgraded the strategy to `PreferReplica`, so reads silently went to arbitrary nodes. A `client_az` that is empty, whitespace-only, contains a NUL byte, or has leading/trailing whitespace is rejected ([#316](https://github.com/valkey-io/valkey-glide-php/issues/316)) + ### Changes * Add `READ_FROM_AZ_AFFINITY_ALL_NODES` read strategy for standalone and cluster clients ([#316](https://github.com/valkey-io/valkey-glide-php/issues/316)) diff --git a/tests/ConnectionRequestTest.php b/tests/ConnectionRequestTest.php index 4e5c6e2eb..d0049fd29 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -361,6 +361,30 @@ function () use ($value) { } } + /** + * A client_az with leading or trailing whitespace (e.g. "us-east-1a\n" from getenv or + * file_get_contents) is rejected. It has content but would never match a node under the core's + * exact-equality compare, silently falling back to reading across all nodes. Rejecting it + * surfaces the misconfiguration at client creation instead. + */ + public function testClientAzRejectsSurroundingWhitespace() + { + $padded = ["us-east-1a\n", " us-east-1a", "us-east-1a ", "\tus-east-1a", " us-east-1a "]; + + foreach ($padded as $value) { + $this->assertThrowsMatch( + null, + function () use ($value) { + ClientConstructorMock::simulate_standalone_constructor( + read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY_ALL_NODES, + client_az: $value + ); + }, + '/client_az must not have leading or trailing whitespace/' + ); + } + } + public function testStandaloneNodeDiscoveryModeDefault() { $request = ClientConstructorMock::simulate_standalone_constructor(); diff --git a/valkey_glide.c b/valkey_glide.c index dee5e6dcc..f555086a3 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -224,12 +224,15 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa * changing AZ-based routing without the caller's knowledge. A NUL is never part of a legitimate * availability-zone name, so reject any value containing one. * - * A value that is empty or only whitespace is treated as absent (NULL). The core compares - * availability zones with exact equality and never trims, so a blank value such as " " would - * otherwise satisfy the AZ-affinity requirement below, engage the strategy, match no node, and - * silently spread reads across all nodes. Rejecting it here surfaces the misconfiguration at - * client creation instead. Only the borrowed PHP pointer is forwarded, never a copy, so client - * config ownership is unchanged. */ + * The core compares availability zones with exact equality and never trims. Therefore: + * - An empty or whitespace-only value is treated as absent (NULL); the AZ-affinity + * requirement below then rejects it, surfacing the misconfiguration at client creation instead + * of letting the strategy engage, match no node, and silently spread reads across all nodes. + * - A value with leading or trailing whitespace (e.g. "us-east-1a\n" from + * getenv/file_get_contents) has content but would never match a node under the core's + * exact-equality compare, producing the same silent all-nodes fallback. It is rejected rather + * than forwarded. Only the borrowed PHP pointer is forwarded, never a copy, so client config + * ownership is unchanged. */ config->client_az = NULL; if (params->client_az && params->client_az_len > 0) { bool az_has_content = false; @@ -247,6 +250,19 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa } } if (az_has_content) { + char first_ch = params->client_az[0]; + char last_ch = params->client_az[params->client_az_len - 1]; + bool is_space = + (first_ch == ' ' || first_ch == '\t' || first_ch == '\n' || first_ch == '\r' || + first_ch == '\f' || first_ch == '\v' || last_ch == ' ' || last_ch == '\t' || + last_ch == '\n' || last_ch == '\r' || last_ch == '\f' || last_ch == '\v'); + if (is_space) { + const char* az_error_message = + "client_az must not have leading or trailing whitespace."; + VALKEY_LOG_ERROR("valkey_glide_build_client_config_base", az_error_message); + zend_throw_exception(get_valkey_glide_exception_ce(), az_error_message, 0); + return FAILURE; + } config->client_az = params->client_az; } }