From 480bb711d85e12d09b571694a64f7bb16abbca98 Mon Sep 17 00:00:00 2001 From: Hari Dara Date: Sun, 14 Jun 2026 21:15:45 +0530 Subject: [PATCH] HBASE-30222 Avoid re-parsing HBase configuration on every CipherProvider construction DefaultCipherProvider and CryptoCipherProvider initialized their conf field inline with HBaseConfiguration.create(), parsing hbase-default.xml and hbase-site.xml on every construction. The value was immediately discarded: Encryption.getCipherProvider() builds the provider via ReflectionUtils.newInstance(), which runs the no-arg constructor and then calls setConf(), overwriting the field. The eager parse was pure waste. Drop the inline initializer so conf is set only via setConf(), and add a fail-fast Preconditions guard in getConf() so a caller that forgets setConf() fails loudly instead of NPE-ing deep inside cipher construction. Mark the test-only getInstance() accessor as LimitedPrivate(UNITTEST). Add guard tests covering both providers. Generated-by: Claude Code (Opus 4.8) --- .../hbase/io/crypto/CryptoCipherProvider.java | 9 +++++++-- .../hbase/io/crypto/DefaultCipherProvider.java | 9 +++++++-- .../hbase/io/crypto/TestCipherProvider.java | 17 +++++++++++++++++ 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/CryptoCipherProvider.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/CryptoCipherProvider.java index 7f5c58883f2d..c08208aa270c 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/CryptoCipherProvider.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/CryptoCipherProvider.java @@ -18,10 +18,12 @@ package org.apache.hadoop.hbase.io.crypto; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.hbase.HBaseConfiguration; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.io.crypto.aes.CommonsCryptoAES; import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; + /** * The default cipher provider. Supports AES via the Commons Crypto. */ @@ -30,6 +32,7 @@ public final class CryptoCipherProvider implements CipherProvider { private static CryptoCipherProvider instance; + @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.UNITTEST) @edu.umd.cs.findbugs.annotations.SuppressWarnings(value = "MS_EXPOSE_REP", justification = "singleton pattern") public static CryptoCipherProvider getInstance() { @@ -40,7 +43,7 @@ public static CryptoCipherProvider getInstance() { return instance; } - private Configuration conf = HBaseConfiguration.create(); + private Configuration conf; // set via setConf() before use // Prevent instantiation private CryptoCipherProvider() { @@ -48,6 +51,8 @@ private CryptoCipherProvider() { @Override public Configuration getConf() { + Preconditions.checkState(conf != null, + "Configuration not set; call setConf() before using this provider"); return conf; } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/DefaultCipherProvider.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/DefaultCipherProvider.java index a4c3f3b7ca8b..ba3fb7236557 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/DefaultCipherProvider.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/DefaultCipherProvider.java @@ -18,10 +18,12 @@ package org.apache.hadoop.hbase.io.crypto; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.hbase.HBaseConfiguration; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.io.crypto.aes.AES; import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; + /** * The default cipher provider. Supports AES via the JCE. */ @@ -30,6 +32,7 @@ public final class DefaultCipherProvider implements CipherProvider { private static DefaultCipherProvider instance; + @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.UNITTEST) @edu.umd.cs.findbugs.annotations.SuppressWarnings(value = "MS_EXPOSE_REP", justification = "singleton pattern") public static DefaultCipherProvider getInstance() { @@ -40,7 +43,7 @@ public static DefaultCipherProvider getInstance() { return instance; } - private Configuration conf = HBaseConfiguration.create(); + private Configuration conf; // set via setConf() before use // Prevent instantiation private DefaultCipherProvider() { @@ -48,6 +51,8 @@ private DefaultCipherProvider() { @Override public Configuration getConf() { + Preconditions.checkState(conf != null, + "Configuration not set; call setConf() before using this provider"); return conf; } diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/TestCipherProvider.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/TestCipherProvider.java index 67e07b538b4c..98038bd9a217 100644 --- a/hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/TestCipherProvider.java +++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/TestCipherProvider.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; @@ -153,4 +154,20 @@ public void testDefaultProvider() { assertEquals(AES.KEY_LENGTH, a.getKeyLength()); } + @Test + public void testDefaultProviderGetConfWithoutConf() { + // The singleton no longer eagerly parses a Configuration at construction; getConf() must fail + // loudly when called before setConf() rather than NPE-ing deep inside cipher construction. + DefaultCipherProvider provider = DefaultCipherProvider.getInstance(); + provider.setConf(null); + assertThrows(IllegalStateException.class, provider::getConf); + } + + @Test + public void testCryptoProviderGetConfWithoutConf() { + CryptoCipherProvider provider = CryptoCipherProvider.getInstance(); + provider.setConf(null); + assertThrows(IllegalStateException.class, provider::getConf); + } + }