IGNITE-14999v3 : Support dynamic restoration of encrypted snapshots. #9313

Vladsz83 · 2021-08-10T19:01:36Z

This PR extends 'static' encrypted snapshots #9269.

The new general features are:

Dynamic restoration of encrypted snapshot on a running node ( IgniteSnapshotManager#restoreSnapshot() )
Validation of encrypted snapshot ( IgniteSnapshotManager#checkSnapshot() )

Same master key is still required.

…tests.

timoninmaxim · 2021-08-23T08:37:54Z

modules/core/src/main/java/org/apache/ignite/internal/processors/cache/StoredCacheData.java

    }

+    /**
+     * @return Chipered encryption key for this cache or cache group. {@code Null} if not encrypted.


Here and below, replace "Chipered" with "Ciphered"

timoninmaxim · 2021-08-31T12:22:25Z

.../java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java

+            }
+        }
+
+        this.encrGrpIds.addAll(encrGrpIds);


You guard initialization of encGrpIds with this, then it's assumed that code runs in concurrent env. But the map is an ordinary HashMap, and it's allowed to being modified (addAll) in separate thread without sync.

In previous comment you said that it's possible for encrGrpIds be nulled after encrGrpIds != null. So we need a guard for this case there too.

Nope. Once encrGrpIds gets assigned, it never takes null. User cannot null it.

no encrypted groups ids reside in SnapshotMeta any more. inactual

timoninmaxim · 2021-08-31T12:25:45Z

.../java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java

+     * @return {@code True} if cache group is encrypted. {@code False} otherwise.
+     */
+    public boolean isCacheGroupEncrypted(int grpId) {
+        Set<Integer> encrGrpIds = this.encrGrpIds;


What is a purpose of this local var?

to avoid NPE if class member has been nulled after encrGrpIds != null

But the reference encrGrpIds is not synced: reference isn't volatile, method doesn't acquire a lock. So this reference isn't observable for other threads within this method, so this var doesn't make sense.

Make var volatile, or use lock there.

Exactly. Local variable which is not affected by other thred. It guaranties the reference won't change during several reads. Please check #9377 , EncryptedSnapshotTest.testNPE(). Try uncomment // Set<Integer> encrGrpIds = this.encrGrpIds; in SnapshotMetadata.isCacheGroupEncrypted()

This is common technique to avoid NPE to me, introducing local varaible. But in our case, as mentioned, once encrGrpIds is assigned, it won't set to null. We can check it once. But for general purpose I wrote it correctly

You've initialized encrGrpIds with the DCL pattern (that requires the object to be declared with volatile keyword, and you missed it). It means that encrGrpIds is assigned from concurrent context, but you just copy thread local var (that is not observable from other threads and may can't see that encrGrpIds is initialized) to local var.

Exactly. This brings thread-safety. The test shows it. Makes sense?

No encrypted groups ids reside in SnapshotMeta any more. Inactual.

timoninmaxim · 2021-08-31T12:35:43Z

.../java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java

    private final List<Integer> grpIds;

+    /** Encrypted group ids. */
+    private Set<Integer> encrGrpIds;


Should it be marked with GridToStringInclude? The same question for the masterKeyDigest

timoninmaxim · 2021-08-31T12:55:06Z

.../org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

-                    for (SnapshotMetadata meta : nodeMetas)
+                    for (SnapshotMetadata meta : nodeMetas) {
+                        if (meta.masterKeyDigest() != null && !Arrays.equals(meta.masterKeyDigest(),
+                            kctx0.config().getEncryptionSpi().masterKeyDigest())) {


getEncryptionSpi().masterKeyDigest() performs KeystoreEncryptionSpi#makeDigest operation on every iteration for this loop. Let's make a var for it before this loop.

It's possible to get NPE for kctx0.config().getEncryptionSpi().masterKeyDigest() for config with disabled encryption.

"performs KeystoreEncryptionSpi#makeDigest operation on every" But verifying snapshot is slow operation which involves many disk reading, page reading one by one from the partition files. Is it worth?

"It's possible to get NPE for kctx0.config().getEncryptionSpi().masterKeyDigest() for config with disabled encryption." Impossible. Toy can find if (cfg.getEncryptionSpi() == null) cfg.setEncryptionSpi(new NoopEncryptionSpi()); in IgniteEx.initializeDefaultSpi() and in EncryptedSnapshotTest.testValidatingSnapshotFailsWithNoEncryption

Is it worth?

But masterKeyDigest() is method of public interface EncryptionSpi that can be overloaded with Ignite user, you never knows what is going under the hood. It can goes by to external storage over unreliable network. Let's prevent this.

timoninmaxim · 2021-08-31T13:09:30Z

.../org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

                "prior to snapshot operation start: " + leftNodes));
        }

+        if (!cctx.localNode().isClient() && cctx.kernalContext().encryption().isMasterKeyChangeInProgress()


In the construction A && B || C the C operation will be invoked despite of whether the A is true of false. So it may lead to NPE, as cctx is null for client nodes.

If cctx is null for client, we fail at start: if (cctx.kernalContext().clientNode(). Also snapshots is disables for client nodes because clisents do not have data and have nothing to snapshot.

we fail at start

Yes, so, do we need an explicit check for client, if it always pass or raise NPE? Maybe replace it with assert cctx != null?

I don't think so. We do not affect snapshot-on-client behavior in this ticket. It has been working well till now. Shapshots-on-client hasn't raised NPE before the ticket. I believe, because this DistributedProcess doesn't work on client regardless encrypted or not. There is cctx.kernalContext().clientNode() before. It hasn't been asserted earlier. I don't think we should care.

Yes, you're right. GridCacheContext is null for client node in GridQueryProcessor only.

So, what about to fix boolean logic then - A && (B || C)?

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyTask.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotRestoreBaseTest.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotRestoreSelfTest.java # modules/core/src/test/java/org/apache/ignite/testsuites/IgniteBasicWithPersistenceTestSuite.java

timoninmaxim · 2021-09-03T08:43:49Z

...core/src/main/java/org/apache/ignite/internal/managers/encryption/GridEncryptionManager.java

                "The previous change was not completed."));
        }

+        if (ctx.cache().context().snapshotMgr().isSnapshotCreating()


What if snapshot create/restore started after this check and before the next line masterKeyChangeRequest = req;? Do we need a lock (instead of volatile var) that syncs changing of master key and snapshot process?

I see it like this:

SnapshotManager tries to acquire the lock on master key, then wait for lock if other process own it.

MasterKeyChangeProcess tries to acquire lock, and if failed then return error to user (without wait for lock).

WDYT? Is the sync of those processes are already guaranteed with different approach?

Master key change and snapshots are distributed processes based on discovery messages which are guaranteed be sequential. Once one process is active, another process will see this state. There is no pure concurrency. Master key change isn't allowed during snapshot. Snapshot is not allowed during master key change and re-encryption. You can check testSnapshotFailsDuringCacheKeyChange(), testSnapshotFailsDuringMasterKeyChange(), testMasterKeyChangeDuringSnapshot(), testMasterKeyChangeDuringRestore(), testReencryptionDuringRestore(), testReencryptionDuringSnapshot()

timoninmaxim · 2021-09-03T08:50:28Z

modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java


        GridPlainClosure2<Collection<byte[]>, byte[], IgniteInternalFuture<Boolean>> startCacheClsr =
            (grpKeys, masterKeyDigest) -> {
-            List<DynamicCacheChangeRequest> srvReqs = null;


Could you please revert tabulation of this block? It's pretty hard to understand what is exactly changed.

Why don't we follow the checkstyle? This is the code format. What's wrong?

timoninmaxim · 2021-09-03T08:53:18Z

.../org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

                "prior to snapshot operation start: " + leftNodes));
        }

+        if (!cctx.localNode().isClient() && cctx.kernalContext().encryption().isMasterKeyChangeInProgress()


we fail at start

Yes, so, do we need an explicit check for client, if it always pass or raise NPE? Maybe replace it with assert cctx != null?

timoninmaxim · 2021-09-03T09:01:33Z

.../java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java

+     * @return {@code True} if cache group is encrypted. {@code False} otherwise.
+     */
+    public boolean isCacheGroupEncrypted(int grpId) {
+        Set<Integer> encrGrpIds = this.encrGrpIds;


But the reference encrGrpIds is not synced: reference isn't volatile, method doesn't acquire a lock. So this reference isn't observable for other threads within this method, so this var doesn't make sense.

Make var volatile, or use lock there.

timoninmaxim · 2021-09-03T09:03:27Z

.../java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java

+            }
+        }
+
+        this.encrGrpIds.addAll(encrGrpIds);


In previous comment you said that it's possible for encrGrpIds be nulled after encrGrpIds != null. So we need a guard for this case there too.

timoninmaxim · 2021-09-06T11:09:35Z

modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java

+                        encrKey != null ? masterKeyDigest : null);
+
+                    if (encrKey != null)
+                        req.encryptionKeyId(encrKey.id());


Let's move this setting inside the prepareCacheChangeRequest method. Now you have 2 places to fill DynamicCacheChangeRequest with info about encrypted key - key and id are set separately.

timoninmaxim · 2021-09-06T11:19:01Z

.../org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

-                    for (SnapshotMetadata meta : nodeMetas)
+                    for (SnapshotMetadata meta : nodeMetas) {
+                        if (meta.masterKeyDigest() == null)
+                            continue;


Should we remove the meta partitions from the grpIds collection (see 1113 line)? Now your changes affect only encrypted cached, what about non-encrypted?

What is "meta partitions"? it is snapshot metadata. We check encryption, master keys here. However, another fixes applied.

timoninmaxim · 2021-09-06T11:22:20Z

.../org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

+                Collection<ClusterNode> bltNodes = F.view(cctx.discovery().serverNodes(AffinityTopologyVersion.NONE),
+                    (node) -> CU.baselineNode(node, kctx0.state().clusterState()));
+
+                kctx0.task().setThreadContext(TC_SKIP_AUTH, true);


It duplicates lines below (L1132). What is a purpose?

timoninmaxim · 2021-09-06T12:25:59Z

...e/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyHandler.java

+     */
+    private static class SnapshotEncrKeyProvider implements EncryptionCacheKeyProvider {
+        /** Kernal context */
+        private GridKernalContext ctx;


Here and below, make field final.

timoninmaxim · 2021-09-14T09:52:47Z

...core/src/test/java/org/apache/ignite/testframework/junits/common/GridCommonAbstractTest.java

+     * Cleans persistence directory including snapshots.
     */
    protected void cleanPersistenceDir() throws Exception {
+        cleanPersistenceDir(true);


I don't like an idea that we change a signature of this wide used method. It is extremely rare case to not clean snapshots in tests. What are alternatives? For example, create snapshots in different directory for tests that don't clean them, or introduce a method for copy-pasting snapshots. WDYT?

But we didn't change the signature. We added new method. You can use previous method as usual.

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/managers/encryption/GridEncryptionManager.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIOFactory.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/FilePageStoreManager.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotFutureTask.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotRestoreProcess.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/AbstractSnapshotSelfTest.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotRestoreBaseTest.java

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheUtils.java

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

…e#9313)

Vladsz83 added 30 commits June 8, 2021 14:32

experimets

961c5c7

first impl

4467c1e

tests passed first time

212a34f

+ test attempt

b55a658

first almost working

9d7e5f5

first stabilized tests

cad3a8c

refactoring

c5d08a0

+ many tests passed

aa334d8

first almost working

1399d84

+ Snapshot meta redords. + comments. + KeyNotFoundException. + Fixed …

af77e8d

…tests.

better tests. in progress

851de5a

+ tests

158e2ab

checking

f3d4238

fixes

78e06ae

fix

c3db940

Merge branch 'master' into IGNITE-14999

9e44a16

codestyle fix

e84fb09

fix codestyle

6173f98

test fix

3da206d

tests fixed

b77ecc3

Checkstyle fix

aa1f5ad

minority

8dd3286

Backward support for snapshot metas

22dd14e

Tests fix.

c3c5ad7

Removed accident test param

569ef5e

Checkstyle fix

6687d83

Fixed group key id transfer.

b7690c6

minority

8b1bd08

minority

3bdb9e7

+awaitPartitionMapExchange() to avoid flacky test

51358cd

minority

28ed711

timoninmaxim reviewed Aug 31, 2021

View reviewed changes

Vladsz83 added 2 commits August 31, 2021 23:47

Fixes after merge

54ff5f4

timoninmaxim requested changes Sep 3, 2021

View reviewed changes

timoninmaxim reviewed Sep 6, 2021

View reviewed changes

Vladsz83 added 2 commits September 7, 2021 18:48

fixes after review

d54cd39

refactored encrypted groups ids in snapshot meta

a85c3a3

timoninmaxim reviewed Sep 14, 2021

View reviewed changes

fix: no extra cleanPersistenceDir()

45b7f07

timoninmaxim approved these changes Sep 14, 2021

View reviewed changes

Vladsz83 added 3 commits October 8, 2021 12:47

Merge branch 'master' into IGNITE-14999v3

6ab2dfc

Merged master

b483a7b

Vladsz83 changed the title ~~IGNITE-14999v3~~ IGNITE-14999v3 : Support dynamic restoration of encrypted snapshots. Oct 8, 2021

Vladsz83 added 4 commits November 12, 2021 12:50

Merge branch 'master' into IGNITE-14999v3

6d75266

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheUtils.java

fixes after review

2667cf8

Merge branch 'master'

6c9a4a4

Fixes after review

c878898

NSAmelchev approved these changes Feb 3, 2022

View reviewed changes

Vladsz83 added 2 commits February 3, 2022 14:11

Merge branch 'master' into IGNITE-14999v3

e135903

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java

merged master

a03379f

Mmuzaf approved these changes Mar 2, 2022

View reviewed changes

Merge branch 'master' into IGNITE-14999v3

be4432f

NSAmelchev merged commit 84d005f into apache:master Mar 5, 2022

Vladsz83 deleted the IGNITE-14999v3 branch March 5, 2022 13:04

xintrian pushed a commit to xintrian/ignite that referenced this pull request May 27, 2022

IGNITE-14999 Added dynamic restoration of encrypted snapshots. (apach…

4cabd17

…e#9313)

IGNITE-14999v3 : Support dynamic restoration of encrypted snapshots. #9313

IGNITE-14999v3 : Support dynamic restoration of encrypted snapshots. #9313

Uh oh!

Conversation

Vladsz83 commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Vladsz83 Sep 3, 2021 • 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!

timoninmaxim Sep 3, 2021 • 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!

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!

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!

Vladsz83 Sep 3, 2021 • 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!

Choose a reason for hiding this comment

Vladsz83 commented Aug 10, 2021 •

edited

Loading

Vladsz83 Sep 3, 2021 •

edited

Loading

timoninmaxim Sep 3, 2021 •

edited

Loading

Vladsz83 Sep 3, 2021 •

edited

Loading