diff --git a/CHANGELOG.md b/CHANGELOG.md index 252866ba9..535c80520 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,13 @@ ## 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)) * 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..d0049fd29 100644 --- a/tests/ConnectionRequestTest.php +++ b/tests/ConnectionRequestTest.php @@ -204,16 +204,187 @@ 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 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 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() + { + 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/' + ); + } + } + + /** + * 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/' + ); + } + } + + /** + * 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/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 6fd5175cb..f555086a3 100644 --- a/valkey_glide.c +++ b/valkey_glide.c @@ -217,8 +217,55 @@ 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. + * + * 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. + * + * 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; + 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 == '\0') { + 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') { + az_has_content = true; + } + } + 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; + } + } /* Set lazy connect option */ config->lazy_connect = params->lazy_connect_is_null ? false : params->lazy_connect; @@ -241,6 +288,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 +299,33 @@ 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..8c48cd1b6 100644 --- a/valkey_glide.stub.php +++ b/valkey_glide.stub.php @@ -195,6 +195,17 @@ 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. + * + * 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; + /** * @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);