Uh oh!
There was an error while loading. Please reload this page.
Always issue HELLO when the server should understand it, not just for RESP3 - #3175
Merged
Conversation
… RESP3 Fixes#2968 (remaining concern). When INFO is unavailable, discovery fell back to `SET {guid} replica-read-only PX 1 NX` to detect a read-only replica. That probe writes a random, unprefixable key, so it can never be allow-listed by an ACL key pattern - which is exactly the situation that makes INFO unavailable in the first place (INFO and CONFIG are both @dangerous). HELLO's reply carries "role", needs no key, and is in the @connection category. So: issue HELLO whenever it is available and the assumed server version is 6.0+, and skip the key-based probe when HELLO is going to tell us (or has already told us) the role. Two independent opt-outs remain: `$hello=` in the command map, and a sub-6.0 `defaultVersion`. As a bonus, RESP2 connections now learn the server version, mode and connection id from the handshake as well. The RESP2 HELLO is deliberately not the same message as the RESP3 one: - `HELLO 3` stays first-in-pipeline and carries the credentials, as it must for RESP3 negotiation to happen at all on a secured server. - `HELLO 2` is a bare HELLO issued *after* AUTH, on the interactive connection only. It isn't negotiating anything, and folding credentials in would change how credential failures surface (there is a pre-existing difference in behaviour between AUTH failing on its own and AUTH failing inside HELLO; that is a separate bug, not one to inherit here). Also excludes HELLO from the twemproxy and envoyproxy command maps, and verifies against both proxies locally: - twemproxy 0.5.0 (the newest release) *closes the connection* on an unsupported command, so HELLO is fatal. Since v3 raised the assumed default version to 6.0, RESP3 - and therefore HELLO - became the default, and a default-configured twemproxy connection never became usable at all. That is a v3 regression against v2, where the assumed version of 3.0 meant no HELLO was ever sent. - envoy up to ~1.31 answers "unsupported command"; 1.39 instead proxies HELLO to an arbitrary backend node, so the version/role/mode in the reply describe some other server. Taking that at face value would flip a proxy endpoint into cluster mode or mark it as a replica, so HELLO's mode/role are now only applied to server types that support auto-configure - proxies are excluded even if a hand-rolled command map re-enables the command. Bumps the test-topology envoy pin from v1.31 to v1.39 (8 minors stale, and the two versions behave differently here). Tests use a recording in-process server to pin exactly which commands the handshake issues: HELLO at the right protocol level, absent when disabled either way, and the replica probe present only when HELLO cannot tell us the role. The Resp3HandshakeTests matrix no longer skips RESP2 clients (they issue HELLO now too), and asserts the negotiated protocol.
EventId 110 is in use in another in-progress PR, and this doesn't warrant a logger message of its own - the handshake HELLO is already visible in the detail/parse logs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#2968 (the remaining concern), and fixes a v3 regression against twemproxy found on the way.
The #2968 problem
With
INFOunavailable, discovery fell back toSET {guid} replica-read-only PX 1 NXto detect a read-only replica. That probe writes a random, unprefixable key, so it can never be allow-listed by an ACL key pattern - which is precisely the situation that madeINFOunavailable in the first place (INFOandCONFIGare both@dangerous). The reporter was left with permanent ACL-log noise they had no way to silence.HELLO's reply carriesrole, needs no key, and is in the@connectioncategory. It also reports the version, mode (standalone/sentinel/cluster) and connection id. We were only issuing it when negotiating RESP3.The change
ConfigurationOptions.TryHello(out int protocolVersion)alongsideTryResp3(), plusRedisFeatures.Hello(6.0).HELLOis issued whenever it is available and the assumed server version is 6.0+. Two independent opt-outs:$hello=in the command map, and a sub-6.0defaultVersion.HELLOis going to report the role (same handshake batch) or has already reported it (sticky per-endpoint flag, for later reconfigure sweeps).The RESP2
HELLOis deliberately not the same message as the RESP3 one:HELLO 3stays first-in-pipeline and carries the credentials, as it must for RESP3 negotiation to work on a secured server.HELLO 2is a bareHELLOissued afterAUTH, on the interactive connection only. It negotiates nothing, and folding credentials in would change how credential failures surface - there is a pre-existing difference betweenAUTHfailing on its own andAUTHfailing insideHELLO, which is a separate bug and not one to inherit here.Proxies (measured against live proxies, not assumed)
HELLObehaviourparsed unsupported command 'hello', then closes the connection-unsupported command 'hello'HELLO 2proxied to an arbitrary backend node;HELLO 3->-NOPROTOConsequences, both fixed here by excluding
HELLOfromCommandMap.TwemproxyandCommandMap.Envoyproxy:TryResp3()was false and noHELLOwas sent; v3 raised the assumed default to 6.0, and the twemproxy map did not excludeHELLO. Verified with a real twemproxy:SocketClosed (0-read), connection never usable.HELLO 2reportedversion=8.9.241, mode=cluster, role=replica- i.e. some backend node, not the endpoint.mode/rolefromHELLOare therefore only applied to server types that support auto-configure, so proxies are excluded even if a hand-rolled command map re-enables the command.Also bumps the test-topology envoy pin from v1.31 to v1.39 (8 minors stale, and the two behave differently here).
Tests
HelloHandshakeTestsuses a recording in-process server (captures every command and its args) to pin exactly what the handshake issues:HELLOat the right protocol level for both RESP2 and RESP3HELLOwhen disabled via the command map, or whendefaultVersionis below 6.0INFO/CONFIGare unavailable butHELLOis, withIsReplicacoming fromHELLO'sroleHELLOunavailable too -> the probe is issuedResp3HandshakeTestsno longer skips the RESP2-client half of its matrix (128 -> 192 cases), so the "server does not understand HELLO" and "server insists on RESP2" spoofs now cover RESP2 clients too, and it asserts the negotiated protocol. Plus aCommandMapUnitTestscase pinning the proxy exclusions.Full suite green;
EnvoyTestsverified against a rebuilt v1.39 container.Follow-ups (not in this PR)
HELLOfails (bad credentials, timeout, down-level server), the protocol downgrade mid-handshake leaves queuedSUBSCRIBEtraffic on the interactive connection. Looks like the same mechanism as RESP3→RESP2 reconnect can replay SUBSCRIBE onto the interactive connection, permanently poisoning it #3154, and plausibly related to Error connecting to Sentinel with v3 due to Resp3 #3172.EXISTS {guid}tracer fallback ignoring hash slots, and skipping the tie-breaker on cluster.