timoninmaxim commented on a change in pull request #9313:
URL: https://github.com/apache/ignite/pull/9313#discussion_r701714269
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/managers/encryption/GridEncryptionManager.java
##########
@@ -1517,6 +1525,12 @@ public void
applyReencryptionStartRecord(ReencryptionStartRecord rec) {
"The previous change was not completed."));
}
+ if (ctx.cache().context().snapshotMgr().isSnapshotCreating()
Review comment:
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:
1. SnapshotManager tries to acquire the lock on master key, then wait for
lock if other process own it.
2. 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?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java
##########
@@ -170,6 +178,54 @@ public boolean sameSnapshot(SnapshotMetadata compare) {
Objects.equals(baselineNodes(), compare.baselineNodes());
}
+ /**
+ * @param grpId Cache id or cache group id.
+ * @return {@code True} if cache group is encrypted. {@code False}
otherwise.
+ */
+ public boolean isCacheGroupEncrypted(int grpId) {
+ Set<Integer> encrGrpIds = this.encrGrpIds;
Review comment:
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.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java
##########
@@ -170,6 +178,54 @@ public boolean sameSnapshot(SnapshotMetadata compare) {
Objects.equals(baselineNodes(), compare.baselineNodes());
}
+ /**
+ * @param grpId Cache id or cache group id.
+ * @return {@code True} if cache group is encrypted. {@code False}
otherwise.
+ */
+ public boolean isCacheGroupEncrypted(int grpId) {
+ Set<Integer> encrGrpIds = this.encrGrpIds;
+
+ return encrGrpIds != null && encrGrpIds.contains(grpId);
+ }
+
+ /**
+ * @param masterKeyDigest Master key digest for encrypted caches.
+ * @return this meta.
+ */
+ public SnapshotMetadata masterKeyDigest(@Nullable byte[] masterKeyDigest) {
+ this.masterKeyDigest = masterKeyDigest == null ? null :
masterKeyDigest.clone();
+
+ return this;
+ }
+
+ /**
+ * @return Master key digest for encrypted caches.
+ */
+ public byte[] masterKeyDigest() {
+ byte[] masterKeyDigest = this.masterKeyDigest;
+
+ return masterKeyDigest == null ? null : masterKeyDigest.clone();
+ }
+
+ /**
+ * Stores ids of encrypted cache groups.
+ *
+ * @return this meta.
+ */
+ public SnapshotMetadata encrGrpIds(Collection<Integer> encrGrpIds) {
+ // Might be null even if final due to deserialization of previous
version the object;
+ if (this.encrGrpIds == null) {
+ synchronized (this) {
+ if (this.encrGrpIds == null)
+ this.encrGrpIds = new HashSet<>();
+ }
+ }
+
+ this.encrGrpIds.addAll(encrGrpIds);
Review comment:
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.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java
##########
@@ -3778,43 +3780,50 @@ private void checkReadOnlyState(
GridPlainClosure2<Collection<byte[]>, byte[],
IgniteInternalFuture<Boolean>> startCacheClsr =
(grpKeys, masterKeyDigest) -> {
- List<DynamicCacheChangeRequest> srvReqs = null;
Review comment:
Could you please revert tabulation of this block? It's pretty hard to
understand what is exactly changed.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -597,6 +606,12 @@ public File snapshotTmpDir() {
"prior to snapshot operation start: " + leftNodes));
}
+ if (!cctx.localNode().isClient() &&
cctx.kernalContext().encryption().isMasterKeyChangeInProgress()
Review comment:
> 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`?
--
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]
