[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678459279



##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -176,22 +176,6 @@
       "One-line configuration property controlling the network locations "
           + "(hostnames) that are allowed to impersonate other users",
       "1.7.1"),
-  // Crypto-related properties
-  @Experimental
-  INSTANCE_CRYPTO_PREFIX("instance.crypto.opts.", null, PropertyType.PREFIX,
-      "Properties related to on-disk file encryption.", "2.0.0"),
-  @Experimental
-  @Sensitive
-  INSTANCE_CRYPTO_SENSITIVE_PREFIX("instance.crypto.opts.sensitive.", null, PropertyType.PREFIX,
-      "Sensitive properties related to on-disk file encryption.", "2.0.0"),
-  @Experimental
-  INSTANCE_CRYPTO_SERVICE("instance.crypto.service",
-      "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
-      "The class which executes on-disk file encryption. The default does nothing. To enable "
-          + "encryption, replace this classname with an implementation of the"
-          + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
-      "2.0.0"),
-

Review comment:
       Part of the benefit of having crypto properties as `instance.` properties ensures that all tservers are using the same config. The potential for different tservers to have different crypto services means that data written by one tserver might not be able to be read by another, as they could have wildly different behaviors. This is one reason why I think we should keep an `instance.` property to refer to the crypto service factory, which is the overall module that supports all the different crypto that will be needed, and that per-tserver and per-table properties should only modify the behavior of the crypto services produced by this factory with some options.

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
   TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
       PropertyType.PREFIX,
       "Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+  // Crypto-related properties
+  @Experimental
+  TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+      "Properties related to on-disk file encryption.", "2.1.0"),
+  @Experimental
+  @Sensitive
+  TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+      "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+  @Experimental
+  TABLE_CRYPTO_SERVICE("table.crypto.service",
+      "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+      "The class which executes on-disk table encryption/decryption. The default does "
+          + "nothing. This property must be a classname with an implementation of the "
+          + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+      "2.1.0"),

Review comment:
       If there were a crypto service factory specified as an `instance.` option, then there's no need to specify a specific crypto service here, as that would be determined by the factory, given the TABLE scope and the options (either the options in the config, in the case of writing a new file, or the options read from the file in the case of reading a previously written file).

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -474,6 +458,21 @@
       PropertyType.TIMEDURATION,
       "The maximum amount of time to wait after a failure to create or write a write-ahead log.",
       "1.7.1"),
+  // Crypto-related properties
+  @Experimental
+  TSERV_WAL_CRYPTO_PREFIX("tserver.wal.crypto.opts.", null, PropertyType.PREFIX,
+      "Properties related to on-disk file encryption.", "2.1.0"),
+  @Experimental
+  @Sensitive
+  TSERV_WAL_CRYPTO_SENSITIVE_PREFIX("tserver.wal.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+      "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+  @Experimental
+  TSERV_WAL_CRYPTO_SERVICE("tserver.wal.crypto.service",
+      "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+      "The class which executes on-disk write ahead log encryption/decryption. The default does "
+          + "nothing. This property must be a classname with an implementation of the "
+          + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+      "2.1.0"),

Review comment:
       I would expect all tservers to use the same WAL options, so that files written by one tserver will be able to be read by another. This implies the use of `instance.` properties for WAL crypto options, not `tserver.` Also, if a crypto service factory is specified in an `instance.` property, then there's no need to specify a specific crypto service here, as the factory would make that decision based on the options and scope.




-- 
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@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org