You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/09/01 09:23:29 UTC
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9313: IGNITE-14999v3
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org