You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "nivykani (via GitHub)" <gi...@apache.org> on 2026/09/18 05:41:25 UTC
[PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
nivykani opened a new pull request, #5182:
URL: https://github.com/apache/cassandra/pull/5182
This patch adds CRC validation on the receiving side, after an entire sstable is received via zero-copy streaming. This detects SSTables that were already corrupt on the sender's disk and prevents them from being accepted, which stops the corruption from spreading.
Since it's possible for the digest component not to be streamed, this check is disabled by default in the config via `entire_sstable_stream_digest_validation_enabled`, and fails open (if no digest is present, the streaming succeeds).
Validation costs one sequential read of the data file per received SSTable, and any mismatch makes the streaming session fail and increments a metric for `entireSSTableDigestMismatches`.
The [Cassandra Jira](https://issues.apache.org/jira/browse/CASSANDRA-21694)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4063688081
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -811,6 +811,11 @@ public static class SSTableConfig
public volatile int max_concurrent_automatic_sstable_upgrades = 1;
public boolean stream_entire_sstables = true;
+ /**
+ * When receiving a zero-copy stream, validate Digest.crc32 before the sstable is made visible.
+ */
+ public volatile boolean entire_sstable_stream_digest_validation_enabled = false;
Review Comment:
That makes sense to me; I've updated the default to be true for 6.0 and 7.0, and added the NEWS.txt entry for 6.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#issuecomment-5821784745
Good point! I've added a distributed test (I put it in `HostReplacementTest` but can move it into streaming/ if we want a more generic scenario). The setup is:
1. Start a 2 node cluster with compactions disabled on node1
2. Write 2 sstables to node1. Keep one healthy, and corrupt the other one.
3. Stop and replace node2 with node3.
4. Check that when the config is true, node3 receives neither sstables from streaming, specifically because of the digest mismatch.
5. Meanwhile when the config is false, node3 receives both sstables from streaming. I didn't add this test case because I'm not sure if we'd want to assert on a bug, but it passes locally.
(Originally I tried directly checking that the corrupt sstable isn't on node3, but it's hard to know which bytes are safe to force-corrupt and not file metadata. So we can check the existence of the healthy file instead, because streaming should be all-or-nothing.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4051265327
##########
src/java/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamReader.java:
##########
@@ -157,6 +164,38 @@ public SSTableMultiWriter read(DataInputPlus in) throws IOException
}
}
+ /**
+ * Validates the received {@code Data.db} against {@code Digest.crc32}.
+ * @throws IOException if a digest was streamed and the received data file does not match it
+ */
+ private void validateDataDigest(Descriptor descriptor, ComponentManifest manifest) throws IOException
+ {
+ if (!DatabaseDescriptor.getEntireSSTableStreamDigestValidationEnabled())
+ return;
+
+ List<Component> components = manifest.components();
+ if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))
+ {
+ NoSpamLogger.log(logger, NoSpamLogger.Level.WARN, 1, TimeUnit.MINUTES,
+ "Accepting zero-copy sstable stream from {} without validating CRC due to " +
+ "missing {} or {} from streamed components: {}",
+ session.peer, Components.DATA, Components.DIGEST, components);
+ return;
+ }
+
+ try
+ {
+ new DataIntegrityMetadata.FileDigestValidator(descriptor.fileFor(Components.DATA),
+ descriptor.fileFor(Components.DIGEST)).validate();
Review Comment:
`validate()` throws a `NumberFormatException` if the digest is malformed. Do we want that to count against `countEntireSSTableDigestMismatch()` or just flow through?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4050460217
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -811,6 +811,11 @@ public static class SSTableConfig
public volatile int max_concurrent_automatic_sstable_upgrades = 1;
public boolean stream_entire_sstables = true;
+ /**
+ * When receiving a zero-copy stream, validate Digest.crc32 before the sstable is made visible.
+ */
+ public volatile boolean entire_sstable_stream_digest_validation_enabled = false;
Review Comment:
In 4.0, 4.1, and 5.0, I could see leaving this `false` by default to preserve behavior on minor upgrade. In 6.0+ I think we want it to be `true` by default, with an accompanying note in the `NEWS.txt` "Upgrading" section for 6.0.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4126917938
##########
src/java/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamReader.java:
##########
@@ -157,6 +164,38 @@ public SSTableMultiWriter read(DataInputPlus in) throws IOException
}
}
+ /**
+ * Validates the received {@code Data.db} against {@code Digest.crc32}.
+ * @throws IOException if a digest was streamed and the received data file does not match it
+ */
+ private void validateDataDigest(Descriptor descriptor, ComponentManifest manifest) throws IOException
+ {
+ if (!DatabaseDescriptor.getEntireSSTableStreamDigestValidationEnabled())
+ return;
+
+ List<Component> components = manifest.components();
+ if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))
Review Comment:
I think the list of components is small enough that the optimization isn't worth adding a new method, especially if there's only one caller as you said. It's true we could iterate through it once instead of twice, but the readability seems worth it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "gandherva (via GitHub)" <gi...@apache.org>.
gandherva commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4129315281
##########
src/java/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamReader.java:
##########
@@ -157,6 +164,38 @@ public SSTableMultiWriter read(DataInputPlus in) throws IOException
}
}
+ /**
+ * Validates the received {@code Data.db} against {@code Digest.crc32}.
+ * @throws IOException if a digest was streamed and the received data file does not match it
+ */
+ private void validateDataDigest(Descriptor descriptor, ComponentManifest manifest) throws IOException
+ {
+ if (!DatabaseDescriptor.getEntireSSTableStreamDigestValidationEnabled())
+ return;
+
+ List<Component> components = manifest.components();
+ if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))
Review Comment:
Sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani closed pull request #5182: CASSANDRA-21694: Validate CRC before zero-copy streaming sstables
URL: https://github.com/apache/cassandra/pull/5182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4126822660
##########
test/unit/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamWriterTest.java:
##########
@@ -173,6 +190,124 @@ public void testBlockReadingAndWritingOverWire() throws Throwable
}
}
+ @Test
+ public void testCorruptDataComponentFailsDigestValidation() throws Throwable
+ {
+ ByteBuf serializedFile = Unpooled.buffer(8192);
+ ComponentManifest manifest = writeEntireSSTable(serializedFile);
+ corruptDataComponent(serializedFile, manifest);
+
+ DatabaseDescriptor.setEntireSSTableStreamDigestValidationEnabled(true);
+ assertThatThrownBy(() -> readEntireSSTable(serializedFile, manifest))
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("Digest mismatch")
+ .hasCauseInstanceOf(IOException.class);
+ }
+
Review Comment:
Thanks, good idea! For 1, I've added a new `DataIntegrityMetadataTest.java` to the 5.0/6.0/7.0 patches, and it exercises the path that throws NumberFormatException. For 2, I think this scenario isn't technically possible (where the data file is streamed but the digest file isn't?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "gandherva (via GitHub)" <gi...@apache.org>.
gandherva commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4129385961
##########
test/unit/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamWriterTest.java:
##########
@@ -173,6 +190,124 @@ public void testBlockReadingAndWritingOverWire() throws Throwable
}
}
+ @Test
+ public void testCorruptDataComponentFailsDigestValidation() throws Throwable
+ {
+ ByteBuf serializedFile = Unpooled.buffer(8192);
+ ComponentManifest manifest = writeEntireSSTable(serializedFile);
+ corruptDataComponent(serializedFile, manifest);
+
+ DatabaseDescriptor.setEntireSSTableStreamDigestValidationEnabled(true);
+ assertThatThrownBy(() -> readEntireSSTable(serializedFile, manifest))
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("Digest mismatch")
+ .hasCauseInstanceOf(IOException.class);
+ }
+
Review Comment:
For item 2, I meant we could use a test where the Components.DATA is absent while Components.Digest is present.
The source code in this PR handles that case from the following code by skipping the check. So I meant if we are going to handle something in the source code, then it good to test it in UT so that we verify the intended behavior from the source code and also helps alert others when they modify the source code against the intended behavior.
Also In the PR, there is already a test for the sibling scenario(Components.DIGEST absent and Components.DATA present).
if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))Expand commentComment on line R177Failed to resolve thread
{
NoSpamLogger.log(logger, NoSpamLogger.Level.WARN, 1, TimeUnit.MINUTES,
"Accepting zero-copy sstable stream from {} without validating CRC due to " +
"missing {} or {} from streamed components: {}",
session.peer, Components.DATA, Components.DIGEST, components);
return;
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "gandherva (via GitHub)" <gi...@apache.org>.
gandherva commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4117586626
##########
test/unit/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamWriterTest.java:
##########
@@ -173,6 +190,124 @@ public void testBlockReadingAndWritingOverWire() throws Throwable
}
}
+ @Test
+ public void testCorruptDataComponentFailsDigestValidation() throws Throwable
+ {
+ ByteBuf serializedFile = Unpooled.buffer(8192);
+ ComponentManifest manifest = writeEntireSSTable(serializedFile);
+ corruptDataComponent(serializedFile, manifest);
+
+ DatabaseDescriptor.setEntireSSTableStreamDigestValidationEnabled(true);
+ assertThatThrownBy(() -> readEntireSSTable(serializedFile, manifest))
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("Digest mismatch")
+ .hasCauseInstanceOf(IOException.class);
+ }
+
Review Comment:
(1) It would be good to a test for Long.parseLong(digestReader.readLine()) throwing NumberFormatException, the handling of which is newly added in code.
(2) It would also be good to add a test for Components.DATA not being present(while Digest is present), which the code handles by skipping the check
( if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))... )
##########
src/java/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamReader.java:
##########
@@ -157,6 +164,38 @@ public SSTableMultiWriter read(DataInputPlus in) throws IOException
}
}
+ /**
+ * Validates the received {@code Data.db} against {@code Digest.crc32}.
+ * @throws IOException if a digest was streamed and the received data file does not match it
+ */
+ private void validateDataDigest(Descriptor descriptor, ComponentManifest manifest) throws IOException
+ {
+ if (!DatabaseDescriptor.getEntireSSTableStreamDigestValidationEnabled())
+ return;
+
+ List<Component> components = manifest.components();
+ if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))
Review Comment:
Optional minor optimization: if we want to avoid the going through all the List elements twice to find Data and Digest, we could instead iterate them once and find if Data/Digest is present. (small list though).
The other option would be to add a method in ComponentManifest.containsComponent(Component) to check if it contains a particular Component as it stores a Set of Components. (but it seems that only this caller would use that)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21694: Validate CRC before zero-copy streaming sstables [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #5182:
URL: https://github.com/apache/cassandra/pull/5182#discussion_r4051682415
##########
src/java/org/apache/cassandra/db/streaming/CassandraEntireSSTableStreamReader.java:
##########
@@ -157,6 +164,38 @@ public SSTableMultiWriter read(DataInputPlus in) throws IOException
}
}
+ /**
+ * Validates the received {@code Data.db} against {@code Digest.crc32}.
+ * @throws IOException if a digest was streamed and the received data file does not match it
+ */
+ private void validateDataDigest(Descriptor descriptor, ComponentManifest manifest) throws IOException
+ {
+ if (!DatabaseDescriptor.getEntireSSTableStreamDigestValidationEnabled())
+ return;
+
+ List<Component> components = manifest.components();
+ if (!components.contains(Components.DATA) || !components.contains(Components.DIGEST))
+ {
+ NoSpamLogger.log(logger, NoSpamLogger.Level.WARN, 1, TimeUnit.MINUTES,
+ "Accepting zero-copy sstable stream from {} without validating CRC due to " +
+ "missing {} or {} from streamed components: {}",
+ session.peer, Components.DATA, Components.DIGEST, components);
+ return;
+ }
+
+ try
+ {
+ new DataIntegrityMetadata.FileDigestValidator(descriptor.fileFor(Components.DATA),
+ descriptor.fileFor(Components.DIGEST)).validate();
Review Comment:
Good catch, it makes more sense to count it toward `countEntireSSTableDigestMismatch` since it technically still is one. But I notice the other place that validates digests (`SortedTableVerifier.verifyDigest()`) doesn't catch a `NumberFormatException`, only `IOException`. The more reliable fix seems to be throwing an IOException, not a NumberFormatException, so it's correctly caught and handled in both sites. Updated with this change!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org