timoninmaxim commented on a change in pull request #9313:
URL: https://github.com/apache/ignite/pull/9313#discussion_r693773671
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/StoredCacheData.java
##########
@@ -118,6 +122,20 @@ public StoredCacheData sql(boolean sql) {
return this;
}
+ /**
+ * @return Chipered encryption key for this cache or cache group. {@code
Null} if not encrypted.
Review comment:
Here and below, replace "Chipered" with "Ciphered"
##########
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:
What is a purpose of this local var?
##########
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:
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.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -993,8 +1008,19 @@ public void cancelLocalSnapshotTask(String name) {
grps.stream().collect(Collectors.toMap(CU::cacheId, v ->
v));
for (List<SnapshotMetadata> nodeMetas : metas.values()) {
- for (SnapshotMetadata meta : nodeMetas)
+ for (SnapshotMetadata meta : nodeMetas) {
+ if (meta.masterKeyDigest() != null &&
!Arrays.equals(meta.masterKeyDigest(),
+
kctx0.config().getEncryptionSpi().masterKeyDigest())) {
Review comment:
It's possible to get NPE for
`kctx0.config().getEncryptionSpi().masterKeyDigest()` for config with disabled
encryption.
##########
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:
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.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -993,8 +1008,19 @@ public void cancelLocalSnapshotTask(String name) {
grps.stream().collect(Collectors.toMap(CU::cacheId, v ->
v));
for (List<SnapshotMetadata> nodeMetas : metas.values()) {
- for (SnapshotMetadata meta : nodeMetas)
+ for (SnapshotMetadata meta : nodeMetas) {
+ if (meta.masterKeyDigest() != null &&
!Arrays.equals(meta.masterKeyDigest(),
+
kctx0.config().getEncryptionSpi().masterKeyDigest())) {
Review comment:
`getEncryptionSpi().masterKeyDigest()` performs
`KeystoreEncryptionSpi#makeDigest` operation on every iteration for this loop.
Let's make a var for it before this loop.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadata.java
##########
@@ -58,6 +60,9 @@
@GridToStringInclude
private final List<Integer> grpIds;
+ /** Encrypted group ids. */
+ private Set<Integer> encrGrpIds;
Review comment:
Should it be marked with `GridToStringInclude`? The same question for
the `masterKeyDigest`
--
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]
