Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the TLS and REPS issues about CLUSTER SLOTS cache #581

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

soloestoy
Copy link
Member

@soloestoy soloestoy commented May 31, 2024

PR #53 introduced the cache of CLUSTER SLOTS response, but the cache has some problems for different types of clients:

  1. the RESP version is wrongly ignored:

    $./valkey-cli
    127.0.0.1:6379> cluster slots
    1) 1) (integer) 0
       2) (integer) 16383
       3) 1) ""
          2) (integer) 6379
          3) "f1aeceb352401ce57acd432c68c60b359c00ef85"
          4) (empty array)
    127.0.0.1:6379> hello 3
    1# "server" => "valkey"
    2# "version" => "255.255.255"
    3# "proto" => (integer) 3
    4# "id" => (integer) 3
    5# "mode" => "cluster"
    6# "role" => "master"
    7# "modules" => (empty array)
    127.0.0.1:6379> cluster slots
    1) 1) (integer) 0
       2) (integer) 16383
       3) 1) ""
          2) (integer) 6379
          3) "f1aeceb352401ce57acd432c68c60b359c00ef85"
          4) (empty array)
    

    RESP3 should get "empty hash" but get RESP2's "empty array"

  2. we should use the original client's connect type, or lua/function and module would get wrong port:

    $./valkey-cli --tls --insecure -p 6789
    127.0.0.1:6789> config get port tls-port
    1) "tls-port"
    2) "6789"
    3) "port"
    4) "6379"
    127.0.0.1:6789> cluster slots
    1) 1) (integer) 0
       2) (integer) 16383
       3) 1) ""
          2) (integer) 6789
          3) "f1aeceb352401ce57acd432c68c60b359c00ef85"
          4) (empty array)
    127.0.0.1:6789> eval "return redis.call('cluster','slots')" 0
    1) 1) (integer) 0
       2) (integer) 16383
       3) 1) ""
          2) (integer) 6379
          3) "f1aeceb352401ce57acd432c68c60b359c00ef85"
          4) (empty array)
        ```
    

soloestoy added 2 commits May 31, 2024 11:37
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.00%. Comparing base (5a51bf5) to head (eb956a2).
Report is 4 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #581      +/-   ##
============================================
- Coverage     70.17%   70.00%   -0.18%     
============================================
  Files           110      110              
  Lines         60077    60081       +4     
============================================
- Hits          42160    42057     -103     
- Misses        17917    18024     +107     
Files Coverage Δ
src/cluster.c 87.79% <100.00%> (+0.01%) ⬆️
src/cluster_legacy.c 85.79% <100.00%> (+<0.01%) ⬆️
src/networking.c 85.42% <100.00%> (+0.05%) ⬆️
src/server.h 100.00% <ø> (ø)

... and 7 files with indirect coverage changes

@hpatro
Copy link
Contributor

hpatro commented May 31, 2024

Nice catch @soloestoy!

Could we add test cases for these scenario for coverage?

src/cluster.c Show resolved Hide resolved
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I forgot that we added a map response at the end. I assumed the response would be the same.

I'm wondering now why the response framework didn't catch this.

Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Overall.

src/cluster.c Show resolved Hide resolved
src/cluster.c Show resolved Hide resolved
src/cluster.c Show resolved Hide resolved
src/server.h Outdated Show resolved Hide resolved
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
src/cluster.c Show resolved Hide resolved
src/cluster.c Show resolved Hide resolved
@soloestoy soloestoy merged commit 4fbe31a into valkey-io:unstable Jun 28, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants