Add Cluster Bus Port out of range error message for Cluster Meet command

[hwware]

Now for the cluster meet command, we first check the port and cport number, and then the function clusterStartHandshake() is called.
In the function clusterStartHandshake(), ip address is checked first and then port and cport range are checked.
Even port or cport range is out of bound, only error message "Invalid node address specified" is reported.
This is not correct.

In this PR, I just make the port and cport check together before clusterStartHandshake() function. Thus the port number and range can be checked together.

One example:

Current behavior:

127.0.0.1:7000> cluster meet 10.21.96.98 65000
(error) ERR Invalid node address specified: 10.21.96.98:65000
127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379
(error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379

Whatever the wrong ip address or incorrect bus port number, user get the same error message.

New behavior:

127.0.0.1:7000> cluster meet 10.21.96.98 65000
(error) ERR Cluster bus port number is out of range
127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379
(error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379

User can get much clearer error message.

Related note pr: valkey-io/valkey-doc#240

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.17%. Comparing base (79d5047) to head (e634138).
Report is 5 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/cluster_legacy.c	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #1686   +/-   ##
=========================================
  Coverage     71.17%   71.17%           
=========================================
  Files           123      123           
  Lines         65641    65644    +3     
=========================================
+ Hits          46720    46724    +4     
+ Misses        18921    18920    -1     

Files with missing lines	Coverage Δ
src/cluster_legacy.c	86.12% <33.33%> (+0.22%)	⬆️

... and 13 files with indirect coverage changes

[zuiderkwast]

The behavior change is that CLUSTER MEET can return an error instead of OK when the ports are invalid. This means the user can see the error early and understand what's wrong.

This is the user-visible change, so it should be the title of the PR I think. The rest is just refactoring which is not important for users and release notes.

[enjoy-binbin]

why we need this? The old code also seems to be very easy to understand and maintain

[hwware]

why we need this? The old code also seems to be very easy to understand and maintain

Current old cause developers hard to identify the problem. Existing logic is to check port,, and then in function clusterStartHandshake check the ip first and check the port again.

The easier way to put all port checking logic together, and then check the ip.

[zuiderkwast]

@enjoy-binbin @hwware The first commit is smaller. It just moves the check from clusterStartHandshake to CLUSTER MEET so it can return an error, without extra refactoring.

ce4c0d4

[hwware]

@enjoy-binbin @hwware The first commit is smaller. It just moves the check from clusterStartHandshake to CLUSTER MEET so it can return an error, without extra refactoring.

ce4c0d4

Yes, I just make thing easier. It looks like i update codes to the fist commit, and check the port and cport in the Cluster Meet part.
Sometimes simple is good enough

[zuiderkwast]

Yes, this is simpler. I like it more. @enjoy-binbin WDYT?

I have just a minor comment about the error message.

[zuiderkwast]

LGTM.

Before merge, please update the PR title to clarify the user-visible behavior, that CLUSTER MEET validates the ports and returns error for invalid ports.

That is what we should mention in the release notes.

[enjoy-binbin]

The first commit is smaller. It just moves the check from clusterStartHandshake to CLUSTER MEET so it can return an error, without extra refactoring.

we do return an error in this check.

        if (clusterStartHandshake(c->argv[2]->ptr, port, cport) == 0 && errno == EINVAL) {
            addReplyErrorFormat(c, "Invalid node address specified: %s:%s", (char *)c->argv[2]->ptr,
                                (char *)c->argv[3]->ptr);
        } else {
            sds client = catClientInfoShortString(sdsempty(), c, server.hide_user_data_from_log);
            serverLog(LL_NOTICE, "Cluster meet %s:%lld (user request from '%s').", (char *)c->argv[2]->ptr, port,
                      client);
            sdsfree(client);
            addReply(c, shared.ok);
        }

Current old cause developers hard to identify the problem. Existing logic is to check port,, and then in function clusterStartHandshake check the ip first and check the port again.

how so... they are doing different thing, one is extract the number and check it range, the other is check the port range. clusterStartHandshake can has it own check i think, the code is pretty simple why it would casue the trouble to the developers.

[hwware]

The first commit is smaller. It just moves the check from clusterStartHandshake to CLUSTER MEET so it can return an error, without extra refactoring.

we do return an error in this check.

        if (clusterStartHandshake(c->argv[2]->ptr, port, cport) == 0 && errno == EINVAL) {
            addReplyErrorFormat(c, "Invalid node address specified: %s:%s", (char *)c->argv[2]->ptr,
                                (char *)c->argv[3]->ptr);
        } else {
            sds client = catClientInfoShortString(sdsempty(), c, server.hide_user_data_from_log);
            serverLog(LL_NOTICE, "Cluster meet %s:%lld (user request from '%s').", (char *)c->argv[2]->ptr, port,
                      client);
            sdsfree(client);
            addReply(c, shared.ok);
        }

Current old cause developers hard to identify the problem. Existing logic is to check port,, and then in function clusterStartHandshake check the ip first and check the port again.

how so... they are doing different thing, one is extract the number and check it range, the other is check the port range. clusterStartHandshake can has it own check i think, the code is pretty simple why it would casue the trouble to the developers.

Through this PR, user experience is much clearer and easy to identify where the problem is, for example:

Current behavior:

127.0.0.1:7000> cluster meet 10.21.96.98 65000
(error) ERR Invalid node address specified: 10.21.96.98:65000
127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379
(error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379

Whatever the wrong ip address or incorrect bus port number, user get the same error message.

New behavior:

127.0.0.1:7000> cluster meet 10.21.96.98 65000
(error) ERR Cluster bus port number is out of range
127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379
(error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379

User can get much clearer error message.