From 52c17b6169033f2bba23aab6024f50b656980bce Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Tue, 16 Dec 2025 20:08:28 +0530 Subject: [PATCH 1/4] HBASE-29779: Call super coprocessor instead of returning for system tables --- .../security/access/ReadOnlyController.java | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java index 3ec772798be2..c4a44e8d55f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java @@ -106,6 +106,10 @@ private void internalReadOnlyGuard() throws DoNotRetryIOException { } } + private boolean isOperationOnNonMetaTable(final ObserverContext c){ + return !c.getEnvironment().getRegionInfo().getTable().equals(TableName.META_TABLE_NAME); + } + @Override public void start(CoprocessorEnvironment env) throws IOException { if (env instanceof MasterCoprocessorEnvironment) { @@ -133,7 +137,9 @@ public Optional getRegionObserver() { @Override public void preFlush(final ObserverContext c, FlushLifeCycleTracker tracker) throws IOException { - internalReadOnlyGuard(); + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); + } RegionObserver.super.preFlush(c, tracker); } @@ -201,53 +207,45 @@ public InternalScanner preCompact(ObserverContext c, Put put, WALEdit edit, Durability durability) throws IOException { - TableName tableName = c.getEnvironment().getRegionInfo().getTable(); - if (tableName.isSystemTable()) { - return; + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); } - internalReadOnlyGuard(); RegionObserver.super.prePut(c, put, edit, durability); } @Override public void prePut(ObserverContext c, Put put, WALEdit edit) throws IOException { - TableName tableName = c.getEnvironment().getRegionInfo().getTable(); - if (tableName.isSystemTable()) { - return; + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); } - internalReadOnlyGuard(); RegionObserver.super.prePut(c, put, edit); } @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit, Durability durability) throws IOException { - if (c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { - return; + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); } - internalReadOnlyGuard(); RegionObserver.super.preDelete(c, delete, edit, durability); } @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit) throws IOException { - if (c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { - return; + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); } - internalReadOnlyGuard(); RegionObserver.super.preDelete(c, delete, edit); } @Override public void preBatchMutate(ObserverContext c, MiniBatchOperationInProgress miniBatchOp) throws IOException { - TableName tableName = c.getEnvironment().getRegionInfo().getTable(); - if (tableName.isSystemTable()) { - return; + if(isOperationOnNonMetaTable(c)){ + internalReadOnlyGuard(); } - internalReadOnlyGuard(); RegionObserver.super.preBatchMutate(c, miniBatchOp); } @@ -289,7 +287,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndDelete(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (!c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { + if(isOperationOnNonMetaTable(c)){ internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, @@ -299,7 +297,7 @@ public boolean preCheckAndDelete(ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (!c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { + if(isOperationOnNonMetaTable(c)){ internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, filter, delete, result); @@ -310,7 +308,7 @@ public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (!c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { + if(isOperationOnNonMetaTable(c)){ internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, @@ -321,7 +319,7 @@ public boolean preCheckAndDeleteAfterRowLock( public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (!c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { + if(isOperationOnNonMetaTable(c)){ internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); From f9c49409798a912f91699d0a2b1d2b1a89c401ad Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Wed, 17 Dec 2025 13:28:23 +0530 Subject: [PATCH 2/4] Instead of system table just allow operation for hbase meta only There are some other system tables such as acl or namespace which are shared with active cluster hence allowing operation with them in readonly cluster will make system inconsistent. --- .../security/access/ReadOnlyController.java | 74 +++-- .../TestReadOnlyControllerRegionObserver.java | 260 +++++++++++++++++- 2 files changed, 303 insertions(+), 31 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java index c4a44e8d55f3..24db390b7f32 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java @@ -106,7 +106,8 @@ private void internalReadOnlyGuard() throws DoNotRetryIOException { } } - private boolean isOperationOnNonMetaTable(final ObserverContext c){ + private boolean + isOperationOnNonMetaTable(final ObserverContext c) { return !c.getEnvironment().getRegionInfo().getTable().equals(TableName.META_TABLE_NAME); } @@ -135,25 +136,29 @@ public Optional getRegionObserver() { } @Override - public void preFlush(final ObserverContext c, - FlushLifeCycleTracker tracker) throws IOException { - if(isOperationOnNonMetaTable(c)){ + public void preFlushScannerOpen(ObserverContext c, + Store store, ScanOptions options, FlushLifeCycleTracker tracker) throws IOException { + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } - RegionObserver.super.preFlush(c, tracker); + RegionObserver.super.preFlushScannerOpen(c, store, options, tracker); } @Override - public void preFlushScannerOpen(ObserverContext c, - Store store, ScanOptions options, FlushLifeCycleTracker tracker) throws IOException { - internalReadOnlyGuard(); - RegionObserver.super.preFlushScannerOpen(c, store, options, tracker); + public void preFlush(final ObserverContext c, + FlushLifeCycleTracker tracker) throws IOException { + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } + RegionObserver.super.preFlush(c, tracker); } @Override public InternalScanner preFlush(ObserverContext c, Store store, InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preFlush(c, store, scanner, tracker); } @@ -184,7 +189,9 @@ public InternalScanner preMemStoreCompactionCompact( public void preCompactSelection(ObserverContext c, Store store, List candidates, CompactionLifeCycleTracker tracker) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } RegionObserver.super.preCompactSelection(c, store, candidates, tracker); } @@ -192,7 +199,9 @@ public void preCompactSelection(ObserverContext c, Store store, ScanType scanType, ScanOptions options, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } RegionObserver.super.preCompactScannerOpen(c, store, scanType, options, tracker, request); } @@ -200,14 +209,16 @@ public void preCompactScannerOpen(ObserverContext c, Store store, InternalScanner scanner, ScanType scanType, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preCompact(c, store, scanner, scanType, tracker, request); } @Override public void prePut(ObserverContext c, Put put, WALEdit edit, Durability durability) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit, durability); @@ -216,7 +227,7 @@ public void prePut(ObserverContext c, Pu @Override public void prePut(ObserverContext c, Put put, WALEdit edit) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit); @@ -225,7 +236,7 @@ public void prePut(ObserverContext c, Pu @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit, Durability durability) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit, durability); @@ -234,7 +245,7 @@ public void preDelete(ObserverContext c, @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit); @@ -243,7 +254,7 @@ public void preDelete(ObserverContext c, @Override public void preBatchMutate(ObserverContext c, MiniBatchOperationInProgress miniBatchOp) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } RegionObserver.super.preBatchMutate(c, miniBatchOp); @@ -253,7 +264,9 @@ public void preBatchMutate(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); } @@ -261,7 +274,9 @@ public boolean preCheckAndPut(ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preCheckAndPut(c, row, filter, put, result); } @@ -270,7 +285,9 @@ public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, comparator, put, result); } @@ -279,7 +296,9 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - internalReadOnlyGuard(); + if (isOperationOnNonMetaTable(c)) { + internalReadOnlyGuard(); + } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, filter, put, result); } @@ -287,7 +306,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndDelete(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, @@ -297,7 +316,7 @@ public boolean preCheckAndDelete(ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, filter, delete, result); @@ -308,7 +327,7 @@ public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, @@ -319,7 +338,7 @@ public boolean preCheckAndDeleteAfterRowLock( public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if(isOperationOnNonMetaTable(c)){ + if (isOperationOnNonMetaTable(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); @@ -407,7 +426,8 @@ public void preCommitStoreFile(ObserverContext ctx, WALKey key, WALEdit edit) throws IOException { - if (!key.getTableName().isSystemTable()) { + // Only allow this operation for meta table + if (!key.getTableName().equals(TableName.META_TABLE_NAME)) { internalReadOnlyGuard(); } RegionObserver.super.preWALAppend(ctx, key, edit); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java index 12fe32c0d232..3844d16ca94b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java @@ -177,6 +177,10 @@ public void tearDown() throws Exception { } + private void mockOperationForMetaTable() { + when(regionInfo.getTable()).thenReturn(TableName.META_TABLE_NAME); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -188,6 +192,18 @@ public void testPreFlushV1NoException() throws IOException { readOnlyController.preFlush(c, flushLifeCycleTracker); } + @Test + public void testPreFlushV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preFlush(c, flushLifeCycleTracker); + } + + @Test + public void testPreFlushV1MetaNoException() throws IOException { + readOnlyController.preFlush(c, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -199,6 +215,18 @@ public void testPreFlushV2NoException() throws IOException { readOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); } + @Test + public void testPreFlushV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); + } + + @Test + public void testPreFlushV2MetaNoException() throws IOException { + readOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushScannerOpenReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -210,6 +238,18 @@ public void testPreFlushScannerOpenNoException() throws IOException { readOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); } + @Test + public void testPreFlushScannerOpenReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); + } + + @Test + public void testPreFlushScannerOpenMetaNoException() throws IOException { + readOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreMemStoreCompactionReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -254,6 +294,18 @@ public void testPreCompactSelectionNoException() throws IOException { readOnlyController.preCompactSelection(c, store, candidates, compactionLifeCycleTracker); } + @Test + public void testPreCompactSelectionReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCompactSelection(c, store, candidates, compactionLifeCycleTracker); + } + + @Test + public void testPreCompactSelectionMetaNoException() throws IOException { + readOnlyController.preCompactSelection(c, store, candidates, compactionLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCompactScannerOpenReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -267,6 +319,20 @@ public void testPreCompactScannerOpenNoException() throws IOException { compactionLifeCycleTracker, compactionRequest); } + @Test + public void testPreCompactScannerOpenReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCompactScannerOpen(c, store, scanType, options, + compactionLifeCycleTracker, compactionRequest); + } + + @Test + public void testPreCompactScannerOpenMetaNoException() throws IOException { + readOnlyController.preCompactScannerOpen(c, store, scanType, options, + compactionLifeCycleTracker, compactionRequest); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCompactReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -280,6 +346,20 @@ public void testPreCompactNoException() throws IOException { compactionRequest); } + @Test + public void testPreCompactReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCompact(c, store, scanner, scanType, compactionLifeCycleTracker, + compactionRequest); + } + + @Test + public void testPreCompactMetaNoException() throws IOException { + readOnlyController.preCompact(c, store, scanner, scanType, compactionLifeCycleTracker, + compactionRequest); + } + @Test(expected = DoNotRetryIOException.class) public void testPrePutV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -291,6 +371,18 @@ public void testPrePutV1NoException() throws IOException { readOnlyController.prePut(c, put, edit); } + @Test + public void testPrePutV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.prePut(c, put, edit); + } + + @Test + public void testPrePutV1MetaNoException() throws IOException { + readOnlyController.prePut(c, put, edit); + } + @Test(expected = DoNotRetryIOException.class) public void testPrePutV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -302,6 +394,18 @@ public void testPrePutV2NoException() throws IOException { readOnlyController.prePut(c, put, edit, durability); } + @Test + public void testPrePutV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.prePut(c, put, edit, durability); + } + + @Test + public void testPrePutV2MetaNoException() throws IOException { + readOnlyController.prePut(c, put, edit, durability); + } + @Test(expected = DoNotRetryIOException.class) public void testPreDeleteV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -313,6 +417,18 @@ public void testPreDeleteV1NoException() throws IOException { readOnlyController.preDelete(c, delete, edit); } + @Test + public void testPreDeleteV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preDelete(c, delete, edit); + } + + @Test + public void testPreDeleteV1MetaNoException() throws IOException { + readOnlyController.preDelete(c, delete, edit); + } + @Test(expected = DoNotRetryIOException.class) public void testPreDeleteV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -324,11 +440,41 @@ public void testPreDeleteV2NoException() throws IOException { readOnlyController.preDelete(c, delete, edit, durability); } + @Test + public void testPreDeleteV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preDelete(c, delete, edit, durability); + } + + @Test + public void testPreDeleteV2MetaNoException() throws IOException { + readOnlyController.preDelete(c, delete, edit, durability); + } + + @Test(expected = DoNotRetryIOException.class) + public void testPreBatchMutateReadOnlyException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + readOnlyController.preBatchMutate(c, miniBatchOp); + } + @Test public void testPreBatchMutateNoException() throws IOException { readOnlyController.preBatchMutate(c, miniBatchOp); } + @Test + public void testPreBatchMutateReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preBatchMutate(c, miniBatchOp); + } + + @Test + public void testPreBatchMutateMetaNoException() throws IOException { + readOnlyController.preBatchMutate(c, miniBatchOp); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -340,6 +486,18 @@ public void testPreCheckAndPutV1NoException() throws IOException { readOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); } + @Test + public void testPreCheckAndPutV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); + } + + @Test + public void testPreCheckAndPutV1MetaNoException() throws IOException { + readOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -351,6 +509,18 @@ public void testPreCheckAndPutV2NoException() throws IOException { readOnlyController.preCheckAndPut(c, row, filter, put, result); } + @Test + public void testPreCheckAndPutV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndPut(c, row, filter, put, result); + } + + @Test + public void testPreCheckAndPutV2MetaNoException() throws IOException { + readOnlyController.preCheckAndPut(c, row, filter, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutAfterRowLockV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -364,6 +534,20 @@ public void testPreCheckAndPutAfterRowLockV1NoException() throws IOException { result); } + @Test + public void testPreCheckAndPutAfterRowLockV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, comparator, put, + result); + } + + @Test + public void testPreCheckAndPutAfterRowLockV1MetaNoException() throws IOException { + readOnlyController.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, comparator, put, + result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutAfterRowLockV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -375,6 +559,18 @@ public void testPreCheckAndPutAfterRowLockV2NoException() throws IOException { readOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); } + @Test + public void testPreCheckAndPutAfterRowLockV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); + } + + @Test + public void testPreCheckAndPutAfterRowLockV2MetaNoException() throws IOException { + readOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -386,6 +582,18 @@ public void testPreCheckAndDeleteV1NoException() throws IOException { readOnlyController.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, result); } + @Test + public void testPreCheckAndDeleteV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, result); + } + + @Test + public void testPreCheckAndDeleteV1MetaNoException() throws IOException { + readOnlyController.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -397,6 +605,18 @@ public void testPreCheckAndDeleteV2NoException() throws IOException { readOnlyController.preCheckAndDelete(c, row, filter, delete, result); } + @Test + public void testPreCheckAndDeleteV2ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndDelete(c, row, filter, delete, result); + } + + @Test + public void testPreCheckAndDeleteV2MetaNoException() throws IOException { + readOnlyController.preCheckAndDelete(c, row, filter, delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteAfterRowLockV1ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -410,6 +630,20 @@ public void testPreCheckAndDeleteAfterRowLockV1NoException() throws IOException delete, result); } + @Test + public void testPreCheckAndDeleteAfterRowLockV1ReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + mockOperationForMetaTable(); + readOnlyController.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, comparator, + delete, result); + } + + @Test + public void testPreCheckAndDeleteAfterRowLockV1MetaNoException() throws IOException { + readOnlyController.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, comparator, + delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); @@ -420,11 +654,17 @@ public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyException() throws IOExce public void testPreCheckAndDeleteAfterRowLockV2NoException() throws IOException { readOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); } - - @Test(expected = DoNotRetryIOException.class) - public void testPreBatchMutateReadOnlyException() throws IOException { + + @Test + public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyMetaNoException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf); - readOnlyController.preBatchMutate(c, miniBatchOp); + mockOperationForMetaTable(); + readOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); + } + + @Test + public void testPreCheckAndDeleteAfterRowLockV2MetaNoException() throws IOException { + readOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); } @Test(expected = DoNotRetryIOException.class) @@ -558,4 +798,16 @@ public void testPreWALAppendReadOnlyException() throws IOException { public void testPreWALAppendNoException() throws IOException { readOnlyController.preWALAppend(ctx, key, edit); } + + @Test + public void testPreWALAppendReadOnlyMetaNoException() throws IOException { + readOnlyController.onConfigurationChange(readOnlyConf); + when(key.getTableName()).thenReturn(TableName.META_TABLE_NAME); + readOnlyController.preWALAppend(ctx, key, edit); + } + + @Test + public void testPreWALAppendMetaNoException() throws IOException { + readOnlyController.preWALAppend(ctx, key, edit); + } } From 5852cd860eb87f6896699b2ddae0c0801b2d0f94 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Thu, 18 Dec 2025 16:13:38 +0530 Subject: [PATCH 3/4] Invert the methods name and add negation to caller --- .../security/access/ReadOnlyController.java | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java index 24db390b7f32..e37e6e810116 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java @@ -106,9 +106,8 @@ private void internalReadOnlyGuard() throws DoNotRetryIOException { } } - private boolean - isOperationOnNonMetaTable(final ObserverContext c) { - return !c.getEnvironment().getRegionInfo().getTable().equals(TableName.META_TABLE_NAME); + private boolean isOnMeta(final ObserverContext c) { + return c.getEnvironment().getRegionInfo().getTable().equals(TableName.META_TABLE_NAME); } @Override @@ -138,7 +137,7 @@ public Optional getRegionObserver() { @Override public void preFlushScannerOpen(ObserverContext c, Store store, ScanOptions options, FlushLifeCycleTracker tracker) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preFlushScannerOpen(c, store, options, tracker); @@ -147,7 +146,7 @@ public void preFlushScannerOpen(ObserverContext c, FlushLifeCycleTracker tracker) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preFlush(c, tracker); @@ -156,7 +155,7 @@ public void preFlush(final ObserverContext c, Store store, InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preFlush(c, store, scanner, tracker); @@ -189,7 +188,7 @@ public InternalScanner preMemStoreCompactionCompact( public void preCompactSelection(ObserverContext c, Store store, List candidates, CompactionLifeCycleTracker tracker) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preCompactSelection(c, store, candidates, tracker); @@ -199,7 +198,7 @@ public void preCompactSelection(ObserverContext c, Store store, ScanType scanType, ScanOptions options, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preCompactScannerOpen(c, store, scanType, options, tracker, request); @@ -209,7 +208,7 @@ public void preCompactScannerOpen(ObserverContext c, Store store, InternalScanner scanner, ScanType scanType, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCompact(c, store, scanner, scanType, tracker, request); @@ -218,7 +217,7 @@ public InternalScanner preCompact(ObserverContext c, Put put, WALEdit edit, Durability durability) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit, durability); @@ -227,7 +226,7 @@ public void prePut(ObserverContext c, Pu @Override public void prePut(ObserverContext c, Put put, WALEdit edit) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit); @@ -236,7 +235,7 @@ public void prePut(ObserverContext c, Pu @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit, Durability durability) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit, durability); @@ -245,7 +244,7 @@ public void preDelete(ObserverContext c, @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit); @@ -254,7 +253,7 @@ public void preDelete(ObserverContext c, @Override public void preBatchMutate(ObserverContext c, MiniBatchOperationInProgress miniBatchOp) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } RegionObserver.super.preBatchMutate(c, miniBatchOp); @@ -264,7 +263,7 @@ public void preBatchMutate(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPut(c, row, family, qualifier, op, comparator, put, @@ -274,7 +273,7 @@ public boolean preCheckAndPut(ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPut(c, row, filter, put, result); @@ -285,7 +284,7 @@ public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, @@ -296,7 +295,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, filter, put, result); @@ -306,7 +305,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndDelete(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, @@ -316,7 +315,7 @@ public boolean preCheckAndDelete(ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, filter, delete, result); @@ -327,7 +326,7 @@ public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, @@ -338,7 +337,7 @@ public boolean preCheckAndDeleteAfterRowLock( public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (isOperationOnNonMetaTable(c)) { + if (!isOnMeta(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); From 2d548d1a18485a41d31448e9a3df79b1df740689 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Thu, 18 Dec 2025 16:34:33 +0530 Subject: [PATCH 4/4] Instead of static variable comparison use the API from TableName class This is done do avoid any conflicts after the changes in HBASE-29691: Change TableName.META_TABLE_NAME from being a global static --- .../hadoop/hbase/security/access/ReadOnlyController.java | 4 ++-- .../security/access/TestReadOnlyControllerRegionObserver.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java index e37e6e810116..163d4267faed 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java @@ -107,7 +107,7 @@ private void internalReadOnlyGuard() throws DoNotRetryIOException { } private boolean isOnMeta(final ObserverContext c) { - return c.getEnvironment().getRegionInfo().getTable().equals(TableName.META_TABLE_NAME); + return TableName.isMetaTableName(c.getEnvironment().getRegionInfo().getTable()); } @Override @@ -426,7 +426,7 @@ public void preCommitStoreFile(ObserverContext ctx, WALKey key, WALEdit edit) throws IOException { // Only allow this operation for meta table - if (!key.getTableName().equals(TableName.META_TABLE_NAME)) { + if (!TableName.isMetaTableName(key.getTableName())) { internalReadOnlyGuard(); } RegionObserver.super.preWALAppend(ctx, key, edit); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java index 3844d16ca94b..cc9fa6f1a156 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java @@ -654,7 +654,7 @@ public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyException() throws IOExce public void testPreCheckAndDeleteAfterRowLockV2NoException() throws IOException { readOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); } - + @Test public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyMetaNoException() throws IOException { readOnlyController.onConfigurationChange(readOnlyConf);