dlmarion commented on code in PR #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r960956356
##########
test/src/main/java/org/apache/accumulo/test/performance/scan/CollectTabletStats.java:
##########
@@ -464,7 +464,7 @@ private static int readFiles(VolumeManager fs,
AccumuloConfiguration aconf,
for (TabletFile file : files) {
FileSystem ns = fs.getFileSystemByPath(file.getPath());
FileSKVIterator reader = FileOperations.getInstance().newReaderBuilder()
- .forFile(file.getPathStr(), ns, ns.getConf(),
CryptoServiceFactory.newDefaultInstance())
+ .forFile(file.getPathStr(), ns, ns.getConf(),
NoCryptoServiceFactory.NONE)
Review Comment:
I'm confused how this would work if the File is encrypted.
##########
test/src/main/java/org/apache/accumulo/test/performance/scan/CollectTabletStats.java:
##########
@@ -497,7 +497,7 @@ private static int readFilesUsingIterStack(VolumeManager
fs, ServerContext conte
for (TabletFile file : files) {
FileSystem ns = fs.getFileSystemByPath(file.getPath());
readers.add(FileOperations.getInstance().newReaderBuilder()
- .forFile(file.getPathStr(), ns, ns.getConf(),
CryptoServiceFactory.newDefaultInstance())
+ .forFile(file.getPathStr(), ns, ns.getConf(),
NoCryptoServiceFactory.NONE)
Review Comment:
I'm confused how this would work if the File is encrypted.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -384,11 +384,11 @@ MetadataTime computeRootTabletTime(ServerContext context,
Collection<String> goo
Path path = new Path(good);
FileSystem ns = context.getVolumeManager().getFileSystemByPath(path);
+ var tableConf = context.getTableConfiguration(RootTable.ID);
long maxTime = -1;
try (FileSKVIterator reader =
FileOperations.getInstance().newReaderBuilder()
- .forFile(path.toString(), ns, ns.getConf(),
context.getCryptoService())
-
.withTableConfiguration(context.getTableConfiguration(RootTable.ID)).seekToBeginning()
- .build()) {
+ .forFile(path.toString(), ns, ns.getConf(),
tableConf.getCryptoService())
Review Comment:
Should NoCryptoService be used in the upgrade from version 9 to 10?
##########
server/base/src/main/java/org/apache/accumulo/server/util/FileUtil.java:
##########
@@ -522,15 +506,15 @@ public static Map<TabletFile,FileInfo>
tryToGetFirstAndLastRows(ServerContext co
}
public static WritableComparable<Key> findLastKey(ServerContext context,
- Collection<TabletFile> mapFiles) throws IOException {
+ TableConfiguration tableConf, Collection<TabletFile> mapFiles) throws
IOException {
Key lastKey = null;
for (TabletFile file : mapFiles) {
FileSystem ns =
context.getVolumeManager().getFileSystemByPath(file.getPath());
FileSKVIterator reader = FileOperations.getInstance().newReaderBuilder()
- .forFile(file.getPathStr(), ns, ns.getConf(),
context.getCryptoService())
-
.withTableConfiguration(context.getConfiguration()).seekToBeginning().build();
+ .forFile(file.getPathStr(), ns, ns.getConf(),
tableConf.getCryptoService())
+ .withTableConfiguration(tableConf).seekToBeginning().build();
Review Comment:
I wonder if `FileOperations` should change, something like:
```
FileOperations.getInstance().newReaderBuilder().table(TableConfiguration)
.file(String, FileSystem, Configuration);
```
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/PerTableCryptoServiceFactory.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.crypto;
+
+import static java.util.Objects.requireNonNull;
+import static
org.apache.accumulo.core.conf.Property.GENERAL_ARBITRARY_PROP_PREFIX;
+import static org.apache.accumulo.core.conf.Property.TABLE_CRYPTO_PREFIX;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.accumulo.core.data.TableId;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A factory that loads a CryptoService based on {@link TableId}.
+ */
+public class PerTableCryptoServiceFactory implements CryptoServiceFactory {
+ private static final Logger log =
LoggerFactory.getLogger(PerTableCryptoServiceFactory.class);
+ private final ConcurrentHashMap<TableId,CryptoService> cryptoServiceMap =
+ new ConcurrentHashMap<>();
+
+ public static final String WAL_NAME_PROP = GENERAL_ARBITRARY_PROP_PREFIX +
"crypto.wal.service";
+ public static final String RECOVERY_NAME_PROP =
+ GENERAL_ARBITRARY_PROP_PREFIX + "crypto.recovery.service";
+ public static final String TABLE_SERVICE_NAME_PROP = TABLE_CRYPTO_PREFIX +
"service";
+ // The WALs do not have table IDs so these fake IDs are used for caching
+ private static final TableId WAL_FAKE_ID =
TableId.of("WAL_CryptoService_FAKE_ID");
+ private static final TableId REC_FAKE_ID =
TableId.of("RECOVERY_CryptoService_FAKE_ID");
+
+ @Override
+ public CryptoService getService(CryptoEnvironment environment,
Map<String,String> props) {
+ if (environment.getScope() == CryptoEnvironment.Scope.WAL) {
+ return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) ->
getWALServiceInitialized(props));
+ } else if (environment.getScope() == CryptoEnvironment.Scope.RECOVERY) {
+ return cryptoServiceMap.computeIfAbsent(REC_FAKE_ID,
+ (id) -> getRecoveryServiceInitialized(props));
+ } else {
+ if (environment.getTableId().isEmpty()) {
+ log.debug("No tableId present in crypto env: " + environment);
+ return NoCryptoServiceFactory.NONE;
+ }
+ TableId tableId = environment.getTableId().get();
+ if (props == null || props.isEmpty() ||
props.get(TABLE_SERVICE_NAME_PROP) == null) {
+ return NoCryptoServiceFactory.NONE;
+ }
+ if (environment.getScope() == CryptoEnvironment.Scope.TABLE) {
+ return cryptoServiceMap.computeIfAbsent(tableId,
+ (id) -> getTableServiceInitialized(tableId, props));
+ }
+ }
+ throw new IllegalStateException("Invalid config for crypto " + environment
+ " " + props);
+ }
+
+ private CryptoService getWALServiceInitialized(Map<String,String> props) {
+ String name = requireNonNull(props.get(WAL_NAME_PROP),
+ "The property " + WAL_NAME_PROP + " is required for encrypting WALs.");
+ log.debug("New CryptoService for WAL scope {}={}", WAL_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ private CryptoService getRecoveryServiceInitialized(Map<String,String>
props) {
+ String name = requireNonNull(props.get(RECOVERY_NAME_PROP),
+ "The property " + RECOVERY_NAME_PROP + " is required for encrypting
during recovery.");
+ log.debug("New CryptoService for Recovery scope {}={}",
RECOVERY_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ private CryptoService getTableServiceInitialized(TableId tableId,
Map<String,String> props) {
+ String name = requireNonNull(props.get(TABLE_SERVICE_NAME_PROP),
+ "The property " + TABLE_SERVICE_NAME_PROP + " is required for
encrypting tables.");
+ log.debug("New CryptoService for TABLE({}) {}={}", tableId,
TABLE_SERVICE_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ /**
+ * Loads a crypto service based on the name provided.
+ */
+ private CryptoService newCryptoService(String cryptoServiceName) {
+ CryptoService cs;
+ try {
+ cs =
PerTableCryptoServiceFactory.class.getClassLoader().loadClass(cryptoServiceName)
+
.asSubclass(CryptoService.class).getDeclaredConstructor().newInstance();
+ } catch (ClassNotFoundException | IllegalAccessException |
InvocationTargetException
Review Comment:
can replace all of these with `ReflectiveOperationException`
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/NoCryptoServiceFactory.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.crypto;
+
+import java.util.Map;
+
+public class NoCryptoServiceFactory implements CryptoServiceFactory {
+ public static final CryptoService NONE = new NoCryptoService();
Review Comment:
I wonder if `NONE` should be renamed to `SERVICE`. There are a bunch of
references to `NoCryptoServiceFactory.NONE` and it seems redundant.
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/GenericCryptoServiceFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.crypto;
+
+import static
org.apache.accumulo.core.conf.Property.GENERAL_ARBITRARY_PROP_PREFIX;
+import static org.apache.accumulo.core.conf.Property.TABLE_CRYPTO_PREFIX;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.Map;
+
+/**
+ * Factory that will load a crypto service configured, first checking
+ * {@link #GENERAL_SERVICE_NAME_PROP} and then {@link
#TABLE_SERVICE_NAME_PROP}. Useful for general
+ * purpose on disk encryption, with no Table context.
+ */
+public class GenericCryptoServiceFactory implements CryptoServiceFactory {
+ public static final String GENERAL_SERVICE_NAME_PROP =
+ GENERAL_ARBITRARY_PROP_PREFIX + "crypto.service";
+ public static final String TABLE_SERVICE_NAME_PROP = TABLE_CRYPTO_PREFIX +
"service";
+
+ @Override
+ public CryptoService getService(CryptoEnvironment environment,
Map<String,String> properties) {
+ if (properties == null || properties.isEmpty()) {
+ return NoCryptoServiceFactory.NONE;
+ }
+
+ String cryptoServiceName = properties.get(GENERAL_SERVICE_NAME_PROP);
+ if (cryptoServiceName == null) {
+ cryptoServiceName = properties.get(TABLE_SERVICE_NAME_PROP);
+ if (cryptoServiceName == null) {
+ return NoCryptoServiceFactory.NONE;
+ }
+ }
+ var cs = newCryptoService(cryptoServiceName);
+ cs.init(properties);
+ return cs;
+ }
+
+ /**
+ * Loads a crypto service based on the name provided
+ */
+ private CryptoService newCryptoService(String cryptoServiceName) {
+ CryptoService cs;
+ try {
+ cs =
GenericCryptoServiceFactory.class.getClassLoader().loadClass(cryptoServiceName)
Review Comment:
See PerTableCryptoServiceFactory for comments, they apply here too. Maybe a
default method is better on the interface?
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/PerTableCryptoServiceFactory.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.crypto;
+
+import static java.util.Objects.requireNonNull;
+import static
org.apache.accumulo.core.conf.Property.GENERAL_ARBITRARY_PROP_PREFIX;
+import static org.apache.accumulo.core.conf.Property.TABLE_CRYPTO_PREFIX;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.accumulo.core.data.TableId;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A factory that loads a CryptoService based on {@link TableId}.
+ */
+public class PerTableCryptoServiceFactory implements CryptoServiceFactory {
+ private static final Logger log =
LoggerFactory.getLogger(PerTableCryptoServiceFactory.class);
+ private final ConcurrentHashMap<TableId,CryptoService> cryptoServiceMap =
+ new ConcurrentHashMap<>();
+
+ public static final String WAL_NAME_PROP = GENERAL_ARBITRARY_PROP_PREFIX +
"crypto.wal.service";
+ public static final String RECOVERY_NAME_PROP =
+ GENERAL_ARBITRARY_PROP_PREFIX + "crypto.recovery.service";
+ public static final String TABLE_SERVICE_NAME_PROP = TABLE_CRYPTO_PREFIX +
"service";
+ // The WALs do not have table IDs so these fake IDs are used for caching
+ private static final TableId WAL_FAKE_ID =
TableId.of("WAL_CryptoService_FAKE_ID");
+ private static final TableId REC_FAKE_ID =
TableId.of("RECOVERY_CryptoService_FAKE_ID");
+
+ @Override
+ public CryptoService getService(CryptoEnvironment environment,
Map<String,String> props) {
+ if (environment.getScope() == CryptoEnvironment.Scope.WAL) {
+ return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) ->
getWALServiceInitialized(props));
+ } else if (environment.getScope() == CryptoEnvironment.Scope.RECOVERY) {
+ return cryptoServiceMap.computeIfAbsent(REC_FAKE_ID,
+ (id) -> getRecoveryServiceInitialized(props));
+ } else {
+ if (environment.getTableId().isEmpty()) {
+ log.debug("No tableId present in crypto env: " + environment);
+ return NoCryptoServiceFactory.NONE;
+ }
+ TableId tableId = environment.getTableId().get();
+ if (props == null || props.isEmpty() ||
props.get(TABLE_SERVICE_NAME_PROP) == null) {
+ return NoCryptoServiceFactory.NONE;
+ }
+ if (environment.getScope() == CryptoEnvironment.Scope.TABLE) {
+ return cryptoServiceMap.computeIfAbsent(tableId,
+ (id) -> getTableServiceInitialized(tableId, props));
+ }
+ }
+ throw new IllegalStateException("Invalid config for crypto " + environment
+ " " + props);
+ }
+
+ private CryptoService getWALServiceInitialized(Map<String,String> props) {
+ String name = requireNonNull(props.get(WAL_NAME_PROP),
+ "The property " + WAL_NAME_PROP + " is required for encrypting WALs.");
+ log.debug("New CryptoService for WAL scope {}={}", WAL_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ private CryptoService getRecoveryServiceInitialized(Map<String,String>
props) {
+ String name = requireNonNull(props.get(RECOVERY_NAME_PROP),
+ "The property " + RECOVERY_NAME_PROP + " is required for encrypting
during recovery.");
+ log.debug("New CryptoService for Recovery scope {}={}",
RECOVERY_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ private CryptoService getTableServiceInitialized(TableId tableId,
Map<String,String> props) {
+ String name = requireNonNull(props.get(TABLE_SERVICE_NAME_PROP),
+ "The property " + TABLE_SERVICE_NAME_PROP + " is required for
encrypting tables.");
+ log.debug("New CryptoService for TABLE({}) {}={}", tableId,
TABLE_SERVICE_NAME_PROP, name);
+ CryptoService cs = newCryptoService(name);
+ cs.init(props);
+ return cs;
+ }
+
+ /**
+ * Loads a crypto service based on the name provided.
+ */
+ private CryptoService newCryptoService(String cryptoServiceName) {
+ CryptoService cs;
+ try {
+ cs =
PerTableCryptoServiceFactory.class.getClassLoader().loadClass(cryptoServiceName)
Review Comment:
```suggestion
return ClassLoaderUtil.loadClass(null, cryptoServiceName,
CryptoService.class).getDeclaredConstructor().newInstance();
```
--
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-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
