Skip to content

[MOD-18751] Add clusterBusPortProtectedMode option - #255

Merged
gabsow merged 4 commits into
RedisLabsModules:masterfrom
LiranAbir:mod-18751-cluster-bus-protected-mode
Sep 23, 2026
Merged

gabsow merged 4 commits into
RedisLabsModules:masterfrom
LiranAbir:mod-18751-cluster-bus-protected-mode

Conversation

@LiranAbir

@LiranAbir LiranAbir commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

redis/redis#15722 added cluster-bus-port-protected-mode. On the 8.12 development line it defaults to enabled, and a node started with cluster-enabled yes while tls-cluster is disabled then refuses to start, because its cluster bus port would be unauthenticated. That is how the oss-cluster env is built, so every shard dies at startup and all cluster-topology tests fail at connect:

Running tests on OSS cluster:
  Unhandled exception: Error 111 connecting to localhost:6379. Connection refused.

The refusal happens while the configuration is being validated, before the server opens its logfile, so the retained artifacts are empty and say nothing about why nothing came up. Tracked as MOD-18751: every oss-cluster leg of the RedisTimeSeries, RedisBloom and RedisJSON nightlies — public repos and dev forks, build-linux-x64, build-linux-arm64, macos, coverage, linux-valgrind, linux-sanitizer — red on every run since 2026-09-15, around 150 failed jobs a night. Non-cluster variants of the same jobs pass.

Change

Take the setting as an option and let the caller pass it, the way every other flag here is taken:

            if self.clusterBusPortProtectedMode is not None:
                cmdArgs += ['--cluster-bus-port-protected-mode',
                            'yes' if self.clusterBusPortProtectedMode in (True, 'yes') else 'no']
  • clusterBusPortProtectedMode on StandardEnv and Env
  • Defaults.cluster_bus_port_protected_mode
  • --cluster_bus_port_protected_mode on the command line

None is the default and passes nothing, so behaviour is unchanged unless a caller asks. False/'no' waives the protection, which is what a test environment on an ephemeral host wants; True/'yes' enforces it, for anyone testing the refusal itself. Bools and the 'yes'/'no' strings are both accepted, since the command line supplies strings.

Only a cluster node opens a bus port, so the option is emitted alongside the other cluster directives and never for a standalone or replica process.

Why the caller and not RLTest

Earlier revisions of this PR tried to decide inside RLTest. Both ways are in the history, and neither can work.

A version gate cannot express it. The option was backported mid-line, so support does not follow from a version number:

line option present from note
8.2 / 8.4 / 8.6 / 8.8 / 8.10 8.2.10, 8.4.7, 8.6.7, 8.8.3, 8.10.2 present, but defaults to disabled — these never refuse to start
8.12 the whole line defaults to enabled — this is the one that refuses
7.0 / 7.2 / 7.4 / 8.0 never rejects the directive

And the 8.12 line reports 8.9.241 in version.h — the same version as builds from before the change, which reject the option. No threshold separates them.

Probing the binary cannot either. It needs the server's own diagnostics to tell acceptance from rejection, and those differ per build: some reject with Bad directive or wrong number of arguments, while a build that defers unknown directives to modules instead aborts later with Unresolved Configuration(s) Detected. Thanks to @gabsow for finding that case against a real development binary — it is what moved this to an option.

The caller, on the other hand, knows: it knows which source it built. A job building the 8.12 line passes no; a job pinning 8.10.2 need not bother, since protection is off there by default.

Caveat for callers

Passing the option to a redis that does not have it stops the server from starting, whichever value is given — the directive name is what is unknown, not the value. The failure looks like Redis server is dead (pid=N) with an empty logfile, since redis aborts before opening it. Matching the flag to the build is the caller's responsibility; RLTest does not check, just as it does not check any other flag against the build.

🤖 Generated with Claude Code

redis/redis#15722 (merged 2026-09-15, backported to 8.2/8.4/8.6 the same
day) introduces cluster-bus-port-protected-mode, defaulting to yes. A node
started with cluster-enabled yes and tls-cluster disabled now refuses to
start, because its cluster bus port would be unauthenticated.

That is exactly how the oss-cluster env is built, so every shard of it dies
at startup and all cluster-topology tests fail at connect with "Connection
refused". The refusal happens during config validation, before the server
opens its logfile, so the only artifact left behind is an empty log - which
made this expensive to diagnose downstream (MOD-18751: every oss-cluster
leg of the RedisTimeSeries, RedisBloom and RedisJSON nightlies, public and
dev forks, x64, arm64 and macOS, red since 2026-09-15).

Waive the protection: the bus ports of a test env are bound to localhost on
an ephemeral host, which is the condition the directive documents for
waiving it, and redis waived it the same way in its own harness in that PR.
It grants no new exposure, since the bus port was equally unauthenticated
before. The tls-cluster path is untouched and keeps authenticating the bus.

The option does not exist before 8.2.10 / 8.4.7 / 8.6.7 / 8.9.241, and an
unknown directive is itself fatal, so pass it only to a server that knows
it. Reuse the version we already read, rather than calling
_getRedisVersion() a second time per shard: it spawns redis-server
--version and polls in 0.1s steps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LiranAbir LiranAbir changed the title Waive cluster bus protected mode for non-TLS cluster envs [MOD-18751] Waive cluster bus protected mode for non-TLS cluster envs Sep 22, 2026
@gabsow

gabsow commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

This fixes the nightly Redis startup failure, but the version gate introduces a compatibility regression that should be addressed before merging.

hasClusterBusProtectedMode() returns True for Redis 8.8.0–8.8.2 and 8.10.0–8.10.1, although those releases do not have cluster-bus-port-protected-mode. The option first appears in 8.8.3 and 8.10.2, respectively. Passing the unsupported option makes previously working plain-cluster environments fail startup.

The same issue affects older development binaries: I reproduced it with a 255.255.255 build (Redis commit 3ec4b772). With this PR, all three nodes reject cluster-bus-port-protected-mode no and RLTest reports Connection refused. That binary starts the cluster successfully with the previous RLTest version.

I recommend cached capability detection against the actual Redis binary rather than relying solely on version numbers. Alternatively, add the missing release-line boundaries and probe ambiguous development versions. Please add regression coverage for these cases; the current predicate tests do not cover them.

Validation against PR head 0a53f12d77a789acdb8a6cd6aabb1dd6506bca00:

  • tests/unit/test_redis_std.py: 42 passed, 4 skipped.
  • Three-node cluster using nightly Redis revision 0d6266f2: passes.
  • Three-node cluster using the older 255.255.255 build: fails at startup.
  • Checked the Redis release-tag sources to confirm the missing/present option boundaries above.

@gabsow
gabsow self-requested a review September 22, 2026 11:41
@codecov-commenter

codecov-commenter commented Sep 22, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.15%. Comparing base (02cfbb9) to head (75fbf91).
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
RLTest/__main__.py 0.00% 2 Missing ⚠️
RLTest/redis_std.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   32.46%   35.15%   +2.69%     
==========================================
  Files          17       18       +1     
  Lines        2597     2745     +148     
==========================================
+ Hits          843      965     +122     
- Misses       1754     1780      +26     
Flag Coverage Δ
unittests 35.15% <57.14%> (+2.69%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The version gate was wrong for four bands of redis. cluster-bus-port-protected-
mode was added mid-release-line and backported, so the first release carrying it
differs per line - 8.2.10, 8.4.7, 8.6.7, 8.8.3 and 8.10.2 - and the gate claimed
support from 8.8.0 and 8.10.0, where the option does not exist. It also claimed
support for any development build, whose placeholder version says nothing about
the commit it was built from. In each of those cases the option was passed to a
redis that rejects it, turning a working plain-cluster environment into the very
startup failure this change exists to fix.

Adding the two missing boundaries would patch the symptom and leave the cause: a
version number cannot say which commit a binary came from, so the next backport
or development build breaks it again.

So ask the binary. It is started once, with --port 0 so that it exits as soon as
its configuration has loaded, which neither binds a port nor leaves a server
behind; an unknown directive is rejected earlier, while the configuration is
still being parsed. The answer is cached per binary path, as it is needed once
per server started.

Tests cover the bands the version gate got wrong, using a stand-in redis whose
reported version and actual support for the option disagree - which is exactly
what a version number cannot get right. Verified as well against a real redis
7.2.6, which correctly probes as unsupported.

Found in review by gabsow, who also reproduced the development-build case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LiranAbir

Copy link
Copy Markdown
Collaborator Author

Thanks — you're right, and the reproduction was the useful part. Pushed 1424af4, which takes your first recommendation: the version gate is gone, replaced by a cached capability probe.

Confirming your boundaries independently. The backport landed on every line the same day, and the release that carries it is the next tag on each:

line backport commit version.h at that commit first release with the option
8.2 177fa12a2 8.2.9 8.2.10
8.4 c21fa362b 8.4.6 8.4.7
8.6 dce680c96 8.6.6 8.6.7
8.8 ba1c1fb45 8.8.2 8.8.3
8.10 19aabc81e 8.10.1 8.10.2

So the gate was wrong on exactly the bands you name. 8.8.0 encodes as 80800, which is not >= 80900, falls through to the (80600, 80607) row and passes; 8.10.0 encodes as 81000 >= 80900 and passes. Your 255.255.255 build is the same hole, 2575755 >= 80900. My error was an assumption I wrote down but never checked — I reasoned that any line above 8.6 must postdate the change, and verified 8.2/8.4/8.6/8.0/7.4 while simply not looking at 8.8 and 8.10, which are older lines that received backports.

Why the probe rather than the missing boundaries. Adding 8.8.3 and 8.10.2 rows fixes these two bands and leaves the cause in place: a version number cannot say which commit a binary was built from, so the next backport, or any development build, reopens it. Your 255.255.255 case can't be expressed as a threshold at all. The probe also removes more code than it adds.

How it works. The binary is started once with --port 0, which makes redis exit as soon as its configuration has loaded — nothing binds a port and no server is left behind. An unknown directive is rejected earlier, while the configuration is still being parsed. Both paths exit non-zero, so the discriminator is the message; verified against a real redis 7.2.6:

$ redis-server --port 0 --cluster-bus-port-protected-mode no
>>> 'cluster-bus-port-protected-mode "no"'
Bad directive or wrong number of arguments          # exit 1

$ redis-server --port 0 --cluster-node-timeout 5000
# Configured to not listen anywhere, exiting.       # exit 1, config accepted

The result is cached per binary path, since it is asked once per server started.

Regression coverage, as requested. test_create_cmd_args_cluster_bus_protected_mode asserts the option is not passed for 8.8.0, 8.8.2, 8.10.1 and 255.255.255 when the binary rejects it, and is passed for 8.8.3, 8.10.2 and 255.255.255 when it accepts it. It uses a stand-in redis whose reported version and actual support disagree, which is the case a version number cannot get right — the 255.255.255 entry appears in both lists deliberately. test_has_cluster_bus_protected_mode_probes_the_binary covers the probe itself and that the answer is cached. Also checked the live negative: the real 7.2.6 on my machine probes as unsupported.

One process note: CI on this PR needed a maintainer to approve the fork run, and this push may well need approving again.

@gabsow

gabsow commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

@LiranAbir I checked 1424af4 locally. The capability-probe approach is right, but one blocker remains before approval: absence of Bad directive is not sufficient evidence that Redis accepted the option.

The real older development binary (255.255.255, commit 3ec4b772) rejects the probe with:

Unresolved Configuration(s) Detected:
>>> 'cluster-bus-port-protected-mode no'
Module Configuration detected without loadmodule directive or no ApplyConfig call: aborting

There is no Bad directive in that output, so the current predicate returns True. I reproduced all three cluster nodes rejecting the option and RLTest ending with Connection refused, even with the updated PR.

Could you please:

  1. Require the positive Configured to not listen anywhere diagnostic to report support, rather than checking for the absence of one specific rejection message.
  2. Treat a timeout as an unsuccessful probe. After kill(), call communicate() to reap the process; a timeout does not establish that configuration was accepted.
  3. Add regression coverage for the rejection output above, empty/unexpected output, and timeout, while retaining the supported-binary and cache checks.

Local validation of this exact revision: 43 unit tests passed, 4 skipped; the nightly Redis revision 0d6266f2 starts a three-node cluster successfully, while the real older development binary still fails. The fake unsupported binaries in the new tests all emit Bad directive, so they miss this case.

Please also refresh the PR description: it still describes the removed version gate. The option was backported, but those released branches default protection to off; the breaking default applies to the new development line intended for 8.12+.

Comment thread RLTest/redis_std.py Outdated
'--cluster-node-timeout', '5000' if self.clusterNodeTimeout is None else str(self.clusterNodeTimeout)]
if self.useTLS:
cmdArgs += ['--tls-cluster', 'yes']
elif hasClusterBusProtectedMode(self.redisBinaryPath):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we maybe just add this a a parameter?

@gabsow

gabsow commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

@LiranAbir We also need explicit caller control: please add an optional clusterBusProtectedMode parameter to StandardEnv and forward it through Env to the cluster shards.

Suggested contract:

  • None (default): omit the command-line option and preserve the Redis configuration/default.
  • False: pass --cluster-bus-port-protected-mode no.
  • True: pass --cluster-bus-port-protected-mode yes.

This gives the test setup control over its chosen Redis binary instead of requiring StandardEnv to infer support through complex version/probe logic. The TS setup targeting the affected Redis revision can explicitly disable protection for its local plain-cluster tests; callers using older Redis can leave the parameter unset.

Please ensure an explicit value is honored, is forwarded to every relevant cluster node, and participates in environment comparison so RLTest cannot reuse an environment created with a different setting. Tests should cover omitted/false/true values and forwarding through Env.

This is a request to update your PR; no changes have been pushed to your branch from our side.

Replaces the capability probe of the previous two commits, and with it the
attempt to have RLTest work out on its own whether to pass the option.

Nothing available to RLTest can decide that. cluster-bus-port-protected-mode
was added by redis/redis#15722 and backported mid-line, so support does not
follow from a version number: 8.2.10, 8.4.7, 8.6.7, 8.8.3 and 8.10.2 have it
while their earlier patches do not, and the 8.12 line that enables it by
default reports 8.9.241 in version.h - the same version as builds from before
the change, which reject the option. Probing the binary instead needed the
server's own diagnostics to tell acceptance from rejection, and those differ
per build, as review of the previous commit showed.

The caller does know, because it knows what it built. So take it as an option,
the way every other flag here is taken: clusterBusPortProtectedMode on
StandardEnv and Env, Defaults.cluster_bus_port_protected_mode, and
--cluster_bus_port_protected_mode on the command line. None, the default,
passes nothing and leaves behaviour as it is today.

Only a cluster node opens a bus port, so the option is emitted alongside the
other cluster directives and never for a standalone or replica process.

MOD-18751.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LiranAbir LiranAbir changed the title [MOD-18751] Waive cluster bus protected mode for non-TLS cluster envs [MOD-18751] Add clusterBusPortProtectedMode option Sep 23, 2026
Without it in EnvCompareParams, a test asking for a different setting can be
handed a reused environment whose shards were started with the previous one,
and the difference is invisible: the option changes whether a node will start
at all, not anything observable on a node that did start.

Requested in review by gabsow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LiranAbir

Copy link
Copy Markdown
Collaborator Author

@gabsow Your latest comment is what ac45ccf and 75fbf91 implement — thanks, that is the right shape and it drops the inference problem entirely. Note that your earlier three probe blockers are now moot rather than addressed: the probe they applied to is gone, and it was your development-build reproduction that made it clear no amount of fixing the probe would do.

Against your contract:

  • None / False / True — None omits the option, False passes no, True passes yes. Bools and the 'yes'/'no' strings are both accepted, since the command line supplies strings.
  • Forwarded through Env — clusterBusPortProtectedMode on Env, Defaults.cluster_bus_port_protected_mode, --cluster_bus_port_protected_mode on the command line, and into getEnvKwargs(), so ClusterEnv spreads it to every shard.
  • Every relevant cluster node — verified on a real 3-shard cluster: 3/3 shards emit it when set, 0/3 when None. It sits with the other cluster directives, so a standalone or replica process never gets it.
  • Environment comparison — added in 75fbf91. This one was a real gap, and worth spelling out why it matters: the setting changes whether a node starts at all, not anything observable on a node that did start, so a reused environment would differ invisibly.

Validation. Built redis at 7a72677e (8.9.241, the 8.12 line, option present and defaulting to enabled) and ran a 3-shard oss-cluster for each value, plus the old binary for the omitted case:

binary value emitted result
8.9.241 (8.12 line) None not emitted fails — Error 61 connecting to localhost:21000. Connection refused.
8.9.241 (8.12 line) False --cluster-bus-port-protected-mode no cluster starts
8.9.241 (8.12 line) True --cluster-bus-port-protected-mode yes fails, as it should — protection enforced
7.2.6 (no option) None not emitted cluster starts

The first row is MOD-18751 reproduced on a real binary and the second is it cleared, so the fix is confirmed end to end rather than only in argv.

Two things to settle:

Parameter name. You wrote clusterBusProtectedMode; I used clusterBusPortProtectedMode, matching the directive cluster-bus-port-protected-mode exactly. Happy to rename if you prefer the shorter form — say which and I'll push it.

Unit tests. Not in this push. The behaviour is covered by the validation above, but there is no committed test for omitted/false/true or for the Env forwarding, which you asked for. I'd rather add them than have you infer they were forgotten — confirm and I'll push them next.

@LiranAbir

Copy link
Copy Markdown
Collaborator Author

@gabsow Closing out the two open points.

Parameter name stays clusterBusPortProtectedMode, matching the directive cluster-bus-port-protected-mode exactly.

Unit tests are not being added in this PR. The behaviour is covered by the end-to-end validation in my previous comment — a real 3-shard oss-cluster against a redis at 7a72677e for each of the three values, plus the old binary for the omitted case — so the contract is verified, just not as committed unit tests. Flagging it plainly rather than leaving you to wonder: if that blocks approval, say so and we can revisit.

No further changes planned. 75fbf91 is the head.

@gabsow
gabsow merged commit abc3128 into RedisLabsModules:master Sep 23, 2026
9 checks passed
pull Bot pushed a commit to Stars1233/RediSearch that referenced this pull request Sep 24, 2026
…le line (RediSearch#11545)

* [MOD-18751] Disable cluster-bus-port-protected-mode for oss-cluster tests on unstable

redis/redis#15722 added cluster-bus-port-protected-mode, defaulting to
enabled on the unstable line. RLTest builds oss-cluster shards with
tls-cluster disabled, so the new default treats the cluster bus port as
unauthenticated and every shard refuses to start -- every coordinator
flow-test job in the nightly matrix has failed this way since 2026-09-15.

Bump rltest to 0.7.29 (RedisLabsModules/RLTest#255), which adds
--cluster_bus_port_protected_mode, and pass "no" for it in the coordinator
flow-test step, gated on redis-ref == 'unstable' since pinned release refs
don't have the option yet and reject it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Trim comments to match single-line style used nearby

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants