Skip to content

Fix tls port update not reflected in CLUSTER SLOTS#13966

Merged
sundb merged 1 commit intoredis:unstablefrom
vitahlin:fix-cluster-tls-port
Apr 24, 2025
Merged

Fix tls port update not reflected in CLUSTER SLOTS#13966
sundb merged 1 commit intoredis:unstablefrom
vitahlin:fix-cluster-tls-port

Conversation

@vitahlin
Copy link
Contributor

@vitahlin vitahlin commented Apr 22, 2025

Problem

A previous PR (#13932) fixed the TCP port issue in CLUSTER SLOTS, but it seems the handling of the TLS port was overlooked.

There is this comment in the addNodeToNodeReply function in the cluster.c file:

    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);

Fixed

This PR fixes the TLS port issue and adds relevant tests.

@sundb sundb requested review from ShooterIT and StavRLevi April 22, 2025 13:59
@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Apr 22, 2025
@vitahlin
Copy link
Contributor Author

Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

thanks, LGTM

@sundb sundb merged commit 9f99dd5 into redis:unstable Apr 24, 2025
18 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Redis 8.2 Apr 24, 2025
@vitahlin vitahlin deleted the fix-cluster-tls-port branch April 24, 2025 02:11
@YaacovHazan YaacovHazan moved this from Todo to Pending in Redis 7.4 Backport Apr 29, 2025
@YaacovHazan YaacovHazan moved this from Todo to pending in Redis 7.2 Backport Apr 29, 2025
@YaacovHazan YaacovHazan moved this from Pending to Todo in Redis 7.4 Backport May 25, 2025
@YaacovHazan YaacovHazan moved this from pending to Todo in Redis 7.2 Backport May 25, 2025
@YaacovHazan YaacovHazan moved this from To Do to In progress in Redis 6.2 Backport May 26, 2025
@YaacovHazan YaacovHazan moved this from In progress to To Do in Redis 6.2 Backport May 26, 2025
@YaacovHazan YaacovHazan moved this from To Do to Pending in Redis 6.2 Backport May 26, 2025
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request May 26, 2025
### Problem 

A previous PR (redis#13932) fixed the TCP
port issue in CLUSTER SLOTS, but it seems the handling of the TLS port
was overlooked.

There is this comment in the `addNodeToNodeReply` function in the
`cluster.c` file:
```c
    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);
```

### Fixed 

This PR fixes the TLS port issue and adds relevant tests.
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request May 26, 2025
### Problem 

A previous PR (redis#13932) fixed the TCP
port issue in CLUSTER SLOTS, but it seems the handling of the TLS port
was overlooked.

There is this comment in the `addNodeToNodeReply` function in the
`cluster.c` file:
```c
    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);
```

### Fixed 

This PR fixes the TLS port issue and adds relevant tests.
YaacovHazan pushed a commit that referenced this pull request May 27, 2025
### Problem 

A previous PR (#13932) fixed the TCP
port issue in CLUSTER SLOTS, but it seems the handling of the TLS port
was overlooked.

There is this comment in the `addNodeToNodeReply` function in the
`cluster.c` file:
```c
    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);
```

### Fixed 

This PR fixes the TLS port issue and adds relevant tests.
YaacovHazan pushed a commit that referenced this pull request May 27, 2025
### Problem 

A previous PR (#13932) fixed the TCP
port issue in CLUSTER SLOTS, but it seems the handling of the TLS port
was overlooked.

There is this comment in the `addNodeToNodeReply` function in the
`cluster.c` file:
```c
    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);
```

### Fixed 

This PR fixes the TLS port issue and adds relevant tests.
@YaacovHazan YaacovHazan moved this from Todo to Done in Redis 7.2 Backport Jun 29, 2025
@YaacovHazan YaacovHazan moved this from Todo to Done in Redis 7.4 Backport Jun 29, 2025
@sundb sundb removed this from Redis 8.2 Aug 15, 2025
@sundb sundb added this to Redis 8.0 Aug 15, 2025
@sundb sundb moved this from Todo to Done in Redis 8.0 Aug 15, 2025
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
### Problem 

A previous PR (redis#13932) fixed the TCP
port issue in CLUSTER SLOTS, but it seems the handling of the TLS port
was overlooked.

There is this comment in the `addNodeToNodeReply` function in the
`cluster.c` file:
```c
    /* Report TLS ports to TLS client, and report non-TLS port to non-TLS client. */
    addReplyLongLong(c, clusterNodeClientPort(node, shouldReturnTlsInfo()));
    addReplyBulkCBuffer(c, clusterNodeGetName(node), CLUSTER_NAMELEN);
```

### Fixed 

This PR fixes the TLS port issue and adds relevant tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: Pending
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants