Make Lucene serialization able to encrypt, … #3607

MMcM · 2025-09-17T18:25:50Z

… using the same serialization key manager as record serialization.

ScottDugas

I didn't get to really think about whether the encryption code is doing the right thing. It looks pretty similar to the record encryption though.

ScottDugas · 2025-09-25T19:50:35Z

...er-lucene/src/main/java/com/apple/foundationdb/record/lucene/directory/LuceneSerializer.java

-            return encrypted;
+        public void setKeyNumber(final int keyNumber) {
+            this.keyNumber = keyNumber;
        }


It does look like TransformedRecordSerializerState is a super set of this class. Would it make sense to move this out, and have TransformedRecordSerializerState have one of these.

Refactored to share the base class with just the intended / found format.

ScottDugas · 2025-09-25T20:00:24Z

...er-lucene/src/main/java/com/apple/foundationdb/record/lucene/directory/LuceneSerializer.java

+                                                 @Nonnull byte[] encodedData, int prefixLength) {
+        final byte[] encoded = new byte[originalData.length + prefixLength];
+        System.arraycopy(encodedData, 0, encoded, 0, prefixLength);
+        encoded[0] &= ~ENCODING_COMPRESSED;


Is this guaranteed to be in byte[0] (i.e. what if prefixLength>1)

Yes, I believe so. Varints are 7-bit little-endian, with a "continuation" bit to know when to stop. So, the compressed flag bit, which is in the low bits, is always in the first byte, no matter how many higher bits there are in the following bytes.

ScottDugas · 2025-09-25T20:56:34Z

...cord-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/FDBLuceneQueryTest.java

+    void encrypted(long seed) throws Exception {
+        final RollingTestKeyManager rollingTestKeyManager = new RollingTestKeyManager(seed);
+        keyManager = rollingTestKeyManager;
+        // Save in many transactions to get more segments and so more blocks.


Probably also a good idea to test having a single segment with multiple blocks

I think these documents write on the order of 9 blocks already. I see that we don't actually have a metric for that, so I added one. It's hard to assert about that, though, because the commit method in the test fixture clears the timer and that's when the work gets done.

ScottDugas · 2025-09-25T20:57:35Z

...cord-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/FDBLuceneQueryTest.java

+
+    @ParameterizedTest
+    @RandomSeedSource
+    void encrypted(long seed) throws Exception {


Perhaps worth having some of the tests in LuceneIndexMaintenanceTest chose to randomly encrypt and-or compress.

I added these serialization states as an argument to the existing tests that took a bunch of arguments.

ScottDugas · 2025-09-25T20:59:13Z

...cord-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/FDBLuceneQueryTest.java

+            try (FDBRecordContext context = openContext()) {
+                openRecordStore(context);
+                recordStore.saveRecord(doc);
+                commit(context);


This should be merging in-line when you commit. Would it be worthwhile to test merging separately? Perhaps that's best covered by LuceneIndexMaintenanceTest.

…on key manager as record serialization.

ohadzeliger · 2025-09-26T21:21:16Z

...ava/com/apple/foundationdb/record/provider/common/CompressedAndEncryptedSerializerState.java

+    }
+
+    public void setKeyNumber(final int keyNumber) {
+        this.keyNumber = keyNumber;


Is 0 a special key number (if the setter is not called and it remains uninitialized)?

Not particularly at this level; if encryption is not enabled, it does remain unset, but should also be unused. Did you have something in mind to make this clearer?

I thought maybe making the class immutable (values in constructor) or initializing the field to some illegal value to protect ourselves from a bug where it is not initialized.

ohadzeliger · 2025-09-26T21:37:13Z

...er-lucene/src/main/java/com/apple/foundationdb/record/lucene/directory/LuceneSerializer.java


    @Nullable
-    public static byte[] encode(@Nullable byte[] data, boolean compress, boolean encrypt) {
+    public byte[] encode(@Nullable byte[] data, boolean compress, boolean encrypt) {


Would it make sense to move the compress and encrypt params to be fields of the class instead?

I moved compressionEnabled and encryptionEnabled from FDBDirectory into the serializer.

...er-lucene/src/main/java/com/apple/foundationdb/record/lucene/directory/LuceneSerializer.java

...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

…ecord/lucene/directory/LuceneSerializer.java Co-authored-by: ohadzeliger <70664918+ohadzeliger@users.noreply.github.com>

… from the directory to its serializer.

MMcM added the enhancement New feature or request label Sep 17, 2025

MMcM force-pushed the lucene-encrypted-serialization branch 2 times, most recently from 122976b to c94febd Compare September 24, 2025 14:49

MMcM marked this pull request as ready for review September 24, 2025 14:50

MMcM requested review from ohadzeliger and ScottDugas September 24, 2025 16:44

MMcM force-pushed the lucene-encrypted-serialization branch from c94febd to a51cfbc Compare September 25, 2025 00:02

ScottDugas reviewed Sep 25, 2025

View reviewed changes

MMcM added 4 commits September 26, 2025 06:36

Make Lucene serialization able to encrypt, using the same serializati…

e4a904c

…on key manager as record serialization.

Extract a common subclass

7b47f2f

Add clarifying comment

6d948b8

Randomize compression and encryption in LuceneIndexMaintenanceTest

92ebe52

MMcM force-pushed the lucene-encrypted-serialization branch from a51cfbc to 92ebe52 Compare September 26, 2025 14:45

Add a metric for the number of Lucene block writes

9297fc8

ohadzeliger requested changes Sep 29, 2025

View reviewed changes

MMcM and others added 4 commits September 29, 2025 08:59

Update fdb-record-layer-lucene/src/main/java/com/apple/foundationdb/r…

9aeeb04

…ecord/lucene/directory/LuceneSerializer.java Co-authored-by: ohadzeliger <70664918+ohadzeliger@users.noreply.github.com>

Add a test of failing decryption in Lucene

4f8cb6c

Move the {compression,encryption}Enabled state gotten from properties…

8c4657e

… from the directory to its serializer.

Fix missing annotation

cf6e5c6

ohadzeliger approved these changes Sep 30, 2025

View reviewed changes

Make Lucene serialization able to encrypt, … #3607

Are you sure you want to change the base?

Make Lucene serialization able to encrypt, … #3607

Conversation

MMcM commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScottDugas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MMcM Sep 26, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

MMcM Sep 26, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MMcM Sep 29, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MMcM commented Sep 17, 2025 •

edited

Loading

MMcM Sep 26, 2025 •

edited

Loading

MMcM Sep 26, 2025 •

edited

Loading

MMcM Sep 29, 2025 •

edited

Loading