virajjasani commented on code in PR #7584:
URL: https://github.com/apache/hbase/pull/7584#discussion_r2662767775
##########
hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto:
##########
@@ -420,4 +420,13 @@ service AdminService {
rpc GetCachedFilesList(GetCachedFilesListRequest)
returns(GetCachedFilesListResponse);
+
+ rpc RefreshSystemKeyCache(EmptyMsg)
+ returns(EmptyMsg);
+
+ rpc EjectManagedKeyDataCacheEntry(ManagedKeyEntryRequest)
+ returns(BooleanMsg);
+
+ rpc ClearManagedKeyDataCache(EmptyMsg)
+ returns(EmptyMsg);
Review Comment:
Are we expecting more apis in future?
We might want to consider having ManagedKeyAdmin interface?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -7592,37 +7654,60 @@ public String toString() {
}
// Utility methods
+ @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.UNITTEST)
+ public static HRegion newHRegion(Path tableDir, WAL wal, FileSystem fs,
Configuration conf,
+ RegionInfo regionInfo, final TableDescriptor htd, RegionServerServices
rsServices) {
+ return newHRegion(tableDir, wal, fs, conf, regionInfo, htd, rsServices,
null);
+ }
+
/**
* A utility method to create new instances of HRegion based on the {@link
HConstants#REGION_IMPL}
* configuration property.
- * @param tableDir qualified path of directory where region should be
located, usually the table
- * directory.
- * @param wal The WAL is the outbound log for any updates to the
HRegion The wal file is a
- * logfile from the previous execution that's
custom-computed for this HRegion.
- * The HRegionServer computes and sorts the appropriate
wal info for this
- * HRegion. If there is a previous file (implying that the
HRegion has been
- * written-to before), then read it from the supplied path.
- * @param fs is the filesystem.
- * @param conf is global configuration settings.
- * @param regionInfo - RegionInfo that describes the region is new), then
read them from the
- * supplied path.
- * @param htd the table descriptor
+ * @param tableDir qualified path of directory where region
should be located, usually
+ * the table directory.
+ * @param wal The WAL is the outbound log for any updates
to the HRegion The wal
+ * file is a logfile from the previous execution
that's
+ * custom-computed for this HRegion. The
HRegionServer computes and
+ * sorts the appropriate wal info for this
HRegion. If there is a
+ * previous file (implying that the HRegion has
been written-to
+ * before), then read it from the supplied path.
+ * @param fs is the filesystem.
+ * @param conf is global configuration settings.
+ * @param regionInfo - RegionInfo that describes the region is
new), then read them from
+ * the supplied path.
+ * @param htd the table descriptor
+ * @param keyManagementService reference to {@link KeyManagementService} or
null
* @return the new instance
*/
public static HRegion newHRegion(Path tableDir, WAL wal, FileSystem fs,
Configuration conf,
- RegionInfo regionInfo, final TableDescriptor htd, RegionServerServices
rsServices) {
+ RegionInfo regionInfo, final TableDescriptor htd, RegionServerServices
rsServices,
+ final KeyManagementService keyManagementService) {
+ List<Class<?>> ctorArgTypes =
+ Arrays.asList(Path.class, WAL.class, FileSystem.class,
Configuration.class, RegionInfo.class,
+ TableDescriptor.class, RegionServerServices.class,
KeyManagementService.class);
+ List<Object> ctorArgs =
+ Arrays.asList(tableDir, wal, fs, conf, regionInfo, htd, rsServices,
keyManagementService);
+
+ try {
+ return createInstance(conf, ctorArgTypes, ctorArgs);
+ } catch (Throwable e) {
+ // Try the old signature for the sake of test code.
+ return createInstance(conf, ctorArgTypes.subList(0, ctorArgTypes.size()
- 1),
+ ctorArgs.subList(0, ctorArgs.size() - 1));
+ }
Review Comment:
Is this fallback necessary? We can catching any type of Throwable here. Or
is it going to be a lot of changes in tests to use new constructor?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/SecurityUtil.java:
##########
@@ -17,15 +17,33 @@
*/
package org.apache.hadoop.hbase.security;
+import java.io.IOException;
+import java.security.Key;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.io.crypto.Cipher;
+import org.apache.hadoop.hbase.io.crypto.Encryption;
+import org.apache.hadoop.hbase.io.hfile.FixedFileTrailer;
+import org.apache.hadoop.hbase.keymeta.ManagedKeyDataCache;
+import org.apache.hadoop.hbase.keymeta.SystemKeyCache;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.yetus.audience.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Security related generic utility methods.
*/
@InterfaceAudience.Private
@InterfaceStability.Evolving
-public class SecurityUtil {
+public final class SecurityUtil {
+ private static final Logger LOG =
LoggerFactory.getLogger(SecurityUtil.class);
Review Comment:
nit: do we need this later?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/ManagedKeyData.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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
+ *
+ * http://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.hadoop.hbase.io.crypto;
+
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * STUB IMPLEMENTATION - Feature not yet complete. This class represents
encryption key data. Full
+ * implementation will be in HBASE-29368 feature PR.
+ */
[email protected]
Review Comment:
Are we expecting downstreamers to directly use ManagerKeyData class? If no,
we should keep IA.Private. If yes, we should still add IS.Unstable with
IA.Public for now.
These are not final consumable APIs anyway so we can't only keep IA.Public.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Encryption.java:
##########
@@ -490,36 +499,37 @@ public static void decryptWithSubjectKey(OutputStream
out, InputStream in, int o
if (key == null) {
throw new IOException("No key found for subject '" + subject + "'");
}
- Decryptor d = cipher.getDecryptor();
- d.setKey(key);
- d.setIv(iv); // can be null
try {
- decrypt(out, in, outLen, d);
+ decryptWithGivenKey(key, out, in, outLen, cipher, iv);
} catch (IOException e) {
// If the current cipher algorithm fails to unwrap, try the alternate
cipher algorithm, if one
// is configured
String alternateAlgorithm =
conf.get(HConstants.CRYPTO_ALTERNATE_KEY_ALGORITHM_CONF_KEY);
if (alternateAlgorithm != null) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Unable to decrypt data with current cipher algorithm '"
- + conf.get(HConstants.CRYPTO_KEY_ALGORITHM_CONF_KEY,
HConstants.CIPHER_AES)
- + "'. Trying with the alternate cipher algorithm '" +
alternateAlgorithm
- + "' configured.");
- }
+ LOG.debug(
+ "Unable to decrypt data with current cipher algorithm '{}'. "
+ + "Trying with the alternate cipher algorithm '{}' configured.",
+ conf.get(HConstants.CRYPTO_KEY_ALGORITHM_CONF_KEY,
HConstants.CIPHER_AES),
+ alternateAlgorithm);
Cipher alterCipher = Encryption.getCipher(conf, alternateAlgorithm);
if (alterCipher == null) {
throw new RuntimeException("Cipher '" + alternateAlgorithm + "' not
available");
}
- d = alterCipher.getDecryptor();
- d.setKey(key);
- d.setIv(iv); // can be null
- decrypt(out, in, outLen, d);
+ decryptWithGivenKey(key, out, in, outLen, alterCipher, iv);
} else {
- throw new IOException(e);
+ throw e;
Review Comment:
don't we want to retain `throw new IOException(e)`? unless this is easier to
catch later
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/keymeta/KeymetaAdmin.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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
+ *
+ * http://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.hadoop.hbase.keymeta;
+
+import java.io.IOException;
+import java.security.KeyException;
+import java.util.List;
+import org.apache.hadoop.hbase.io.crypto.ManagedKeyData;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * KeymetaAdmin is an interface for administrative functions related to
managed keys. It handles the
+ * following methods:
+ */
[email protected]
Review Comment:
Same here, applies to all new interfaces and apis
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/Bytes.java:
##########
@@ -1694,10 +1694,23 @@ public static byte[] add(final byte[] a, final byte[]
b) {
* @return New array made from a, b and c
*/
public static byte[] add(final byte[] a, final byte[] b, final byte[] c) {
- byte[] result = new byte[a.length + b.length + c.length];
+ return add(a, b, c, EMPTY_BYTE_ARRAY);
+ }
+
+ /**
+ * Concatenate byte arrays.
+ * @param a first fourth
+ * @param b second fourth
+ * @param c third fourth
+ * @param d fourth fourth
+ * @return New array made from a, b, c, and d
+ */
+ public static byte[] add(final byte[] a, final byte[] b, final byte[] c,
final byte[] d) {
Review Comment:
Instead of adding more byte[], how about we let core logic to keep using
existing utilities? e.g. if we have to concatenate 4 byte[] (a, b, c, d), we
can first concatenate c + d, and then concatenate (a + b + result of c+d)?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -7848,19 +8002,15 @@ public NavigableMap<byte[], Integer>
getReplicationScope() {
* @param reporter An interface we can report progress against.
* @return new HRegion
*/
+ @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.UNITTEST)
public static HRegion openHRegion(final HRegion other, final
CancelableProgressable reporter)
throws IOException {
HRegionFileSystem regionFs = other.getRegionFileSystem();
HRegion r = newHRegion(regionFs.getTableDir(), other.getWAL(),
regionFs.getFileSystem(),
- other.baseConf, other.getRegionInfo(), other.getTableDescriptor(), null);
+ other.baseConf, other.getRegionInfo(), other.getTableDescriptor(), null,
null);
return r.openHRegion(reporter);
}
- public static Region openHRegion(final Region other, final
CancelableProgressable reporter)
- throws IOException {
- return openHRegion((HRegion) other, reporter);
- }
Review Comment:
While HRegion is IA.Private, downstreamers with coproc still tend to use
some utilities. It would be better to keep this. It is more generic Region
instead of HRegion anyway.
--
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]
