rpuch commented on code in PR #10118:
URL: https://github.com/apache/ignite/pull/10118#discussion_r910906210
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/serializer/RecordDataV1Serializer.java:
##########
@@ -210,22 +208,15 @@ public RecordDataV1Serializer(GridCacheSharedContext
cctx) {
@Override public WALRecord readRecord(RecordType type,
ByteBufferBackedDataInput in, int size)
throws IOException, IgniteCheckedException {
if (type == ENCRYPTED_RECORD || type == ENCRYPTED_RECORD_V2) {
- if (encSpi == null) {
Review Comment:
Why was this check dropped? `readEncryptedData()` accesses `encSpi`. Is this
because `encMgr` will always be `null` if `encSpi` is null? If this is true,
then it could still make sense to still add a check that `encSpi` is not null
inside `readEncryptedData()`.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/serializer/EncryptedData.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.cache.persistence.wal.serializer;
+
+import java.nio.ByteBuffer;
+import org.apache.ignite.internal.pagemem.wal.record.WALRecord;
+import
org.apache.ignite.internal.processors.cache.persistence.wal.ByteBufferBackedDataInput;
+import
org.apache.ignite.internal.processors.cache.persistence.wal.ByteBufferBackedDataInputImpl;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that represents information about an encrypted WAL record or data
entry.
+ */
+class EncryptedData {
Review Comment:
The name seems questionable. It makes you think it contains encrypted data,
but it actually POSSIBLY contains DEcrypted data (that was originall
encrypted), or it contains no data meaning it could not be decrypted.
I suggest the following:
1. Rename the class to `DecryptionResult`. 'result' seems better because a
result might be a failure; `DecryptedData` would suggest that the decryption
was successful and we can get plain text data (which is not true).
2. Rename `isDecrypted()` to `isDecryptedSuccessfully()`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/serializer/EncryptedData.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.cache.persistence.wal.serializer;
+
+import java.nio.ByteBuffer;
+import org.apache.ignite.internal.pagemem.wal.record.WALRecord;
+import
org.apache.ignite.internal.processors.cache.persistence.wal.ByteBufferBackedDataInput;
+import
org.apache.ignite.internal.processors.cache.persistence.wal.ByteBufferBackedDataInputImpl;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that represents information about an encrypted WAL record or data
entry.
+ */
+class EncryptedData {
+ /** Decrypted WAL record or {@code null} if it couldn't be decrypted. */
+ @Nullable
+ private final ByteBufferBackedDataInput data;
Review Comment:
How about renaming the field to `plainTextData` to highlight that it is the
data without encryption?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/serializer/RecordDataV1Serializer.java:
##########
@@ -210,22 +208,15 @@ public RecordDataV1Serializer(GridCacheSharedContext
cctx) {
@Override public WALRecord readRecord(RecordType type,
ByteBufferBackedDataInput in, int size)
throws IOException, IgniteCheckedException {
if (type == ENCRYPTED_RECORD || type == ENCRYPTED_RECORD_V2) {
- if (encSpi == null) {
- T2<Integer, RecordType> knownData = skipEncryptedRecord(in,
true);
+ EncryptedData encryptedData = readEncryptedData(in, true, type ==
ENCRYPTED_RECORD_V2);
Review Comment:
It seems a bit weird that a public method of a class related to v1 actually
does all the work for both v1 and v2. Probaly, this led to confusion and caused
the bug you are fixiing here.
How about the following:
1. Leave the implementation of `readRecord()` here, but make it a template
method
2. Extract things which define the differences between v1 and v2 to
protected methods and overriding them in `RecordDataV2Serializer`?
For example, the template method could look like this:
```
@Override public WALRecord readRecord(RecordType type,
ByteBufferBackedDataInput in, int size)
throws IOException, IgniteCheckedException {
if (type == encryptedRecordType()) {
EncryptedData encryptedData = readEncryptedData(in, true);
if (encryptedData.isDecrypted()) {
ByteBufferBackedDataInput data = encryptedData.data();
return readPlainRecord(encryptedData.recordType(), data,
true, data.buffer().capacity());
}
else
return new EncryptedRecord(encryptedData.grpId(),
encryptedData.recordType());
}
return readPlainRecord(type, in, false, size);
}
```
and there would also be
```
protected RecordType encryptedRecordType();
```
returning `ENCRYPTED_RECORD` and `ENCRYPTED_RECORD_V2` in the corresponding
implementations, and
```
protected EncryptedData readEncryptedData(
ByteBufferBackedDataInput in,
boolean readType
) throws IOException, IgniteCheckedException {
```
calling `readEncryptedData(ByteBufferBackedDataInput, boolean, boolean)`
with the corresponding third argument specific for v1 or v2.
WDYT?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/serializer/RecordDataV1Serializer.java:
##########
@@ -287,57 +278,45 @@ private boolean needEncryption(int grpId) {
* @throws IOException If failed.
* @throws IgniteCheckedException If failed.
*/
- private T3<ByteBufferBackedDataInput, Integer, RecordType>
readEncryptedData(
+ private EncryptedData readEncryptedData(
ByteBufferBackedDataInput in,
boolean readType,
boolean readKeyId
) throws IOException, IgniteCheckedException {
int grpId = in.readInt();
int encRecSz = in.readInt();
- RecordType plainRecType = null;
-
- if (readType)
- plainRecType = RecordV1Serializer.readRecordType(in);
+ RecordType plainRecType = readType ?
RecordV1Serializer.readRecordType(in) : null;
int keyId = readKeyId ? in.readUnsignedByte() :
GridEncryptionManager.INITIAL_KEY_ID;
- byte[] encData = new byte[encRecSz];
+ // Encryption Manager can be null during offline WAL iteration
+ if (encMgr == null) {
+ int skipped = in.skipBytes(encRecSz);
- in.readFully(encData);
+ assert skipped == encRecSz;
- GroupKey grpKey = encMgr.groupKey(grpId, keyId);
+ return new EncryptedData(null, plainRecType, grpId);
+ }
- if (grpKey == null)
- return new T3<>(null, grpId, plainRecType);
+ GroupKey grpKey = encMgr.groupKey(grpId, keyId);
- byte[] clData = encSpi.decrypt(encData, grpKey.key());
+ // Encryption key is not available when restoring the MetaStorage
+ if (grpKey == null) {
+ int skipped = in.skipBytes(encRecSz);
Review Comment:
This block duplicates the block above. How about extracting it to a tiny
method?
##########
modules/dev-utils/src/test/java/org/apache/ignite/development/utils/IgniteEncryptedWalConverterTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.development.utils;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.PrintStream;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.encryption.AbstractEncryptionTest;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.spi.encryption.keystore.KeystoreEncryptionSpi;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static java.util.Collections.emptyList;
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_BINARY_METADATA_PATH;
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_MARSHALLER_PATH;
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_WAL_ARCHIVE_PATH;
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_WAL_PATH;
+import static org.apache.ignite.development.utils.IgniteWalConverter.convert;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Class that contains tests on interaction between the {@link
IgniteWalConverter} and encrypted WALs.
+ */
+public class IgniteEncryptedWalConverterTest extends GridCommonAbstractTest {
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ cleanPersistenceDir();
+
+ super.afterTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String
igniteInstanceName) throws Exception {
+ KeystoreEncryptionSpi encSpi = new KeystoreEncryptionSpi();
+
+ encSpi.setKeyStorePath(AbstractEncryptionTest.KEYSTORE_PATH);
+
encSpi.setKeyStorePassword(AbstractEncryptionTest.KEYSTORE_PASSWORD.toCharArray());
+
+ return super.getConfiguration(igniteInstanceName)
+ .setEncryptionSpi(encSpi)
+
.setCacheConfiguration(defaultCacheConfiguration().setEncryptionEnabled(true))
+ .setDataStorageConfiguration(new DataStorageConfiguration()
+ .setDefaultDataRegionConfiguration(new
DataRegionConfiguration()
+ .setPersistenceEnabled(true))
+ );
+ }
+
+ /**
+ * Populates an encrypted cache and checks that its WAL contains encrypted
records.
+ */
+ @Test
+ public void testIgniteWalConverter() throws Exception {
+ String nodeFolder = createWal();
+
+ ByteArrayOutputStream outByte = new ByteArrayOutputStream();
+
+ PrintStream out = new PrintStream(outByte);
+
+ IgniteWalConverterArguments arg = new IgniteWalConverterArguments(
+ U.resolveWorkDirectory(U.defaultWorkDirectory(), DFLT_WAL_PATH,
false),
+ U.resolveWorkDirectory(U.defaultWorkDirectory(),
DFLT_WAL_ARCHIVE_PATH, false),
+ DataStorageConfiguration.DFLT_PAGE_SIZE,
+ new File(U.resolveWorkDirectory(U.defaultWorkDirectory(),
DFLT_BINARY_METADATA_PATH, false), nodeFolder),
+ U.resolveWorkDirectory(U.defaultWorkDirectory(),
DFLT_MARSHALLER_PATH, false),
+ false,
+ null,
+ null,
+ null,
+ null,
+ null,
+ false,
+ false,
+ emptyList()
+ );
+
+ convert(out, arg);
Review Comment:
Should we avoid a static import here? It does not seem to bring any
benefits, but it increases confusion
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
