HCD-238 Harden cross cluster node joins checks by bereng · Pull Request #2289 · datastax/cassandra

bereng · 2026-03-30T05:52:53Z

This ticket ports DSP-24965 adding cluster membership checks for nodes in some extra code paths to prevent foreign nodes from joining.

What is the issue

In some rare scenarios, some with bad config, some with the wrong data folders plugged into the wrong node, operator mistake, etc nodes from cluster A can join nodes from cluster B. That breaks topology badly both for A and B.

What does this PR fix and why was it fixed

This PR extends the original cluster membership check to all other susceptible code paths to prevent these.

github-actions · 2026-03-30T05:53:06Z

This ticket ports DSP-24965 adding cluster membership checks for nodes in some extra code paths to prevent foreing nodes from joining.

sonarqubecloud · 2026-03-30T12:55:17Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot · 2026-03-30T12:59:42Z

❌ Build ds-cassandra-pr-gate/PR-2289 rejected by Butler

3 regressions found
See build details here

Found 3 new test failures

Test	Explanation	Runs	Upstream
junit.framework.TestSuite.org.apache.cassandra.gms.EndpointStateTest-compression.jdk11	REGRESSION	🔵🔴	0 / 30
o.a.c.index.sai.cql.VectorCompaction100dTest.testZeroOrOneToManyCompaction[version=ec enableNVQ=false]	REGRESSION	🔴🔴	0 / 30
o.a.c.index.sai.cql.VectorSiftSmallTest.testMultiSegmentBuild[version=fa enableNVQ=false enableFused=false]	REGRESSION	🔴⚪	0 / 30

Found 8 known test failures

bereng · 2026-03-31T09:28:33Z

CI failures lgtm:

They pass locally or are flaky
The only legit one was EndpointStateTest now fixed
Vector family tests also seems to fail on nighties
CNDB PR

djatnieks

Suggest maybe to also have the DSP ticket reviewer also review this

JeremiahDJordan

This patch was rejected from Apache Cassandra. Do we really want to add new gossip states that are not going to end up in the upstream? @driftx thoughts? ideas for how we can protect against this without adding new gossip states?

driftx · 2026-03-31T20:15:04Z

I believe the community rejected this approach not because of adding new states, but because of sticking them in a new json field and adding json processing to gossip. I think it would be better received if we were just adding CLUSTER_NAME and PARTITIONER_NAME as new states in the existing framework.

bereng · 2026-04-01T05:56:08Z

Thx for the comments.

As for the DSP ticket reviewer he is on parental leave atm.

On the JSON state this mirrors what we've had in DSE for years. We also preferred not burning 2 states. On top of that adding states messes up ApplicationStates alignment during upgrade tests needing careful attention. You don't want to be having these problems again in the CC world so the DSE solution seems the best future proof approach imho.

On OSS it was rejected, iiuc, as it was seen as a non problem and node to node auth should be used instead.

Yes we would like to add this to CC as this is a DSE fix requested by Apple now being ported to HCD (CC).

I'm happy to go either way: JSON or 2 extra states. I just need a decision to what the preferred solution is.

JeremiahDJordan · 2026-04-01T14:14:53Z

@bereng I do not think we should add new JSON stuff to CC. You mention the DSE JSON payload. I would be less against adding the same state that DSE has. That would just be giving us DSE compatibility, and OSS already marked that DSE one as a "dead" state. So it won't cause compatibility issues.

bereng · 2026-04-02T05:30:54Z

Oh that is an excellent idea tbh. I like it a lot. It is ordinal 10 so would now match X_11_PADDING. I'd have to check the code yet but sounds great Thx.

michaelsembwever · 2026-04-11T15:03:29Z

-        {
-            InetAddressAndPort from = message.from();
            logger.trace("Received a GossipDigestAck2Message from {}", from);
-        }


nit: this change can be avoided. it's better to call message.from() again on line 49, then touch more lines of code. this is for the sake of rebases. new lines are easy, changes lines like this^ quickly are painful.

michaelsembwever · 2026-04-11T15:05:21Z

+
+        InetAddressAndPort nodeAddress = FBUtilities.getBroadcastAddressAndPort();
+        // Send cluster and partitioner names for cluster foreign node checks
+        if (deltaEpStateMap.get(nodeAddress) != null &&
+            !deltaEpStateMap.get(nodeAddress).containsApplicationState(ApplicationState.JSON_PAYLOAD))
+        {
+            deltaEpStateMap.get(nodeAddress).addApplicationState(ApplicationState.JSON_PAYLOAD,
+                                                                 ApplicationState.serializeJsonPayload(ApplicationState.initialJsonPayload));
+
+        }
+        deltaEpStateMap = removeForeignClusterNodes(deltaEpStateMap);
+


nit: can we make this a method ? removing the duplicated code in createShadowReply(), and making rebases that little bit easier.

michaelsembwever · 2026-04-11T15:05:54Z

        return endpointStateMap.size();
    }

+    public Map<InetAddressAndPort, EndpointState> getEndpointStateMapUnsafeForTest()


Suggested change

public Map<InetAddressAndPort, EndpointState> getEndpointStateMapUnsafeForTest()

@VisibleForTesting

public Map<InetAddressAndPort, EndpointState> getEndpointStateMapUnsafeForTest()

?

michaelsembwever · 2026-04-11T15:22:11Z

+    public static <T> Message<T> synthetic(InetAddressAndPort from, Verb verb, T payload)
+    {
+        return new Message<>(new Header(-1, verb, from, -1, -1, 0, NO_PARAMS), payload);
+    }


what is this ?

michaelsembwever · 2026-04-11T15:26:13Z

+            if (jsonMap != null)
+            {
+                String auxValue = (String) jsonMap.get(ApplicationState.JsonPayload.CLUSTER_NAME.name());
+                if (auxValue != null && !auxValue.equals(DatabaseDescriptor.getClusterName()))


nit: can be simplified

Suggested change

if (auxValue != null && !auxValue.equals(DatabaseDescriptor.getClusterName()))

if (DatabaseDescriptor.getClusterName().equals(auxValue)))

can be applied to line 2644 too

michaelsembwever · 2026-04-11T15:28:44Z

    private void markAlive(final InetAddressAndPort addr, final EndpointState localState)
    {
+        if (!maybeBelongsInCluster(addr, localState))
+            logger.error("Not sending ECHO to {} which doesn't belong in this cluster", addr);


(question)
i see that the gossiper just ignores an unknown node (over different verbs). how does the unknown node itself respond and behave to this ? if the operator is primarily observing that node will they see enough to know what is now happening here ?

michaelsembwever · 2026-04-11T15:29:39Z

+            if (!maybeBelongsInCluster(entry.getKey(), entry.getValue()))
+            {
+                logger.error("Ignoring Gossip from node {} because its state has info from other clusters {}", entry.getKey(), entry.getValue());
+                it.remove();


is this (thread) safe ?

michaelsembwever · 2026-04-11T15:30:29Z

        // for each test case, the MIGRATION_DELAY time is adjusted accordingly
        savedMigrationDelay = CassandraRelevantProperties.MIGRATION_DELAY.getLong();
        CassandraRelevantProperties.MIGRATION_DELAY.setLong(ManagementFactory.getRuntimeMXBean().getUptime() + savedMigrationDelay);
+        DatabaseDescriptor.clientInitialization();


heh ? what's with all the tests getting this added (but otherwise not changing in any way) ?

michaelsembwever · 2026-04-11T15:38:59Z

Do we really want to add new gossip states that are not going to end up in the upstream?

I share this question, without really having a solid opinion.

I'm uneasy about all gossip/verb changes that makes CC communicate differently to C*, and there's a more solid proper solution to this: implementing internode encryption using per-node certs+ and host verification. I don't know if that's appropriate for the customer, or if this is a guardrail that adds enough value to overcome the concerns…

What's our future compatibility plan on this ? Are we expecting CC5 to also provide it ? Are we expecting mix-version clusters to provide it ? What happens if such a strange node is added to a cluster in the middle of a C* to CC upgrade ? This might seem esoteric, but nice to have brief thoughts/answers written down to it…

HCD-238 Harden cross cluster node joins checks

309f88d

This ticket ports DSP-24965 adding cluster membership checks for nodes in some extra code paths to prevent foreing nodes from joining.

bereng force-pushed the HCD-238-main branch from 4b08a2b to 309f88d Compare March 30, 2026 06:03

fixes

353475e

djatnieks approved these changes Mar 31, 2026

View reviewed changes

JeremiahDJordan requested changes Mar 31, 2026

View reviewed changes

driftx self-requested a review March 31, 2026 21:16

michaelsembwever reviewed Apr 11, 2026

View reviewed changes

	public Map<InetAddressAndPort, EndpointState> getEndpointStateMapUnsafeForTest()
	@VisibleForTesting
	public Map<InetAddressAndPort, EndpointState> getEndpointStateMapUnsafeForTest()

	if (auxValue != null && !auxValue.equals(DatabaseDescriptor.getClusterName()))
	if (DatabaseDescriptor.getClusterName().equals(auxValue)))

Conversation

bereng commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the issue

What does this PR fix and why was it fixed

Uh oh!

github-actions bot commented Mar 30, 2026 • edited by bereng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist before you submit for review

Uh oh!

sonarqubecloud bot commented Mar 30, 2026

Quality Gate passed

Uh oh!

cassci-bot commented Mar 30, 2026

❌ Build ds-cassandra-pr-gate/PR-2289 rejected by Butler

Found 3 new test failures

Found 8 known test failures

Uh oh!

bereng commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djatnieks left a comment

Choose a reason for hiding this comment

Uh oh!

JeremiahDJordan left a comment

Choose a reason for hiding this comment

Uh oh!

driftx commented Mar 31, 2026

Uh oh!

bereng commented Apr 1, 2026

Uh oh!

JeremiahDJordan commented Apr 1, 2026

Uh oh!

bereng commented Apr 2, 2026

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

michaelsembwever Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelsembwever commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bereng commented Mar 30, 2026 •

edited

Loading

github-actions bot commented Mar 30, 2026 •

edited by bereng

Loading

bereng commented Mar 31, 2026 •

edited

Loading

michaelsembwever Apr 11, 2026 •

edited

Loading