Skip to content

client_az is never validated for AZ-affinity read strategies #334

Description

@jamesx-improving

Description

The PHP client accepts client_az but never validates that it is present when an AZ-affinity read_from is selected. Every other GLIDE binding rejects that combination at construction time; PHP forwards it, and the core silently downgrades the strategy to PreferReplica. A caller who mistypes the option name, or omits it entirely, gets a working client whose reads ignore availability zones — with no exception, no warning surfaced to PHP, and no indication the strategy is not in effect.

valkey_glide_core_commands.c:200-206 selects the strategy, and :278-280 forwards the AZ only if the pointer is non-NULL:

} else if (config->read_from == VALKEY_GLIDE_READ_FROM_AZ_AFFINITY) {
    ...
} else if (config->read_from == VALKEY_GLIDE_READ_FROM_AZ_AFFINITY_REPLICAS_AND_PRIMARY) {
    ...
}
...
if (config->client_az) {
    conn_req.client_az = config->client_az;
}

Nothing anywhere raises when the strategy requires an AZ and none was given. A search for must be set / client_az across the repo returns no validation, only the parameter plumbing in valkey_glide_cluster.c:177-284 and the stub docs.

Two distinct defects

1. A missing client_az is not rejected. In the core, read_from = AZAffinity with an empty client_az hits the None branch of chars_to_string_option (glide-core/src/client/types.rs:263-269), which logs a warning and downgrades to ReadFrom::PreferReplica (types.rs:284-325). The client works; the requested strategy is silently not the one in use.

2. A whitespace-only client_az is worse, because it skips even that downgrade. chars_to_string_option only tests is_empty(), so " " becomes Some(" ") and constructs a live AZAffinity(" "). AZ comparisons in the core are exact and never trimmed — node_az == client_az for standalone, az_for_address(&address).as_deref() == Some(client_az) for cluster (connections_container.rs:500,543,561) — so no node matches and every read takes the no-in-AZ-node fallback across all nodes.

Reproduction

// No exception today. Expected: an exception naming client_az.
$client = new ValkeyGlideCluster(
    [['host' => 'localhost', 'port' => 7000]],
    read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY,
);
// Reads ignore AZs entirely (core downgraded to PreferReplica).

// Also no exception, and worse — the strategy engages but matches nothing:
$client = new ValkeyGlideCluster(
    [['host' => 'localhost', 'port' => 7000]],
    read_from: ValkeyGlide::READ_FROM_AZ_AFFINITY,
    client_az: '   ',
);

Suggested fix

Reject both cases in PHP, before the ConnectionRequest is built, treating a blank string as absent. client_az_len is already captured next to the pointer (valkey_glide_cluster.c:177-178), so the check has what it needs without new plumbing. Forward the original value unnormalized when it is valid — a legitimate AZ with incidental padding should be accepted, and trimming it would silently change what the caller asked for.

Error wording used by the other bindings, for consistency: client_az must be set when read_from is set to <strategy>.

The whitebox constructor-argument test framework from #24 is the natural place for coverage: assert the exception for a missing AZ and for a whitespace-only AZ, plus a companion asserting a padded-but-valid AZ still reaches the ConnectionRequest unchanged.

Notes

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingphpPull requests that update php code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions