HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -4504,13 +4504,11 @@ public void onConfigurationChange(Configuration newConf) {
     boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
       .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled,
+    // newConf and this.conf reference the same Configuration object, so it doesn't matter which
+    // one we update
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, originalIsReadOnlyEnabled,
       this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode,
-      this.toString(), conf -> {
-        this.initializeCoprocessorHost(conf);
-        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
-          CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
-      });
+      this.toString(), this::initializeCoprocessorHost);
 
     boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil
       .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -383,7 +383,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
   private MobFileCache mobFileCache;
   private final WAL wal;
   private final HRegionFileSystem fs;
-  protected final Configuration conf;
+  protected Configuration conf;
   private final Configuration baseConf;
   private final int rowLockWaitDuration;
   static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000;
@@ -492,6 +492,10 @@ public long getSmallestReadPoint() {
     }
   }
 
+  public Configuration getConfiguration() {
+    return this.conf;
+  }
+
   /*
    * Data structure of write state flags used coordinating flushes, compactions and closes.
    */
@@ -8991,13 +8995,18 @@ public void onConfigurationChange(Configuration newConf) {
     boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
       .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled,
-      this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(),
-      conf -> {
-        decorateRegionConfiguration(conf);
-        this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf);
-        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
-          CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
+    // HRegion's this.conf is a special Configuration type called CompoundConfiguration. This means
+    // we don't want to use the newConf provided in onConfigurationChange() for creating a new
+    // RegionCoprocessorHost. Instead, we update this.conf and use that for decorating the region
+    // config and updating this.coprocessorHost. Also, we do not need to explicitly update
+    // hbase.global.readonly.enabled in this.conf because the CompoundConfiguration holds a
+    // reference to HBaseServerBase's conf, which was updated automatically.
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf,
+      originalIsReadOnlyEnabled, this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
+      false, this.toString(), conf -> {
+        this.conf = new CompoundConfiguration().add(conf).addBytesMap(this.htableDescriptor.getValues());
+        decorateRegionConfiguration(this.conf);
+        this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, this.conf);
       });
 
     boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf);

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

@@ -3499,13 +3499,11 @@ public void onConfigurationChange(Configuration newConf) {
     boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
       .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled,
+    // newConf and this.conf reference the same Configuration object, so it doesn't matter which
+    // one we update
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, originalIsReadOnlyEnabled,
       this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(),
-      conf -> {
-        this.rsHost = new RegionServerCoprocessorHost(this, conf);
-        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
-          CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
-      });
+      conf -> this.rsHost = new RegionServerCoprocessorHost(this, conf));
   }
 
   @Override

hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java

@@ -39,6 +39,7 @@
 import org.apache.hbase.thirdparty.com.google.common.base.Strings;
 import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList;
 import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 
 /**
  * Helper class for coprocessor host when configuration changes.
@@ -211,26 +212,6 @@ public static boolean areReadOnlyCoprocessorsLoaded(Configuration conf,
     return allCoprocessors.containsAll(readOnlyCoprocessors);
   }
 
-  /**
-   * Takes an updated configuration and updates the coprocessors for that configuration key in the
-   * current configuration.
-   * @param currentConf        the configuration currently used by the master, region server, or
-   *                           region
-   * @param updatedConf        the updated version of the configuration whose coprocessors we want
-   *                           to copy
-   * @param coprocessorConfKey configuration key used for setting master, region server, or region
-   *                           coprocessors
-   */
-  public static void updateCoprocessorListInConf(Configuration currentConf,
-    Configuration updatedConf, String coprocessorConfKey) {
-    String[] updatedCoprocessorList = updatedConf.getStrings(coprocessorConfKey);
-    if (updatedCoprocessorList != null) {
-      currentConf.setStrings(coprocessorConfKey, updatedCoprocessorList);
-    } else {
-      currentConf.unset(coprocessorConfKey);
-    }
-  }
-
   /**
    * Gets the name of a component based on the provided coprocessor configuration key.
    * @param coprocessorConfKey configuration key used for setting master, region server, or region
@@ -253,6 +234,8 @@ public static String getComponentName(String coprocessorConfKey) {
    * configuration. If a change is detected, then new coprocessors are loaded using the provided
    * reload method. The new value for the read-only config variable is updated as well.
    * @param newConf                   an updated configuration
+   * @param confToUpdate              the actual configuration we want to update, which may or may
+   *                                  not be the same as {@code newConf}
    * @param originalIsReadOnlyEnabled the original value for
    *                                  {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
    * @param coprocessorHost           the coprocessor host for HMaster, HRegionServer, or HRegion
@@ -264,28 +247,44 @@ public static String getComponentName(String coprocessorConfKey) {
    * @param reloadTask                lambda function that reloads coprocessors on the master,
    *                                  region server, or region
    */
-  public static void maybeUpdateCoprocessors(Configuration newConf,
+  public static void maybeUpdateCoprocessors(Configuration newConf, Configuration confToUpdate,
     boolean originalIsReadOnlyEnabled, CoprocessorHost<?, ?> coprocessorHost,
     String coprocessorConfKey, boolean isMaintenanceMode, String instance,
     CoprocessorReloadTask reloadTask) {
 
-    boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf);
-    boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != maybeUpdatedReadOnlyMode;
+    String componentName = getComponentName(coprocessorConfKey);
+    boolean currentReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf);
+    boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != currentReadOnlyMode;
     boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil
       .checkConfigurationChange(coprocessorHost, newConf, coprocessorConfKey);
 
-    // update region server coprocessor if the configuration has changed.
     if ((hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !isMaintenanceMode) {
       LOG.info("Updating coprocessors for {} {} because the configuration has changed",
-        getComponentName(coprocessorConfKey), instance);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey);
-      reloadTask.reload(newConf);
+        componentName, instance);
+      if (hasReadOnlyModeChanged) {
+        confToUpdate.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode);
+        CoprocessorConfigurationUtil.syncReadOnlyConfigurations(confToUpdate, coprocessorConfKey);
+      }
+      reloadTask.reload(confToUpdate);
     }
 
     if (hasReadOnlyModeChanged) {
       LOG.info("Config {} has been dynamically changed to {} for {} {}",
-        HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode,
-        getComponentName(coprocessorConfKey), instance);
+        HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode, componentName, instance);
     }
   }
+
+  /**
+   * Similar to the other implementation of maybeUpdateCoprocessors(), except newConf is the conf
+   * being updated. This is useful for onConfigurationChange() in HMaster and HRegionServer, where
+   * their this.conf variable references the same object as the newConf arg in their respective
+   * onConfigurationChange() method.
+   */
+  public static void maybeUpdateCoprocessors(Configuration newConf,
+    boolean originalIsReadOnlyEnabled, CoprocessorHost<?, ?> coprocessorHost,
+    String coprocessorConfKey, boolean isMaintenanceMode, String instance,
+    CoprocessorReloadTask reloadTask) {
+    maybeUpdateCoprocessors(newConf, newConf, originalIsReadOnlyEnabled, coprocessorHost,
+      coprocessorConfKey, isMaintenanceMode, instance, reloadTask);
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java

@@ -159,11 +159,13 @@
 import org.apache.hadoop.hbase.regionserver.wal.WALUtil;
 import org.apache.hadoop.hbase.replication.regionserver.ReplicationObserver;
 import org.apache.hadoop.hbase.security.User;
+import org.apache.hadoop.hbase.security.access.RegionReadOnlyController;
 import org.apache.hadoop.hbase.test.MetricsAssertHelper;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.testclassification.VerySlowRegionServerTests;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.CommonFSUtils;
+import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManagerTestHelper;
 import org.apache.hadoop.hbase.util.HFileArchiveUtil;
@@ -7891,9 +7893,9 @@ public void testRegionOnCoprocessorsChange() throws IOException {
       NoOpRegionCoprocessor.class.getName());
     // trigger configuration change
     region.onConfigurationChange(newConf);
-    assertTrue(region.getCoprocessorHost() != null);
+    assertNotNull(region.getCoprocessorHost());
     Set<String> coprocessors = region.getCoprocessorHost().getCoprocessors();
-    assertTrue(coprocessors.size() == 2);
+    assertEquals(2, coprocessors.size());
     assertTrue(region.getCoprocessorHost().getCoprocessors()
       .contains(MetaTableMetrics.class.getSimpleName()));
     assertTrue(region.getCoprocessorHost().getCoprocessors()
@@ -7902,9 +7904,9 @@ public void testRegionOnCoprocessorsChange() throws IOException {
     // remove region coprocessor and keep only user region coprocessor
     newConf.unset(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
     region.onConfigurationChange(newConf);
-    assertTrue(region.getCoprocessorHost() != null);
+    assertNotNull(region.getCoprocessorHost());
     coprocessors = region.getCoprocessorHost().getCoprocessors();
-    assertTrue(coprocessors.size() == 1);
+    assertEquals(1, coprocessors.size());
     assertTrue(region.getCoprocessorHost().getCoprocessors()
       .contains(NoOpRegionCoprocessor.class.getSimpleName()));
   }
@@ -8006,4 +8008,39 @@ public void testHRegionInitializeFailsWithDeletedRegionDir() throws Exception {
       TEST_UTIL.cleanupTestDir();
     }
   }
+
+  /**
+   * Regression for HBASE-30180: a region can miss read-only coprocessors after repeated toggles if
+   * {@code maybeUpdateCoprocessors} syncs to the server configuration instead of the region's
+   * {@link org.apache.hadoop.hbase.CompoundConfiguration}.
+   */
+  @Test
+  public void testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle()
+    throws IOException {
+    byte[] cf1 = Bytes.toBytes("CF1");
+    Configuration serverConf = new Configuration(CONF);
+    serverConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false);
+
+    region = initHRegion(tableName, method, serverConf, new byte[][] { cf1 });
+    region.setCoprocessorHost(new RegionCoprocessorHost(region, null, region.getConfiguration()));
+
+    String regionCpKey = CoprocessorHost.REGION_COPROCESSOR_CONF_KEY;
+    boolean[] toggleSequence = new boolean[] { true, false, true, true, false };
+
+    for (boolean readOnlyEnabled : toggleSequence) {
+      Configuration newConf = new Configuration(serverConf);
+      newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyEnabled);
+      region.onConfigurationChange(newConf);
+
+      if (readOnlyEnabled) {
+        assertNotNull(
+          region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName()));
+      } else {
+        assertNull(
+          region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName()));
+      }
+      assertEquals(readOnlyEnabled, CoprocessorConfigurationUtil
+        .areReadOnlyCoprocessorsLoaded(region.getConfiguration(), regionCpKey));
+    }
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java

@@ -17,6 +17,7 @@
  */
 package org.apache.hadoop.hbase.security.access;
 
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -81,7 +82,7 @@ public void tearDown() throws Exception {
 
   private void setupMiniCluster(boolean isReadOnlyEnabled) throws Exception {
     conf = TEST_UTIL.getConfiguration();
-    conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled);
+    conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled);
     TEST_UTIL.startMiniCluster(1);
 
     master = TEST_UTIL.getMiniHBaseCluster().getMaster();