timoninmaxim commented on a change in pull request #9269:
URL: https://github.com/apache/ignite/pull/9269#discussion_r687485834
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -218,11 +210,26 @@
* @throws IOException If failed.
*/
private void encrypt(ByteBuffer srcBuf, ByteBuffer res) throws IOException
{
Review comment:
This method without `res.rewind()` looks strange. Also this method is
used only inside `#encrypt(ByteBuffer)` and never more. Let's merge those 2
methods.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1505,6 +1508,16 @@ SnapshotFutureTask registerSnapshotTask(
if (prev != null)
return new SnapshotFutureTask(new
IgniteCheckedException("Snapshot with requested name is already scheduled: " +
snpName));
+ for (Integer grpId : parts.keySet()) {
+ if (!withMetaStorage && cctx.cache().isEncrypted(grpId)) {
Review comment:
Let's move check `!withMetaStorage` before loop. If the metastore is
included in snapshot then it's useless to iterate over all `parts`.
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.snapshot;
+
+import java.util.Collections;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.encryption.AbstractEncryptionTest;
+import org.apache.ignite.internal.util.distributed.FullMessage;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.lang.IgniteFuture;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+/**
+ * Snapshot test for encrypted snapshots.
+ */
+public class EncryptedSnapshotTest extends AbstractSnapshotSelfTest {
+ /** Parameters. */
+ @Parameterized.Parameters(name = "Encryption is enabled.")
+ public static Iterable<Boolean> enableEncryption() {
+ return Collections.singletonList(true);
+ }
+
+ /** Name of additional encrypted cache. */
+ private static final String NOT_ENCRYPTED_CACHE_NAME = "notEncryptedCache";
+
+ /** {@inheritDoc} */
+ @Override protected Function<Integer, Object> valueBuilder() {
+ return (i -> new Account(i, i));
+ }
+
+ /** Checks re-encryption fails during snapshot restoration. */
+ @Test
+ public void testReencryptDuringRestore() throws Exception {
+ checkActionFailsDuringSnapshotOperation(true,
this::chageCacheGroupKey, "Cache group key change was " +
+ "rejected.", IgniteException.class);
+ }
+
+ /** Checks master key changing failes during snapshot restoration. */
+ @Test
+ public void testMasterKeyChangeDuringRestore() throws Exception {
+ checkActionFailsDuringSnapshotOperation(true, this::chageMasterKey,
"Master key change was rejected.",
+ IgniteException.class);
+ }
+
+ /** Checks re-encryption fails during snapshot creation. */
+ @Test
+ public void testReencryptDuringSnapshot() throws Exception {
+ checkActionFailsDuringSnapshotOperation(false,
this::chageCacheGroupKey, "Cache group key change was " +
+ "rejected.", IgniteException.class);
+ }
+
+ /** Checks master key changing fails during snapshot creation. */
+ @Test
+ public void testMasterKeyChangeDuringSnapshot() throws Exception {
+ checkActionFailsDuringSnapshotOperation(false, this::chageMasterKey,
"Master key change was rejected.",
+ IgniteException.class);
+ }
+
+ /** Checks snapshot action fail during cache group key change. */
+ @Test
+ public void testSnapshotFailsDuringCacheKeyChange() throws Exception {
+ checkSnapshotActionFailsDuringReencryption(this::chageCacheGroupKey);
+ }
+
+ /** Checks snapshot action fail during master key cnahge. */
+ @Test
+ public void testSnapshotFailsDuringMasterKeyChange() throws Exception {
+ checkSnapshotActionFailsDuringReencryption(this::chageMasterKey);
+ }
+
+ /** Checks snapshot restoration fails if different master key is contained
in the snapshot. */
+ @Test
+ public void testStartFromSnapshotFailedWithOtherMasterKey() throws
Exception {
+ IgniteEx ig = startGridsWithCache(1, CACHE_KEYS_RANGE, valueBuilder(),
dfltCacheCfg);
+
+ ig.snapshot().createSnapshot(SNAPSHOT_NAME).get();
+
+ ig.destroyCache(dfltCacheCfg.getName());
+
+ ensureCacheAbsent(dfltCacheCfg);
+
+ stopAllGrids(false);
+
+ masterKeyName = AbstractEncryptionTest.MASTER_KEY_NAME_2;
+
+ GridTestUtils.assertThrowsAnyCause(
+ log,
+ () -> startGridsFromSnapshot(1, SNAPSHOT_NAME),
+ IgniteCheckedException.class,
+ "bad key is used during decryption"
+ );
+ }
+
+ /** Checks it is unavailable to register snapshot task for encrypted
caches witout metastore. */
+ @Test
+ public void testSnapshotTaskIsBlockedWithoutMetastore() throws Exception {
+ // Start grid node with data before each test.
+ IgniteEx ig = startGridsWithCache(1, CACHE_KEYS_RANGE, valueBuilder(),
dfltCacheCfg);
+
+ GridTestUtils.assertThrowsAnyCause(log,
+ () -> snp(ig).registerSnapshotTask(SNAPSHOT_NAME,
ig.localNode().id(), F.asMap(CU.cacheId(dfltCacheCfg.getName()), null),
+ false,
snp(ig).localSnapshotSenderFactory().apply(SNAPSHOT_NAME)).get(TIMEOUT),
+ IgniteCheckedException.class,
+ "Metastore is requird because it contains encryption keys");
Review comment:
s/requird/required/
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws
Exception {
assertTrue("Threads created: " + createdThreads, createdThreads <
iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after
snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting
snapshots deltas causes several page writes into file.
Review comment:
there and below: s/Writting/Writing/
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.snapshot;
+
+import java.util.Collections;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.encryption.AbstractEncryptionTest;
+import org.apache.ignite.internal.util.distributed.FullMessage;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.lang.IgniteFuture;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+/**
+ * Snapshot test for encrypted snapshots.
+ */
+public class EncryptedSnapshotTest extends AbstractSnapshotSelfTest {
Review comment:
There are only failure-pass tests. What about add some happy-pass cases?
For example:
1. Currently there is no check that data in snapshot is encrypted indeed.
And it doesn't affect data for non-encrypted cache.
2. Is it a valuable test to check pipeline: Snapshot -> optional Restore ->
Reencryption -> Snapshot -> Restore?
3. Check that doesn't fail pipeline: Create non-encrypted cache -> Snapshot
-> Destroy -> Create an encrypted-cache with the same name, and the same in
reverse order.
Maybe there are some more happy cases to test? WDYT?
##########
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckWithIndexesTest.java
##########
@@ -36,6 +38,12 @@
* Cluster-wide snapshot test check command with indexes.
*/
public class IgniteClusterSnapshotCheckWithIndexesTest extends
AbstractSnapshotSelfTest {
+ /** Parameters. Encryption is not supported by snapshot validation. */
Review comment:
What is result if we start validate snapshot with both encrypted and
non-encrypted caches? Can we check only non-encrypted caches then?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1505,6 +1508,16 @@ SnapshotFutureTask registerSnapshotTask(
if (prev != null)
return new SnapshotFutureTask(new
IgniteCheckedException("Snapshot with requested name is already scheduled: " +
snpName));
+ for (Integer grpId : parts.keySet()) {
+ if (!withMetaStorage && cctx.cache().isEncrypted(grpId)) {
+ snpFutTask.onDone(new IgniteCheckedException("Snapshot
contains encrypted cache group " + grpId + " but doesn't " +
+ "include metastore. Metastore is requird because it
contains encryption keys required to start with encrypted " +
Review comment:
s/requird/required/
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws
Exception {
assertTrue("Threads created: " + createdThreads, createdThreads <
iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after
snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting
snapshots deltas causes several page writes into file.
+ // Every page write calls encrypt(). Every repatable encrypt()
produces different record (different bytes) even for same original
Review comment:
s/repatable/repeatable/ ?
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws
Exception {
assertTrue("Threads created: " + createdThreads, createdThreads <
iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after
snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting
snapshots deltas causes several page writes into file.
+ // Every page write calls encrypt(). Every repatable encrypt()
produces different record (different bytes) even for same original
+ // data. Re-writting pages from delta to partition file in the
shanpshot leads to additional encryption before writting to the
+ // snapshot partition file. Thus, page in original partition and in
snapshot partiton has different encrypted CRC and same
+ // de-crypted CRC. Different encrypted CRC looks like different data
in point of view of third-party observer.
Review comment:
> partiton has different encrypted CRC and same de-crypted CRC.
Why we can't deencrypt them before this check?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1438,9 +1439,11 @@ public StandaloneGridKernalContext
createStandaloneKernalContext(String snpName,
File snpPart = getPartitionFile(new File(snapshotLocalDir(snpName),
databaseRelativePath(folderName)),
grps.get(0).getName(), partId);
- FilePageStore pageStore = (FilePageStore)storeFactory
- .apply(CU.cacheId(grpName), false)
- .createPageStore(getTypeByPartId(partId),
+ int grpId = CU.cacheId(grpName);
+
+ FilePageStore pageStore =
(FilePageStore)storeMgr.getPageStoreFactory(grpId,
+ cctx.kernalContext().encryption().getActiveKey(grpId) != null).
Review comment:
In this file on 2021 line you have different logic to find whether a
CacheGroup is encrypted or not. What is a difference? Trying to leverage on
internal collection to get info about configuration looks strange for me.
`boolean encrypted = cctx.cache().isEncrypted(pair.getGroupId());`
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotFutureTask.java
##########
@@ -620,9 +617,9 @@ private void addPartitionWriters(int grpId, Set<Integer>
parts, String dirName)
PageStore store = pageStore.getStore(grpId, partId);
- partDeltaWriters.put(pair,
- new PageStoreSerialWriter(store,
- partDeltaFile(cacheWorkDir(tmpConsIdDir, dirName),
partId)));
+ partDeltaWriters.put(pair, new PageStoreSerialWriter(store,
+ partDeltaFile(cacheWorkDir(tmpConsIdDir, dirName), partId),
+ cctx.cache().isEncrypted(grpId) ? grpId : null));
Review comment:
Can move getting `encryptionGrpId` before loop.
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotSelfTest.java
##########
@@ -330,6 +330,10 @@ public void testClusterSnapshotConsistencyUnderLoad()
throws Exception {
startGridsWithCache(grids, clientsCnt, key -> new Account(key,
balance), eastCcfg, westCcfg);
+ // To prevent encrypted cache creation from client, we create it on
the server side.
Review comment:
Don't found such restriction in docs. What is a reason?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -179,10 +179,14 @@
/** {@inheritDoc} */
@Override public int write(ByteBuffer srcBuf) throws IOException {
- assert position() == 0;
- assert headerSize == srcBuf.capacity();
+ if (headerSize > 0) {
Review comment:
I think we need more javadocs in this class about a structure of a page
file, or provide a link to such doc.
It's worth to mention about header (is not encrypted by design, can be set
to 0 for some specific cases). Also it looks like implementation of methods
depends on headerSize. For example we can invoke `read(ByteBuffer)` only if
`headerSize > 0`. But we can `write(ByteBuffer)` if `headerSize == 0`. Also
`read(ByteBuffer, position)` and `readFully(ByteBuffer, position)` has
different checks but those methods in parent class FileIO do have the same
javadocs. Looks very unclear without specific docs.
Also unclear that the `write` methods make rewind() while the `read` methods
don't. Should we mention it in docs too?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -109,7 +109,7 @@
/** {@inheritDoc} */
@Override public int read(ByteBuffer destBuf) throws IOException {
- assert position() == 0;
+ assert position() == 0 && headerSize > 0;
return plainFileIO.read(destBuf);
Review comment:
Why we don't decrypt there? The other method `read(ByteBuffer, long)`
decrypts underlying data.
--
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]
